On 2023/07/05 14:43, Ani Sinha wrote:
On 05-Jul-2023, at 7:09 AM, Akihiko Odaki <akihiko.od...@daynix.com> wrote:
On 2023/07/05 0:07, Ani Sinha wrote:
On 04-Jul-2023, at 7:58 PM, Igor Mammedov <imamm...@redhat.com> wrote:
On Tue, 4 Jul 2023 19:20:00 +0530
Ani Sinha <anisi...@redhat.com> wrote:
On 04-Jul-2023, at 6:18 PM, Igor Mammedov <imamm...@redhat.com> wrote:
On Tue, 4 Jul 2023 21:02:09 +0900
Akihiko Odaki <akihiko.od...@daynix.com> wrote:
On 2023/07/04 20:59, Ani Sinha wrote:
On 04-Jul-2023, at 5:24 PM, Akihiko Odaki <akihiko.od...@daynix.com> wrote:
On 2023/07/04 20:25, Ani Sinha wrote:
PCI Express ports only have one slot, so PCI Express devices can only be
plugged into slot 0 on a PCIE port. Add a warning to let users know when the
invalid configuration is used. We may enforce this more strongly later on once
we get more clarity on whether we are introducing a bad regression for users
currenly using the wrong configuration.
The change has been tested to not break or alter behaviors of ARI capable
devices by instantiating seven vfs on an emulated igb device (the maximum
number of vfs the linux igb driver supports). The vfs instantiated correctly
and are seen to have non-zero device/slot numbers in the conventional PCI BDF
representation.
CC: jus...@redhat.com
CC: imamm...@redhat.com
CC: m...@redhat.com
CC: akihiko.od...@daynix.com
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
Signed-off-by: Ani Sinha <anisi...@redhat.com>
Reviewed-by: Julia Suvorova <jus...@redhat.com>
---
hw/pci/pci.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e2eb4c3b4a..47517ba3db 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -65,6 +65,7 @@ bool pci_available = true;
static char *pcibus_get_dev_path(DeviceState *dev);
static char *pcibus_get_fw_dev_path(DeviceState *dev);
static void pcibus_reset(BusState *qbus);
+static bool pcie_has_upstream_port(PCIDevice *dev);
static Property pci_props[] = {
DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
@@ -2121,6 +2122,20 @@ static void pci_qdev_realize(DeviceState *qdev, Error
**errp)
}
}
+ /*
+ * With SRIOV and ARI, vfs can have non-zero slot in the conventional
+ * PCI interpretation as all five bits reserved for slot addresses are
+ * also used for function bits for the various vfs. Ignore that case.
You don't have to mention SR/IOV; it affects all ARI-capable devices. A PF can
also have non-zero slot number in the conventional interpretation so you
shouldn't call it vf either.
Can you please help write a comment that explains this properly for all cases -
ARI/non-ARI, PFs and VFs? Once everyone agrees that its clear and correct, I
will re-spin.
Simply, you can say:
With ARI, the slot number field in the conventional PCI interpretation
can have a non-zero value as the field bits are reused to extend the
function number bits. Ignore that case.
mentioning 'conventional PCI interpretation' in comment and then immediately
checking 'pci_is_express(pci_dev)' is confusing. Since comment belongs
only to PCIE branch it would be better to talk in only about PCIe stuff
and referring to relevant portions of spec.
Ok so how about this?
* With ARI, devices can have non-zero slot in the traditional BDF
* representation as all five bits reserved for slot addresses are
* also used for function bits. Ignore that case.
you still refer to traditional (which I misread as 'conventional'),
steal the linux comment and argument it with ARI if necessary,
something like this (probably needs some more massaging):
The comment messaging in these patches seems to exceed the value of the patch
itself :-)
How about this?
/*
* A PCIe Downstream Port normally leads to a Link with only Device
* 0 on it (PCIe spec r3.1, sec 7.3.1).
* With ARI, PCI_SLOT() can return non-zero value as all five bits
* reserved for slot addresses are also used for function bits.
* Hence, ignore ARI capable devices.
*/
Perhaps: s/normally leads to/must lead to/
From the kernel perspective, they may need to deal with a quirky hardware that
does not conform with the specification, but from QEMU perspective, it is what
we *must* conform with.
PCI base spec 4.0, rev 3, section 7.3.1 says:
"
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. Configuration Requests 15 targeting the Bus Number associated with a
Link specifying Device Number 0 are delivered to the device attached to the
Link; Configuration Requests specifying all other Device Numbers (1-31) must be
terminated by the Switch Downstream Port or the Root Port with an Unsupported
Request Completion Status (equivalent to Master Abort in PCI). Non-ARI Devices
must not assume that Device Number 0 is associated with their Upstream Port,
but must capture their assigned Device Number as discussed in Section 2.2.6.2.
Non-ARI Devices must respond to all Type 0 Configuration Read Requests,
regardless of the Device Number specified in the Request.
…
With an ARI Device, its Device Number is implied to be 0 rather than specified
by a field within an ID. The traditional 5-bit Device Number and 3-bit Function
Number fields in its associated Routing IDs, Requester IDs, and Completer IDs
are interpreted as a single 8-bit Function Number. See Section 6.13. Any Type 0
Configuration Request targeting an unimplemented Function in an ARI Device must
be handled as an Unsupported Request.
“
So it seems they do indeed use the “must” clause. I prefer to use the line from
the spec verbatim as possible. Hence, this is what I am going with and be done
with this patchset:
/*
* A PCIe Downstream Port that do not have ARI Forwarding enabled must
* associate only Device 0 with the device attached to the bus
* representing the Link from the Port (PCIe base spec rev 4.0 ver 0.3,
* sec 7.3.1).
* With ARI, PCI_SLOT() can return non-zero value as the traditional
* 5-bit Device Number and 3-bit Function Number fields in its associated
* Routing IDs, Requester IDs and Completer IDs are interpreted as a
* single 8-bit Function Number. Hence, ignore ARI capable devices.
*/
Looks perfect.
Otherwise looks good to me.
/*
* A PCIe Downstream Port normally leads to a Link with only Device
* 0 on it (PCIe spec r3.1, sec 7.3.1).
However PCI_SLOT() is broken if ARI is enabled, hence work around it
by skipping check if the later cap is present.
*/
(for example see how it's done in kernel code: only_one_child(...)
PS:
kernel can be forced to scan for !0 device numbers, but that's rather
a hack, so we shouldn't really care about that.
+ */
+ if (pci_is_express(pci_dev) &&
+ !pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) &&
+ pcie_has_upstream_port(pci_dev) &&
+ PCI_SLOT(pci_dev->devfn)) {
+ warn_report("PCI: slot %d is not valid for %s,"
+ " parent device only allows plugging into slot 0.",
+ PCI_SLOT(pci_dev->devfn), pci_dev->name);
+ }
+
if (pci_dev->failover_pair_id) {
if (!pci_bus_is_express(pci_get_bus(pci_dev))) {
error_setg(errp, "failover primary device must be on "
