From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48624) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a4Be4-0004Ha-1P for qemu-devel@nongnu.org; Wed, 02 Dec 2015 12:56:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a4Be0-0007YW-CU for qemu-devel@nongnu.org; Wed, 02 Dec 2015 12:56:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49859) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a4Be0-0007YS-42 for qemu-devel@nongnu.org; Wed, 02 Dec 2015 12:56:44 -0500 References: <1449078547-8557-1-git-send-email-shmulik.ladkani@ravellosystems.com> From: Marcel Apfelbaum Message-ID: <565F30D5.7080205@redhat.com> Date: Wed, 2 Dec 2015 19:56:37 +0200 MIME-Version: 1.0 In-Reply-To: <1449078547-8557-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 v3 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 07:49 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 v2: Negate QEMU_PCI_CAP_EXPRESS capability if not an express > endpoint, as suggested by Michael S. Tsirkin > Since v1: Naming change, as suggested by Marcel Apfelbaum > > hw/virtio/virtio-pci.c | 30 +++++++++++++++++++++++++----- > hw/virtio/virtio-pci.h | 1 + > 2 files changed, 26 insertions(+), 5 deletions(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index dd485629da..94667e6256 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); > > @@ -1832,6 +1829,12 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > * PCI Power Management Interface Specification. > */ > pci_set_word(pci_dev->config + pos + PCI_PM_PMC, 0x3); > + } else { > + /* > + * make future invocations of pci_is_express() return false > + * and pci_config_size() return PCI_CONFIG_SPACE_SIZE. > + */ > + pci_dev->cap_present &= ~QEMU_PCI_CAP_EXPRESS; > } > > virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); > @@ -1879,10 +1882,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 +1908,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 ffb74bb908..a104ff2072 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; > > Reviewed-by: Marcel Apfelbaum I just finished to prepare the same patch (with the same change), and since is exactly the same my tests hold: Tested-by: Marcel Apfelbaum Thanks, Marcel