Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridgeDxe: Add readback after final Cfg-Write

["Joe L" <jlotwo@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5963 bytes --]

> 
> (1) I'd like (a) the problem report, and the full reasoning by Ard and
> Michael to be captured in the commit message, and (b) *minimally* a hint
> at the possible reordering, and at the PCI spec-based workaround, to be
> placed in the code comment as well.
> 

Laszlo, please forgive me if this is the wrong way to reply (I am new to the patch process on edk2, should I instead send a new [PATCH v2] with changes based on your feedback?)

Including the problem report and reasoning in the commit/comment:

REF: https://edk2.groups.io/g/devel/topic/102310377#110456

Problem Report:
On AARCH64, there is no ordering guarantee between configuration
space (ECAM) writes and memory space reads (MMIO). ARM AMBA CHI
only guarantees ordering for reads and writes within a single address region,
however, on some systems MMIO and ECAM may be split into separate
address regions.
A problem may arise when an ECAM write is issued a completion before a subsequent
MMIO read is issued and receives a completion.
For example, a typical PCI software flow is the following:
1. ECAM write to device command register to enable memory space
2. MMIO read from device memory space for which access was enabled
in step 1.
There is no guarantee that step 2. will not begin before the completion of step 1.
on systems where ECAM/MMIO are specified as separate address regions, even
if both spaces have the memory attributes device-nGnRnE.

- Add a read after the final PCI Configuration space write
in RootBridgeIoPciAccess.

- When configuration space is strongly ordered, this ensures
that program execution cannot continue until the completion
is received for the previous Cfg-Write, which may have side-effects.

Cc: Leif Lindholm <quic_llindhol@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Sami Mujawar <sami.mujawar@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Liming Gao <gaoliming@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Ray Ni <ray.ni@...>
Cc: Pedro Falcato <pedro.falcato@...>
Cc: Michael Brown <mcb30@...>
Signed-off-by: Joe Lopez <jlotwo@...>
---
MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index 157a0ada80..4bc774b574 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -1238,6 +1238,13 @@ RootBridgeIoPciAccess (
}
}

+ //
+ // Perform readback after write to confirm completion was received for the last write
+  // before subsequent memory operations can be issued.
+ //
+ if (!Read) {
+ PciSegmentRead8 (Address - InStride);
+ }
+
return EFI_SUCCESS;
}

> 
> 
> 
> (2) This is a significant change; please file a new tianocore BZ about
> it. If we include it in the upcoming stable release, the BZ should be
> listed here, too:
> 
> 
> 

Is a separate thread (other than this patch thread) needed to ensure that the BZ is created for this issue?

> 
> (3) I seem to understand that the outcome of the discusson thus far is
> that reading back any config space register should be without side
> effects. (In turn, this should be documented in the comment and the
> commit message! But, my more important point here is:)

In the PCI Base Spec version 6.1 section 7.4 "Configuration Register Types" all configuration space registers are assigned one of the attributes (quoting Michael Brown in the previous thread)

> 
> 
> 
> If reads are not allowed to have side effects (e.g. read-clear registers)
> then this seems safe. The PCIe specification "Configuration Register
> Types" list comprises (in version 3.0, at least):
> 
> HwInit - read-only, no read side effects
> 
> RO - read-only, no read side effects
> 
> RW - read-write, no read side effects
> 
> RW1C - write 1 to clear bits, no read side effects
> 
> ROS - read-only, no read side effects
> 
> RWS - read-write, no read side effects
> 
> RW1CS - write 1 to clear bits, no read side effects
> 
> RsvdP - read-write, no read side effects
> 
> RsvdZ - read-write, no read side effects
> 
> So, unless newer versions of the PCIe specification have allowed for the
> existence of configuration register types with read side effects, then the
> approach of always reading back from ECAM seems to be safe for any
> conforming PCIe device.
> 
> 
> 

It is my understanding as well that reads to configuration space registers should never have side-effects.

In addition, a read of any size from anywhere in configuration space should be enough to ensure that a previous ECAM reads or writes should have completed on ARM systems, given that the entirety of the devices ECAM space is mapped into the same contiguous Address Region and the address region has strongly ordered memory attributes ie device-nGnRnE.

The alternative solution to the "potential reordering of ECAM/MMIO reads and writes" is a memory barrier (Data Synchronization Barrier or DSB) instruction placed at the end of RootBridgeIoPciAccess() in place of the readback originally proposed by this patch. This would ensure that the processor will not execute instructions until a completion is received by the processor for the most recent ECAM write (implying that all preceding ECAM operations have also completed if the memory has strongly-ordered attributes). The DSB would ensure ordering on Arm systems but the readback is an architecturally-agnostic solution.

Thanks,
Joe


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110735): https://edk2.groups.io/g/devel/message/110735
Mute This Topic: https://groups.io/mt/102354842/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 13268 bytes --]