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

[Qemu-devel] [PATCH v2 0/2] 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.

If ARI Forwarding is disabled, according to PCIe spec
section 7.3.1, only slot 0 with the device attached to
logic bus representing the link from downstream
ports and root ports.

So, adding check for PCIe downstream ports and root ports,
which avoid useless operation, both hotplug and coldplug.

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 (2):
  qdev: Introduce a function to get qbus's parent
  pci: add check for pcie root ports and downstream ports

 hw/core/qdev.c         |  7 +++++++
 hw/pci/pci.c           | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/qdev-core.h |  1 +
 3 files changed, 59 insertions(+)

-- 
1.7.12.4

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

[arei.gonglei]

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

We need to use qbus's parent and get its name.

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

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index da1ba48..5c49e08 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -616,6 +616,13 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
     return bus;
 }
 
+DeviceState *qbus_get_parent(BusState *bus)
+{
+    DeviceState *parent = bus->parent;
+
+    return parent;
+}
+
 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

Re: [Qemu-devel] [PATCH v2 1/2] qdev: Introduce a function to get qbus's parent

[Hu Tao]

On Thu, Aug 21, 2014 at 05:47:45PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> We need to use qbus's parent and get its name.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/core/qdev.c         | 7 +++++++
>  include/hw/qdev-core.h | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index da1ba48..5c49e08 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -616,6 +616,13 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>      return bus;
>  }
>  
> +DeviceState *qbus_get_parent(BusState *bus)
> +{
> +    DeviceState *parent = bus->parent;
> +
> +    return parent;
> +}

You can return bus->parent directly, or safer:

if (bus) {
    return bus->parent;
}

return NULL;

And, the function is simple enough to make it inline.

> +
>  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
> 
> 

Re: [Qemu-devel] [PATCH v2 1/2] qdev: Introduce a function to get qbus's parent

[Gonglei (Arei)]

> From: Hu Tao [mailto:hutao@cn.fujitsu.com]
> On Thu, Aug 21, 2014 at 05:47:45PM +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > We need to use qbus's parent and get its name.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  hw/core/qdev.c         | 7 +++++++
> >  include/hw/qdev-core.h | 1 +
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index da1ba48..5c49e08 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -616,6 +616,13 @@ BusState *qbus_create(const char *typename,
> DeviceState *parent, const char *nam
> >      return bus;
> >  }
> >
> > +DeviceState *qbus_get_parent(BusState *bus)
> > +{
> > +    DeviceState *parent = bus->parent;
> > +
> > +    return parent;
> > +}
> 
> You can return bus->parent directly, or safer:
> 
> if (bus) {
>     return bus->parent;
> }
> 
> return NULL;
> 

It should not happen, at the moment, but it is more safer, indeed.

Thanks.

> And, the function is simple enough to make it inline.
> 

Yes, it is simple. I just follow the realization of qdev_get_parent_bus()

Best regards,
-Gonglei

[Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and downstream ports

[arei.gonglei]

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

If ARI Forwarding is disabled, according to PCIe spec
section 7.3.1, only slot 0 with the device attached to
logic bus representing the link from downstream
ports and root ports.

So, adding check for PCIe downstream ports and root ports,
which avoid useless operation, both hotplug and coldplug.

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

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index daeaeac..aa0af0c 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
     return 0;
 }
 
+static int pci_check_pcie_port(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.
+         *
+         * If ARI Forwarding is not enabled on root ports and downstream
+         * ports, only support the devices with slot non-0, regardless of
+         * hotplug or coldplug.
+         */
+        port_type = pcie_cap_get_type(pci_dev);
+        if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
+            port_type == PCI_EXP_TYPE_ROOT_PORT) {
+            if (!pcie_cap_is_ari_enabled(pci_dev)) {
+                if (PCI_SLOT(dev->devfn) != 0) {
+                    error_report("PCIe: Port's ARI Forwarding is disabled, "
+                                 "device can't be populated in slot %d",
+                                 PCI_SLOT(dev->devfn));
+                    return -1;
+                }
+            }
+        }
+    }
+
+    return 0;
+}
+
 static void pci_config_alloc(PCIDevice *pci_dev)
 {
     int config_size = pci_config_size(pci_dev);
@@ -827,6 +873,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
 
     pci_dev->bus = bus;
     pci_dev->devfn = devfn;
+
+    if (pci_check_pcie_port(bus, pci_dev)) {
+        return NULL;
+    }
+
     dma_as = pci_device_iommu_address_space(pci_dev);
 
     memory_region_init_alias(&pci_dev->bus_master_enable_region,
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and downstream ports

[Marcel Apfelbaum]

On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> If ARI Forwarding is disabled, according to PCIe spec
> section 7.3.1, only slot 0 with the device attached to
> logic bus representing the link from downstream
> ports and root ports.
> 
> So, adding check for PCIe downstream ports and root ports,
> which avoid useless operation, both hotplug and coldplug.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/pci/pci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index daeaeac..aa0af0c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
>      return 0;
>  }
>  
> +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev)
Hi,

This function is specific to pcie, I think it should be placed
in hw/pci/pcie.c. hw/pci/pci.c is for general pci functionality.

Other thing, I think the function name does not completely match
the functionality, maybe "pcie_check_slot" or "pcie_validate_slot"? 

Thanks,
Marcel 

> +{
> +    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.
> +         *
> +         * If ARI Forwarding is not enabled on root ports and downstream
> +         * ports, only support the devices with slot non-0, regardless of
> +         * hotplug or coldplug.
> +         */
> +        port_type = pcie_cap_get_type(pci_dev);
> +        if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> +            port_type == PCI_EXP_TYPE_ROOT_PORT) {
> +            if (!pcie_cap_is_ari_enabled(pci_dev)) {
> +                if (PCI_SLOT(dev->devfn) != 0) {
> +                    error_report("PCIe: Port's ARI Forwarding is disabled, "
> +                                 "device can't be populated in slot %d",
> +                                 PCI_SLOT(dev->devfn));
> +                    return -1;
> +                }
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static void pci_config_alloc(PCIDevice *pci_dev)
>  {
>      int config_size = pci_config_size(pci_dev);
> @@ -827,6 +873,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>  
>      pci_dev->bus = bus;
>      pci_dev->devfn = devfn;
> +
> +    if (pci_check_pcie_port(bus, pci_dev)) {
> +        return NULL;
> +    }
> +
>      dma_as = pci_device_iommu_address_space(pci_dev);
>  
>      memory_region_init_alias(&pci_dev->bus_master_enable_region,

Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and downstream ports

[Gonglei (Arei)]

> From: Marcel Apfelbaum [mailto:marcel.a@redhat.com]
> Sent: Friday, August 22, 2014 2:34 PM
> Subject: Re: [PATCH v2 2/2] pci: add check for pcie root ports and downstream
> ports
> 
> On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > If ARI Forwarding is disabled, according to PCIe spec
> > section 7.3.1, only slot 0 with the device attached to
> > logic bus representing the link from downstream
> > ports and root ports.
> >
> > So, adding check for PCIe downstream ports and root ports,
> > which avoid useless operation, both hotplug and coldplug.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  hw/pci/pci.c | 51
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index daeaeac..aa0af0c 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus,
> PCIDevice *dev)
> >      return 0;
> >  }
> >
> > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev)
> Hi,
> 
> This function is specific to pcie, I think it should be placed
> in hw/pci/pcie.c. hw/pci/pci.c is for general pci functionality.
> 
Good idea.

> Other thing, I think the function name does not completely match
> the functionality, maybe "pcie_check_slot" or "pcie_validate_slot"?
> 
OK, agreed. Thanks!

Best regards,
-Gonglei

> Thanks,
> Marcel
> 
> > +{
> > +    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.
> > +         *
> > +         * If ARI Forwarding is not enabled on root ports and downstream
> > +         * ports, only support the devices with slot non-0, regardless of
> > +         * hotplug or coldplug.
> > +         */
> > +        port_type = pcie_cap_get_type(pci_dev);
> > +        if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> > +            port_type == PCI_EXP_TYPE_ROOT_PORT) {
> > +            if (!pcie_cap_is_ari_enabled(pci_dev)) {
> > +                if (PCI_SLOT(dev->devfn) != 0) {
> > +                    error_report("PCIe: Port's ARI Forwarding is
> disabled, "
> > +                                 "device can't be populated in
> slot %d",
> > +                                 PCI_SLOT(dev->devfn));
> > +                    return -1;
> > +                }
> > +            }
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static void pci_config_alloc(PCIDevice *pci_dev)
> >  {
> >      int config_size = pci_config_size(pci_dev);
> > @@ -827,6 +873,11 @@ static PCIDevice
> *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >
> >      pci_dev->bus = bus;
> >      pci_dev->devfn = devfn;
> > +
> > +    if (pci_check_pcie_port(bus, pci_dev)) {
> > +        return NULL;
> > +    }
> > +
> >      dma_as = pci_device_iommu_address_space(pci_dev);
> >
> >      memory_region_init_alias(&pci_dev->bus_master_enable_region,
> 
> 

Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and downstream ports

[Knut Omang]

On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> If ARI Forwarding is disabled, according to PCIe spec
> section 7.3.1, only slot 0 with the device attached to
> logic bus representing the link from downstream
> ports and root ports.
> 
> So, adding check for PCIe downstream ports and root ports,
> which avoid useless operation, both hotplug and coldplug.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/pci/pci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index daeaeac..aa0af0c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
>      return 0;
>  }
>  
> +static int pci_check_pcie_port(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.
> +         *
> +         * If ARI Forwarding is not enabled on root ports and downstream
> +         * ports, only support the devices with slot non-0, regardless of
> +         * hotplug or coldplug.
> +         */

My interpretation of this section of the spec is that if ARI forwarding
is not available, only the normal 8 functions can be accessed for each
device, eg. device/functions 0.0 -> 0.7  - if a device has more than 8
functions, it will need the second device's namespace, eg. devfn 1.0++,
which would not be routed correctly in a non-ari forward capable device.

As far as I understand, with this fix you restrict an non-ARI capable
switch to only expose one device?

Knut

> +        port_type = pcie_cap_get_type(pci_dev);
> +        if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> +            port_type == PCI_EXP_TYPE_ROOT_PORT) {
> +            if (!pcie_cap_is_ari_enabled(pci_dev)) {
> +                if (PCI_SLOT(dev->devfn) != 0) {
> +                    error_report("PCIe: Port's ARI Forwarding is disabled, "
> +                                 "device can't be populated in slot %d",
> +                                 PCI_SLOT(dev->devfn));
> +                    return -1;
> +                }
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static void pci_config_alloc(PCIDevice *pci_dev)
>  {
>      int config_size = pci_config_size(pci_dev);
> @@ -827,6 +873,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>  
>      pci_dev->bus = bus;
>      pci_dev->devfn = devfn;
> +
> +    if (pci_check_pcie_port(bus, pci_dev)) {
> +        return NULL;
> +    }
> +
>      dma_as = pci_device_iommu_address_space(pci_dev);
>  
>      memory_region_init_alias(&pci_dev->bus_master_enable_region,

Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and downstream ports

[Gonglei (Arei)]

> -----Original Message-----
> From: Knut Omang [mailto:knut.omang@oracle.com]
> Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and
> downstream ports
> 
> On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > If ARI Forwarding is disabled, according to PCIe spec
> > section 7.3.1, only slot 0 with the device attached to
> > logic bus representing the link from downstream
> > ports and root ports.
> >
> > So, adding check for PCIe downstream ports and root ports,
> > which avoid useless operation, both hotplug and coldplug.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  hw/pci/pci.c | 51
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index daeaeac..aa0af0c 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus,
> PCIDevice *dev)
> >      return 0;
> >  }
> >
> > +static int pci_check_pcie_port(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.
> > +         *
> > +         * If ARI Forwarding is not enabled on root ports and downstream
> > +         * ports, only support the devices with slot non-0, regardless of
> > +         * hotplug or coldplug.
> > +         */
> 
> My interpretation of this section of the spec is that if ARI forwarding
> is not available, only the normal 8 functions can be accessed for each
> device, eg. device/functions 0.0 -> 0.7  - if a device has more than 8
> functions, it will need the second device's namespace, eg. devfn 1.0++,
> which would not be routed correctly in a non-ari forward capable device.
> 
Yes.

> As far as I understand, with this fix you restrict an non-ARI capable
> switch to only expose one device?
> 
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)

Best regards,
-Gonglei

> Knut
> 
> > +        port_type = pcie_cap_get_type(pci_dev);
> > +        if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> > +            port_type == PCI_EXP_TYPE_ROOT_PORT) {
> > +            if (!pcie_cap_is_ari_enabled(pci_dev)) {
> > +                if (PCI_SLOT(dev->devfn) != 0) {
> > +                    error_report("PCIe: Port's ARI Forwarding is
> disabled, "
> > +                                 "device can't be populated in
> slot %d",
> > +                                 PCI_SLOT(dev->devfn));
> > +                    return -1;
> > +                }
> > +            }
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static void pci_config_alloc(PCIDevice *pci_dev)
> >  {
> >      int config_size = pci_config_size(pci_dev);
> > @@ -827,6 +873,11 @@ static PCIDevice
> *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >
> >      pci_dev->bus = bus;
> >      pci_dev->devfn = devfn;
> > +
> > +    if (pci_check_pcie_port(bus, pci_dev)) {
> > +        return NULL;
> > +    }
> > +
> >      dma_as = pci_device_iommu_address_space(pci_dev);
> >
> >      memory_region_init_alias(&pci_dev->bus_master_enable_region,
> 

Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and downstream ports

[Knut Omang]

On Mon, 2014-08-25 at 09:23 +0000, Gonglei (Arei) wrote:
> > -----Original Message-----
> > From: Knut Omang [mailto:knut.omang@oracle.com]
> > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and
> > downstream ports
> > 
> > On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote:
> > > From: Gonglei <arei.gonglei@huawei.com>
> > >
> > > If ARI Forwarding is disabled, according to PCIe spec
> > > section 7.3.1, only slot 0 with the device attached to
> > > logic bus representing the link from downstream
> > > ports and root ports.
> > >
> > > So, adding check for PCIe downstream ports and root ports,
> > > which avoid useless operation, both hotplug and coldplug.
> > >
> > > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > > ---
> > >  hw/pci/pci.c | 51
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 51 insertions(+)
> > >
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index daeaeac..aa0af0c 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus,
> > PCIDevice *dev)
> > >      return 0;
> > >  }
> > >
> > > +static int pci_check_pcie_port(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.
> > > +         *
> > > +         * If ARI Forwarding is not enabled on root ports and downstream
> > > +         * ports, only support the devices with slot non-0, regardless of
> > > +         * hotplug or coldplug.
> > > +         */
> > 
> > My interpretation of this section of the spec is that if ARI forwarding
> > is not available, only the normal 8 functions can be accessed for each
> > device, eg. device/functions 0.0 -> 0.7  - if a device has more than 8
> > functions, it will need the second device's namespace, eg. devfn 1.0++,
> > which would not be routed correctly in a non-ari forward capable device.
> > 
> Yes.
> 
> > As far as I understand, with this fix you restrict an non-ARI capable
> > switch to only expose one device?
> > 
> 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)

Ah - I see!
I think this also explains why I have been seeing this error and failure
to hotplug recently (with an ARIfwd enabled root port and an ARI capable
device)...

It seems your patch is preventing this from happening in the non-arifwd
case, but will still be a problem with a single ARI capable device if
ARIfwd is enabled, even if no more than one function is exposed by the
device?

Best regards,

Knut

> Best regards,
> -Gonglei
> 
> > Knut
> > 
> > > +        port_type = pcie_cap_get_type(pci_dev);
> > > +        if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> > > +            port_type == PCI_EXP_TYPE_ROOT_PORT) {
> > > +            if (!pcie_cap_is_ari_enabled(pci_dev)) {
> > > +                if (PCI_SLOT(dev->devfn) != 0) {
> > > +                    error_report("PCIe: Port's ARI Forwarding is
> > disabled, "
> > > +                                 "device can't be populated in
> > slot %d",
> > > +                                 PCI_SLOT(dev->devfn));
> > > +                    return -1;
> > > +                }
> > > +            }
> > > +        }
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  static void pci_config_alloc(PCIDevice *pci_dev)
> > >  {
> > >      int config_size = pci_config_size(pci_dev);
> > > @@ -827,6 +873,11 @@ static PCIDevice
> > *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> > >
> > >      pci_dev->bus = bus;
> > >      pci_dev->devfn = devfn;
> > > +
> > > +    if (pci_check_pcie_port(bus, pci_dev)) {
> > > +        return NULL;
> > > +    }
> > > +
> > >      dma_as = pci_device_iommu_address_space(pci_dev);
> > >
> > >      memory_region_init_alias(&pci_dev->bus_master_enable_region,
> > 
> 

Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and downstream ports

[Gonglei (Arei)]

> From: Knut Omang [mailto:knut.omang@oracle.com]
> Sent: Monday, August 25, 2014 5:51 PM
> Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and
> downstream ports
> 
> On Mon, 2014-08-25 at 09:23 +0000, Gonglei (Arei) wrote:
> > > -----Original Message-----
> > > From: Knut Omang [mailto:knut.omang@oracle.com]
> > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports
> and
> > > downstream ports
> > >
> > > On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote:
> > > > From: Gonglei <arei.gonglei@huawei.com>
> > > >
> > > > If ARI Forwarding is disabled, according to PCIe spec
> > > > section 7.3.1, only slot 0 with the device attached to
> > > > logic bus representing the link from downstream
> > > > ports and root ports.
> > > >
> > > > So, adding check for PCIe downstream ports and root ports,
> > > > which avoid useless operation, both hotplug and coldplug.
> > > >
> > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > > > ---
> > > >  hw/pci/pci.c | 51
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 51 insertions(+)
> > > >
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index daeaeac..aa0af0c 100644
> > > > --- a/hw/pci/pci.c
> > > > +++ b/hw/pci/pci.c
> > > > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus,
> > > PCIDevice *dev)
> > > >      return 0;
> > > >  }
> > > >
> > > > +static int pci_check_pcie_port(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.
> > > > +         *
> > > > +         * If ARI Forwarding is not enabled on root ports and
> downstream
> > > > +         * ports, only support the devices with slot non-0, regardless
> of
> > > > +         * hotplug or coldplug.
> > > > +         */
> > >
> > > My interpretation of this section of the spec is that if ARI forwarding
> > > is not available, only the normal 8 functions can be accessed for each
> > > device, eg. device/functions 0.0 -> 0.7  - if a device has more than 8
> > > functions, it will need the second device's namespace, eg. devfn 1.0++,
> > > which would not be routed correctly in a non-ari forward capable device.
> > >
> > Yes.
> >
> > > As far as I understand, with this fix you restrict an non-ARI capable
> > > switch to only expose one device?
> > >
> > 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)
> 
> Ah - I see!
> I think this also explains why I have been seeing this error and failure
> to hotplug recently (with an ARIfwd enabled root port and an ARI capable
> device)...
> 
> It seems your patch is preventing this from happening in the non-arifwd
> case, but will still be a problem with a single ARI capable device if
> ARIfwd is enabled, even if no more than one function is exposed by the
> device?
> 
Sorry for my poor English. 
What's your mean about " a single ARI capable device..." ?

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and downstream ports

[Knut Omang]

On Mon, 2014-08-25 at 10:51 +0000, Gonglei (Arei) wrote:
> > From: Knut Omang [mailto:knut.omang@oracle.com]
> > Sent: Monday, August 25, 2014 5:51 PM
> > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and
> > downstream ports
> > 
> > On Mon, 2014-08-25 at 09:23 +0000, Gonglei (Arei) wrote:
> > > > -----Original Message-----
> > > > From: Knut Omang [mailto:knut.omang@oracle.com]
> > > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports
> > and
> > > > downstream ports
> > > >
> > > > On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote:
> > > > > From: Gonglei <arei.gonglei@huawei.com>
> > > > >
> > > > > If ARI Forwarding is disabled, according to PCIe spec
> > > > > section 7.3.1, only slot 0 with the device attached to
> > > > > logic bus representing the link from downstream
> > > > > ports and root ports.
> > > > >
> > > > > So, adding check for PCIe downstream ports and root ports,
> > > > > which avoid useless operation, both hotplug and coldplug.
> > > > >
> > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > > > > ---
> > > > >  hw/pci/pci.c | 51
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 51 insertions(+)
> > > > >
> > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > > index daeaeac..aa0af0c 100644
> > > > > --- a/hw/pci/pci.c
> > > > > +++ b/hw/pci/pci.c
> > > > > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus,
> > > > PCIDevice *dev)
> > > > >      return 0;
> > > > >  }
> > > > >
> > > > > +static int pci_check_pcie_port(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.
> > > > > +         *
> > > > > +         * If ARI Forwarding is not enabled on root ports and
> > downstream
> > > > > +         * ports, only support the devices with slot non-0, regardless
> > of
> > > > > +         * hotplug or coldplug.
> > > > > +         */
> > > >
> > > > My interpretation of this section of the spec is that if ARI forwarding
> > > > is not available, only the normal 8 functions can be accessed for each
> > > > device, eg. device/functions 0.0 -> 0.7  - if a device has more than 8
> > > > functions, it will need the second device's namespace, eg. devfn 1.0++,
> > > > which would not be routed correctly in a non-ari forward capable device.
> > > >
> > > Yes.
> > >
> > > > As far as I understand, with this fix you restrict an non-ARI capable
> > > > switch to only expose one device?
> > > >
> > > 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)
> > 
> > Ah - I see!
> > I think this also explains why I have been seeing this error and failure
> > to hotplug recently (with an ARIfwd enabled root port and an ARI capable
> > device)...
> > 
> > It seems your patch is preventing this from happening in the non-arifwd
> > case, but will still be a problem with a single ARI capable device if
> > ARIfwd is enabled, even if no more than one function is exposed by the
> > device?
> > 
> Sorry for my poor English. 
> What's your mean about " a single ARI capable device..." ?

I just meant a single device with an ARI PCI express extended
capability, as opposed to the situation you describe (if I understand
right) where there are multiple PCIe devices involved, each in different
slots on the same switch.

This can happen if a device provides a potential for more than 8
functions, and as such "cross into" the function space of the next
"slot", in my case on a root port which physically speaking has only a
single slot, but where the device nonetheless are able to respond to and
send requests on devfn 1.x.

Knut

> Best regards,
> -Gonglei

Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and downstream ports

[Gonglei (Arei)]

> -----Original Message-----
> From: Knut Omang [mailto:knut.omang@oracle.com]
> Sent: Monday, August 25, 2014 7:02 PM
> Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and
> downstream ports
> 
> On Mon, 2014-08-25 at 10:51 +0000, Gonglei (Arei) wrote:
> > > From: Knut Omang [mailto:knut.omang@oracle.com]
> > > Sent: Monday, August 25, 2014 5:51 PM
> > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports
> and
> > > downstream ports
> > >
> > > On Mon, 2014-08-25 at 09:23 +0000, Gonglei (Arei) wrote:
> > > > > -----Original Message-----
> > > > > From: Knut Omang [mailto:knut.omang@oracle.com]
> > > > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root
> ports
> > > and
> > > > > downstream ports
> > > > >
> > > > > On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote:
> > > > > > From: Gonglei <arei.gonglei@huawei.com>
> > > > > >
> > > > > > If ARI Forwarding is disabled, according to PCIe spec
> > > > > > section 7.3.1, only slot 0 with the device attached to
> > > > > > logic bus representing the link from downstream
> > > > > > ports and root ports.
> > > > > >
> > > > > > So, adding check for PCIe downstream ports and root ports,
> > > > > > which avoid useless operation, both hotplug and coldplug.
> > > > > >
> > > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > > > > > ---
> > > > > >  hw/pci/pci.c | 51
> > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 51 insertions(+)
> > > > > >
> > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > > > index daeaeac..aa0af0c 100644
> > > > > > --- a/hw/pci/pci.c
> > > > > > +++ b/hw/pci/pci.c
> > > > > > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus
> *bus,
> > > > > PCIDevice *dev)
> > > > > >      return 0;
> > > > > >  }
> > > > > >
> > > > > > +static int pci_check_pcie_port(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.
> > > > > > +         *
> > > > > > +         * If ARI Forwarding is not enabled on root ports and
> > > downstream
> > > > > > +         * ports, only support the devices with slot non-0,
> regardless
> > > of
> > > > > > +         * hotplug or coldplug.
> > > > > > +         */
> > > > >
> > > > > My interpretation of this section of the spec is that if ARI forwarding
> > > > > is not available, only the normal 8 functions can be accessed for each
> > > > > device, eg. device/functions 0.0 -> 0.7  - if a device has more than 8
> > > > > functions, it will need the second device's namespace, eg. devfn 1.0++,
> > > > > which would not be routed correctly in a non-ari forward capable device.
> > > > >
> > > > Yes.
> > > >
> > > > > As far as I understand, with this fix you restrict an non-ARI capable
> > > > > switch to only expose one device?
> > > > >
> > > > 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)
> > >
> > > Ah - I see!
> > > I think this also explains why I have been seeing this error and failure
> > > to hotplug recently (with an ARIfwd enabled root port and an ARI capable
> > > device)...
> > >
> > > It seems your patch is preventing this from happening in the non-arifwd
> > > case, but will still be a problem with a single ARI capable device if
> > > ARIfwd is enabled, even if no more than one function is exposed by the
> > > device?
> > >
> > Sorry for my poor English.
> > What's your mean about " a single ARI capable device..." ?
> 
> I just meant a single device with an ARI PCI express extended
> capability, as opposed to the situation you describe (if I understand
> right) where there are multiple PCIe devices involved, each in different
> slots on the same switch.
> 
OK. Thanks for your explanation.

> This can happen if a device provides a potential for more than 8
> functions, and as such "cross into" the function space of the next
> "slot", in my case on a root port which physically speaking has only a
> single slot, but where the device nonetheless are able to respond to and
> send requests on devfn 1.x.
> 
I think you are right. But I haven't tested this case, TBH.

Knut, could you tell me how to enable the ARI Forwarding for a PCIe device?
Just call the pcie_ari_init function? Which kind of device do you use? Thanks!

Best regards,
-Gonglei

> Knut
> 
> > Best regards,
> > -Gonglei
> 

Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and downstream ports

[Knut Omang]

On Mon, 2014-08-25 at 11:23 +0000, Gonglei (Arei) wrote:
> > -----Original Message-----
> > From: Knut Omang [mailto:knut.omang@oracle.com]
> > Sent: Monday, August 25, 2014 7:02 PM
> > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and
> > downstream ports
> > 
> > On Mon, 2014-08-25 at 10:51 +0000, Gonglei (Arei) wrote:
> > > > From: Knut Omang [mailto:knut.omang@oracle.com]
> > > > Sent: Monday, August 25, 2014 5:51 PM
> > > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports
> > and
> > > > downstream ports
> > > >
> > > > On Mon, 2014-08-25 at 09:23 +0000, Gonglei (Arei) wrote:
> > > > > > -----Original Message-----
> > > > > > From: Knut Omang [mailto:knut.omang@oracle.com]
> > > > > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root
> > ports
> > > > and
> > > > > > downstream ports
> > > > > >
> > > > > > On Thu, 2014-08-21 at 17:47 +0800, arei.gonglei@huawei.com wrote:
> > > > > > > From: Gonglei <arei.gonglei@huawei.com>
> > > > > > >
> > > > > > > If ARI Forwarding is disabled, according to PCIe spec
> > > > > > > section 7.3.1, only slot 0 with the device attached to
> > > > > > > logic bus representing the link from downstream
> > > > > > > ports and root ports.
> > > > > > >
> > > > > > > So, adding check for PCIe downstream ports and root ports,
> > > > > > > which avoid useless operation, both hotplug and coldplug.
> > > > > > >
> > > > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > > > > > > ---
> > > > > > >  hw/pci/pci.c | 51
> > > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 51 insertions(+)
> > > > > > >
> > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > > > > index daeaeac..aa0af0c 100644
> > > > > > > --- a/hw/pci/pci.c
> > > > > > > +++ b/hw/pci/pci.c
> > > > > > > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus
> > *bus,
> > > > > > PCIDevice *dev)
> > > > > > >      return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +static int pci_check_pcie_port(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.
> > > > > > > +         *
> > > > > > > +         * If ARI Forwarding is not enabled on root ports and
> > > > downstream
> > > > > > > +         * ports, only support the devices with slot non-0,
> > regardless
> > > > of
> > > > > > > +         * hotplug or coldplug.
> > > > > > > +         */
> > > > > >
> > > > > > My interpretation of this section of the spec is that if ARI forwarding
> > > > > > is not available, only the normal 8 functions can be accessed for each
> > > > > > device, eg. device/functions 0.0 -> 0.7  - if a device has more than 8
> > > > > > functions, it will need the second device's namespace, eg. devfn 1.0++,
> > > > > > which would not be routed correctly in a non-ari forward capable device.
> > > > > >
> > > > > Yes.
> > > > >
> > > > > > As far as I understand, with this fix you restrict an non-ARI capable
> > > > > > switch to only expose one device?
> > > > > >
> > > > > 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)
> > > >
> > > > Ah - I see!
> > > > I think this also explains why I have been seeing this error and failure
> > > > to hotplug recently (with an ARIfwd enabled root port and an ARI capable
> > > > device)...
> > > >
> > > > It seems your patch is preventing this from happening in the non-arifwd
> > > > case, but will still be a problem with a single ARI capable device if
> > > > ARIfwd is enabled, even if no more than one function is exposed by the
> > > > device?
> > > >
> > > Sorry for my poor English.
> > > What's your mean about " a single ARI capable device..." ?
> > 
> > I just meant a single device with an ARI PCI express extended
> > capability, as opposed to the situation you describe (if I understand
> > right) where there are multiple PCIe devices involved, each in different
> > slots on the same switch.
> > 
> OK. Thanks for your explanation.
> 
> > This can happen if a device provides a potential for more than 8
> > functions, and as such "cross into" the function space of the next
> > "slot", in my case on a root port which physically speaking has only a
> > single slot, but where the device nonetheless are able to respond to and
> > send requests on devfn 1.x.
> > 
> I think you are right. But I haven't tested this case, TBH.

> Knut, could you tell me how to enable the ARI Forwarding for a PCIe device?
> Just call the pcie_ari_init function? 

That would add the ARI extended capability to the list of PCIe
capabilities in config space, but then you need to have a device with
enough functions to actually need ARI, eg. more than 8.

> Which kind of device do you use? Thanks!

This is where the SR/IOV patch comes in, working on it,...

Knut

> 
> Best regards,
> -Gonglei
> 
> > Knut
> > 
> > > Best regards,
> > > -Gonglei
> > 
> 

Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and downstream ports

[Michael S. Tsirkin]

On Thu, Aug 21, 2014 at 05:47:46PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> If ARI Forwarding is disabled, according to PCIe spec
> section 7.3.1, only slot 0 with the device attached to
> logic bus representing the link from downstream
> ports and root ports.
> 
> So, adding check for PCIe downstream ports and root ports,
> which avoid useless operation, both hotplug and coldplug.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/pci/pci.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index daeaeac..aa0af0c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
>      return 0;
>  }
>  
> +static int pci_check_pcie_port(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.
> +         *
> +         * If ARI Forwarding is not enabled on root ports and downstream
> +         * ports, only support the devices with slot non-0, regardless of
> +         * hotplug or coldplug.
> +         */
> +        port_type = pcie_cap_get_type(pci_dev);
> +        if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> +            port_type == PCI_EXP_TYPE_ROOT_PORT) {
> +            if (!pcie_cap_is_ari_enabled(pci_dev)) {

Won't this mean cold-plugging devices is broken?
I guess you could check for ARI capability instead.

> +                if (PCI_SLOT(dev->devfn) != 0) {
> +                    error_report("PCIe: Port's ARI Forwarding is disabled, "
> +                                 "device can't be populated in slot %d",
> +                                 PCI_SLOT(dev->devfn));
> +                    return -1;
> +                }
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static void pci_config_alloc(PCIDevice *pci_dev)
>  {
>      int config_size = pci_config_size(pci_dev);
> @@ -827,6 +873,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>  
>      pci_dev->bus = bus;
>      pci_dev->devfn = devfn;
> +
> +    if (pci_check_pcie_port(bus, pci_dev)) {
> +        return NULL;
> +    }
> +
>      dma_as = pci_device_iommu_address_space(pci_dev);
>  
>      memory_region_init_alias(&pci_dev->bus_master_enable_region,
> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and downstream ports

[Gonglei (Arei)]

> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Subject: Re: [PATCH v2 2/2] pci: add check for pcie root ports and downstream
> ports
> 
> On Thu, Aug 21, 2014 at 05:47:46PM +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > If ARI Forwarding is disabled, according to PCIe spec
> > section 7.3.1, only slot 0 with the device attached to
> > logic bus representing the link from downstream
> > ports and root ports.
> >
> > So, adding check for PCIe downstream ports and root ports,
> > which avoid useless operation, both hotplug and coldplug.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  hw/pci/pci.c | 51
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index daeaeac..aa0af0c 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -773,6 +773,52 @@ static int pci_init_multifunction(PCIBus *bus,
> PCIDevice *dev)
> >      return 0;
> >  }
> >
> > +static int pci_check_pcie_port(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.
> > +         *
> > +         * If ARI Forwarding is not enabled on root ports and downstream
> > +         * ports, only support the devices with slot non-0, regardless of
> > +         * hotplug or coldplug.
> > +         */
> > +        port_type = pcie_cap_get_type(pci_dev);
> > +        if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> > +            port_type == PCI_EXP_TYPE_ROOT_PORT) {
> > +            if (!pcie_cap_is_ari_enabled(pci_dev)) {
> 
> Won't this mean cold-plugging devices is broken?

Yes. If root ports and downstream ports' ARP forwarding is disabled.
The device with 'slot > 0' will not recognized by guest os.
 
> I guess you could check for ARI capability instead.
> 
You mean that we check the PCIe devices with ARI capability,
but not root ports and downstream ports? If switch ports don't enable 
ARI Forwarding, we should not permit a device with 'slot > 0'. 
Of course, you said the scenario is also should be checked too.

Maybe I can add a if condition like:

If (!pcie_cap_is_arifwd_enabled(pci_dev) || !pcie_device_ari_cap_set(dev)) {
   error_report();
   return;
}

Any comments? Thanks!

Best regards,
-Gonglei

> > +                if (PCI_SLOT(dev->devfn) != 0) {
> > +                    error_report("PCIe: Port's ARI Forwarding is
> disabled, "
> > +                                 "device can't be populated in
> slot %d",
> > +                                 PCI_SLOT(dev->devfn));
> > +                    return -1;
> > +                }
> > +            }
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static void pci_config_alloc(PCIDevice *pci_dev)
> >  {
> >      int config_size = pci_config_size(pci_dev);
> > @@ -827,6 +873,11 @@ static PCIDevice
> *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >
> >      pci_dev->bus = bus;
> >      pci_dev->devfn = devfn;
> > +
> > +    if (pci_check_pcie_port(bus, pci_dev)) {
> > +        return NULL;
> > +    }
> > +
> >      dma_as = pci_device_iommu_address_space(pci_dev);
> >
> >      memory_region_init_alias(&pci_dev->bus_master_enable_region,
> > --
> > 1.7.12.4
> >