From: joelopez333 <jlotwo@gmail.com>

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 <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Cc: Michael Brown <mcb30@ipxe.org>
Signed-off-by: Joe Lopez <jlotwo@gmail.com>
---
 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;
 }
 
-- 
2.25.1



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

On 11/3/23 01:03, Joe L wrote:
> From: joelopez333 <jlotwo@gmail.com>
> 
> 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 <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Michael Brown <mcb30@ipxe.org>
> Signed-off-by: Joe Lopez <jlotwo@gmail.com>
> ---
>  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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Friday, November 3, 2023 12:19 AM
> To: devel@edk2.groups.io; jlotwo@gmail.com
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>; Michael
> Brown <mcb30@ipxe.org>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridgeDxe: Add
> readback after final Cfg-Write
> 
> On 11/3/23 01:03, Joe L wrote:
> > From: joelopez333 <jlotwo@gmail.com>
> >
> > 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 <quic_llindhol@quicinc.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Pedro Falcato <pedro.falcato@gmail.com>
> > Cc: Michael Brown <mcb30@ipxe.org>
> > Signed-off-by: Joe Lopez <jlotwo@gmail.com>
> > ---
> >  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;
> >  }
> >

PCI Configuration read/write operations are non-posted, so the PCI
Configuration write operation should complete without the need for
an additional transaction.  If you are seeing an issue, then it may
be in the implementation of the PciLib/PciSegmentLib that is not
guaranteeing this non-posted behavior.  

Please investigate further and provide details if you think there is
an issue in the Pci*Lib implementations.

> 
> (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 (#110645): https://edk2.groups.io/g/devel/message/110645
Mute This Topic: https://groups.io/mt/102354842/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 03/11/2023 16:03, Kinney, Michael D wrote:
> PCI Configuration read/write operations are non-posted, so the PCI
> Configuration write operation should complete without the need for
> an additional transaction.  If you are seeing an issue, then it may
> be in the implementation of the PciLib/PciSegmentLib that is not
> guaranteeing this non-posted behavior.

 From the PCIe spec:

   "The ECAM converts memory transactions from the host CPU into
    Configuration Requests on the PCI Express fabric. This conversion
    potentially creates ordering problems for the software, because
    writes to memory addresses are typically posted transactions but
    writes to Configuration Space are not posted on the PCI Express
    fabric."

My understanding from the above is that the posted write occurs at the 
level of the CPU performing a memory write to the ECAM window.  By the 
time this write reaches the ECAM and becomes a non-posted write 
transaction within the downstream PCIe fabric, the CPU has already moved 
on and is not waiting for completion of the ECAM memory write.

Reading back from the PCI configuration register will cause the CPU to 
wait until the read from the ECAM window has completed, which cannot 
happen until the corresponding downstream PCI configuration read has 
completed.  Since the CPU will (with appropriate barrier operations) not 
reorder the memory read ahead of the preceding memory write, the overall 
effect is to guarantee that the memory write has reached the ECAM, and 
that the memory write reached the ECAM before the subsequent memory read 
from the same location.

There is an implicit assumption in the above that the ECAM and PCIe 
fabric will themselves not reorder the PCI configuration read ahead of 
the PCI configuration write.  Unfortunately, on a closer reading of the 
spec, this may not be a valid assumption: other parts of the PCIe spec 
state that non-posted transactions (e.g. configuration reads and writes) 
may be freely reordered.

It seems slightly insane that PCIe bridges would be allowed to 
arbitrarily reorder configuration cycles to downstream devices, but 
that's what the spec seems to state.  The upshot seems to be that:

a) software is entirely responsible for ensuring that PCI configuration 
writes via ECAM have completed, and

b) software has no available mechanism for ensuring that PCI 
configuration writes via ECAM have completed.

Ard: any alternative suggestions on ways we can wait for completion, 
given that even reading back from the PCI configuration register is 
apparently not guaranteed?

Thanks,

Michael



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

> 
> (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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-