[Qemu-devel] [PATCH v3 0/3] add check for PCIe root ports and downstream ports

[Qemu-devel] [PATCH v3 0/3] add check for PCIe root ports and downstream ports

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Root ports and downstream ports of switches are the hot
pluggable ports in a PCI Express hierarchy.
PCI Express supports chip-to-chip interconnect, a PCIe link can
only connect one pci device/Switch/EndPoint or PCI-bridge.

7.3. Configuration Transaction Rules (PCI Express specification 3.0)
7.3.1. Device Number

Downstream Ports that do not have ARI Forwarding enabled must
associate only Device 0 with the device attached to the Logical Bus
representing the Link from the Port.

In QEMU, ARI Forwarding is enabled defualt at emulation of PCIe
ports. ARI Forwarding enable setting at firmware/OS Control handoff.
If the bit is Set when a non-ARI Device is present, the non-ARI
Device can respond to Configuration Space accesses under what it
interprets as being different Device Numbers, and its Functions can
be aliased under multiple Device Numbers, generally leading to
undesired behavior.

So, for pci devices attached in pcie root ports or downstream pots,
we shoud assure that its slot is non-zero. For pcie devcies, which
ARP capbility is not enabled, we also should assure that its slot
is non-zero.

Changes since v2:
 - make patch 1/3 more simpler and safer.(Hu Tao)
 - change check logic from pci.c to pcie.c and change function's name
 - judge devcies' ARI capbility instead of PCIe ports' ARI Forwarding
   (Michael)
 - add trivial patch 3/3
 - update patch's commit messages and code comments.

Thanks for your reviewing.
 
Changes since v1:
 - using object_dynamic_cast() instead of simple string comparing (Paolo)
 - add ARI Forwarding enable bit check
 - using pcie_cap_get_type() instead of simple string comparing (Marcel)
 - fix some other comments.

Gonglei (3):
  qdev: Introduce a function to get qbus's parent
  pcie: add check for ari capability of pcie devices
  pcie: remove confused comments

 hw/core/qdev.c         |  9 ++++++++
 hw/pci/pci.c           |  4 ++++
 hw/pci/pcie.c          | 59 +++++++++++++++++++++++++++++++++++++++++++-------
 include/hw/pci/pcie.h  |  1 +
 include/hw/qdev-core.h |  1 +
 5 files changed, 66 insertions(+), 8 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v3 1/3] qdev: Introduce a function to get qbus's parent

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

We may use it check type of qbus's parent.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/core/qdev.c         | 9 +++++++++
 include/hw/qdev-core.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index da1ba48..e42d313 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -616,6 +616,15 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
     return bus;
 }
 
+DeviceState *qbus_get_parent(BusState *bus)
+{
+    if (bus) {
+        return bus->parent;
+    }
+
+    return NULL;
+}
+
 static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
 {
     BusClass *bc = BUS_GET_CLASS(bus);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 0799ff2..760e726 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -301,6 +301,7 @@ typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
 void qbus_create_inplace(void *bus, size_t size, const char *typename,
                          DeviceState *parent, const char *name);
 BusState *qbus_create(const char *typename, DeviceState *parent, const char *name);
+DeviceState *qbus_get_parent(BusState *bus);
 /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion,
  *         < 0 if either devfn or busfn terminate walk somewhere in cursion,
  *           0 otherwise. */
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 2/3] pcie: add check for ari capability of pcie devices

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

In QEMU, ARI Forwarding is enabled defualt at emulation of PCIe
ports. ARI Forwarding enable setting at firmware/OS Control handoff.
If the bit is Set when a non-ARI Device is present, the non-ARI
Device can respond to Configuration Space accesses under what it
interprets as being different Device Numbers, and its Functions can
be aliased under multiple Device Numbers, generally leading to
undesired behavior.

So, for pci devices attached in pcie root ports or downstream pots,
we shoud assure that its slot is non-zero. For pcie devcies, which
ARP capbility is not enabled, we also should assure that its slot
is non-zero.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/pci/pci.c          |  4 ++++
 hw/pci/pcie.c         | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci/pcie.h |  1 +
 3 files changed, 56 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index daeaeac..a9b8c3e 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1769,6 +1769,10 @@ static int pci_qdev_init(DeviceState *qdev)
         }
     }
 
+    if (pcie_cap_ari_check(bus, pci_dev)) {
+        return -1;
+    }
+
     /* rom loading */
     is_default_rom = false;
     if (pci_dev->romfile == NULL && pc->romfile != NULL) {
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 1babddf..30dd481 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -633,3 +633,54 @@ void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn)
                         offset, PCI_ARI_SIZEOF);
     pci_set_long(dev->config + offset + PCI_ARI_CAP, (nextfn & 0xff) << 8);
 }
+
+int pcie_cap_ari_check(PCIBus *bus, PCIDevice *dev)
+{
+    Object *obj = OBJECT(bus);
+
+    if (pci_bus_is_root(bus)) {
+        return 0;
+    }
+
+    if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) {
+        DeviceState *parent = qbus_get_parent(BUS(obj));
+        PCIDevice *pci_dev = PCI_DEVICE(parent);
+        uint8_t port_type;
+        /*
+         * Root ports and downstream ports of switches are the hot
+         * pluggable ports in a PCI Express hierarchy.
+         * PCI Express supports chip-to-chip interconnect, a PCIe link can
+         * only connect one pci device/Switch/EndPoint or PCI-bridge.
+         *
+         * 7.3. Configuration Transaction Rules (PCI Express specification 3.0)
+         * 7.3.1. Device Number
+         *
+         * Downstream Ports that do not have ARI Forwarding enabled must
+         * associate only Device 0 with the device attached to the Logical Bus
+         * representing the Link from the Port.
+         *
+         * In QEMU, ARI Forwarding is enabled defualt at emulation of PCIe
+         * ports. ARI Forwarding enable setting at firmware/OS Control handoff.
+         * If the bit is Set when a non-ARI Device is present, the non-ARI
+           Device can respond to Configuration Space accesses under what it
+         * interprets as being different Device Numbers, and its Functions can
+         * be aliased under multiple Device Numbers, generally leading to
+         * undesired behavior.
+         */
+        port_type = pcie_cap_get_type(pci_dev);
+        if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
+            port_type == PCI_EXP_TYPE_ROOT_PORT) {
+            if (!pci_is_express(dev) ||
+                (pci_is_express(dev) &&
+                !pcie_find_capability(dev, PCI_EXT_CAP_ID_ARI))) {
+                if (PCI_SLOT(dev->devfn) != 0) {
+                    error_report("PCIe: non-ARI device can't be populated"
+                                 " in slot %d", PCI_SLOT(dev->devfn));
+                    return -1;
+                }
+            }
+        }
+    }
+
+    return 0;
+}
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index d139d58..ab2a44a 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -115,6 +115,7 @@ void pcie_add_capability(PCIDevice *dev,
                          uint16_t offset, uint16_t size);
 
 void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
+int pcie_cap_ari_check(PCIBus *bus, PCIDevice *dev);
 
 extern const VMStateDescription vmstate_pcie_device;
 
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 3/3] pcie: remove confused comments

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

The below functions is not allocation funcitons, but
find helper function. The only allocation function is
pcie_add_capability(), but its argument names is not the
same with the comments. So remove this comments which
make people confused.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/pci/pcie.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 30dd481..75b70d0 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -528,14 +528,6 @@ bool pcie_cap_is_arifwd_enabled(const PCIDevice *dev)
         PCI_EXP_DEVCTL2_ARI;
 }
 
-/**************************************************************************
- * pci express extended capability allocation functions
- * uint16_t ext_cap_id (16 bit)
- * uint8_t cap_ver (4 bit)
- * uint16_t cap_offset (12 bit)
- * uint16_t ext_cap_size
- */
-
 static uint16_t pcie_find_capability_list(PCIDevice *dev, uint16_t cap_id,
                                           uint16_t *prev_p)
 {
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v3 0/3] add check for PCIe root ports and downstream ports

[Gonglei (Arei)]

Ping... Michael? Thanks!


Best regards,
-Gonglei


> -----Original Message-----
> From: Gonglei (Arei)
> Sent: Monday, September 01, 2014 9:29 PM
> To: qemu-devel@nongnu.org
> Cc: mst@redhat.com; Huangweidong (C); pbonzini@redhat.com;
> afaerber@suse.de; imammedo@redhat.com; peter.crosthwaite@xilinx.com;
> Huangpeng (Peter); armbru@redhat.com; marcel.a@redhat.com; Luonengjun;
> Gonglei (Arei)
> Subject: [PATCH v3 0/3] add check for PCIe root ports and downstream ports
> Importance: High
> 
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Root ports and downstream ports of switches are the hot
> pluggable ports in a PCI Express hierarchy.
> PCI Express supports chip-to-chip interconnect, a PCIe link can
> only connect one pci device/Switch/EndPoint or PCI-bridge.
> 
> 7.3. Configuration Transaction Rules (PCI Express specification 3.0)
> 7.3.1. Device Number
> 
> Downstream Ports that do not have ARI Forwarding enabled must
> associate only Device 0 with the device attached to the Logical Bus
> representing the Link from the Port.
> 
> In QEMU, ARI Forwarding is enabled defualt at emulation of PCIe
> ports. ARI Forwarding enable setting at firmware/OS Control handoff.
> If the bit is Set when a non-ARI Device is present, the non-ARI
> Device can respond to Configuration Space accesses under what it
> interprets as being different Device Numbers, and its Functions can
> be aliased under multiple Device Numbers, generally leading to
> undesired behavior.
> 
> So, for pci devices attached in pcie root ports or downstream pots,
> we shoud assure that its slot is non-zero. For pcie devcies, which
> ARP capbility is not enabled, we also should assure that its slot
> is non-zero.
> 
> Changes since v2:
>  - make patch 1/3 more simpler and safer.(Hu Tao)
>  - change check logic from pci.c to pcie.c and change function's name
>  - judge devcies' ARI capbility instead of PCIe ports' ARI Forwarding
>    (Michael)
>  - add trivial patch 3/3
>  - update patch's commit messages and code comments.
> 
> Thanks for your reviewing.
> 
> Changes since v1:
>  - using object_dynamic_cast() instead of simple string comparing (Paolo)
>  - add ARI Forwarding enable bit check
>  - using pcie_cap_get_type() instead of simple string comparing (Marcel)
>  - fix some other comments.
> 
> Gonglei (3):
>   qdev: Introduce a function to get qbus's parent
>   pcie: add check for ari capability of pcie devices
>   pcie: remove confused comments
> 
>  hw/core/qdev.c         |  9 ++++++++
>  hw/pci/pci.c           |  4 ++++
>  hw/pci/pcie.c          | 59
> +++++++++++++++++++++++++++++++++++++++++++-------
>  include/hw/pci/pcie.h  |  1 +
>  include/hw/qdev-core.h |  1 +
>  5 files changed, 66 insertions(+), 8 deletions(-)
> 
> --
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH v3 0/3] add check for PCIe root ports and downstream ports

[Gonglei (Arei)]

> -----Original Message-----
> From: Gonglei (Arei)
> Sent: Thursday, September 04, 2014 8:10 PM
> Subject: RE: [PATCH v3 0/3] add check for PCIe root ports and downstream
> ports
> 
> Ping... Michael? Thanks!
> 

Ping... again. Thanks!

> 
> Best regards,
> -Gonglei
> 
> 
> > -----Original Message-----
> > From: Gonglei (Arei)
> > Sent: Monday, September 01, 2014 9:29 PM
> > To: qemu-devel@nongnu.org
> > Cc: mst@redhat.com; Huangweidong (C); pbonzini@redhat.com;
> > afaerber@suse.de; imammedo@redhat.com; peter.crosthwaite@xilinx.com;
> > Huangpeng (Peter); armbru@redhat.com; marcel.a@redhat.com;
> Luonengjun;
> > Gonglei (Arei)
> > Subject: [PATCH v3 0/3] add check for PCIe root ports and downstream ports
> > Importance: High
> >
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > Root ports and downstream ports of switches are the hot
> > pluggable ports in a PCI Express hierarchy.
> > PCI Express supports chip-to-chip interconnect, a PCIe link can
> > only connect one pci device/Switch/EndPoint or PCI-bridge.
> >
> > 7.3. Configuration Transaction Rules (PCI Express specification 3.0)
> > 7.3.1. Device Number
> >
> > Downstream Ports that do not have ARI Forwarding enabled must
> > associate only Device 0 with the device attached to the Logical Bus
> > representing the Link from the Port.
> >
> > In QEMU, ARI Forwarding is enabled defualt at emulation of PCIe
> > ports. ARI Forwarding enable setting at firmware/OS Control handoff.
> > If the bit is Set when a non-ARI Device is present, the non-ARI
> > Device can respond to Configuration Space accesses under what it
> > interprets as being different Device Numbers, and its Functions can
> > be aliased under multiple Device Numbers, generally leading to
> > undesired behavior.
> >
> > So, for pci devices attached in pcie root ports or downstream pots,
> > we shoud assure that its slot is non-zero. For pcie devcies, which
> > ARP capbility is not enabled, we also should assure that its slot
> > is non-zero.
> >
> > Changes since v2:
> >  - make patch 1/3 more simpler and safer.(Hu Tao)
> >  - change check logic from pci.c to pcie.c and change function's name
> >  - judge devcies' ARI capbility instead of PCIe ports' ARI Forwarding
> >    (Michael)
> >  - add trivial patch 3/3
> >  - update patch's commit messages and code comments.
> >
> > Thanks for your reviewing.
> >
> > Changes since v1:
> >  - using object_dynamic_cast() instead of simple string comparing (Paolo)
> >  - add ARI Forwarding enable bit check
> >  - using pcie_cap_get_type() instead of simple string comparing (Marcel)
> >  - fix some other comments.
> >
> > Gonglei (3):
> >   qdev: Introduce a function to get qbus's parent
> >   pcie: add check for ari capability of pcie devices
> >   pcie: remove confused comments
> >
> >  hw/core/qdev.c         |  9 ++++++++
> >  hw/pci/pci.c           |  4 ++++
> >  hw/pci/pcie.c          | 59
> > +++++++++++++++++++++++++++++++++++++++++++-------
> >  include/hw/pci/pcie.h  |  1 +
> >  include/hw/qdev-core.h |  1 +
> >  5 files changed, 66 insertions(+), 8 deletions(-)
> >
> > --
> > 1.7.12.4
> >

Re: [Qemu-devel] [PATCH v3 0/3] add check for PCIe root ports and downstream ports

[Gonglei (Arei)]

Hi, Michael

Would you give me some response? 
I have pinged this patch series the third time. :(
Thanks a lot!

Best regards,
-Gonglei


> -----Original Message-----
> From: Gonglei (Arei)
> Sent: Monday, September 01, 2014 9:29 PM
> To: qemu-devel@nongnu.org
> Cc: mst@redhat.com; Huangweidong (C); pbonzini@redhat.com;
> afaerber@suse.de; imammedo@redhat.com; peter.crosthwaite@xilinx.com;
> Huangpeng (Peter); armbru@redhat.com; marcel.a@redhat.com; Luonengjun;
> Gonglei (Arei)
> Subject: [PATCH v3 0/3] add check for PCIe root ports and downstream ports
> Importance: High
> 
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Root ports and downstream ports of switches are the hot
> pluggable ports in a PCI Express hierarchy.
> PCI Express supports chip-to-chip interconnect, a PCIe link can
> only connect one pci device/Switch/EndPoint or PCI-bridge.
> 
> 7.3. Configuration Transaction Rules (PCI Express specification 3.0)
> 7.3.1. Device Number
> 
> Downstream Ports that do not have ARI Forwarding enabled must
> associate only Device 0 with the device attached to the Logical Bus
> representing the Link from the Port.
> 
> In QEMU, ARI Forwarding is enabled defualt at emulation of PCIe
> ports. ARI Forwarding enable setting at firmware/OS Control handoff.
> If the bit is Set when a non-ARI Device is present, the non-ARI
> Device can respond to Configuration Space accesses under what it
> interprets as being different Device Numbers, and its Functions can
> be aliased under multiple Device Numbers, generally leading to
> undesired behavior.
> 
> So, for pci devices attached in pcie root ports or downstream pots,
> we shoud assure that its slot is non-zero. For pcie devcies, which
> ARP capbility is not enabled, we also should assure that its slot
> is non-zero.
> 
> Changes since v2:
>  - make patch 1/3 more simpler and safer.(Hu Tao)
>  - change check logic from pci.c to pcie.c and change function's name
>  - judge devcies' ARI capbility instead of PCIe ports' ARI Forwarding
>    (Michael)
>  - add trivial patch 3/3
>  - update patch's commit messages and code comments.
> 
> Thanks for your reviewing.
> 
> Changes since v1:
>  - using object_dynamic_cast() instead of simple string comparing (Paolo)
>  - add ARI Forwarding enable bit check
>  - using pcie_cap_get_type() instead of simple string comparing (Marcel)
>  - fix some other comments.
> 
> Gonglei (3):
>   qdev: Introduce a function to get qbus's parent
>   pcie: add check for ari capability of pcie devices
>   pcie: remove confused comments
> 
>  hw/core/qdev.c         |  9 ++++++++
>  hw/pci/pci.c           |  4 ++++
>  hw/pci/pcie.c          | 59
> +++++++++++++++++++++++++++++++++++++++++++-------
>  include/hw/pci/pcie.h  |  1 +
>  include/hw/qdev-core.h |  1 +
>  5 files changed, 66 insertions(+), 8 deletions(-)
> 
> --
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH v3 2/3] pcie: add check for ari capability of pcie devices

[Michael S. Tsirkin]

On Mon, Sep 01, 2014 at 09:29:18PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> In QEMU, ARI Forwarding is enabled defualt at emulation of PCIe
> ports. ARI Forwarding enable setting at firmware/OS Control handoff.
> If the bit is Set when a non-ARI Device is present, the non-ARI
> Device can respond to Configuration Space accesses under what it
> interprets as being different Device Numbers, and its Functions can
> be aliased under multiple Device Numbers, generally leading to
> undesired behavior.
> 
> So, for pci devices attached in pcie root ports or downstream pots,
> we shoud assure that its slot is non-zero. For pcie devcies, which
> ARP capbility is not enabled, we also should assure that its slot
> is non-zero.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/pci/pci.c          |  4 ++++
>  hw/pci/pcie.c         | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci/pcie.h |  1 +
>  3 files changed, 56 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index daeaeac..a9b8c3e 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1769,6 +1769,10 @@ static int pci_qdev_init(DeviceState *qdev)
>          }
>      }
>  
> +    if (pcie_cap_ari_check(bus, pci_dev)) {
> +        return -1;
> +    }
> +
>      /* rom loading */
>      is_default_rom = false;
>      if (pci_dev->romfile == NULL && pc->romfile != NULL) {
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 1babddf..30dd481 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -633,3 +633,54 @@ void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn)
>                          offset, PCI_ARI_SIZEOF);
>      pci_set_long(dev->config + offset + PCI_ARI_CAP, (nextfn & 0xff) << 8);
>  }
> +
> +int pcie_cap_ari_check(PCIBus *bus, PCIDevice *dev)

What does this function check, really?

> +{
> +    Object *obj = OBJECT(bus);
> +
> +    if (pci_bus_is_root(bus)) {
> +        return 0;
> +    }
> +
> +    if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) {
> +        DeviceState *parent = qbus_get_parent(BUS(obj));
> +        PCIDevice *pci_dev = PCI_DEVICE(parent);
> +        uint8_t port_type;
> +        /*
> +         * Root ports and downstream ports of switches are the hot
> +         * pluggable ports in a PCI Express hierarchy.
> +         * PCI Express supports chip-to-chip interconnect, a PCIe link can
> +         * only connect one pci device/Switch/EndPoint or PCI-bridge.
> +         *
> +         * 7.3. Configuration Transaction Rules (PCI Express specification 3.0)
> +         * 7.3.1. Device Number
> +         *
> +         * Downstream Ports that do not have ARI Forwarding enabled must
> +         * associate only Device 0 with the device attached to the Logical Bus
> +         * representing the Link from the Port.
> +         *
> +         * In QEMU, ARI Forwarding is enabled defualt at emulation of PCIe
> +         * ports. ARI Forwarding enable setting at firmware/OS Control handoff.
> +         * If the bit is Set when a non-ARI Device is present, the non-ARI
> +           Device can respond to Configuration Space accesses under what it
> +         * interprets as being different Device Numbers, and its Functions can
> +         * be aliased under multiple Device Numbers, generally leading to
> +         * undesired behavior.


A bunch of typos in comments and commit log.
Please run ispell before posting.

> +         */
> +        port_type = pcie_cap_get_type(pci_dev);
> +        if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> +            port_type == PCI_EXP_TYPE_ROOT_PORT) {
> +            if (!pci_is_express(dev) ||
> +                (pci_is_express(dev) &&
> +                !pcie_find_capability(dev, PCI_EXT_CAP_ID_ARI))) {
> +                if (PCI_SLOT(dev->devfn) != 0) {
> +                    error_report("PCIe: non-ARI device can't be populated"
> +                                 " in slot %d", PCI_SLOT(dev->devfn));
> +                    return -1;
> +                }
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index d139d58..ab2a44a 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -115,6 +115,7 @@ void pcie_add_capability(PCIDevice *dev,
>                           uint16_t offset, uint16_t size);
>  
>  void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
> +int pcie_cap_ari_check(PCIBus *bus, PCIDevice *dev);
>  
>  extern const VMStateDescription vmstate_pcie_device;
>  
> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH v3 0/3] add check for PCIe root ports and downstream ports

[Michael S. Tsirkin]

On Mon, Sep 01, 2014 at 09:29:16PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Root ports and downstream ports of switches are the hot
> pluggable ports in a PCI Express hierarchy.
> PCI Express supports chip-to-chip interconnect, a PCIe link can
> only connect one pci device/Switch/EndPoint or PCI-bridge.
> 
> 7.3. Configuration Transaction Rules (PCI Express specification 3.0)
> 7.3.1. Device Number
> 
> Downstream Ports that do not have ARI Forwarding enabled must
> associate only Device 0 with the device attached to the Logical Bus
> representing the Link from the Port.
> 
> In QEMU, ARI Forwarding is enabled defualt at emulation of PCIe
> ports. ARI Forwarding enable setting at firmware/OS Control handoff.
> If the bit is Set when a non-ARI Device is present, the non-ARI
> Device can respond to Configuration Space accesses under what it
> interprets as being different Device Numbers, and its Functions can
> be aliased under multiple Device Numbers, generally leading to
> undesired behavior.
> 
> So, for pci devices attached in pcie root ports or downstream pots,
> we shoud assure that its slot is non-zero. For pcie devcies, which
> ARP capbility is not enabled, we also should assure that its slot
> is non-zero.

So what is this patchset about?
Is there a broken configuration that this helps prevent?
Can you show the command-line please?

In particular, non-express devices behind an express bus
shouldn't exist according to spec, but do in practice, and guests
seem to be able to handle them.

> Changes since v2:
>  - make patch 1/3 more simpler and safer.(Hu Tao)
>  - change check logic from pci.c to pcie.c and change function's name
>  - judge devcies' ARI capbility instead of PCIe ports' ARI Forwarding
>    (Michael)
>  - add trivial patch 3/3
>  - update patch's commit messages and code comments.
> 
> Thanks for your reviewing.
>  
> Changes since v1:
>  - using object_dynamic_cast() instead of simple string comparing (Paolo)
>  - add ARI Forwarding enable bit check
>  - using pcie_cap_get_type() instead of simple string comparing (Marcel)
>  - fix some other comments.
> 
> Gonglei (3):
>   qdev: Introduce a function to get qbus's parent
>   pcie: add check for ari capability of pcie devices
>   pcie: remove confused comments
> 
>  hw/core/qdev.c         |  9 ++++++++
>  hw/pci/pci.c           |  4 ++++
>  hw/pci/pcie.c          | 59 +++++++++++++++++++++++++++++++++++++++++++-------
>  include/hw/pci/pcie.h  |  1 +
>  include/hw/qdev-core.h |  1 +
>  5 files changed, 66 insertions(+), 8 deletions(-)
> 
> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH v3 2/3] pcie: add check for ari capability of pcie devices

[Gonglei (Arei)]

> >
> > In QEMU, ARI Forwarding is enabled defualt at emulation of PCIe
> > ports. ARI Forwarding enable setting at firmware/OS Control handoff.
> > If the bit is Set when a non-ARI Device is present, the non-ARI
> > Device can respond to Configuration Space accesses under what it
> > interprets as being different Device Numbers, and its Functions can
> > be aliased under multiple Device Numbers, generally leading to
> > undesired behavior.
> >
> > So, for pci devices attached in pcie root ports or downstream pots,
> > we shoud assure that its slot is non-zero. For pcie devcies, which
> > ARP capbility is not enabled, we also should assure that its slot
> > is non-zero.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  hw/pci/pci.c          |  4 ++++
> >  hw/pci/pcie.c         | 51
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/pci/pcie.h |  1 +
> >  3 files changed, 56 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index daeaeac..a9b8c3e 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1769,6 +1769,10 @@ static int pci_qdev_init(DeviceState *qdev)
> >          }
> >      }
> >
> > +    if (pcie_cap_ari_check(bus, pci_dev)) {
> > +        return -1;
> > +    }
> > +
> >      /* rom loading */
> >      is_default_rom = false;
> >      if (pci_dev->romfile == NULL && pc->romfile != NULL) {
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 1babddf..30dd481 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -633,3 +633,54 @@ void pcie_ari_init(PCIDevice *dev, uint16_t offset,
> uint16_t nextfn)
> >                          offset, PCI_ARI_SIZEOF);
> >      pci_set_long(dev->config + offset + PCI_ARI_CAP, (nextfn & 0xff) << 8);
> >  }
> > +
> > +int pcie_cap_ari_check(PCIBus *bus, PCIDevice *dev)
> 
> What does this function check, really?
> 
Checking dev's slot is zero or not when the bus is PCIe bus.
Maybe I should change the name of this function.

Named pcie_slot_check()?

> > +{
> > +    Object *obj = OBJECT(bus);
> > +
> > +    if (pci_bus_is_root(bus)) {
> > +        return 0;
> > +    }
> > +
> > +    if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) {
> > +        DeviceState *parent = qbus_get_parent(BUS(obj));
> > +        PCIDevice *pci_dev = PCI_DEVICE(parent);
> > +        uint8_t port_type;
> > +        /*
> > +         * Root ports and downstream ports of switches are the hot
> > +         * pluggable ports in a PCI Express hierarchy.
> > +         * PCI Express supports chip-to-chip interconnect, a PCIe link can
> > +         * only connect one pci device/Switch/EndPoint or PCI-bridge.
> > +         *
> > +         * 7.3. Configuration Transaction Rules (PCI Express specification
> 3.0)
> > +         * 7.3.1. Device Number
> > +         *
> > +         * Downstream Ports that do not have ARI Forwarding enabled
> must
> > +         * associate only Device 0 with the device attached to the Logical
> Bus
> > +         * representing the Link from the Port.
> > +         *
> > +         * In QEMU, ARI Forwarding is enabled defualt at emulation of
> PCIe
> > +         * ports. ARI Forwarding enable setting at firmware/OS Control
> handoff.
> > +         * If the bit is Set when a non-ARI Device is present, the non-ARI
> > +           Device can respond to Configuration Space accesses under
> what it
> > +         * interprets as being different Device Numbers, and its Functions
> can
> > +         * be aliased under multiple Device Numbers, generally leading to
> > +         * undesired behavior.
> 
> 
> A bunch of typos in comments and commit log.
> Please run ispell before posting.
> 
OK, my fault! Will check them. :(

Best regards,
-Gonglei

> > +         */
> > +        port_type = pcie_cap_get_type(pci_dev);
> > +        if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> > +            port_type == PCI_EXP_TYPE_ROOT_PORT) {
> > +            if (!pci_is_express(dev) ||
> > +                (pci_is_express(dev) &&
> > +                !pcie_find_capability(dev, PCI_EXT_CAP_ID_ARI))) {
> > +                if (PCI_SLOT(dev->devfn) != 0) {
> > +                    error_report("PCIe: non-ARI device can't be
> populated"
> > +                                 " in slot %d",
> PCI_SLOT(dev->devfn));
> > +                    return -1;
> > +                }
> > +            }
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> > index d139d58..ab2a44a 100644
> > --- a/include/hw/pci/pcie.h
> > +++ b/include/hw/pci/pcie.h
> > @@ -115,6 +115,7 @@ void pcie_add_capability(PCIDevice *dev,
> >                           uint16_t offset, uint16_t size);
> >
> >  void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
> > +int pcie_cap_ari_check(PCIBus *bus, PCIDevice *dev);
> >
> >  extern const VMStateDescription vmstate_pcie_device;
> >
> > --
> > 1.7.12.4
> >

Re: [Qemu-devel] [PATCH v3 0/3] add check for PCIe root ports and downstream ports

[Gonglei (Arei)]

Hi,

> Subject: Re: [PATCH v3 0/3] add check for PCIe root ports and downstream
> ports
> 
> On Mon, Sep 01, 2014 at 09:29:16PM +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > Root ports and downstream ports of switches are the hot
> > pluggable ports in a PCI Express hierarchy.
> > PCI Express supports chip-to-chip interconnect, a PCIe link can
> > only connect one pci device/Switch/EndPoint or PCI-bridge.
> >
> > 7.3. Configuration Transaction Rules (PCI Express specification 3.0)
> > 7.3.1. Device Number
> >
> > Downstream Ports that do not have ARI Forwarding enabled must
> > associate only Device 0 with the device attached to the Logical Bus
> > representing the Link from the Port.
> >
> > In QEMU, ARI Forwarding is enabled defualt at emulation of PCIe
> > ports. ARI Forwarding enable setting at firmware/OS Control handoff.
> > If the bit is Set when a non-ARI Device is present, the non-ARI
> > Device can respond to Configuration Space accesses under what it
> > interprets as being different Device Numbers, and its Functions can
> > be aliased under multiple Device Numbers, generally leading to
> > undesired behavior.
> >
> > So, for pci devices attached in pcie root ports or downstream pots,
> > we shoud assure that its slot is non-zero. For pcie devcies, which
> > ARP capbility is not enabled, we also should assure that its slot
> > is non-zero.
> 
> So what is this patchset about?

Sorry, it should s/"is non-zero"/"is not non-zero"/ about the above descriptions.

> Is there a broken configuration that this helps prevent?

Yes. Otherwise it will confuse users who configure a device with 'slot > 0 ',
and the interface return OK, but the guest os report errors as below:

[ 159.035250] pciehp 0000:05:00.0:pcie24: Button pressed on Slot (0 - 4)
[ 159.035274] pciehp 0000:05:00.0:pcie24: Card present on Slot (0 - 4)
[ 159.036517] pciehp 0000:05:00.0:pcie24: PCI slot #0 - 4 - powering on due to button press.
[ 159.188049] pciehp 0000:05:00.0:pcie24: Failed to check link status
[ 159.201968] pciehp 0000:05:00.0:pcie24: Card not present on Slot (0 - 4)
[ 159.202529] pciehp 0000:05:00.0:pcie24: Already disabled on Slot (0 - 4)

> Can you show the command-line please?
> 
Of course I can.

$./qemu-system-x86_64 -enable-kvm -m 2048 -machine q35 -device ide-drive,bus=ide.2,drive=MacHDD \
 -drive id=MacHDD,if=none,file=/home/redhat_q35.img -monitor stdio -vnc :10 -readconfig \
 ../docs/q35-chipset.cfg.ori -device virtio-net-pci,id=nic1,bus=pcie-switch-downstream-port-1-1,addr=1.0
(qemu) info network
hub 0
 \ user.0: index=0,type=user,net=10.0.2.0,restrict=off
 \ e1000.0: index=0,type=nic,model=e1000,macaddr=52:54:00:12:34:56
(qemu)

The nic1 is not identified by guest os because I configured "addr=1.0"( slot > 0) .
If we configured "addr=0.0", it will be OK. (The blow steps show a process
hotplugging a virtio-net-pci with "slot = 0")

(qemu) device_add virtio-net-pci,id=nic2,bus=pcie-switch-downstream-port-1-1,addr=0
(qemu) info network
hub 0
 \ user.0: index=0,type=user,net=10.0.2.0,restrict=off
 \ e1000.0: index=0,type=nic,model=e1000,macaddr=52:54:00:12:34:56
nic2: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:58
(qemu)

Best regards,
-Gonglei

> In particular, non-express devices behind an express bus
> shouldn't exist according to spec, but do in practice, and guests
> seem to be able to handle them.
> 
> > Changes since v2:
> >  - make patch 1/3 more simpler and safer.(Hu Tao)
> >  - change check logic from pci.c to pcie.c and change function's name
> >  - judge devcies' ARI capbility instead of PCIe ports' ARI Forwarding
> >    (Michael)
> >  - add trivial patch 3/3
> >  - update patch's commit messages and code comments.
> >
> > Thanks for your reviewing.
> >
> > Changes since v1:
> >  - using object_dynamic_cast() instead of simple string comparing (Paolo)
> >  - add ARI Forwarding enable bit check
> >  - using pcie_cap_get_type() instead of simple string comparing (Marcel)
> >  - fix some other comments.
> >
> > Gonglei (3):
> >   qdev: Introduce a function to get qbus's parent
> >   pcie: add check for ari capability of pcie devices
> >   pcie: remove confused comments
> >
> >  hw/core/qdev.c         |  9 ++++++++
> >  hw/pci/pci.c           |  4 ++++
> >  hw/pci/pcie.c          | 59
> +++++++++++++++++++++++++++++++++++++++++++-------
> >  include/hw/pci/pcie.h  |  1 +
> >  include/hw/qdev-core.h |  1 +
> >  5 files changed, 66 insertions(+), 8 deletions(-)
> >
> > --
> > 1.7.12.4
> >