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

[Marcel Apfelbaum <marcel@redhat.com>]

On 07/19/2016 08:47 PM, Michael S. Tsirkin wrote:
> On Tue, Jul 19, 2016 at 07:45:44PM +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 <marcel@redhat.com>
>
>
> Sounds good, but I think we need to stick to existing
> defaults for old machine types.
> Pls add compat entries accordingly.
>

Added and posted v2. Thanks for the quick review!
Marcel

>> ---
>>
>> Hi,
>>
>> 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, 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 ++
>>   2 files changed, 26 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));
>>
>>       /*
>>        * 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)) {
>> +        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;
>> +    }
>> +
>> +    if (pcie_port && pci_is_express(pci_dev)) {
>>           int pos;
>>
>>           pos = pcie_endpoint_cap_init(pci_dev, 0);
>
> down the road, I really think we need a new type of
> property that does not need this manual code.
> E.g. specify a location to store the bit.
>
> This can wait though.
>
>
>> @@ -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;
>> --
>> 2.4.3