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

[David Gibson]

On Tue, Mar 10, 2020 at 11:56:11AM +0000, Daniel P. Berrangé wrote:
> 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.

Yes, I'm aware.

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

Yeah.. I'm concerned about this, but I'm not sure what to do about it.

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

Yeah.. x86 also has the advantage of enough visibility that it can
reasonbly easily get mgmt layers to do stuff about it.  This is much
harder for POWER :/.

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

It doesn't really make sense to do so.  For x86 - or more strictly for
pc - the change from PCI to PCIe is a pretty fundamental system change
with affects in lots of places, which makes a whole new versioned
series of machine types a reasonable option.

For pseries - that's not really the case.  PCI under PAPR is
paravirtualized, and it always has been.  The interface we're matching
is not real hardware, but the PAPR spec and to a lesser extent the
existing PowerVM implementation of it.

[Aside: you've made a subtle but common x86-centric assumption above
 that there's only one important platform design per ISA.  There
 is a real PCIe based PPC  machine type in "pnv" (and maybe some of
 the embedded ones as well), but that's not the environment we care
 about for guests in production, since we can't use KVM with it]

What PAPR has is an odd hybrid - individual devices can be PCIe (we
have calls to access extended config space) - but the overall bus
structure is more-or-less like vanilla PCI.

I think it would be possible to kind of expose a more PCIe like
structure, but a) it would be weirdly artificial, b) it doesn't match
the PAPR interfaces very well, c) it would make our behaviour
different from PowerVM.

It would certainly be possible to better handle PCIe devices on a root
bus.  That's been on my todo someday list for ages, but I've kept
putting it off because the tangible benefits are pretty minimal.

Note that several things that I believe are now in the PCIe spec, but
really derive more from PC legacy considerations, don't apply at all
for PAPR.  e.g. there's no meaningful distinction between integrated
and slotted devices, multiple independent host bridges is routine and
doesn't require any (virtual) hardware visible domain numbers.

> > 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);
> 
> Regards,
> Daniel

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

signature.asc
Description: PGP signature