From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50979) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a4Aaw-0000uv-J3 for qemu-devel@nongnu.org; Wed, 02 Dec 2015 11:49:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a4Aat-0000XF-MW for qemu-devel@nongnu.org; Wed, 02 Dec 2015 11:49:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39144) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a4Aat-0000XB-F7 for qemu-devel@nongnu.org; Wed, 02 Dec 2015 11:49:27 -0500 Date: Wed, 2 Dec 2015 18:49:24 +0200 From: "Michael S. Tsirkin" Message-ID: <20151202184610-mutt-send-email-mst@redhat.com> References: <1449066801-3002-1-git-send-email-shmulik.ladkani@ravellosystems.com> <565F0A20.8050507@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <565F0A20.8050507@redhat.com> 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: Marcel Apfelbaum Cc: Shmulik Ladkani , qemu-devel@nongnu.org On Wed, Dec 02, 2015 at 05:11:28PM +0200, Marcel Apfelbaum wrote: > 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. Problem is, if this happens migration breaks as we changed config space size from 2.4. > Reviewed-by: Marcel Apfelbaum > > Thanks, > Marcel