From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42885) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a494D-0002Jc-Ea for qemu-devel@nongnu.org; Wed, 02 Dec 2015 10:11:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a494A-0006Zf-NZ for qemu-devel@nongnu.org; Wed, 02 Dec 2015 10:11:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58745) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a494A-0006ZG-Fo for qemu-devel@nongnu.org; Wed, 02 Dec 2015 10:11:34 -0500 References: <1449066801-3002-1-git-send-email-shmulik.ladkani@ravellosystems.com> From: Marcel Apfelbaum Message-ID: <565F0A20.8050507@redhat.com> Date: Wed, 2 Dec 2015 17:11:28 +0200 MIME-Version: 1.0 In-Reply-To: <1449066801-3002-1-git-send-email-shmulik.ladkani@ravellosystems.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 for-2.5] virtio-pci: Set the QEMU_PCI_CAP_EXPRESS capability early in its DeviceClass realize method List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Shmulik Ladkani , "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org 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 > --- > > 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/qemu-devel@nongnu.org/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 Thanks, Marcel