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

[Michael S. Tsirkin]

On Mon, Sep 01, 2014 at 09:29:18PM +0800, address@hidden wrote:
> From: Gonglei <address@hidden>
> 
> 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?

> +{
> +    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.

> +         */
> +        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
>