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

[Marcel Apfelbaum]

On 12/01/2015 12:29 PM, Shmulik Ladkani wrote:
> 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.

Hi,
Thanks for catching it. It is a good time for a fix since we don't have yet
an actual PCIe express capability.

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

Give it a try, sure, but you don't need to expose pci_qdev_realize,
you can 'hijack' parent's realize method before replacing it.

Thanks,
Marcel


> Regards,
> Shmulik