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

[Marcel Apfelbaum]

On Tue, 2014-08-19 at 21:08 +0800, address@hidden wrote:
> From: Gonglei <address@hidden>
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 <address@hidden>
> ---
>  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)