[PATCH 0/2] spapr: Use vIOMMU translation for virtio by default

[PATCH 0/2] spapr: Use vIOMMU translation for virtio by default

[David Gibson]

Upcoming Secure VM support for pSeries machines introduces some
complications for virtio, since the transfer buffers need to be
explicitly shared so that the hypervisor can access them.

While it's not strictly speaking dependent on it, the fact that virtio
devices bypass normal platform IOMMU translation complicates the issue
on the guest side.  Since there are some significan downsides to
bypassing the vIOMMU anyway, let's just disable that.

There's already a flag to do this in virtio, just turn it on by
default for forthcoming pseries machine types.

The implementation does this with a compat_props_add() from the latest
machine type.  This breaks the previous convention that the setup for
the latest machine type didn't do anything, instead just taking all
the defaults from the abstract base class.  However,
compat_props_add() can't be used from the base class, because
mc->compat_props is explicitly uninitialized for abstract classes.  If
anyone knows a better way to handle this, let me know.

David Gibson (2):
  spapr: Disable legacy virtio devices for pseries-5.0 and later
  spapr: Enable virtio iommu_platform=on by default

 hw/ppc/spapr.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

-- 
2.24.1

[PATCH 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later

[David Gibson]

PAPR specifies a kind of odd, paravirtualized PCI bus, which looks to
the guess mostly like classic PCI, even if some of the individual
devices on the bus are PCI Express.  One consequence of that is that
virtio-pci devices still default to being in transitional mode, though
legacy mode is now disabled by default on current q35 x86 machine
types.

Legacy mode virtio devices aren't really necessary any more, and are
causing some problems for future changes.  Therefore, for the
pseries-5.0 machine type (and onwards), switch to modern-only
virtio-pci devices by default.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c9b2e0a5e0..216d3b34dc 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -65,6 +65,7 @@
 
 #include "hw/pci/pci.h"
 #include "hw/scsi/scsi.h"
+#include "hw/virtio/virtio-pci.h"
 #include "hw/virtio/virtio-scsi.h"
 #include "hw/virtio/vhost-scsi-common.h"
 
@@ -4512,7 +4513,14 @@ static const TypeInfo spapr_machine_info = {
  */
 static void spapr_machine_5_0_class_options(MachineClass *mc)
 {
-    /* Defaults for the latest behaviour inherited from the base class */
+    /* Most defaults for the latest behaviour are inherited from the
+     * base class, but we need to override the (non ppc specific)
+     * default behaviour for virtio */
+    static GlobalProperty compat[] = {
+        { TYPE_VIRTIO_PCI, "disable-legacy", "on", },
+    };
+
+    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
 }
 
 DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
@@ -4523,11 +4531,15 @@ DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
 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" },
+    };
 
     spapr_machine_5_0_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
     smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
     smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF;
+    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
 }
 
 DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
-- 
2.24.1

Re: [PATCH 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later

[Michael S. Tsirkin]

On Fri, Feb 07, 2020 at 03:30:54PM +1100, David Gibson wrote:
> PAPR specifies a kind of odd, paravirtualized PCI bus, which looks to
> the guess mostly like classic PCI, even if some of the individual
> devices on the bus are PCI Express.  One consequence of that is that
> virtio-pci devices still default to being in transitional mode, though
> legacy mode is now disabled by default on current q35 x86 machine
> types.
> 
> Legacy mode virtio devices aren't really necessary any more, and are
> causing some problems for future changes.  Therefore, for the
> pseries-5.0 machine type (and onwards), switch to modern-only
> virtio-pci devices by default.

It's worth noting in the commit log that this disables support
for guests older than Linux 4.0.

> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c9b2e0a5e0..216d3b34dc 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -65,6 +65,7 @@
>  
>  #include "hw/pci/pci.h"
>  #include "hw/scsi/scsi.h"
> +#include "hw/virtio/virtio-pci.h"
>  #include "hw/virtio/virtio-scsi.h"
>  #include "hw/virtio/vhost-scsi-common.h"
>  
> @@ -4512,7 +4513,14 @@ static const TypeInfo spapr_machine_info = {
>   */
>  static void spapr_machine_5_0_class_options(MachineClass *mc)
>  {
> -    /* Defaults for the latest behaviour inherited from the base class */
> +    /* Most defaults for the latest behaviour are inherited from the
> +     * base class, but we need to override the (non ppc specific)
> +     * default behaviour for virtio */
> +    static GlobalProperty compat[] = {
> +        { TYPE_VIRTIO_PCI, "disable-legacy", "on", },
> +    };
> +
> +    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
>  }
>  
>  DEFINE_SPAPR_MACHINE(5_0, "5.0", true);

So this sets the defaults, right?

Problem is we'll then need to remember to carry this in the latest
call. If we forget we get a mess.

How about adding a call to e.g. spapr_machine_latest_class_options
in DEFINE_SPAPR_MACHINE?
Then spapr_machine_latest_class_options can set the per-machine
defaults.

I send a patch for this:
	[PATCH] ppc: function to setup latest class options
feel free to reuse.


> @@ -4523,11 +4531,15 @@ DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
>  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" },
> +    };
>  
>      spapr_machine_5_0_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
>      smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF;
> +    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
>  }
>  
>  DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
> -- 
> 2.24.1

Re: [PATCH 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3693 bytes --]

On Fri, Feb 07, 2020 at 01:54:26AM -0500, Michael S. Tsirkin wrote:
> On Fri, Feb 07, 2020 at 03:30:54PM +1100, David Gibson wrote:
> > PAPR specifies a kind of odd, paravirtualized PCI bus, which looks to
> > the guess mostly like classic PCI, even if some of the individual
> > devices on the bus are PCI Express.  One consequence of that is that
> > virtio-pci devices still default to being in transitional mode, though
> > legacy mode is now disabled by default on current q35 x86 machine
> > types.
> > 
> > Legacy mode virtio devices aren't really necessary any more, and are
> > causing some problems for future changes.  Therefore, for the
> > pseries-5.0 machine type (and onwards), switch to modern-only
> > virtio-pci devices by default.
> 
> It's worth noting in the commit log that this disables support
> for guests older than Linux 4.0.

Oof.  That's more recent than I'd guessed.  I'll have to do some
checking to see if that's reasonable.

> 
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index c9b2e0a5e0..216d3b34dc 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -65,6 +65,7 @@
> >  
> >  #include "hw/pci/pci.h"
> >  #include "hw/scsi/scsi.h"
> > +#include "hw/virtio/virtio-pci.h"
> >  #include "hw/virtio/virtio-scsi.h"
> >  #include "hw/virtio/vhost-scsi-common.h"
> >  
> > @@ -4512,7 +4513,14 @@ static const TypeInfo spapr_machine_info = {
> >   */
> >  static void spapr_machine_5_0_class_options(MachineClass *mc)
> >  {
> > -    /* Defaults for the latest behaviour inherited from the base class */
> > +    /* Most defaults for the latest behaviour are inherited from the
> > +     * base class, but we need to override the (non ppc specific)
> > +     * default behaviour for virtio */
> > +    static GlobalProperty compat[] = {
> > +        { TYPE_VIRTIO_PCI, "disable-legacy", "on", },
> > +    };
> > +
> > +    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
> >  }
> >  
> >  DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
> 
> So this sets the defaults, right?
> 
> Problem is we'll then need to remember to carry this in the latest
> call. If we forget we get a mess.
> 
> How about adding a call to e.g. spapr_machine_latest_class_options
> in DEFINE_SPAPR_MACHINE?
> Then spapr_machine_latest_class_options can set the per-machine
> defaults.
> 
> I send a patch for this:
> 	[PATCH] ppc: function to setup latest class options
> feel free to reuse.

Good idea, thanks.  I've merged that and will rebase these changes on it.
> 
> 
> > @@ -4523,11 +4531,15 @@ DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
> >  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" },
> > +    };
> >  
> >      spapr_machine_5_0_class_options(mc);
> >      compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> >      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> >      smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF;
> > +    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
> >  }
> >  
> >  DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/2] spapr: Enable virtio iommu_platform=on by default

[David Gibson]

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.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 216d3b34dc..78e031e80a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4518,6 +4518,7 @@ static void spapr_machine_5_0_class_options(MachineClass *mc)
      * default behaviour for virtio */
     static GlobalProperty compat[] = {
         { TYPE_VIRTIO_PCI, "disable-legacy", "on", },
+        { TYPE_VIRTIO_DEVICE, "iommu_platform", "on", },
     };
 
     compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
@@ -4533,6 +4534,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);
-- 
2.24.1

Re: [PATCH 2/2] spapr: Enable virtio iommu_platform=on by default

[Michael S. Tsirkin]

On Fri, Feb 07, 2020 at 03:30:55PM +1100, David Gibson 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.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Worth noting that since iommu_platform is mandatory for guests,
this disables support for guests older than Linux 4.8.


> ---
>  hw/ppc/spapr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 216d3b34dc..78e031e80a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4518,6 +4518,7 @@ static void spapr_machine_5_0_class_options(MachineClass *mc)
>       * default behaviour for virtio */
>      static GlobalProperty compat[] = {
>          { TYPE_VIRTIO_PCI, "disable-legacy", "on", },
> +        { TYPE_VIRTIO_DEVICE, "iommu_platform", "on", },
>      };
>  
>      compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
> @@ -4533,6 +4534,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);
> -- 
> 2.24.1