Re: [Qemu-devel] [PATCH v3 2/3] pcie: add check for ari capability of pcie devices

[Gonglei (Arei)]

> >
> > 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 <address@hidden>
> > ---
> >  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 = false;
> >      if (pci_dev->romfile == NULL && pc->romfile != 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)
> 
> What does this function check, really?
> 
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 = 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.
> > +         *
> > +         * 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.
> 
> 
> A bunch of typos in comments and commit log.
> Please run ispell before posting.
> 
OK, my fault! Will check them. :(

Best regards,
-Gonglei

> > +         */
> > +        port_type = pcie_cap_get_type(pci_dev);
> > +        if (port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> > +            port_type == 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) != 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
> >