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

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

[Shmulik Ladkani]

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.ladkani@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;
 
-- 
1.9.1

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

[Marcel Apfelbaum]

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.ladkani@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.

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

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

[Michael S. Tsirkin]

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.ladkani@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 <marcel@redhat.com>
> 
> Thanks,
> Marcel

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

[Michael S. Tsirkin]

On Wed, Dec 02, 2015 at 04:33:21PM +0200, 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.ladkani@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);
>

Now here you want
+  else
+        pci_dev->cap_present &= ~QEMU_PCI_CAP_EXPRESS;
  

This will fix the bug Marcel noticed.

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

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

[Shmulik Ladkani]

Hi Michael,

On Wed, 2 Dec 2015 18:49:24 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2015 at 05:11:28PM +0200, Marcel Apfelbaum wrote:
> > 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.

Correct me if wrong, but as long as a pre 2.5 hardware is used, Marcel's
x-disable-pcie gets on (see HW_COMPAT_2_4), thus the device will not be
a "pci_is_express", and it will have a small config space size. Isn't it
so?

Regards,
Shmulik

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

[Michael S. Tsirkin]

On Wed, Dec 02, 2015 at 07:12:47PM +0200, Shmulik Ladkani wrote:
> Hi Michael,
> 
> On Wed, 2 Dec 2015 18:49:24 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2015 at 05:11:28PM +0200, Marcel Apfelbaum wrote:
> > > 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.
> 
> Correct me if wrong, but as long as a pre 2.5 hardware is used, Marcel's
> x-disable-pcie gets on (see HW_COMPAT_2_4), thus the device will not be
> a "pci_is_express", and it will have a small config space size. Isn't it
> so?
> 
> Regards,
> Shmulik

OK. Still, I do not want to migrate 4k of config space
for pci devices.

-- 
MST

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

[Marcel Apfelbaum]

On 12/02/2015 07:19 PM, Michael S. Tsirkin wrote:
> On Wed, Dec 02, 2015 at 07:12:47PM +0200, Shmulik Ladkani wrote:
>> Hi Michael,
>>
>> On Wed, 2 Dec 2015 18:49:24 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>> On Wed, Dec 02, 2015 at 05:11:28PM +0200, Marcel Apfelbaum wrote:
>>>> 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.
>>
>> Correct me if wrong, but as long as a pre 2.5 hardware is used, Marcel's
>> x-disable-pcie gets on (see HW_COMPAT_2_4), thus the device will not be
>> a "pci_is_express", and it will have a small config space size. Isn't it
>> so?
>>
>> Regards,
>> Shmulik
>
> OK. Still, I do not want to migrate 4k of config space
> for pci devices.
>

Shmulik, will you resend the patch with this minor change, or you prefer me to do it? 2.5 closes soon.

Thanks,
Marcel

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

[Michael S. Tsirkin]

On Wed, Dec 02, 2015 at 07:21:20PM +0200, Marcel Apfelbaum wrote:
> On 12/02/2015 07:19 PM, Michael S. Tsirkin wrote:
> >On Wed, Dec 02, 2015 at 07:12:47PM +0200, Shmulik Ladkani wrote:
> >>Hi Michael,
> >>
> >>On Wed, 2 Dec 2015 18:49:24 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>>On Wed, Dec 02, 2015 at 05:11:28PM +0200, Marcel Apfelbaum wrote:
> >>>>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.
> >>
> >>Correct me if wrong, but as long as a pre 2.5 hardware is used, Marcel's
> >>x-disable-pcie gets on (see HW_COMPAT_2_4), thus the device will not be
> >>a "pci_is_express", and it will have a small config space size. Isn't it
> >>so?
> >>
> >>Regards,
> >>Shmulik
> >
> >OK. Still, I do not want to migrate 4k of config space
> >for pci devices.
> >
> 
> Shmulik, will you resend the patch with this minor change, or you prefer me to do it? 2.5 closes soon.
> 
> Thanks,
> Marcel

Soon is not the right word. Closing in 10.. 9.. 8.. 

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

[Shmulik Ladkani]

Hi,

On Wed, 2 Dec 2015 19:21:20 +0200 Marcel Apfelbaum <marcel@redhat.com> wrote:
> Shmulik, will you resend the patch with this minor change, or you prefer me
> to do it? 2.5 closes soon.

Sent a v3 with Michael's addition.

Havn't had time to runtime test the new "else" part: meaning, if the
virtio pci device is NOT on a pcie bus.
Appreciate if you can take this.

Regards,
Shmulik