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

[Michael S. Tsirkin]

On Tue, Sep 30, 2014 at 09:38:51PM +0800, Gonglei wrote:
> > 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,

Sigh.
What did you do to make it not work?
Which device did you try?
By comparison, what happens after your patch is applied?



> 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

First this is equivalent, and clearer:
           if (!pci_is_express(dev) ||
               !pcie_find_capability(dev, PCI_EXT_CAP_ID_ARI))) {

Second do we need to do the check for non express devices?



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