[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 1/2] spapr: Disable legacy virtio devices for pseries-5.0
From: |
Greg Kurz |
Subject: |
Re: [PATCH v3 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later |
Date: |
Tue, 10 Mar 2020 13:24:50 +0100 |
On Tue, 10 Mar 2020 07:03:34 -0400
"Michael S. Tsirkin" <address@hidden> wrote:
> On Tue, Mar 10, 2020 at 10:43:29AM +0100, Greg Kurz wrote:
> > 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";
>
> Does this actually help? There's no type virtio-pci-transitional, is
> there?
>
Oops you're right. There's no such type and it doesn't help in
the end... I should have tried that with -global before posting
-global virtio-pci-transitional.disable-legacy=on
qemu-system-ppc64: warning: global virtio-pci-transitional.disable-legacy has
invalid class name
Maybe worth adding a warning as well when done from the machine code, but
this is another story. Sorry for the noise.
> >
> > but there's probably a better way to address this.
> >
> > MST, Any suggestion ?
>
> Hmm I'm not sure how to fix this properly. The only idea that comes
> to mind is a new internal-only "x-disable-legacy" that would be on virtio-pci,
> duplicating functionality of disable-legacy but intended for
> globals like this.
>
Maybe but Daniel P. Berrangé jumped in and has some arguments
against what we're trying to achieve with this series...
A suivre.
>
>
> > > > 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);
> > >
>
- [PATCH v3 2/2] spapr: Enable virtio iommu_platform=on by default, (continued)
[PATCH v3 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later, David Gibson, 2020/03/04
Re: [PATCH v3 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later, Daniel P . Berrangé, 2020/03/10
Upstream QEMU guest support policy ? Re: [PATCH v3 0/2] spapr: Use vIOMMU translation for virtio by default, Daniel P . Berrangé, 2020/03/10