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

[Marcel Apfelbaum]

On 12/01/2015 06:23 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 <shmulik.ladk...@ravellosystems.com>
---
  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..cee1b7b 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;

Hi,

Here you should check only for 'pci_is_express(pci_dev)' .

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

And here you should also check:
     pci_bus_is_express(pci_dev->bus) && !pci_bus_is_root(pci_dev->bus))

The reason is the device becomes express only if *all* the conditions are met.

+    }
+
+    vpciklass->saved_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->saved_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..f18e948 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 saved_dc_realize;

I would change the name to parent_realize :)

Also please add "for-2.5" to prefix.

Thanks for posting it!
Marcel

      void (*realize)(VirtIOPCIProxy *vpci_dev, Error **errp);
  } VirtioPCIClass;