On 11/3/23 01:03, Joe L wrote: > From: joelopez333 <[email protected]> > > REF:https://edk2.groups.io/g/devel/topic/102310377#110456 > > - 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 <[email protected]> > Cc: Ard Biesheuvel <[email protected]> > Cc: Sami Mujawar <[email protected]> > Cc: Jian J Wang <[email protected]> > Cc: Liming Gao <[email protected]> > Cc: Hao A Wu <[email protected]> > Cc: Ray Ni <[email protected]> > Cc: Pedro Falcato <[email protected]> > Cc: Michael Brown <[email protected]> > Signed-off-by: Joe Lopez <[email protected]> > --- > 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 > + // > + if (!Read) { > + PciSegmentRead8 (Address - InStride); > + } > + > return EFI_SUCCESS; > } >
(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. (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: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning#proposed-features--bug-fixes (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:) ... assuming Size is not 1, PciSegmentRead8() will not match the size of the most recently performed PciSegmentWriteBuffer(). Is that OK? I'm unsure that *any* config space register (especially one in extended config space) that is larger than one byte per spec (base PCI spec or particular device spec) *guarantees* that a 1-byte read at the front of that register will behave identically to reading back the entire register. ... What's more, I believe that in the previous discussion, it wasn't the outcome that any config space register at all can be read back without side-effects. RootBridgeIoPciAccess() may well read device-specific registers too, and those can have side-effects upon read, can't they? Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110612): https://edk2.groups.io/g/devel/message/110612 Mute This Topic: https://groups.io/mt/102354842/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
