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

[Gonglei (Arei)]

Hi,

> > Right now, ARI Forwarding dose not support in QEMU.
> I would replace the above sentence with "ARI Forwarding is not supported".
> 
OK.

> By the way, there is some support for ARI, I don't know if
> is enabled yet. I'll have a look.
> 
MST had pointed out the pcie_ari_init(), but not completed.

> > 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 <address@hidden>
> > ---
> >  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?
> 
Yes. Paolo has said that object_dynamic_cast() is appropriated.

> > +        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"
> 
OK.

> > +         * 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.
> 
Yes, please. I appreciate very much that you can help me.

> > +            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)?
> 
Agreed, Thanks, Marcel.

Best regards,
-Gonglei