diff mbox

[2/2] pci: add check for pcie root ports and downstream ports

Message ID 1408453707-9972-3-git-send-email-arei.gonglei@huawei.com
State New
Headers show

Commit Message

Gonglei (Arei) Aug. 19, 2014, 1:08 p.m. UTC 
From: Gonglei <arei.gonglei@huawei.com>

Right now, ARI Forwarding dose not support in QEMU.
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 about slot 0 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 | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

Comments

Marcel Apfelbaum Aug. 19, 2014, 2:37 p.m. UTC | #1 
On Tue, 2014-08-19 at 21:08 +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
Hi,

> 
> Right now, ARI Forwarding dose not support in QEMU.
I would replace the above sentence with "ARI Forwarding is not supported".

By the way, there is some support for ARI, I don't know if
is enabled yet. I'll have a look.

> 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 about slot 0 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 | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 351d320..f2d267f 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -773,6 +773,42 @@ 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 (!strcmp(object_get_typename(obj), TYPE_PCIE_BUS)) {
Maybe there is another way to check that this is a PCIe bus?
 
> +        DeviceState *parent = qbus_get_parent(BUS(obj));
> +        const char *name = object_get_typename(OBJECT(parent));
> +
> +        /*
> +         * 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.
> +         *
> +         * Right now, ARI Forwarding dose not support. So, only slot 0 is
As above, maybe replace it with "ARI Forwarding is not supported"

> +         * supported, regardless of hotplug or coldplug.
> +         */
> +        if (!strcmp(name, "ioh3420") || !strcmp(name, "xio3130-downstream")) {
Please use port_type flag from extended configuration space, don't use device names.
If you need help for this, let me know.

> +            if (PCI_SLOT(dev->devfn) != 0) {
> +                error_report("Unsupported PCI slot %d for %s ports, only "
> +                             "supported slot 0", PCI_SLOT(dev->devfn), name);
> +                return -1;
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static void pci_config_alloc(PCIDevice *pci_dev)
>  {
>      int config_size = pci_config_size(pci_dev);
> @@ -871,6 +907,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>          return NULL;
>      }
>  
> +    if (pci_check_pcie_port(bus, pci_dev)) {
> +        do_pci_unregister_device(pci_dev);
> +        return NULL;
> +    }
It is possible to move the above check earlier in do_pci_register_device function?
Maybe move it into the first if statement(s)?

Thanks,
Marcel

> +
>      if (!config_read)
>          config_read = pci_default_read_config;
>      if (!config_write)
Paolo Bonzini Aug. 19, 2014, 3:09 p.m. UTC | #2 
Il 19/08/2014 16:37, Marcel Apfelbaum ha scritto:
>> > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev)
>> > +{
>> > +    Object *obj = OBJECT(bus);
>> > +
>> > +    if (!strcmp(object_get_typename(obj), TYPE_PCIE_BUS)) {
> Maybe there is another way to check that this is a PCIe bus?
>  

It's object_dynamic_cast.

Paolo
Gonglei (Arei) Aug. 20, 2014, 3:10 a.m. UTC | #3 
> -----Original Message-----

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]

> Sent: Tuesday, August 19, 2014 11:09 PM

> To: Marcel Apfelbaum; Gonglei (Arei)

> Cc: qemu-devel@nongnu.org; mst@redhat.com; Huangweidong (C);

> afaerber@suse.de; imammedo@redhat.com; peter.crosthwaite@xilinx.com;

> Huangpeng (Peter); armbru@redhat.com; Luonengjun

> Subject: Re: [PATCH 2/2] pci: add check for pcie root ports and downstream

> ports

> 

> Il 19/08/2014 16:37, Marcel Apfelbaum ha scritto:

> >> > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev)

> >> > +{

> >> > +    Object *obj = OBJECT(bus);

> >> > +

> >> > +    if (!strcmp(object_get_typename(obj), TYPE_PCIE_BUS)) {

> > Maybe there is another way to check that this is a PCIe bus?

> >

> 

> It's object_dynamic_cast.

> 

> Paolo


Great! Thanks. :)

Best regards,
-Gonglei
Gonglei (Arei) Aug. 20, 2014, 3:20 a.m. UTC | #4 
Hi,

> > Right now, ARI Forwarding dose not support in QEMU.

> I would replace the above sentence with "ARI Forwarding is not supported".

> 

OK.

> By the way, there is some support for ARI, I don't know if

> is enabled yet. I'll have a look.

> 

MST had pointed out the pcie_ari_init(), but not completed.

> > 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 about slot 0 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 | 41 +++++++++++++++++++++++++++++++++++++++++

> >  1 file changed, 41 insertions(+)

> >

> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c

> > index 351d320..f2d267f 100644

> > --- a/hw/pci/pci.c

> > +++ b/hw/pci/pci.c

> > @@ -773,6 +773,42 @@ 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 (!strcmp(object_get_typename(obj), TYPE_PCIE_BUS)) {

> Maybe there is another way to check that this is a PCIe bus?

> 

Yes. Paolo has said that object_dynamic_cast() is appropriated.

> > +        DeviceState *parent = qbus_get_parent(BUS(obj));

> > +        const char *name = object_get_typename(OBJECT(parent));

> > +

> > +        /*

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

> > +         *

> > +         * Right now, ARI Forwarding dose not support. So, only slot 0 is

> As above, maybe replace it with "ARI Forwarding is not supported"

> 

OK.

> > +         * supported, regardless of hotplug or coldplug.

> > +         */

> > +        if (!strcmp(name, "ioh3420") || !strcmp(name,

> "xio3130-downstream")) {

> Please use port_type flag from extended configuration space, don't use device

> names.

> If you need help for this, let me know.

> 

Yes, please. I appreciate very much that you can help me.

> > +            if (PCI_SLOT(dev->devfn) != 0) {

> > +                error_report("Unsupported PCI slot %d for %s ports,

> only "

> > +                             "supported slot 0",

> PCI_SLOT(dev->devfn), name);

> > +                return -1;

> > +            }

> > +        }

> > +    }

> > +

> > +    return 0;

> > +}

> > +

> >  static void pci_config_alloc(PCIDevice *pci_dev)

> >  {

> >      int config_size = pci_config_size(pci_dev);

> > @@ -871,6 +907,11 @@ static PCIDevice

> *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,

> >          return NULL;

> >      }

> >

> > +    if (pci_check_pcie_port(bus, pci_dev)) {

> > +        do_pci_unregister_device(pci_dev);

> > +        return NULL;

> > +    }

> It is possible to move the above check earlier in do_pci_register_device

> function?

> Maybe move it into the first if statement(s)?

> 

Agreed, Thanks, Marcel.

Best regards,
-Gonglei
Marcel Apfelbaum Aug. 20, 2014, 8:47 a.m. UTC | #5 
On Wed, 2014-08-20 at 03:20 +0000, Gonglei (Arei) wrote:
> Hi,
> 
> > > Right now, ARI Forwarding dose not support in QEMU.
> > I would replace the above sentence with "ARI Forwarding is not supported".
> > 
> OK.
> 
> > By the way, there is some support for ARI, I don't know if
> > is enabled yet. I'll have a look.
> > 
> MST had pointed out the pcie_ari_init(), but not completed.
> 
> > > 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 about slot 0 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 | 41 +++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 41 insertions(+)
> > >
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 351d320..f2d267f 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -773,6 +773,42 @@ 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 (!strcmp(object_get_typename(obj), TYPE_PCIE_BUS)) {
> > Maybe there is another way to check that this is a PCIe bus?
> > 
> Yes. Paolo has said that object_dynamic_cast() is appropriated.
> 
> > > +        DeviceState *parent = qbus_get_parent(BUS(obj));
> > > +        const char *name = object_get_typename(OBJECT(parent));
> > > +
> > > +        /*
> > > +         * 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.
> > > +         *
> > > +         * Right now, ARI Forwarding dose not support. So, only slot 0 is
> > As above, maybe replace it with "ARI Forwarding is not supported"
> > 
> OK.
> 
> > > +         * supported, regardless of hotplug or coldplug.
> > > +         */
> > > +        if (!strcmp(name, "ioh3420") || !strcmp(name,
> > "xio3130-downstream")) {
> > Please use port_type flag from extended configuration space, don't use device
> > names.
> > If you need help for this, let me know.
> > 
> Yes, please. I appreciate very much that you can help me.
Sure,

I checked and we already have the pcie_cap_get_type function that returns the port type.

    port_type = pcie_cap_get_type(dev);
    if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
        port_type == PCI_EXP_TYPE_ROOT_PORT) {
        ...
    }

Thanks,
Marcel

> 
> > > +            if (PCI_SLOT(dev->devfn) != 0) {
> > > +                error_report("Unsupported PCI slot %d for %s ports,
> > only "
> > > +                             "supported slot 0",
> > PCI_SLOT(dev->devfn), name);
> > > +                return -1;
> > > +            }
> > > +        }
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  static void pci_config_alloc(PCIDevice *pci_dev)
> > >  {
> > >      int config_size = pci_config_size(pci_dev);
> > > @@ -871,6 +907,11 @@ static PCIDevice
> > *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> > >          return NULL;
> > >      }
> > >
> > > +    if (pci_check_pcie_port(bus, pci_dev)) {
> > > +        do_pci_unregister_device(pci_dev);
> > > +        return NULL;
> > > +    }
> > It is possible to move the above check earlier in do_pci_register_device
> > function?
> > Maybe move it into the first if statement(s)?
> > 
> Agreed, Thanks, Marcel.
> 
> Best regards,
> -Gonglei
Gonglei (Arei) Aug. 20, 2014, 9:06 a.m. UTC | #6 
> From: Marcel Apfelbaum [mailto:marcel.a@redhat.com]

> Subject: Re: [PATCH 2/2] pci: add check for pcie root ports and downstream

> ports

> 

> On Wed, 2014-08-20 at 03:20 +0000, Gonglei (Arei) wrote:

> > Hi,

> >

> > > > Right now, ARI Forwarding dose not support in QEMU.

> > > I would replace the above sentence with "ARI Forwarding is not supported".

> > >

> > OK.

> >

> > > By the way, there is some support for ARI, I don't know if

> > > is enabled yet. I'll have a look.

> > >

> > MST had pointed out the pcie_ari_init(), but not completed.

> >

> > > > 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 about slot 0 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 | 41 +++++++++++++++++++++++++++++++++++++++++

> > > >  1 file changed, 41 insertions(+)

> > > >

> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c

> > > > index 351d320..f2d267f 100644

> > > > --- a/hw/pci/pci.c

> > > > +++ b/hw/pci/pci.c

> > > > @@ -773,6 +773,42 @@ 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 (!strcmp(object_get_typename(obj), TYPE_PCIE_BUS)) {

> > > Maybe there is another way to check that this is a PCIe bus?

> > >

> > Yes. Paolo has said that object_dynamic_cast() is appropriated.

> >

> > > > +        DeviceState *parent = qbus_get_parent(BUS(obj));

> > > > +        const char *name = object_get_typename(OBJECT(parent));

> > > > +

> > > > +        /*

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

> > > > +         *

> > > > +         * Right now, ARI Forwarding dose not support. So, only slot 0

> is

> > > As above, maybe replace it with "ARI Forwarding is not supported"

> > >

> > OK.

> >

> > > > +         * supported, regardless of hotplug or coldplug.

> > > > +         */

> > > > +        if (!strcmp(name, "ioh3420") || !strcmp(name,

> > > "xio3130-downstream")) {

> > > Please use port_type flag from extended configuration space, don't use

> device

> > > names.

> > > If you need help for this, let me know.

> > >

> > Yes, please. I appreciate very much that you can help me.

> Sure,

> 

> I checked and we already have the pcie_cap_get_type function that returns the

> port type.

> 

>     port_type = pcie_cap_get_type(dev);

>     if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||

>         port_type == PCI_EXP_TYPE_ROOT_PORT) {

>         ...

>     }

> 

Great! Thanks a lot. :)

Best regards,
-Gonglei
diff mbox

Patch

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 351d320..f2d267f 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -773,6 +773,42 @@  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 (!strcmp(object_get_typename(obj), TYPE_PCIE_BUS)) {
+        DeviceState *parent = qbus_get_parent(BUS(obj));
+        const char *name = object_get_typename(OBJECT(parent));
+
+        /*
+         * 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.
+         *
+         * Right now, ARI Forwarding dose not support. So, only slot 0 is
+         * supported, regardless of hotplug or coldplug.
+         */
+        if (!strcmp(name, "ioh3420") || !strcmp(name, "xio3130-downstream")) {
+            if (PCI_SLOT(dev->devfn) != 0) {
+                error_report("Unsupported PCI slot %d for %s ports, only "
+                             "supported slot 0", PCI_SLOT(dev->devfn), name);
+                return -1;
+            }
+        }
+    }
+
+    return 0;
+}
+
 static void pci_config_alloc(PCIDevice *pci_dev)
 {
     int config_size = pci_config_size(pci_dev);
@@ -871,6 +907,11 @@  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
         return NULL;
     }
 
+    if (pci_check_pcie_port(bus, pci_dev)) {
+        do_pci_unregister_device(pci_dev);
+        return NULL;
+    }
+
     if (!config_read)
         config_read = pci_default_read_config;
     if (!config_write)