From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58559) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJkYW-0006Ws-UI for qemu-devel@nongnu.org; Tue, 19 Aug 2014 10:38:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XJkYS-0000gM-3h for qemu-devel@nongnu.org; Tue, 19 Aug 2014 10:38:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29103) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJkYR-0000f6-ST for qemu-devel@nongnu.org; Tue, 19 Aug 2014 10:38:32 -0400 Message-ID: <1408459079.13594.29.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Tue, 19 Aug 2014 17:37:59 +0300 In-Reply-To: <1408453707-9972-3-git-send-email-arei.gonglei@huawei.com> References: <1408453707-9972-1-git-send-email-arei.gonglei@huawei.com> <1408453707-9972-3-git-send-email-arei.gonglei@huawei.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] pci: add check for pcie root ports and downstream ports List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: arei.gonglei@huawei.com Cc: peter.crosthwaite@xilinx.com, weidong.huang@huawei.com, mst@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, 2014-08-19 at 21:08 +0800, arei.gonglei@huawei.com wrote: > From: Gonglei Hi, > > Right now, ARI Forwarding dose not support in QEMU. I would replace the above sentence with "ARI Forwarding is not supported". By the way, there is some support for ARI, I don't know if is enabled yet. I'll have a look. > According to PCIe spec section 7.3.1, only slot 0 with > the device attached to logic bus representing the link from > downstream ports and root ports. > > So, adding check about slot 0 for PCIe downstream ports and > root ports, which avoid useless operation, both hotplug and > coldplug. > > Signed-off-by: Gonglei > --- > hw/pci/pci.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 351d320..f2d267f 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -773,6 +773,42 @@ static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev) > return 0; > } > > +static int pci_check_pcie_port(PCIBus *bus, PCIDevice *dev) > +{ > + Object *obj = OBJECT(bus); > + > + if (!strcmp(object_get_typename(obj), TYPE_PCIE_BUS)) { Maybe there is another way to check that this is a PCIe bus? > + DeviceState *parent = qbus_get_parent(BUS(obj)); > + const char *name = object_get_typename(OBJECT(parent)); > + > + /* > + * 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. > + * > + * Right now, ARI Forwarding dose not support. So, only slot 0 is As above, maybe replace it with "ARI Forwarding is not supported" > + * supported, regardless of hotplug or coldplug. > + */ > + if (!strcmp(name, "ioh3420") || !strcmp(name, "xio3130-downstream")) { Please use port_type flag from extended configuration space, don't use device names. If you need help for this, let me know. > + if (PCI_SLOT(dev->devfn) != 0) { > + error_report("Unsupported PCI slot %d for %s ports, only " > + "supported slot 0", PCI_SLOT(dev->devfn), name); > + return -1; > + } > + } > + } > + > + return 0; > +} > + > static void pci_config_alloc(PCIDevice *pci_dev) > { > int config_size = pci_config_size(pci_dev); > @@ -871,6 +907,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > return NULL; > } > > + if (pci_check_pcie_port(bus, pci_dev)) { > + do_pci_unregister_device(pci_dev); > + return NULL; > + } It is possible to move the above check earlier in do_pci_register_device function? Maybe move it into the first if statement(s)? Thanks, Marcel > + > if (!config_read) > config_read = pci_default_read_config; > if (!config_write)