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: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)

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