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 <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);
> >
> >@@ -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 <mar...@redhat.com>
>
> Thanks,
> Marcel
