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

[Greg Kurz <groug@kaod.org>]

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);