Improve PCI device post-reset readiness polling

From: Stanislav Spassov <[email protected]>

Arguably, since CRS SV is a capability of Root Ports and determines how Root Ports will handle incoming CRS Completions, it makes more sense to store this flag in the struct pci_bus that represents the port's secondary bus, and to cache it in any buses further down the hierarchy.

However, storing the flag in struct pci_dev allows individual devices to be marked as not supporting CRS properly even when CRS SV is enabled on their parent Root Port. This way, future code that relies on the new flag will not be misled that it is safe to probe a device by relying on CRS SV to not cause platform timeouts (See the note on CRS SV on p. 553 of PCI Express Base Specification r5.0 from May 22, 2019).

Note: Endpoints integrated into the Root Complex (RCiEP) may also be capable of using CRS. In that case, the software visibility is controlled using a Root Complex Register Block (RCRB). This is currently not supported by the kernel. The code introduced here would simply not set the newly introduced flag for RCiEP as for those bus->self is NULL.

Signed-off-by Stanislav Spassov <[email protected]>

---
 drivers/pci/probe.c | 8 +++++++-
 include/linux/pci.h | 3 +++
 2 files changed, 10 insertions(+), 1 deletion(-)

drivers/pci/probe.c +0 -0

index 512cb4312ddd..eeff8a07f330 100644

drivers/pci/probe.c +0 -0

 	ret = pcibios_add_device(dev);
 	WARN_ON(ret < 0);
 

include/linux/pci.h +3 -0

index 3840a541a9de..1c222c7c2572 100644
@@ -354,6 +354,9 @@ struct pci_dev {
 						      user sysfs */
 	unsigned int	clear_retrain_link:1;	/* Need to clear Retrain Link
 						   bit manually */
+	unsigned int	crssv_enabled:1;	/* Configuration Request Retry
+						   Status Software Visibility
+						   enabled on (parent) bridge */
 	unsigned int	d3_delay;	/* D3->D0 transition time in ms */
 	unsigned int	d3cold_delay;	/* D3cold->D0 transition time in ms */
 

Signature · 12 lines

2.25.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Quoted 6 lines · Stanislav Spassov

However, storing the flag in struct pci_dev allows individual devices to be marked as not supporting CRS properly even when CRS SV is enabled on their parent Root Port. This way, future code that relies on the new flag will not be misled that it is safe to probe a device by relying on CRS SV to not cause platform timeouts (See the note on CRS SV on p. 553 of PCI Express Base Specification r5.0 from May 22, 2019).

If we find devices that don't support CRS properly, I think we should quirk them directly with something other than "crssv_enabled".

drivers/pci/pci.c static inline int pci_dev_poll_until_not_equal(struct pci_dev *dev, int where, · line 1104

Quoted 6 lines · Stanislav Spassov

drivers/pci/pci.c +6 -0

+		rc = pci_dev_poll_until_not_equal(dev, PCI_VENDOR_ID, 0xffff,
+						  0x0001, reset_type, timeout,
+						  &waited, &id);
+		if (rc)
+			return rc;
+

If I remember right, this doesn't work for VF sending CRS because VF always returns 0xffff for VENDOR_ID register.

Quoted 10 lines · Sinan Kaya

drivers/pci/pci.c static inline int pci_dev_poll_until_not_equal(struct pci_dev *dev, int where, · line 1104

Quoted 6 lines · Stanislav Spassov

drivers/pci/pci.c +6 -0

+           rc = pci_dev_poll_until_not_equal(dev, PCI_VENDOR_ID, 0xffff,
+                                             0x0001, reset_type, timeout,
+                                             &waited, &id);
+           if (rc)
+                   return rc;
+

If I remember right, this doesn't work for VF sending CRS because VF always returns 0xffff for VENDOR_ID register.

Is this required by the PCIe spec? i think the only requirement is the 1s wait after PF has done the VF enable. See Implementation Note right above section 2.3.1.1 in the Base spec 5.0.

If this behavior is different for maybe a specific SRIOV device we should probably quirk the standard behavior?

The rules are mentioned in so many places, but looking through the SRIOV section's doesn't seem to specify special rules for VF's other than the wait time after VF enable.

So, whenever a VF is providing an actual response to a vid/did read, it will respond with a Successful Completion and the data will be 0xFFFF. However, when the VF is not ready yet after a reset, I would expect it to be returning CRS completions just like any other device (nothing in the spec explicitly confirms or denies this, as far as I can tell). Then, the root port has no idea if a device that it received a CRS completion from is a PF or VF, so it has to treat them equivalently, and therefore (for CRS SV enabled) synthesize 0x0001 for the VID.

From: Stanislav Spassov <[email protected]>

The PCI Express specification dictates minimal amounts of time that the host needs to wait after triggering different kinds of resets before it is allowed to attempt accessing the device. After this waiting period, devices are required to be responsive to Configuration Space reads. However, if a device needs more time to actually complete the reset operation internally, it may respond to the read with a Completion Request Retry Status (CRS), and keep doing so on subsequent reads for as long as necessary. If the device is broken, it may even keep responding with CRS indefinitely.

The specification also mandates that any Root Port that supports CRS and has CRS Software Visibility (CRS SV) enabled will synthesize the special value 0x0001 for the Vendor ID and set any other bits to 1 upon receiving a CRS Completion for a Configuration Read Request that includes both bytes of the Vendor ID (offset 0).

If CRS SV is disabled or a different register (not Vendor ID) is being read, the request is retried autonomously by the Root Port. Platform-specific configuration registers may exist to limit the number of or time taken by such retries.

I think the Root Complex may eventually complete the transaction as failed *regardless* of whether CRS SV is enabled. This is unclear in PCIe r5.0, sec 2.3.2, because the text formatting was broken between r4.0 and r5.0. [...]

A Root Complex implementation may choose to limit the number of Configuration Request/CRS Completion Status loops before determining that something is wrong with the target of the Request and taking appropriate action, e.g., complete the Request to the host as a failed transaction.

Quoted 10 lines · Bjorn Helgaas

I think the Root Complex may eventually complete the transaction as failed *regardless* of whether CRS SV is enabled. This is unclear in PCIe r5.0, sec 2.3.2, because the text formatting was broken between r4.0 and r5.0. [...]

A Root Complex implementation may choose to limit the number of Configuration Request/CRS Completion Status loops before determining that something is wrong with the target of the Request and taking appropriate action, e.g., complete the Request to the host as a failed transaction.

I can provide a bit more background:

The issue that prompted me to implement this patch involved a device that used CRS Completions to signal post-reset (non-)readiness. In some cases, the device would get stuck and continue issuing CRS Completions for all requests indefinitely.

The device was attached directly to a Root Port on a server-grade Intel CPU, and CRS SV was enabled on that Root Port. The original pci_dev_wait() implementation, by virtue of polling the Command register rather than the Vendor ID, would always cause a TOR timeout and associated host crash.

I later understood the specific CPU did have a proprietary register for "limiting the number of loops" that the PCIe spec talks about, and indeed that register was set to "no limit". Coupled with the stuck device, these indefinite retries eventually triggered TOR timeout.

Granted, there are surely Root Complexes that behave differently, since the PCIe spec leaves this up to the implementation. Still, this patch increases robustness by polling the safer Vendor ID register, which is safer at least in some situations, and not any less safe generally. However, it is not a simple matter of switching which register is polled due to the SR-IOV considerations that require a fallback to Command.

Quoted 6 lines · Stanislav

On Sat, 2021-09-11 at 09:03 -0500, Bjorn Helgaas wrote:

I later understood the specific CPU did have a proprietary register for "limiting the number of loops" that the PCIe spec talks about, and indeed that register was set to "no limit". Coupled with the stuck device, these indefinite retries eventually triggered TOR timeout.

"No limit" sounds like a pretty bad choice, given that it means the CPU will essentially hang forever because of a defective I/O device. There should be a timeout so software can recover (the *device* may never recover, but that's no reason why the kernel must crash).

Quoted 8 lines · Bjorn Helgaas

Quoted 6 lines · Stanislav

On Sat, 2021-09-11 at 09:03 -0500, Bjorn Helgaas wrote:

I later understood the specific CPU did have a proprietary register for "limiting the number of loops" that the PCIe spec talks about, and indeed that register was set to "no limit". Coupled with the stuck device, these indefinite retries eventually triggered TOR timeout.

Quoted 5 lines

"No limit" sounds like a pretty bad choice, given that it means the CPU will essentially hang forever because of a defective I/O device. There should be a timeout so software can recover (the *device* may never recover, but that's no reason why the kernel must crash).

Correct. "No limit" is definitely a bad choice for that register, and fixing the value would be preferable to any software solution.

Unfortunately, at least in the case I worked on, that register was not accessible by the kernel.

Intel exposes many CPU configuration
registers in terms of virtual PCI devices residing directly on Root Buses, and the system/platform firmware is able to use vendor-provided means to completely hide some of these pseudo-devices from the OS.

Additionally, the way the PCIe spec is phrased, not every Root Complex implementation is required to even have such a limiting register, while all implementations that advertise CRS SV capability are required to behave as prescribed when PCI_VENDOR_ID is read. Hence why I believe this patch is a general robustness improvement, rather than a workaround for a specific device/platform.

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

From: Stanislav Spassov <[email protected]>

The PCI Express specification dictates minimal amounts of time that the host needs to wait after triggering different kinds of resets before it is allowed to attempt accessing the device. After this waiting period, devices are required to be responsive to Configuration Space reads. However, if a device needs more time to actually complete the reset operation internally, it may respond to the read with a Completion Request Retry Status (CRS), and keep doing so on subsequent reads for as long as necessary. If the device is broken, it may even keep responding with CRS indefinitely.

The specification also mandates that any Root Port that supports CRS and has CRS Software Visibility (CRS SV) enabled will synthesize the special value 0x0001 for the Vendor ID and set any other bits to 1 upon receiving a CRS Completion for a Configuration Read Request that includes both bytes of the Vendor ID (offset 0).

If CRS SV is disabled or a different register (not Vendor ID) is being read, the request is retried autonomously by the Root Port. Platform-specific configuration registers may exist to limit the number of or time taken by such retries.

If CRS is not supported, or a device is responding with CA/UR Completions (rather than CRS), the behavior is platform-dependent, but generally the Root Port synthesizes ~0 to complete the software read.

Previously, pci_dev_wait() avoided taking advantage of CRS. However, on platforms where no retry limit/timeout can be configured, a device responding with CRS for too long (e.g. because it is stuck and cannot complete its reset) may trigger more severe error conditions (e.g. TOR timeout, 3-strike CPU CATERR), because the Root Port never reports back to the lower-level component requesting the transaction.

This patch introduces special handling when CRS is available, and otherwise falls back to the previous behavior of polling COMMAND.

Signed-off-by Stanislav Spassov <[email protected]>

---
 drivers/pci/pci.c | 55 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 8 deletions(-)

drivers/pci/pci.c +47 -8

index 44f5d4907db6..a028147f4471 100644
@@ -1073,17 +1073,56 @@ static inline int pci_dev_poll_until_not_equal(struct pci_dev *dev, int where,
 
 static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
 {
+	int waited = 0;
+	int rc = 0;
+
+
 	/*
 	 * After reset, the device should not silently discard config
 	 * requests, but it may still indicate that it needs more time by
-	 * responding to them with CRS completions.  The Root Port will
-	 * generally synthesize ~0 data to complete the read (except when
-	 * CRS SV is enabled and the read was for the Vendor ID; in that
-	 * case it synthesizes 0x0001 data).
-	 *
-	 * Wait for the device to return a non-CRS completion.  Read the
-	 * Command register instead of Vendor ID so we don't have to
-	 * contend with the CRS SV value.
+	 * responding to them with CRS completions. For such completions:
+	 * - If CRS SV is enabled on the Root Port, and the read request
+	 *   covers both bytes of the Vendor ID register, the Root Port
+	 *   will synthesize the value 0x0001 (and set any extra requested
+	 *   bytes to 0xff)
+	 * - If CRS SV is not enabled on the Root Port, the Root Port must
+	 *   re-issue the Configuration Request as a new Request.
+	 *   Depending on platform-specific Root Complex configurations,
+	 *   the Root Port may stop retrying after a set number of attempts,
+	 *   or a configured timeout is hit, or continue indefinitely
+	 *   (ultimately resulting in non-PCI-specific platform errors, such as
+	 *   a TOR timeout).
+	 */
+	if (dev->crssv_enabled) {
+		u32 id;
+
+		rc = pci_dev_poll_until_not_equal(dev, PCI_VENDOR_ID, 0xffff,
+						  0x0001, reset_type, timeout,
+						  &waited, &id);
+		if (rc)
+			return rc;
+
+		timeout -= waited;
+
+		/*
+		 * If Vendor/Device ID is valid, the device must be ready.
+		 * Note: SR-IOV VFs return ~0 for reads to Vendor/Device
+		 * ID and will not be recognized as ready by this check.
+		 */
+		if (id != 0x0000ffff && id != 0xffff0000 &&
+		    id != 0x00000000 && id != 0xffffffff)
+			return 0;
+	}
+
+	/*
+	 * Root Ports will generally indicate error scenarios (e.g.
+	 * internal timeouts, or received Completion with CA/UR) by
+	 * synthesizing an 'all bits set' value (~0).
+	 * In case CRS is not supported/enabled, as well as for SR-IOV VFs,
+	 * fall back to polling a different register that cannot validly
+	 * contain ~0. As of PCIe 5.0, bits 11-15 of COMMAND are still RsvdP
+	 * and must return 0 when read.
+	 * XXX: These bits might become meaningful in the future
 	 */
 	return pci_dev_poll_until_not_equal(dev, PCI_COMMAND, ~0, ~0,
 					    reset_type, timeout, NULL,

Signature · 12 lines

2.25.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

1) It sounds like this *might* be a workaround for a device defect? Should we infer that there's a device where:

- We reset the device
- We read PCI_COMMAND until it is not ~0, for up to 60 seconds
- The device returns CRS status for each read, until ...
- The platform hardware times out before 60 seconds and fails the transaction, causing a system crash?

But reading PCI_VENDOR_ID instead of PCI_COMMAND somehow avoids the platform timeout?

2) I think this should somehow be integrated with pci_bus_wait_crs(), which also loops looking for CRS status.

3) pci_bus_wait_crs() is used in the enumeration path, and we do a 32-bit read there, which reads both the Vendor ID and the Device ID. Maybe that's some sort of micro-optimization, but apparently there are devices that don't implement CRS SV correctly (see 89665a6a7140 ("PCI: Check only the Vendor ID to identify Configuration Request Retry"), and doing a 16-bit read would avoid that issue.

For pci_dev_wait(), I don't think there's any point in doing a 32-bit read, so maybe we should just do a 16-bit read.

Quoted 80 lines

Signed-off-by Stanislav Spassov <[email protected]>

---
 drivers/pci/pci.c | 55 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 8 deletions(-)

drivers/pci/pci.c +47 -9

index 44f5d4907db6..a028147f4471 100644
@@ -1073,17 +1073,56 @@ static inline int pci_dev_poll_until_not_equal(struct pci_dev *dev, int where,
 
 static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
 {
+     int waited = 0;
+     int rc = 0;
+
+
      /*
       * After reset, the device should not silently discard config
       * requests, but it may still indicate that it needs more time by
-      * responding to them with CRS completions.  The Root Port will
-      * generally synthesize ~0 data to complete the read (except when
-      * CRS SV is enabled and the read was for the Vendor ID; in that
-      * case it synthesizes 0x0001 data).
-      *
-      * Wait for the device to return a non-CRS completion.  Read the
-      * Command register instead of Vendor ID so we don't have to
-      * contend with the CRS SV value.
+      * responding to them with CRS completions. For such completions:
+      * - If CRS SV is enabled on the Root Port, and the read request
+      *   covers both bytes of the Vendor ID register, the Root Port
+      *   will synthesize the value 0x0001 (and set any extra requested
+      *   bytes to 0xff)
+      * - If CRS SV is not enabled on the Root Port, the Root Port must
+      *   re-issue the Configuration Request as a new Request.
+      *   Depending on platform-specific Root Complex configurations,
+      *   the Root Port may stop retrying after a set number of attempts,
+      *   or a configured timeout is hit, or continue indefinitely
+      *   (ultimately resulting in non-PCI-specific platform errors, such as
+      *   a TOR timeout).
+      */
+     if (dev->crssv_enabled) {
+             u32 id;
+
+             rc = pci_dev_poll_until_not_equal(dev, PCI_VENDOR_ID, 0xffff,
+                                               0x0001, reset_type, timeout,
+                                               &waited, &id);
+             if (rc)
+                     return rc;
+
+             timeout -= waited;
+
+             /*
+              * If Vendor/Device ID is valid, the device must be ready.
+              * Note: SR-IOV VFs return ~0 for reads to Vendor/Device
+              * ID and will not be recognized as ready by this check.
+              */
+             if (id != 0x0000ffff && id != 0xffff0000 &&
+                 id != 0x00000000 && id != 0xffffffff)
+                     return 0;
+     }
+
+     /*
+      * Root Ports will generally indicate error scenarios (e.g.
+      * internal timeouts, or received Completion with CA/UR) by
+      * synthesizing an 'all bits set' value (~0).
+      * In case CRS is not supported/enabled, as well as for SR-IOV VFs,
+      * fall back to polling a different register that cannot validly
+      * contain ~0. As of PCIe 5.0, bits 11-15 of COMMAND are still RsvdP
+      * and must return 0 when read.
+      * XXX: These bits might become meaningful in the future
       */
      return pci_dev_poll_until_not_equal(dev, PCI_COMMAND, ~0, ~0,
                                          reset_type, timeout, NULL,
--

2.25.1

Quoted 10 lines

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

From: Stanislav Spassov <[email protected]>

The first version of this patch series can be found here:
https://lore.kernel.org/linux-pci/[email protected]

The goal of this patch series is to solve an issue where pci_dev_wait can cause system crashes. After a reset, a hung device may keep responding with CRS completions indefinitely. If CRS Software Visibility is enabled on the Root Port, attempting to read any register other than PCI_VENDOR_ID will cause the Root Port to autonomously retry the request without reporting back to the CPU core. Unless the number of retries or the amount of time spent retrying is limited by platform-specific means, this scenario leads to low-level platform timeouts (such as a TOR Timeout), which can easily escalate to a crash.

Feedback on the v1 inspired a lot of additional improvements all around the device reset codepaths and reducing post-reset delays. These improvements were published as part of v2 (v3 is just small build fixes).

It looks like there is immediate demand specifically for the CRS work, so I am once again reducing the series to just that. The reset will be posted as a separate patch series that will likely require more time and iterations to stabilize.

Quoted 22 lines · Stanislav Spassov

From: Stanislav Spassov <
[email protected]

Quoted 1 line

The first version of this patch series can be found here:
https://lore.kernel.org/linux-pci/[email protected]

The goal of this patch series is to solve an issue where pci_dev_wait can cause system crashes. After a reset, a hung device may keep responding with CRS completions indefinitely. If CRS Software Visibility is enabled on the Root Port, attempting to read any register other than PCI_VENDOR_ID will cause the Root Port to autonomously retry the request without reporting back to the CPU core. Unless the number of retries or the amount of time spent retrying is limited by platform-specific means, this scenario leads to low-level platform timeouts (such as a TOR Timeout), which can easily escalate to a crash.

Feedback on the v1 inspired a lot of additional improvements all around the device reset codepaths and reducing post-reset delays. These improvements were published as part of v2 (v3 is just small build fixes).

Quoted 4 lines

It looks like there is immediate demand specifically for the CRS work, so I am once again reducing the series to just that. The reset will be posted as a separate patch series that will likely require more time and iterations to stabilize.

Hm, what happened to this?

Bjorn?