> From: Michael S. Tsirkin [mailto:m...@redhat.com] > Subject: Re: [PATCH v2 2/2] pci: add check for pcie root ports and downstream > ports > > On Thu, Aug 21, 2014 at 05:47:46PM +0800, arei.gong...@huawei.com wrote: > > From: Gonglei <arei.gong...@huawei.com> > > > > If ARI Forwarding is disabled, 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 for PCIe downstream ports and root ports, > > which avoid useless operation, both hotplug and coldplug. > > > > Signed-off-by: Gonglei <arei.gong...@huawei.com> > > --- > > hw/pci/pci.c | 51 > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 51 insertions(+) > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index daeaeac..aa0af0c 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -773,6 +773,52 @@ 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 (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. > > + * > > + * If ARI Forwarding is not enabled on root ports and downstream > > + * ports, only support the devices with slot non-0, regardless of > > + * hotplug or coldplug. > > + */ > > + port_type = pcie_cap_get_type(pci_dev); > > + if (port_type == PCI_EXP_TYPE_DOWNSTREAM || > > + port_type == PCI_EXP_TYPE_ROOT_PORT) { > > + if (!pcie_cap_is_ari_enabled(pci_dev)) { > > Won't this mean cold-plugging devices is broken?
Yes. If root ports and downstream ports' ARP forwarding is disabled. The device with 'slot > 0' will not recognized by guest os. > I guess you could check for ARI capability instead. > You mean that we check the PCIe devices with ARI capability, but not root ports and downstream ports? If switch ports don't enable ARI Forwarding, we should not permit a device with 'slot > 0'. Of course, you said the scenario is also should be checked too. Maybe I can add a if condition like: If (!pcie_cap_is_arifwd_enabled(pci_dev) || !pcie_device_ari_cap_set(dev)) { error_report(); return; } Any comments? Thanks! Best regards, -Gonglei > > + if (PCI_SLOT(dev->devfn) != 0) { > > + error_report("PCIe: Port's ARI Forwarding is > disabled, " > > + "device can't be populated in > slot %d", > > + PCI_SLOT(dev->devfn)); > > + return -1; > > + } > > + } > > + } > > + } > > + > > + return 0; > > +} > > + > > static void pci_config_alloc(PCIDevice *pci_dev) > > { > > int config_size = pci_config_size(pci_dev); > > @@ -827,6 +873,11 @@ static PCIDevice > *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > > > > pci_dev->bus = bus; > > pci_dev->devfn = devfn; > > + > > + if (pci_check_pcie_port(bus, pci_dev)) { > > + return NULL; > > + } > > + > > dma_as = pci_device_iommu_address_space(pci_dev); > > > > memory_region_init_alias(&pci_dev->bus_master_enable_region, > > -- > > 1.7.12.4 > >