Re: [Qemu-devel] [PATCH V2] hw/virtio-pci: fix virtio behaviour on modern (PCIe) machines

[Marcel Apfelbaum]

On 07/20/2016 01:56 AM, Michael S. Tsirkin wrote:
> On Tue, Jul 19, 2016 at 09:42:58PM +0300, Marcel Apfelbaum wrote:
> > Modern machines are expected to be used by newer setups with
> > modern guests aiming the use of the latest features.
> >
> > Enable modern and disable legacy for virtio devices
> > plugged into PCIe ports (Root ports or Downstream ports).
> > Using the Virtio 1 mode will remove the limitation
> > of the number of devices that can be attached to a machine
> > by removing the need for the IO BAR.
> >
> > Convert 'disable-modern' and 'disable-legacy' properties to OnOffAuto
> > with default Auto.
> >
> > Signed-off-by: Marcel Apfelbaum <address@hidden>
> > ---
> >
> > Hi,
> >
> > v1 -> v2:
> >    - Stick to existing defaults for old machine types (Michael S. Tsirkin)
> >
> > If everyone agrees, I am thinking about getting it into 2.7
> > to avoid the ~15 virtio devices limitation per machine.
> >
> > Notes:
> >     - The non PCIe machines behaviour should remain the same.
> >     - I hope is OK to make the disable-* properties OnOffAuto. Previous setups
> >       using them can be affected,

Hi Michael,

> why? could you explain pls? isn't onoffauto compatible with bit
> properties?

Yes, indeed. the value of 'Off' is replaced by 'Auto', but the semantics
for the names "on/off" remain.

> > but libvirt is not using them yet (as far as I know)
> >     - My tests were limited to checking all possible disable-* configurations 
> > (and make check for all archs)
> >
> > Thanks,
> > Marcel
> >
> >   hw/virtio/virtio-pci.c | 31 ++++++++++++++++++++++++-------
> >   hw/virtio/virtio-pci.h |  2 ++
> >   include/hw/compat.h    |  8 ++++++++
> >   3 files changed, 34 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 2b34b43..ec9e84f 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1716,6 +1716,8 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error 
> > **errp)
> >   {
> >       VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
> >       VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev);
> > +    bool pcie_port = (pci_bus_is_express(pci_dev->bus) &&
> > +                      !pci_bus_is_root(pci_dev->bus));
>
>
> drop the extra outside ()  please.

OK

> >       /*
> >        * virtio pci bar layout used by default.
> > @@ -1766,8 +1768,23 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error 
> > **errp)
> >
> >       address_space_init(&proxy->modern_as, &proxy->modern_cfg, 
> > "virtio-pci-cfg-as");
> >
> > -    if (pci_is_express(pci_dev) && pci_bus_is_express(pci_dev->bus) &&
> > -        !pci_bus_is_root(pci_dev->bus)) {
> > +    if ((pcie_port && (proxy->disable_modern == ON_OFF_AUTO_AUTO))
> > +         || (proxy->disable_modern == ON_OFF_AUTO_OFF)) {
>
> drop the () around == - logic and math mix naturally in C.

OK

> also, pls put || at end of line, not at the beginning of
> continuation. this way you can see there is
> continuation as you read the code naturally.

The line length would be more than 80, so I needed to chose a way to "lose".
I'll add || at the end of the line, sure.

> > +        proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_MODERN;
> > +        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > +    } else {
> > +        proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_MODERN;
> > +        pci_dev->cap_present &= ~QEMU_PCI_CAP_EXPRESS;
> > +    }
> > +
> > +    if ((pcie_port && (proxy->disable_legacy == ON_OFF_AUTO_AUTO))
> > +        || (proxy->disable_legacy == ON_OFF_AUTO_ON)) {
> > +        proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY;
> > +    } else {
> > +        proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_LEGACY;
> > +    }
> > +
>
> So this is a bit messy.
>
> Can we do:
>
>         if (pcie_port)
>                 default_disable_legacy = false;
>         else
>                 default_disable_legacy = true;
>
> now
>
>         if (proxy->disable_legacy == ON_OFF_AUTO_ON ||
>                 (proxy->disable_legacy == ON_OFF_AUTO_AUTO && 
> default_disable_legacy))
>
>                 proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY;
>         else
>                 proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_LEGACY;
>
> ?

We have 2 *different* logic:
 -disable-modern: make it *false* if plugged into a pcie_port or the user 
requested it.
    In this case update QEMU_PCI_CAP_EXPRESS flag accordingly.
 -disble-legacy: make it *true* if plugged into a pcie_port or the user 
requested it.

If the code seems complicated I'll try to make it more readable.
Maybe I'll add a switch for each flag.


> Also, I wonder how does this interact with devices that play with
> these flags themselves, like virtio gpu?
> I guess we could just set VIRTIO_PCI_FLAG_DISABLE_LEGACY, avoid clearing it.

I thought this method is called before any other init call sites. (like virtio 
gpu or serial)
I'll double-check and update accordingly.

> Setting disable modern might be problematic for same reason, not sure what
> to do about that one.
>
> Did I miss anything?

I'll check this code runs before the other specific devices init code.
If it runs before, we have no problem here, otherwise I'll need to re-think
this patch.

Thank you for the review and I'll provide the answers in the next version.
Marcel

> > +    if (pcie_port && pci_is_express(pci_dev)) {
> >           int pos;
> >
> >           pos = pcie_endpoint_cap_init(pci_dev, 0);
> > @@ -1821,10 +1838,10 @@ static void virtio_pci_reset(DeviceState *qdev)
> >   static Property virtio_pci_properties[] = {
> >       DEFINE_PROP_BIT("virtio-pci-bus-master-bug-migration", VirtIOPCIProxy, 
> > flags,
> >                       VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT, false),
> > -    DEFINE_PROP_BIT("disable-legacy", VirtIOPCIProxy, flags,
> > -                    VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false),
> > -    DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags,
> > -                    VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true),
> > +    DEFINE_PROP_ON_OFF_AUTO("disable-legacy", VirtIOPCIProxy, disable_legacy,
> > +                            ON_OFF_AUTO_AUTO),
> > +    DEFINE_PROP_ON_OFF_AUTO("disable-modern", VirtIOPCIProxy, disable_modern,
> > +                            ON_OFF_AUTO_AUTO),
> >       DEFINE_PROP_BIT("migrate-extra", VirtIOPCIProxy, flags,
> >                       VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT, true),
> >       DEFINE_PROP_BIT("modern-pio-notify", VirtIOPCIProxy, flags,
> > @@ -1841,7 +1858,7 @@ static void virtio_pci_dc_realize(DeviceState *qdev, 
> > Error **errp)
> >       PCIDevice *pci_dev = &proxy->pci_dev;
> >
> >       if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
> > -        !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)) {
> > +        !(proxy->disable_modern == ON_OFF_AUTO_ON)) {
> >           pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> >       }
> >
> > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > index e4548c2..4f219d4 100644
> > --- a/hw/virtio/virtio-pci.h
> > +++ b/hw/virtio/virtio-pci.h
> > @@ -144,6 +144,8 @@ struct VirtIOPCIProxy {
> >       uint32_t modern_mem_bar;
> >       int config_cap;
> >       uint32_t flags;
> > +    OnOffAuto disable_modern;
> > +    OnOffAuto disable_legacy;
> >       uint32_t class_code;
> >       uint32_t nvectors;
> >       uint32_t dfselect;
> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 9914e7a..1531399 100644
> > --- a/include/hw/compat.h
> > +++ b/include/hw/compat.h
> > @@ -6,6 +6,14 @@
> >           .driver   = "virtio-mmio",\
> >           .property = "format_transport_address",\
> >           .value    = "off",\
> > +    },{\
> > +        .driver   = "virtio-pci",\
> > +        .property = "disable-modern",\
> > +        .value    = "on",\
> > +    },{\
> > +        .driver   = "virtio-pci",\
> > +        .property = "disable-legacy",\
> > +        .value    = "off",\
> >       },
> >
> >   #define HW_COMPAT_2_5 \
> > --
> > 2.4.3