Re: [Qemu-devel] [PATCH v2 2/2] pci: add check for pcie root ports and downstream ports

[Gonglei (Arei)]

> From: Michael S. Tsirkin [mailto:address@hidden
> 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, address@hidden wrote:
> > From: Gonglei <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> >  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
> >