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, address@hidden wrote:
> From: Gonglei <address@hidden>
> 
> 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 <address@hidden>
> ---
>  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,