Re: [Qemu-devel] [PATCH V3] hw/virtio-pci: fix virtio behaviour

[Marcel Apfelbaum <marcel@redhat.com>]

On 07/21/2016 11:18 PM, Michael S. Tsirkin wrote:
> On Wed, Jul 20, 2016 at 06:28:21PM +0300, Marcel Apfelbaum wrote:
>> Enable transitional virtio devices by default.
>> Enable virtio-1.0 for devices plugged into
>
> disable legacy is better, I agree.
>
>> 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.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>
> I think you also want to add some comment with a description explaining
> *why* you are disabling legacy for these specific devices.

Hi Michael,
I thought the above paragraph in the commit message explains it:

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


What do you think I should add?

Thanks,
Marcel

>
>
>> ---
>>
>> Hi,
>>
>> v2 -> v3:
>>    - Various code tweaks to simplify if statements (Michael)
>>    - Enable virtio modern by default (Gerd and Cornelia)
>>    - Replace virtio flags with actual fields (Gerd)
>>    - Wrappers for more readable code
>>
>> 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.
>>
>> My tests were limited to checking all possible disable-* configurations (and make check for all archs)
>>
>> Thanks,
>> Marcel
>>
>>   hw/display/virtio-gpu-pci.c |  4 +---
>>   hw/display/virtio-vga.c     |  4 +---
>>   hw/virtio/virtio-pci.c      | 34 ++++++++++++++++++----------------
>>   hw/virtio/virtio-pci.h      | 21 +++++++++++++++++----
>>   include/hw/compat.h         |  8 ++++++++
>>   5 files changed, 45 insertions(+), 26 deletions(-)
>>
>> diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
>> index a71b230..34a724c 100644
>> --- a/hw/display/virtio-gpu-pci.c
>> +++ b/hw/display/virtio-gpu-pci.c
>> @@ -30,9 +30,7 @@ static void virtio_gpu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
>>       int i;
>>
>>       qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
>> -    /* force virtio-1.0 */
>> -    vpci_dev->flags &= ~VIRTIO_PCI_FLAG_DISABLE_MODERN;
>> -    vpci_dev->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY;
>> +    virtio_pci_force_virtio_1(vpci_dev);
>>       object_property_set_bool(OBJECT(vdev), true, "realized", errp);
>>
>>       for (i = 0; i < g->conf.max_outputs; i++) {
>> diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
>> index 315b7fc..5b510a1 100644
>> --- a/hw/display/virtio-vga.c
>> +++ b/hw/display/virtio-vga.c
>> @@ -134,9 +134,7 @@ static void virtio_vga_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
>>
>>       /* init virtio bits */
>>       qdev_set_parent_bus(DEVICE(g), BUS(&vpci_dev->bus));
>> -    /* force virtio-1.0 */
>> -    vpci_dev->flags &= ~VIRTIO_PCI_FLAG_DISABLE_MODERN;
>> -    vpci_dev->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY;
>> +    virtio_pci_force_virtio_1(vpci_dev);
>>       object_property_set_bool(OBJECT(g), true, "realized", &err);
>>       if (err) {
>>           error_propagate(errp, err);
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 2b34b43..11cd634 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -161,7 +161,7 @@ static bool virtio_pci_modern_state_needed(void *opaque)
>>   {
>>       VirtIOPCIProxy *proxy = opaque;
>>
>> -    return !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
>> +    return virtio_pci_modern(proxy);
>>   }
>>
>>   static const VMStateDescription vmstate_virtio_pci_modern_state = {
>> @@ -300,8 +300,8 @@ static int virtio_pci_ioeventfd_assign(DeviceState *d, EventNotifier *notifier,
>>       VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
>>       VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>>       VirtQueue *vq = virtio_get_queue(vdev, n);
>> -    bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY);
>> -    bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
>> +    bool legacy = virtio_pci_legacy(proxy);
>> +    bool modern = virtio_pci_modern(proxy);
>>       bool fast_mmio = kvm_ioeventfd_any_length_enabled();
>>       bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY;
>>       MemoryRegion *modern_mr = &proxy->notify.mr;
>> @@ -1576,8 +1576,8 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>>   {
>>       VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>>       VirtioBusState *bus = &proxy->bus;
>> -    bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY);
>> -    bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
>> +    bool legacy = virtio_pci_legacy(proxy);
>> +    bool modern = virtio_pci_modern(proxy);
>>       bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY;
>>       uint8_t *config;
>>       uint32_t size;
>> @@ -1696,7 +1696,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
>>   static void virtio_pci_device_unplugged(DeviceState *d)
>>   {
>>       VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>> -    bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
>> +    bool modern = virtio_pci_modern(proxy);
>>       bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY;
>>
>>       virtio_pci_stop_ioeventfd(proxy);
>> @@ -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);
>>
>>       /*
>>        * virtio pci bar layout used by default.
>> @@ -1766,8 +1768,11 @@ 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 (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
>> +        proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>> +    }
>> +
>> +    if (pcie_port && pci_is_express(pci_dev)) {
>>           int pos;
>>
>>           pos = pcie_endpoint_cap_init(pci_dev, 0);
>> @@ -1821,10 +1826,9 @@ 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_BOOL("disable-modern", VirtIOPCIProxy, disable_modern, false),
>>       DEFINE_PROP_BIT("migrate-extra", VirtIOPCIProxy, flags,
>>                       VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT, true),
>>       DEFINE_PROP_BIT("modern-pio-notify", VirtIOPCIProxy, flags,
>> @@ -1841,7 +1845,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)) {
>> +        virtio_pci_modern(proxy)) {
>>           pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>>       }
>>
>> @@ -2303,9 +2307,7 @@ static void virtio_input_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
>>       DeviceState *vdev = DEVICE(&vinput->vdev);
>>
>>       qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
>> -    /* force virtio-1.0 */
>> -    vpci_dev->flags &= ~VIRTIO_PCI_FLAG_DISABLE_MODERN;
>> -    vpci_dev->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY;
>> +    virtio_pci_force_virtio_1(vpci_dev);
>>       object_property_set_bool(OBJECT(vdev), true, "realized", errp);
>>   }
>>
>> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
>> index e4548c2..25fbf8a 100644
>> --- a/hw/virtio/virtio-pci.h
>> +++ b/hw/virtio/virtio-pci.h
>> @@ -61,8 +61,6 @@ typedef struct VirtioBusClass VirtioPCIBusClass;
>>   enum {
>>       VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT,
>>       VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT,
>> -    VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT,
>> -    VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT,
>>       VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT,
>>       VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT,
>>       VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT,
>> @@ -77,8 +75,6 @@ enum {
>>   #define VIRTIO_PCI_FLAG_USE_IOEVENTFD   (1 << VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT)
>>
>>   /* virtio version flags */
>> -#define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)
>> -#define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)
>>   #define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT)
>>
>>   /* migrate extra state */
>> @@ -144,6 +140,8 @@ struct VirtIOPCIProxy {
>>       uint32_t modern_mem_bar;
>>       int config_cap;
>>       uint32_t flags;
>> +    bool disable_modern;
>> +    OnOffAuto disable_legacy;
>>       uint32_t class_code;
>>       uint32_t nvectors;
>>       uint32_t dfselect;
>> @@ -158,6 +156,21 @@ struct VirtIOPCIProxy {
>>       VirtioBusState bus;
>>   };
>>
>> +static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy)
>> +{
>> +    return !proxy->disable_modern;
>> +}
>> +
>> +static inline bool virtio_pci_legacy(VirtIOPCIProxy *proxy)
>> +{
>> +    return proxy->disable_legacy == ON_OFF_AUTO_OFF;
>> +}
>> +
>> +static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy)
>> +{
>> +    proxy->disable_modern = false;
>> +    proxy->disable_legacy = ON_OFF_AUTO_ON;
>> +}
>>
>>   /*
>>    * virtio-scsi-pci: This extends VirtioPCIProxy.
>> 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