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

[Daniel P . Berrangé]

On Thu, Mar 05, 2020 at 03:30:08PM +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.

Two things to note here

x86 defaults to the i440fx  machine type, and so defaults
to transitional mode. AFAIK, only RHEL-8 downstream changed
x86 to defualt to q35

With q35 whether you get transitional mode or not is actually
dependent on where you place the device. If it is placed into
a PCIe root port, then it is modern-only. If it is placed into
a PCI bridge, then it is transitional still.

> Legacy mode virtio devices aren't really necessary any more, and are

Legacy mode is required for RHEL-6 which has not reached EOL yet.

> 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.


The challenge I see with pseries, as compared to x86 is around
how apps deal with mgmt / guest setup.  With x86 there are
distinct machine types i440fx / q35, so mgmt apps could decide
what todo based on the chipset & device support. eg they can
determine whether the guest supports PCIe at all, and they
can determine whether the guest supports virtio-1.0. Thus
they can decide between three options

 - Use i440fx
 - Use q35 with placement in PCI bridge
 - Use q35 with placement in PCIe root port

These rules applies no matter what version of q35/i440fx
is in use.

With PPC, we're changing behaviour of the existing pseries
machine type in a minor version. Management apps need to
avoid creating logic that depends on a specific minor version
because these version numbers are all changed by downstream
distro vendors. IOW, as a comparison to x86, this change is
akin to altering behaviour of the i440fx machine type so
that it disables legacy mode despite still being PCI, and
not PCIe.

Is there any liklihood we'll ever introduce a true PCIe
based machine type for PPC, so we get something much
closer to x86/aarch64 machine types in terms of PCIe
architecture ?

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

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|