Re: [Qemu-devel] [PATCH v2 for-2.5] virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its DeviceClass realize method

[Marcel Apfelbaum]

On 12/02/2015 04:33 PM, Shmulik Ladkani wrote:
> In 1811e64 'hw/virtio: Add PCIe capability to virtio devices', the
> QEMU_PCI_CAP_EXPRESS capability was added to virtio's pci_dev, within
> 'virtio_pci_realize' - the pci device object realization method.
>
> This occurs to late, as 'pci_qdev_realize' (DeviceClass.realize of
> TYPE_PCI_DEVICE) has already been called, without knowing that the
> device instance is indeed an "express" instance, thus allocating
> insufficient pci config space.
>
> As a result, device may crash upon attempt to write to the PCIE config
> space.
>
> Fix, by arming the QEMU_PCI_CAP_EXPRESS capability early in virtio-pci's
> own DeviceClass realize method.
>
> This also makes code cleaner, as 'virtio_pci_realize' may now access the
> 'pci_is_express' predicate when needed.
>
> Signed-off-by: Shmulik Ladkani <address@hidden>
> ---
>
> Since v1: naming change, as suggested by Marcel Apfelbaum
>
>   hw/virtio/virtio-pci.c | 24 +++++++++++++++++++-----
>   hw/virtio/virtio-pci.h |  1 +
>   2 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index dd48562..67f4003 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1814,13 +1814,10 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
> Error **errp)
>
>       address_space_init(&proxy->modern_as, &proxy->modern_cfg, 
> "virtio-pci-cfg-as");
>
> -    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)) {
> +    if (pci_is_express(pci_dev) && pci_bus_is_express(pci_dev->bus) &&
> +        !pci_bus_is_root(pci_dev->bus)) {
>           int pos;
>
> -        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>           pos = pcie_endpoint_cap_init(pci_dev, 0);
>           assert(pos > 0);
>
> @@ -1879,10 +1876,25 @@ static Property virtio_pci_properties[] = {
>       DEFINE_PROP_END_OF_LIST(),
>   };
>
> +static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
> +{
> +    VirtioPCIClass *vpciklass = VIRTIO_PCI_GET_CLASS(qdev);
> +    VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
> +    PCIDevice *pci_dev = &proxy->pci_dev;
> +
> +    if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
> +        !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)) {
> +        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> +    }
> +
> +    vpciklass->parent_dc_realize(qdev, errp);
> +}
> +
>   static void virtio_pci_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    VirtioPCIClass *vpciklass = VIRTIO_PCI_CLASS(klass);
>
>       dc->props = virtio_pci_properties;
>       k->realize = virtio_pci_realize;
> @@ -1890,6 +1902,8 @@ static void virtio_pci_class_init(ObjectClass *klass, 
> void *data)
>       k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
>       k->revision = VIRTIO_PCI_ABI_VERSION;
>       k->class_id = PCI_CLASS_OTHERS;
> +    vpciklass->parent_dc_realize = dc->realize;
> +    dc->realize = virtio_pci_dc_realize;
>       dc->reset = virtio_pci_reset;
>   }
>
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index ffb74bb..a104ff2 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -105,6 +105,7 @@ typedef struct {
>
>   typedef struct VirtioPCIClass {
>       PCIDeviceClass parent_class;
> +    DeviceRealize parent_dc_realize;
>       void (*realize)(VirtIOPCIProxy *vpci_dev, Error **errp);
>   } VirtioPCIClass;

Hi Michael,

The only thing I want to mention here, (see earlier discussion: 
https://www.mail-archive.com/address@hidden/msg338963.html)
is that in some cases the PCI config space will have PCIe length, even if the 
device is not express.

To be more precise, the only interesting scenario is when we plug a virtio 
device directly into
the root complex, in this case we'll have a PCI device with a PCIe config space.

However this happens for other devices as well, it looks like a common practice.

Reviewed-by: Marcel Apfelbaum <address@hidden>

Thanks,
Marcel