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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Sep 30, 2014 at 06:11:25PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> In QEMU, ARI Forwarding is enabled default 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 what is the undesired behaviour?
Did you observe such?
Please make this explicit in the commit log.




> 
> So, for PCI devices attached in PCIe root ports or downstream pots,
> we should assure that its slot is not non-zero. For PCIe devices, which
> ARP capability is not enabled, we also should assure that its slot
> is not non-zero.

not non zero => zero

> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  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 6ce75aa..22b7ca0 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1770,6 +1770,10 @@ static int pci_qdev_init(DeviceState *qdev)
>          }
>      }
>  
> +    if (pcie_cap_slot_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..b82211a 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_slot_check(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.
> +         *
> +         * In QEMU, ARI Forwarding is enabled default at emulation of PCIe

s/enabled default/enabled by default/

> +         * ports. ARI Forwarding enable setting at firmware/OS Control handoff.


can parse last sentence. drop it?

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

I don't think any badness really happens.
Did you observe any?



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

I would just skip the test for a non express function.

> +                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..1d8f3f4 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_slot_check(PCIBus *bus, PCIDevice *dev);
>  
>  extern const VMStateDescription vmstate_pcie_device;
>  
> -- 
> 1.7.12.4
>