On 11/6/23 07:55, Joe L wrote: > (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?)
Yes please, the updates for the patch (commit message and code) should be incorporated into a v2 posting. > > Including the problem report and reasoning in the commit/comment: > > REF:https://edk2.groups.io/g/devel/topic/102310377#110456 > <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; > } Thanks for addressing this point. (Apologies if meanwhile other comments have been made in this thread; I've been "write-only" for a few days now, due to a long queue of patch reviews. If I keep fetching new email, just the "triaging" takes so much time that I can't progress with the accrued patches.) > > > > (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? Please register an account at <https://bugzilla.tianocore.org/>, and file a bug there ("EDK2" product). Then link the new ticket into the commit message. We usually add: Ref: <bugzilla ticket URL> just above the Signed-off-by line. > > (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. Ah, great. I didn't understand config register *types*. That is, no matter what config register (common or device specific, normal config space or extended), it's supposed to be one of these types. And none of the types permit read accesses to have side effects. Please include a very short summary of this in the commit message (the long version can go in the BZ ticket); it's very educative. > > > 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. OK, so "any size" is safe enough, too! > > 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. Well I need to apologize here (for drawing out this discussion), but this "alternatives" approach makes me float another idea: what if you introduce a new API to the PciHostBridgeLib class, effectively a hook to be called at the end of RootBridgeIoPciAccess()? The new hook function should take all the original parameters of RootBridgeIoPciAccess() itself, and then do something. In most PciHostBridgeLib instances, the function would do nothing and return EFI_SUCCESS. On ARM64 platforms where the issue can be observed, you could implement *either* approach (as you see fit); i.e., the readback (dependent on the input parameters), or the DSB (which would just ignore the input parameters). In particular, for IA32 / X64 platforms, or for AARCH64 platforms that have single address regions, no extra PCI(e) config space traffic would be created (the function would be empty, only return EFI_SUCCESS). Now, this approach is not without complications: there are *many* PciHostBridgeLib instances, even counting only the open source repos. Edk2 has six instances, and edk2-platforms seems to have 17 (!) instances. So that would take minimally 24 patches -- one patch for each instance, then finally a patch that modifies the lib class header, and makes PciHostBridgeDxe call the new API. Also I can imagine there could be a PciHostBridgeLib instance that is used by both IA32/X64 and AARCH64 platforms; in such cases you might have to add separate [Sources.IA32, Sources.X64] and [Sources.AARCH64] sections to the common INF file, and then implement the new function twice, in separate .c files; once doing nothing, and another time performing the readback (or the DSB). Just theorizing. This would be a lot of work (lots of boilerplate...), but I feel it would match the platform-specific nature of this problem better than adding a readback for all platforms using PciHostBridgeDxe -- even for those that don't need it. What's your take on it? Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110920): https://edk2.groups.io/g/devel/message/110920 Mute This Topic: https://groups.io/mt/102354842/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
