Re: [Qemu-devel] [PATCH V5] hw/virtio: Add PCIe capability to virtio devices

[Shmulik Ladkani]

Hi,

On Tue, 10 Nov 2015 13:41:29 +0200, address@hidden wrote:
> The virtio devices are converted to PCI-Express
> if they are plugged into a PCI-Express bus and
> the 'modern' protocol is enabled.
>
> @@ -1592,6 +1592,26 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
> Error **errp)
>  
> +    if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE)
> +        && !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)
> +        && pci_bus_is_express(pci_dev->bus)
> +        && !pci_bus_is_root(pci_dev->bus)) {
> +        int pos;
> +
> +        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;

Setting QEMU_PCI_CAP_EXPRESS here in 'virtio_pci_realize' is too late.

This is since 'pci_qdev_realize' (DeviceClass.realize of TYPE_PCI_DEVICE)
is invoked prior the PCIDeviceClass's specific 'realize' method
(virtio_pci_realize in this case).

During 'pci_qdev_realize' (specifically, within do_pci_register_device),
the QEMU_PCI_CAP_EXPRESS gets tested indirectly, when pci_is_express
and pci_config_size helpers are called.

For example: 'pci_config_alloc' uses 'pci_config_size' which relies on
QEMU_PCI_CAP_EXPRESS property.

Since virtio_pci sets QEMU_PCI_CAP_EXPRESS *after* pci_qdev_realize
has finished, we end up having an insufficient pci config space
allocated for the virtio "pcie" device.

May I suggest the following:
- Expose 'pci_qdev_realize'
- Have 'virtio_pci_class_init' arm it's own dc->realize,
  which will first set 'QEMU_PCI_CAP_EXPRESS' flag as needed,
  and then call 'pci_qdev_realize'
- Now, in 'virtio_pci_realize' we may use 'pci_is_express' instead of
  directly checking the proxy->flags

If this sounds ok, I'll submit a fix.

Regards,
Shmulik