From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48567) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYr6B-0002T4-M8 for qemu-devel@nongnu.org; Tue, 30 Sep 2014 02:39:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XYr65-0005sU-T5 for qemu-devel@nongnu.org; Tue, 30 Sep 2014 02:39:47 -0400 Received: from szxga01-in.huawei.com ([119.145.14.64]:30503) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYr65-0005r1-1M for qemu-devel@nongnu.org; Tue, 30 Sep 2014 02:39:41 -0400 From: "Gonglei (Arei)" Date: Tue, 30 Sep 2014 06:39:11 +0000 Message-ID: <33183CC9F5247A488A2544077AF1902086DDD497@SZXEMA503-MBS.china.huawei.com> References: <1409578159-9576-1-git-send-email-arei.gonglei@huawei.com> <1409578159-9576-3-git-send-email-arei.gonglei@huawei.com> <20140929160912.GA21268@redhat.com> In-Reply-To: <20140929160912.GA21268@redhat.com> Content-Language: zh-CN Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v3 2/3] pcie: add check for ari capability of pcie devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: "peter.crosthwaite@xilinx.com" , "Huangweidong (C)" , "marcel.a@redhat.com" , "armbru@redhat.com" , Luonengjun , "qemu-devel@nongnu.org" , "Huangpeng (Peter)" , "imammedo@redhat.com" , "pbonzini@redhat.com" , "afaerber@suse.de" > > > > In QEMU, ARI Forwarding is enabled defualt 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, for pci devices attached in pcie root ports or downstream pots, > > we shoud assure that its slot is non-zero. For pcie devcies, which > > ARP capbility is not enabled, we also should assure that its slot > > is non-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 daeaeac..a9b8c3e 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -1769,6 +1769,10 @@ static int pci_qdev_init(DeviceState *qdev) > > } > > } > > > > + if (pcie_cap_ari_check(bus, pci_dev)) { > > + return -1; > > + } > > + > > /* rom loading */ > > is_default_rom =3D false; > > if (pci_dev->romfile =3D=3D NULL && pc->romfile !=3D NULL) { > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > index 1babddf..30dd481 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_ari_check(PCIBus *bus, PCIDevice *dev) >=20 > What does this function check, really? >=20 Checking dev's slot is zero or not when the bus is PCIe bus. Maybe I should change the name of this function. Named pcie_slot_check()? > > +{ > > + Object *obj =3D OBJECT(bus); > > + > > + if (pci_bus_is_root(bus)) { > > + return 0; > > + } > > + > > + if (object_dynamic_cast(obj, TYPE_PCIE_BUS)) { > > + DeviceState *parent =3D qbus_get_parent(BUS(obj)); > > + PCIDevice *pci_dev =3D 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 specifica= tion > 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 Log= ical > Bus > > + * representing the Link from the Port. > > + * > > + * In QEMU, ARI Forwarding is enabled defualt 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 Funct= ions > can > > + * be aliased under multiple Device Numbers, generally leading= to > > + * undesired behavior. >=20 >=20 > A bunch of typos in comments and commit log. > Please run ispell before posting. >=20 OK, my fault! Will check them. :( Best regards, -Gonglei > > + */ > > + port_type =3D pcie_cap_get_type(pci_dev); > > + if (port_type =3D=3D PCI_EXP_TYPE_DOWNSTREAM || > > + port_type =3D=3D PCI_EXP_TYPE_ROOT_PORT) { > > + if (!pci_is_express(dev) || > > + (pci_is_express(dev) && > > + !pcie_find_capability(dev, PCI_EXT_CAP_ID_ARI))) { > > + if (PCI_SLOT(dev->devfn) !=3D 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..ab2a44a 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_ari_check(PCIBus *bus, PCIDevice *dev); > > > > extern const VMStateDescription vmstate_pcie_device; > > > > -- > > 1.7.12.4 > >