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

[Greg Kurz]

On Thu, 5 Mar 2020 11:31:46 +0100
Greg Kurz <address@hidden> wrote:

> On Thu,  5 Mar 2020 15:30:08 +1100
> David Gibson <address@hidden> 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 <address@hidden>
> > ---
> 
> Reviewed-by: Greg Kurz <address@hidden>
> 
> FWIW, I could test the following:
> - allows a RHEL7 guest with pre 4.0 kernel to boot, as mentioned
>   in the changelog
> - breaks boot of older RHEL 6.10 guests as expected
> - allows migration of older machine types to/from QEMU 4.2
> 
> Tested-by: Greg Kurz <address@hidden>
> 

Wait... I gave a try to virtiofsd and there's a problem:

$ ./ppc64-softmmu/qemu-system-ppc64 -device vhost-user-fs-pci
Unexpected error in object_property_find() at 
/home/greg/Work/qemu/qemu-ppc/qom/object.c:1231:
qemu-system-ppc64: -device vhost-user-fs-pci: can't apply global 
virtio-pci.disable-legacy=on: Property '.disable-legacy' not found
Aborted (core dumped)

It is still not possible to set the disable-legacy prop on the
vhost-user-fs-pci device, even without this patch, but QEMU
doesn't abort:

$ ./ppc64-softmmu/qemu-system-ppc64 -device vhost-user-fs-pci -global 
virtio-pci.disable-legacy=on
qemu-system-ppc64: -device vhost-user-fs-pci: can't apply global 
virtio-pci.disable-legacy=on: Property '.disable-legacy' not found
$

It seems to be related to the fact that vhost-user-fs-pci is a non-transitional
only device, as shown with this workaround:

--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4574,7 +4574,7 @@ static void 
spapr_machine_latest_class_options(MachineClass *mc)
      * class since it doesn't have a compat_props.
      */
     static GlobalProperty compat[] = {
-        { TYPE_VIRTIO_PCI, "disable-legacy", "on", },
+        { TYPE_VIRTIO_PCI "-transitional", "disable-legacy", "on", },
     };
 
     mc->alias = "pseries";


but there's probably a better way to address this.

MST, Any suggestion ?

> >  hw/ppc/spapr.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 2eb0d8f70d..3cfc98ac61 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"
> >  
> > @@ -4566,8 +4567,20 @@ static const TypeInfo spapr_machine_info = {
> >  
> >  static void spapr_machine_latest_class_options(MachineClass *mc)
> >  {
> > +    /*
> > +     * 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.  We can't do that from the base
> > +     * class since it doesn't have a compat_props.
> > +     */
> > +    static GlobalProperty compat[] = {
> > +        { TYPE_VIRTIO_PCI, "disable-legacy", "on", },
> > +    };
> > +
> >      mc->alias = "pseries";
> >      mc->is_default = true;
> > +
> > +    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
> >  }
> >  
> >  #define DEFINE_SPAPR_MACHINE(suffix, verstr, latest)                 \
> > @@ -4607,6 +4620,9 @@ 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);
> > @@ -4614,6 +4630,7 @@ static void 
> > spapr_machine_4_2_class_options(MachineClass *mc)
> >      smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF;
> >      smc->rma_limit = 16 * GiB;
> >      mc->nvdimm_supported = false;
> > +    compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
> >  }
> >  
> >  DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
>