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

[Marcel Apfelbaum <marcel.a@redhat.com>]

On Wed, 2014-10-01 at 07:26 +0200, Knut Omang wrote:
> On Tue, 2014-09-30 at 21:38 +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, arei.gonglei@huawei.com wrote:
> > > > From: Gonglei <arei.gonglei@huawei.com>
> > > >
> > > > 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)
> 
> This seems very much like the symptoms I see when I use hotplug after
> the latest changes to the hotplug code - do you have something that
> confirms this has something to do with wrong interpretation of device
> IDs? My suspicion is that something has gone wrong or is missing in the
> hotplug logic but I havent had time to dig deeper into it yet.
Can you please describe me the steps to reproduce the issue?
It should work correctly by now, maybe a "surprise removal/addition"
warning and nothing more.

Thank you,
Marcel

> 
> I am just concerned that this might alleviate the symptoms you see but
> not fix the problem itself,
> 
> Knut
> 
> > > 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 <arei.gonglei@huawei.com>
> > > > ---
> > > >  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
> > > >
> > 
> > 
> 
> 
>