diff mbox series

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

Message ID 20200213005837.131791-2-david@gibson.dropbear.id.au
State New
Headers show
Series spapr: Use vIOMMU translation for virtio by default | expand

Commit Message

David Gibson Feb. 13, 2020, 12:58 a.m. UTC
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.

This does mean we no longer support guest kernels prior to 4.0, unless
they have modern virtio support backported (which some distro kernels
like that in RHEL7 do).

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

Comments

Greg Kurz Feb. 13, 2020, 2:34 p.m. UTC | #1
On Thu, 13 Feb 2020 11:58:36 +1100
David Gibson <david@gibson.dropbear.id.au> 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.
> 
> This does mean we no longer support guest kernels prior to 4.0, unless
> they have modern virtio support backported (which some distro kernels
> like that in RHEL7 do).
> 
> 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 cb220fde45..6e1e467cc6 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"
>  
> @@ -4567,7 +4568,14 @@ static void spapr_machine_latest_class_options(MachineClass *mc)
>   */
>  static void spapr_machine_5_0_class_options(MachineClass *mc)
>  {
> -    /* Defaults for the latest behaviour inherited from the base class */

Hmm... and so it seems we still have to carry this around when we
add a new machine version. At least, that's what I had to do when
adding a dummy 5.1 machine. This is because it is the old machine
type that calls the class_options() function of the new one, not
the other way around.

I was thinking about adapting Michael's patch. Instead of having
a class_options() function that we only call for the latest
machine type, we need a function that sets the default behaviour
and call it for all machine types (which can still change the
behaviour in their own class_options() function).

Something like the following:

-----------------------------------------------------------------------------
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 828e2cc1359a..27e6f79ddc40 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4559,10 +4559,9 @@ static const TypeInfo spapr_machine_info = {
     },
 };
 
-static void spapr_machine_latest_class_options(MachineClass *mc)
+static void spapr_machine_default_class_options(MachineClass *mc)
 {
-    mc->alias = "pseries";
-    mc->is_default = 1;
+    /* Override non ppc specific default behaviour here. */
 }
 
 #define DEFINE_SPAPR_MACHINE(suffix, verstr, latest)                 \
@@ -4570,9 +4569,11 @@ static void spapr_machine_latest_class_options(MachineClass *mc)
                                                     void *data)      \
     {                                                                \
         MachineClass *mc = MACHINE_CLASS(oc);                        \
+        spapr_machine_default_class_options(mc);                     \
         spapr_machine_##suffix##_class_options(mc);                  \
         if (latest) {                                                \
-            spapr_machine_latest_class_options(mc);                  \
+            mc->alias = "pseries";                                   \
+            mc->is_default = 1;                                      \
         }                                                            \
     }                                                                \
     static const TypeInfo spapr_machine_##suffix##_info = {          \
@@ -4591,7 +4592,11 @@ static void spapr_machine_latest_class_options(MachineClass *mc)
  */
 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. If a non ppc specific default behaviour needs to
+     * be overriden, do it in spapr_machine_latest_class_options().
+     */
 }
 
 DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
-----------------------------------------------------------------------------

With the above applied...

> +    /* 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));

... this change would just need to move to spapr_machine_default_class_options()
and any need machine type would have it automatically.

Makes sense ?

>  }
>  
>  DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
> @@ -4578,12 +4586,16 @@ 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;
>      mc->nvdimm_supported = false;
> +    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
>  }
>  
>  DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
David Gibson Feb. 13, 2020, 10:56 p.m. UTC | #2
On Thu, Feb 13, 2020 at 03:34:25PM +0100, Greg Kurz wrote:
> On Thu, 13 Feb 2020 11:58:36 +1100
> David Gibson <david@gibson.dropbear.id.au> 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.
> > 
> > This does mean we no longer support guest kernels prior to 4.0, unless
> > they have modern virtio support backported (which some distro kernels
> > like that in RHEL7 do).
> > 
> > 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 cb220fde45..6e1e467cc6 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"
> >  
> > @@ -4567,7 +4568,14 @@ static void spapr_machine_latest_class_options(MachineClass *mc)
> >   */
> >  static void spapr_machine_5_0_class_options(MachineClass *mc)
> >  {
> > -    /* Defaults for the latest behaviour inherited from the base class */
> 
> Hmm... and so it seems we still have to carry this around when we
> add a new machine version. At least, that's what I had to do when
> adding a dummy 5.1 machine. This is because it is the old machine
> type that calls the class_options() function of the new one, not
> the other way around.

Uh.. no. It can just go in latest_class_options.  I thought I'd
already moved it there, but obviously not.  I've fixed that up for the
next spin.

> I was thinking about adapting Michael's patch. Instead of having
> a class_options() function that we only call for the latest
> machine type, we need a function that sets the default behaviour
> and call it for all machine types (which can still change the
> behaviour in their own class_options() function).

This will be confusing in a different way.  It will reset the default
options on each of the chained old machine types, which means anything
set in the "default" options needs to be overriden in *all* old
machine types that don't want it, not just the latest which doesn't
want it.
diff mbox series

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index cb220fde45..6e1e467cc6 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"
 
@@ -4567,7 +4568,14 @@  static void spapr_machine_latest_class_options(MachineClass *mc)
  */
 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);
@@ -4578,12 +4586,16 @@  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;
     mc->nvdimm_supported = false;
+    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
 }
 
 DEFINE_SPAPR_MACHINE(4_2, "4.2", false);