On Wed, Dec 02, 2015 at 04:33:21PM +0200, 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 <shmulik.ladk...@ravellosystems.com>
> ---
>
> 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);
>
Now here you want
+ else
+ pci_dev->cap_present &= ~QEMU_PCI_CAP_EXPRESS;
This will fix the bug Marcel noticed.
> @@ -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;
>
> --
> 1.9.1
