> Subject: Re: [Qemu-devel] [PATCH v4 2/3] pcie: add check for ari capability of > pcie devices > > 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, arei.gong...@huawei.com > wrote: > > > > From: Gonglei <arei.gong...@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, > > Sigh. > What did you do to make it not work?
I configured a device with 'slot > 0'. > Which device did you try? virtio-net-pci device. > By comparison, what happens after your patch is applied? > Please see my previous email answering you : http://lists.gnu.org/archive/html/qemu-devel/2014-09/msg05781.html > > > > 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-in > terpretation-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.gong...@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 > > 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? > Good catch. Yes! Thanks. 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 > > > > > >
