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

[Michael S. Tsirkin]

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,

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


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

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

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.



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

?


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.

Setting disable modern might be problematic for same reason, not sure what
to do about that one.

Did I miss anything?

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