From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57046) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYw23-0002WD-3Y for qemu-devel@nongnu.org; Tue, 30 Sep 2014 07:55:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XYw1y-0003rx-8L for qemu-devel@nongnu.org; Tue, 30 Sep 2014 07:55:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46278) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYw1x-0003qg-RL for qemu-devel@nongnu.org; Tue, 30 Sep 2014 07:55:46 -0400 Date: Tue, 30 Sep 2014 14:59:00 +0300 From: "Michael S. Tsirkin" Message-ID: <20140930115900.GA9230@redhat.com> References: <1412071886-10192-1-git-send-email-arei.gonglei@huawei.com> <1412071886-10192-3-git-send-email-arei.gonglei@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1412071886-10192-3-git-send-email-arei.gonglei@huawei.com> Subject: Re: [Qemu-devel] [PATCH v4 2/3] pcie: add check for ari capability of pcie devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: arei.gonglei@huawei.com Cc: peter.crosthwaite@xilinx.com, knut.omang@oracle.com, weidong.huang@huawei.com, marcel.a@redhat.com, armbru@redhat.com, luonengjun@huawei.com, qemu-devel@nongnu.org, peter.huangpeng@huawei.com, imammedo@redhat.com, pbonzini@redhat.com, afaerber@suse.de On Tue, Sep 30, 2014 at 06:11:25PM +0800, arei.gonglei@huawei.com wrote: > From: Gonglei > > 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? Please make this explicit in the commit log. > > 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 > > Signed-off-by: Gonglei > --- > 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/ > + * ports. ARI Forwarding enable setting at firmware/OS Control handoff. can parse last sentence. drop it? > + * 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? > + */ > + 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. > + 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 >