[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 2/3] pcie: add check for ari capability of pc
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v4 2/3] pcie: add check for ari capability of pcie devices |
Date: |
Tue, 30 Sep 2014 14:59:00 +0300 |
On Tue, Sep 30, 2014 at 06:11:25PM +0800, address@hidden wrote:
> From: Gonglei <address@hidden>
>
> 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 <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 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
>