[v3,2/2] spapr: Enable virtio iommu_platform=on by default
diff mbox series

Message ID 20200305043009.611636-3-david@gibson.dropbear.id.au
State New
Headers show
Series
  • spapr: Use vIOMMU translation for virtio by default
Related show

Commit Message

David Gibson March 5, 2020, 4:30 a.m. UTC
Traditionally, virtio devices don't do DMA by the usual path on the
guest platform.  In particular they usually bypass any virtual IOMMU
the guest has, using hypervisor magic to access untranslated guest
physical addresses.

There's now the optional iommu_platform flag which can tell virtio
devices to use the platform's normal DMA path, including any IOMMUs.
That flag was motiviated for the case of hardware virtio
implementations, but there are other reasons to want it.

Specifically, the fact that the virtio device doesn't use vIOMMU
translation means that virtio devices are unsafe to pass to nested
guests, or to use with VFIO userspace drivers inside the guest.  This
is particularly noticeable on the pseries platform which *always* has
a guest-visible vIOMMU.

Not using the normal DMA path also causes difficulties for the guest
side driver when using the upcoming POWER Secure VMs (a.k.a. PEF).
While it's theoretically possible to handle this on the guest side,
it's really fiddly.  Given the other problems with the non-translated
virtio device, let's just enable vIOMMU translation for virtio devices
by default in the pseries-5.0 (and later) machine types.

This does mean the new machine type will no longer support guest
kernels older than 4.8, unless they have support for the virtio
IOMMU_PLATFORM flag backported (which some distro kernels like RHEL7
do).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Greg Kurz March 5, 2020, 11:59 a.m. UTC | #1
On Thu,  5 Mar 2020 15:30:09 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Traditionally, virtio devices don't do DMA by the usual path on the
> guest platform.  In particular they usually bypass any virtual IOMMU
> the guest has, using hypervisor magic to access untranslated guest
> physical addresses.
> 
> There's now the optional iommu_platform flag which can tell virtio
> devices to use the platform's normal DMA path, including any IOMMUs.
> That flag was motiviated for the case of hardware virtio
> implementations, but there are other reasons to want it.
> 
> Specifically, the fact that the virtio device doesn't use vIOMMU
> translation means that virtio devices are unsafe to pass to nested
> guests, or to use with VFIO userspace drivers inside the guest.  This
> is particularly noticeable on the pseries platform which *always* has
> a guest-visible vIOMMU.
> 
> Not using the normal DMA path also causes difficulties for the guest
> side driver when using the upcoming POWER Secure VMs (a.k.a. PEF).
> While it's theoretically possible to handle this on the guest side,
> it's really fiddly.  Given the other problems with the non-translated
> virtio device, let's just enable vIOMMU translation for virtio devices
> by default in the pseries-5.0 (and later) machine types.
> 
> This does mean the new machine type will no longer support guest
> kernels older than 4.8, unless they have support for the virtio
> IOMMU_PLATFORM flag backported (which some distro kernels like RHEL7
> do).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

The patch looks good but I'm not sure if we're quite ready to merge
it yet. With this applied, I get zero output on a virtio-serial based
console:

ie.
  -chardev stdio,id=con0 -device virtio-serial -device virtconsole,chardev=con0 

FYI, virtio-serial is a bit broken for spapr with iommu_platform=off already:

(1) pressing a key in the console during SLOF or grub has no effect

(2) the guest kernel boot stays stuck around quiesce

These are regressions introduced by this SLOF update:

a363e9ed8731f45674260932a340a0d81c4b0a6f is the first bad commit
commit a363e9ed8731f45674260932a340a0d81c4b0a6f
Author: Alexey Kardashevskiy <aik@ozlabs.ru>
Date:   Tue Dec 17 11:31:54 2019 +1100
    pseries: Update SLOF firmware image

A trivial fix was already posted on the SLOF list for (1) :

https://patchwork.ozlabs.org/patch/1249338/

(2) is still under investigation but the console is _at least_
functional until the guest OS takes control. This is no longer
the case with this patch.

>  hw/ppc/spapr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3cfc98ac61..5ef099536e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4575,6 +4575,7 @@ static void spapr_machine_latest_class_options(MachineClass *mc)
>       */
>      static GlobalProperty compat[] = {
>          { TYPE_VIRTIO_PCI, "disable-legacy", "on", },
> +        { TYPE_VIRTIO_DEVICE, "iommu_platform", "on", },
>      };
>  
>      mc->alias = "pseries";
> @@ -4622,6 +4623,7 @@ static void spapr_machine_4_2_class_options(MachineClass *mc)
>      SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>      static GlobalProperty compat[] = {
>          { TYPE_VIRTIO_PCI, "disable-legacy", "auto" },
> +        { TYPE_VIRTIO_DEVICE, "iommu_platform", "off", },
>      };
>  
>      spapr_machine_5_0_class_options(mc);
Greg Kurz March 10, 2020, 10:43 a.m. UTC | #2
On Thu, 5 Mar 2020 12:59:03 +0100
Greg Kurz <groug@kaod.org> wrote:

> On Thu,  5 Mar 2020 15:30:09 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Traditionally, virtio devices don't do DMA by the usual path on the
> > guest platform.  In particular they usually bypass any virtual IOMMU
> > the guest has, using hypervisor magic to access untranslated guest
> > physical addresses.
> > 
> > There's now the optional iommu_platform flag which can tell virtio
> > devices to use the platform's normal DMA path, including any IOMMUs.
> > That flag was motiviated for the case of hardware virtio
> > implementations, but there are other reasons to want it.
> > 
> > Specifically, the fact that the virtio device doesn't use vIOMMU
> > translation means that virtio devices are unsafe to pass to nested
> > guests, or to use with VFIO userspace drivers inside the guest.  This
> > is particularly noticeable on the pseries platform which *always* has
> > a guest-visible vIOMMU.
> > 
> > Not using the normal DMA path also causes difficulties for the guest
> > side driver when using the upcoming POWER Secure VMs (a.k.a. PEF).
> > While it's theoretically possible to handle this on the guest side,
> > it's really fiddly.  Given the other problems with the non-translated
> > virtio device, let's just enable vIOMMU translation for virtio devices
> > by default in the pseries-5.0 (and later) machine types.
> > 
> > This does mean the new machine type will no longer support guest
> > kernels older than 4.8, unless they have support for the virtio
> > IOMMU_PLATFORM flag backported (which some distro kernels like RHEL7
> > do).
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> The patch looks good but I'm not sure if we're quite ready to merge
> it yet. With this applied, I get zero output on a virtio-serial based
> console:
> 
> ie.
>   -chardev stdio,id=con0 -device virtio-serial -device virtconsole,chardev=con0 
> 
> FYI, virtio-serial is a bit broken for spapr with iommu_platform=off already:
> 
> (1) pressing a key in the console during SLOF or grub has no effect
> 
> (2) the guest kernel boot stays stuck around quiesce
> 
> These are regressions introduced by this SLOF update:
> 
> a363e9ed8731f45674260932a340a0d81c4b0a6f is the first bad commit
> commit a363e9ed8731f45674260932a340a0d81c4b0a6f
> Author: Alexey Kardashevskiy <aik@ozlabs.ru>
> Date:   Tue Dec 17 11:31:54 2019 +1100
>     pseries: Update SLOF firmware image
> 
> A trivial fix was already posted on the SLOF list for (1) :
> 
> https://patchwork.ozlabs.org/patch/1249338/
> 
> (2) is still under investigation but the console is _at least_
> functional until the guest OS takes control. This is no longer
> the case with this patch.
> 

Some progress was made on the SLOF front:

https://patchwork.ozlabs.org/project/slof/list/?series=163314

With these series applied to SLOF, I can now boot a fedora31 guest
with a virtio-serial console and iommu_platform=on... but now
I'm trying out other virtio devices supported by SLOF and I'm
running into issues around virtio-pci.disable-legacy as mentioned
in some other mail...

It seems we may not be ready to merge this series yet.

> >  hw/ppc/spapr.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 3cfc98ac61..5ef099536e 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -4575,6 +4575,7 @@ static void spapr_machine_latest_class_options(MachineClass *mc)
> >       */
> >      static GlobalProperty compat[] = {
> >          { TYPE_VIRTIO_PCI, "disable-legacy", "on", },
> > +        { TYPE_VIRTIO_DEVICE, "iommu_platform", "on", },
> >      };
> >  
> >      mc->alias = "pseries";
> > @@ -4622,6 +4623,7 @@ static void spapr_machine_4_2_class_options(MachineClass *mc)
> >      SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> >      static GlobalProperty compat[] = {
> >          { TYPE_VIRTIO_PCI, "disable-legacy", "auto" },
> > +        { TYPE_VIRTIO_DEVICE, "iommu_platform", "off", },
> >      };
> >  
> >      spapr_machine_5_0_class_options(mc);
>
Alexey Kardashevskiy March 12, 2020, 4:14 a.m. UTC | #3
On 10/03/2020 21:43, Greg Kurz wrote:
> On Thu, 5 Mar 2020 12:59:03 +0100
> Greg Kurz <groug@kaod.org> wrote:
> 
>> On Thu,  5 Mar 2020 15:30:09 +1100
>> David Gibson <david@gibson.dropbear.id.au> wrote:
>>
>>> Traditionally, virtio devices don't do DMA by the usual path on the
>>> guest platform.  In particular they usually bypass any virtual IOMMU
>>> the guest has, using hypervisor magic to access untranslated guest
>>> physical addresses.
>>>
>>> There's now the optional iommu_platform flag which can tell virtio
>>> devices to use the platform's normal DMA path, including any IOMMUs.
>>> That flag was motiviated for the case of hardware virtio
>>> implementations, but there are other reasons to want it.
>>>
>>> Specifically, the fact that the virtio device doesn't use vIOMMU
>>> translation means that virtio devices are unsafe to pass to nested
>>> guests, or to use with VFIO userspace drivers inside the guest.  This
>>> is particularly noticeable on the pseries platform which *always* has
>>> a guest-visible vIOMMU.
>>>
>>> Not using the normal DMA path also causes difficulties for the guest
>>> side driver when using the upcoming POWER Secure VMs (a.k.a. PEF).
>>> While it's theoretically possible to handle this on the guest side,
>>> it's really fiddly.  Given the other problems with the non-translated
>>> virtio device, let's just enable vIOMMU translation for virtio devices
>>> by default in the pseries-5.0 (and later) machine types.
>>>
>>> This does mean the new machine type will no longer support guest
>>> kernels older than 4.8, unless they have support for the virtio
>>> IOMMU_PLATFORM flag backported (which some distro kernels like RHEL7
>>> do).
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>
>> The patch looks good but I'm not sure if we're quite ready to merge
>> it yet. With this applied, I get zero output on a virtio-serial based
>> console:
>>
>> ie.
>>   -chardev stdio,id=con0 -device virtio-serial -device virtconsole,chardev=con0 
>>
>> FYI, virtio-serial is a bit broken for spapr with iommu_platform=off already:
>>
>> (1) pressing a key in the console during SLOF or grub has no effect
>>
>> (2) the guest kernel boot stays stuck around quiesce
>>
>> These are regressions introduced by this SLOF update:
>>
>> a363e9ed8731f45674260932a340a0d81c4b0a6f is the first bad commit
>> commit a363e9ed8731f45674260932a340a0d81c4b0a6f
>> Author: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Date:   Tue Dec 17 11:31:54 2019 +1100
>>     pseries: Update SLOF firmware image
>>
>> A trivial fix was already posted on the SLOF list for (1) :
>>
>> https://patchwork.ozlabs.org/patch/1249338/
>>
>> (2) is still under investigation but the console is _at least_
>> functional until the guest OS takes control. This is no longer
>> the case with this patch.
>>
> 
> Some progress was made on the SLOF front:
> 
> https://patchwork.ozlabs.org/project/slof/list/?series=163314
> 
> With these series applied to SLOF, I can now boot a fedora31 guest
> with a virtio-serial console and iommu_platform=on... but now
> I'm trying out other virtio devices supported by SLOF and I'm
> running into issues around virtio-pci.disable-legacy as mentioned
> in some other mail...
> 
> It seems we may not be ready to merge this series yet.


fwiw I sent a pull request:

https://lore.kernel.org/qemu-devel/20200312041010.16229-1-aik@ozlabs.ru/T/#u



> 
>>>  hw/ppc/spapr.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 3cfc98ac61..5ef099536e 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -4575,6 +4575,7 @@ static void spapr_machine_latest_class_options(MachineClass *mc)
>>>       */
>>>      static GlobalProperty compat[] = {
>>>          { TYPE_VIRTIO_PCI, "disable-legacy", "on", },
>>> +        { TYPE_VIRTIO_DEVICE, "iommu_platform", "on", },
>>>      };
>>>  
>>>      mc->alias = "pseries";
>>> @@ -4622,6 +4623,7 @@ static void spapr_machine_4_2_class_options(MachineClass *mc)
>>>      SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>>>      static GlobalProperty compat[] = {
>>>          { TYPE_VIRTIO_PCI, "disable-legacy", "auto" },
>>> +        { TYPE_VIRTIO_DEVICE, "iommu_platform", "off", },
>>>      };
>>>  
>>>      spapr_machine_5_0_class_options(mc);
>>
>
Greg Kurz March 12, 2020, 8:02 a.m. UTC | #4
On Thu, 12 Mar 2020 15:14:06 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> 
> 
> On 10/03/2020 21:43, Greg Kurz wrote:
> > On Thu, 5 Mar 2020 12:59:03 +0100
> > Greg Kurz <groug@kaod.org> wrote:
> > 
> >> On Thu,  5 Mar 2020 15:30:09 +1100
> >> David Gibson <david@gibson.dropbear.id.au> wrote:
> >>
> >>> Traditionally, virtio devices don't do DMA by the usual path on the
> >>> guest platform.  In particular they usually bypass any virtual IOMMU
> >>> the guest has, using hypervisor magic to access untranslated guest
> >>> physical addresses.
> >>>
> >>> There's now the optional iommu_platform flag which can tell virtio
> >>> devices to use the platform's normal DMA path, including any IOMMUs.
> >>> That flag was motiviated for the case of hardware virtio
> >>> implementations, but there are other reasons to want it.
> >>>
> >>> Specifically, the fact that the virtio device doesn't use vIOMMU
> >>> translation means that virtio devices are unsafe to pass to nested
> >>> guests, or to use with VFIO userspace drivers inside the guest.  This
> >>> is particularly noticeable on the pseries platform which *always* has
> >>> a guest-visible vIOMMU.
> >>>
> >>> Not using the normal DMA path also causes difficulties for the guest
> >>> side driver when using the upcoming POWER Secure VMs (a.k.a. PEF).
> >>> While it's theoretically possible to handle this on the guest side,
> >>> it's really fiddly.  Given the other problems with the non-translated
> >>> virtio device, let's just enable vIOMMU translation for virtio devices
> >>> by default in the pseries-5.0 (and later) machine types.
> >>>
> >>> This does mean the new machine type will no longer support guest
> >>> kernels older than 4.8, unless they have support for the virtio
> >>> IOMMU_PLATFORM flag backported (which some distro kernels like RHEL7
> >>> do).
> >>>
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>> ---
> >>
> >> The patch looks good but I'm not sure if we're quite ready to merge
> >> it yet. With this applied, I get zero output on a virtio-serial based
> >> console:
> >>
> >> ie.
> >>   -chardev stdio,id=con0 -device virtio-serial -device virtconsole,chardev=con0 
> >>
> >> FYI, virtio-serial is a bit broken for spapr with iommu_platform=off already:
> >>
> >> (1) pressing a key in the console during SLOF or grub has no effect
> >>
> >> (2) the guest kernel boot stays stuck around quiesce
> >>
> >> These are regressions introduced by this SLOF update:
> >>
> >> a363e9ed8731f45674260932a340a0d81c4b0a6f is the first bad commit
> >> commit a363e9ed8731f45674260932a340a0d81c4b0a6f
> >> Author: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> Date:   Tue Dec 17 11:31:54 2019 +1100
> >>     pseries: Update SLOF firmware image
> >>
> >> A trivial fix was already posted on the SLOF list for (1) :
> >>
> >> https://patchwork.ozlabs.org/patch/1249338/
> >>
> >> (2) is still under investigation but the console is _at least_
> >> functional until the guest OS takes control. This is no longer
> >> the case with this patch.
> >>
> > 
> > Some progress was made on the SLOF front:
> > 
> > https://patchwork.ozlabs.org/project/slof/list/?series=163314
> > 
> > With these series applied to SLOF, I can now boot a fedora31 guest
> > with a virtio-serial console and iommu_platform=on... but now
> > I'm trying out other virtio devices supported by SLOF and I'm
> > running into issues around virtio-pci.disable-legacy as mentioned
> > in some other mail...
> > 
> > It seems we may not be ready to merge this series yet.
> 
> 
> fwiw I sent a pull request:
> 
> https://lore.kernel.org/qemu-devel/20200312041010.16229-1-aik@ozlabs.ru/T/#u
> 

Great ! Thanks mate ! :)

> 
> 
> > 
> >>>  hw/ppc/spapr.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>> index 3cfc98ac61..5ef099536e 100644
> >>> --- a/hw/ppc/spapr.c
> >>> +++ b/hw/ppc/spapr.c
> >>> @@ -4575,6 +4575,7 @@ static void spapr_machine_latest_class_options(MachineClass *mc)
> >>>       */
> >>>      static GlobalProperty compat[] = {
> >>>          { TYPE_VIRTIO_PCI, "disable-legacy", "on", },
> >>> +        { TYPE_VIRTIO_DEVICE, "iommu_platform", "on", },
> >>>      };
> >>>  
> >>>      mc->alias = "pseries";
> >>> @@ -4622,6 +4623,7 @@ static void spapr_machine_4_2_class_options(MachineClass *mc)
> >>>      SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> >>>      static GlobalProperty compat[] = {
> >>>          { TYPE_VIRTIO_PCI, "disable-legacy", "auto" },
> >>> +        { TYPE_VIRTIO_DEVICE, "iommu_platform", "off", },
> >>>      };
> >>>  
> >>>      spapr_machine_5_0_class_options(mc);
> >>
> > 
>

Patch
diff mbox series

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3cfc98ac61..5ef099536e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4575,6 +4575,7 @@  static void spapr_machine_latest_class_options(MachineClass *mc)
      */
     static GlobalProperty compat[] = {
         { TYPE_VIRTIO_PCI, "disable-legacy", "on", },
+        { TYPE_VIRTIO_DEVICE, "iommu_platform", "on", },
     };
 
     mc->alias = "pseries";
@@ -4622,6 +4623,7 @@  static void spapr_machine_4_2_class_options(MachineClass *mc)
     SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
     static GlobalProperty compat[] = {
         { TYPE_VIRTIO_PCI, "disable-legacy", "auto" },
+        { TYPE_VIRTIO_DEVICE, "iommu_platform", "off", },
     };
 
     spapr_machine_5_0_class_options(mc);