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 11:23 +0000, Gonglei (Arei) wrote:
> > -----Original Message-----
> > From: Knut Omang [mailto:address@hidden
> > Sent: Monday, August 25, 2014 7:02 PM
> > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports 
> > and
> > downstream ports
> > 
> > On Mon, 2014-08-25 at 10:51 +0000, Gonglei (Arei) wrote:
> > > > From: Knut Omang [mailto:address@hidden
> > > > Sent: Monday, August 25, 2014 5:51 PM
> > > > Subject: Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root 
> > > > ports
> > and
> > > > downstream ports
> > > >
> > > > 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?
> > > >
> > > Sorry for my poor English.
> > > What's your mean about " a single ARI capable device..." ?
> > 
> > I just meant a single device with an ARI PCI express extended
> > capability, as opposed to the situation you describe (if I understand
> > right) where there are multiple PCIe devices involved, each in different
> > slots on the same switch.
> > 
> OK. Thanks for your explanation.
> 
> > This can happen if a device provides a potential for more than 8
> > functions, and as such "cross into" the function space of the next
> > "slot", in my case on a root port which physically speaking has only a
> > single slot, but where the device nonetheless are able to respond to and
> > send requests on devfn 1.x.
> > 
> I think you are right. But I haven't tested this case, TBH.

> Knut, could you tell me how to enable the ARI Forwarding for a PCIe device?
> Just call the pcie_ari_init function? 

That would add the ARI extended capability to the list of PCIe
capabilities in config space, but then you need to have a device with
enough functions to actually need ARI, eg. more than 8.

> Which kind of device do you use? Thanks!

This is where the SR/IOV patch comes in, working on it,...

Knut

> 
> Best regards,
> -Gonglei
> 
> > Knut
> > 
> > > Best regards,
> > > -Gonglei
> > 
>