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:address@hidden
> 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, 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)
> > +{
> > +    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,
>