Re: [Qemu-devel] [PATCH v4 2/3] pcie: add check for ari capability of pcie devices

[Gonglei]

> Subject: Re: [Qemu-devel] [PATCH v4 2/3] pcie: add check for ari capability of
> pcie devices
> 
> On Tue, Sep 30, 2014 at 06:11:25PM +0800, address@hidden wrote:
> > From: Gonglei <address@hidden>
> >
> > In QEMU, ARI Forwarding is enabled default at emulation of PCIe
> > ports. ARI Forwarding enable setting at firmware/OS Control handoff.
> > If the bit is Set when a non-ARI Device is present, the non-ARI
> > Device can respond to Configuration Space accesses under what it
> > interprets as being different Device Numbers, and its Functions can
> > be aliased under multiple Device Numbers, generally leading to
> > undesired behavior.
> 
> So what is the undesired behaviour?
> Did you observe such?

I just observe the PCI device don't work, and the dmesg show me:

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

> Please make this explicit in the commit log.
> 
Sorry, I copied the description from PCIe spec. :(

IMPLEMENTATION NOTE at Page 19:

https://www.pcisig.com/specifications/pciexpress/specifications/ECN-alt-rid-interpretation-070604.pdf


> 
> >
> > So, for PCI devices attached in PCIe root ports or downstream pots,
> > we should assure that its slot is not non-zero. For PCIe devices, which
> > ARP capability is not enabled, we also should assure that its slot
> > is not non-zero.
> 
> not non zero => zero
> 
OK.

> >
> > Signed-off-by: Gonglei <address@hidden>
> > ---
> >  hw/pci/pci.c          |  4 ++++
> >  hw/pci/pcie.c         | 51
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/pci/pcie.h |  1 +
> >  3 files changed, 56 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 6ce75aa..22b7ca0 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1770,6 +1770,10 @@ static int pci_qdev_init(DeviceState *qdev)
> >          }
> >      }
> >
> > +    if (pcie_cap_slot_check(bus, pci_dev)) {
> > +        return -1;
> > +    }
> > +
> >      /* rom loading */
> >      is_default_rom = false;
> >      if (pci_dev->romfile == NULL && pc->romfile != NULL) {
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 1babddf..b82211a 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -633,3 +633,54 @@ void pcie_ari_init(PCIDevice *dev, uint16_t offset,
> uint16_t nextfn)
> >                          offset, PCI_ARI_SIZEOF);
> >      pci_set_long(dev->config + offset + PCI_ARI_CAP, (nextfn & 0xff) << 8);
> >  }
> > +
> > +int pcie_cap_slot_check(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.
> > +         *
> > +         * In QEMU, ARI Forwarding is enabled default at emulation of
> PCIe
> 
> s/enabled default/enabled by default/
> 
OK.

> > +         * ports. ARI Forwarding enable setting at firmware/OS Control
> handoff.
> 
> 
> can parse last sentence. drop it?
> 
OK.

> > +         * If the bit is Set when a non-ARI Device is present, the non-ARI
> > +           Device can respond to Configuration Space accesses under
> what it
> > +         * interprets as being different Device Numbers, and its Functions
> can
> > +         * be aliased under multiple Device Numbers, generally leading to
> > +         * undesired behavior.
> 
> I don't think any badness really happens.
> Did you observe any?
> 
Same with above explanation.

> 
> 
> > +         */
> > +        port_type = pcie_cap_get_type(pci_dev);
> > +        if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> > +            port_type == PCI_EXP_TYPE_ROOT_PORT) {
> > +            if (!pci_is_express(dev) ||
> > +                (pci_is_express(dev) &&
> > +                !pcie_find_capability(dev, PCI_EXT_CAP_ID_ARI))) {
> 
> I would just skip the test for a non express function.
> 
Sorry?

Best regards,
-Gonglei

> > +                if (PCI_SLOT(dev->devfn) != 0) {
> > +                    error_report("PCIe: non-ARI device can't be
> populated"
> > +                                 " in slot %d",
> PCI_SLOT(dev->devfn));
> > +                    return -1;
> > +                }
> > +            }
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> > index d139d58..1d8f3f4 100644
> > --- a/include/hw/pci/pcie.h
> > +++ b/include/hw/pci/pcie.h
> > @@ -115,6 +115,7 @@ void pcie_add_capability(PCIDevice *dev,
> >                           uint16_t offset, uint16_t size);
> >
> >  void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
> > +int pcie_cap_slot_check(PCIBus *bus, PCIDevice *dev);
> >
> >  extern const VMStateDescription vmstate_pcie_device;
> >
> > --
> > 1.7.12.4
> >