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

2023-11-13 Thread Laszlo Ersek 
On 11/8/23 02:06, Michael D Kinney wrote:
> Hi Jose,
> 
>  
> 
>  1. This logic needs to move into an AARCH64 specific directory/file. 
> Other architectures handle this in other ways.
>  2. There are many places throughout edk2 sources that perform PCI
> config write operations.  You are only fixing it in a single
> location.  You may want to look at the MdePkg PciLibs to see if it
> can be addressed there with an AARCH64 specific dir/file, but that
> still may not address all possible PCI config write accesses.  Fill
> analysis of the target platform sources may be required to make sure
> it is fixes for all locations.

Hm.

Perhaps if this change is pushed down as much as possible, into a
special AARCH64 PciLib instance, so that the readback or the DSB (IIRC)
is performed after every PCI config space write, then that might
actually suffice. Because, ultimately, PciHostBridgeDxe too depends on
PciSegmentLib for the access sycles.

(Previously I suggested extending PciHostBridgeLib with a new API, but
that wasn't right, per your second point -- I didn't realize the same
issue must exist (for example) in the PEI phase, before the PCI-related
protocols even exist.)

Laszlo


> 
>  
> 
> Mike
> 
>  
> 
> *From:* Jose Lopez 
> *Sent:* Tuesday, November 7, 2023 10:59 AM
> *To:* devel@edk2.groups.io
> *Cc:* Leif Lindholm ; Ard Biesheuvel
> ; Gao, Liming ;
> Michael Brown ; Pedro Falcato ;
> Ni, Ray ; Wu, Hao A ; Wang, Jian J
> ; Sami Mujawar ;
> ler...@redhat.com; Kinney, Michael D 
> *Subject:* Re: [PATCH v2] MdeModulePkg/PciHostBridgeDxe: Add readback
> after final Cfg-Write
> 
>  
> 
> ++ Laszlo and Michael
> 
>  
> 
> On Tue, Nov 7, 2023 at 10:54 AM Jose Lopez  > wrote:
> 
> ++ CC'd
> 
>  
> 
>  
> 
> On Mon, Nov 6, 2023 at 6:02 PM Joe Lopez  > wrote:
> 
> From: joelopez333 mailto:jlo...@gmail.com>>
> 
>     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.
> 
>     Fix:
> 
>     - 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.
> 
>     - Risk of reading a "write-only" register and causing a CA
> which leaves the device
>       unresponsive. The expectation based on the PCI Base Spec
> v6.1 section 7.4 is that
>       all PCI Spec-defined registers will be readable, however,
> there may exist
>       design-specific registers that fall into this category.
> 
>     Cc: Leif Lindholm  >
>     Cc: Ard Biesheuvel  >
>     Cc: Sami Mujawar  >
>     Cc: Jian J Wang  >
>     Cc: Liming Gao  >
>     Cc: Hao A Wu mailto:hao.a...@intel.com>>
>     Cc: Ray Ni mailto:ray...@intel.com>>
>     Cc: Pedro Falcato  >
>     Cc: Michael Brown mailto:mc...@ipxe.org>>
>     Signed-off-by: Joe Lopez  >
> ---
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 8
> 
>  1 file changed, 8 insertions(+)
> 
> diff --git
> a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>

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

2023-11-07 Thread Michael D Kinney 
Hi Jose,


  1.  This logic needs to move into an AARCH64 specific directory/file.  Other 
architectures handle this in other ways.
  2.  There are many places throughout edk2 sources that perform PCI config 
write operations.  You are only fixing it in a single location.  You may want 
to look at the MdePkg PciLibs to see if it can be addressed there with an 
AARCH64 specific dir/file, but that still may not address all possible PCI 
config write accesses.  Fill analysis of the target platform sources may be 
required to make sure it is fixes for all locations.

Mike

From: Jose Lopez 
Sent: Tuesday, November 7, 2023 10:59 AM
To: devel@edk2.groups.io
Cc: Leif Lindholm ; Ard Biesheuvel 
; Gao, Liming ; Michael 
Brown ; Pedro Falcato ; Ni, Ray 
; Wu, Hao A ; Wang, Jian J 
; Sami Mujawar ; 
ler...@redhat.com; Kinney, Michael D 
Subject: Re: [PATCH v2] MdeModulePkg/PciHostBridgeDxe: Add readback after final 
Cfg-Write

++ Laszlo and Michael

On Tue, Nov 7, 2023 at 10:54 AM Jose Lopez 
mailto:jlo...@gmail.com>> wrote:
++ CC'd


On Mon, Nov 6, 2023 at 6:02 PM Joe Lopez 
mailto:jlo...@gmail.com>> wrote:
From: joelopez333 mailto:jlo...@gmail.com>>

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.

Fix:

- 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.

- Risk of reading a "write-only" register and causing a CA which leaves the 
device
  unresponsive. The expectation based on the PCI Base Spec v6.1 section 7.4 
is that
  all PCI Spec-defined registers will be readable, however, there may exist
  design-specific registers that fall into this category.

Cc: Leif Lindholm 
mailto:quic_llind...@quicinc.com>>
Cc: Ard Biesheuvel 
mailto:ardb%2btianoc...@kernel.org>>
Cc: Sami Mujawar mailto:sami.muja...@arm.com>>
Cc: Jian J Wang mailto:jian.j.w...@intel.com>>
Cc: Liming Gao mailto:gaolim...@byosoft.com.cn>>
Cc: Hao A Wu mailto:hao.a...@intel.com>>
Cc: Ray Ni mailto:ray...@intel.com>>
Cc: Pedro Falcato mailto:pedro.falc...@gmail.com>>
Cc: Michael Brown mailto:mc...@ipxe.org>>
Signed-off-by: Joe Lopez mailto:jlot...@gmail.com>>
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c 
b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index 157a0ada80..c2dc2018d6 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -1238,6 +1238,14 @@ 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.25.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110879): https://edk2.groups.io/g/devel/message/110879
Mute This Topic: https://groups.io/mt/102435564/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2023-11-07 Thread Joe L 
++ Laszlo and Michael

On Tue, Nov 7, 2023 at 10:54 AM Jose Lopez  wrote:

> ++ CC'd
>
>
> On Mon, Nov 6, 2023 at 6:02 PM Joe Lopez  wrote:
>
>> From: joelopez333 
>>
>> 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.
>>
>> Fix:
>>
>> - 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.
>>
>> - Risk of reading a "write-only" register and causing a CA which
>> leaves the device
>>   unresponsive. The expectation based on the PCI Base Spec v6.1
>> section 7.4 is that
>>   all PCI Spec-defined registers will be readable, however, there may
>> exist
>>   design-specific registers that fall into this category.
>>
>> Cc: Leif Lindholm 
>> Cc: Ard Biesheuvel 
>> Cc: Sami Mujawar 
>> Cc: Jian J Wang 
>> Cc: Liming Gao 
>> Cc: Hao A Wu 
>> Cc: Ray Ni 
>> Cc: Pedro Falcato 
>> Cc: Michael Brown 
>> Signed-off-by: Joe Lopez 
>> ---
>>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> index 157a0ada80..c2dc2018d6 100644
>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> @@ -1238,6 +1238,14 @@ 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.25.1
>>
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110876): https://edk2.groups.io/g/devel/message/110876
Mute This Topic: https://groups.io/mt/102435564/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2023-11-07 Thread Joe L 
++ CC'd


On Mon, Nov 6, 2023 at 6:02 PM Joe Lopez  wrote:

> From: joelopez333 
>
> 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.
>
> Fix:
>
> - 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.
>
> - Risk of reading a "write-only" register and causing a CA which
> leaves the device
>   unresponsive. The expectation based on the PCI Base Spec v6.1
> section 7.4 is that
>   all PCI Spec-defined registers will be readable, however, there may
> exist
>   design-specific registers that fall into this category.
>
> Cc: Leif Lindholm 
> Cc: Ard Biesheuvel 
> Cc: Sami Mujawar 
> Cc: Jian J Wang 
> Cc: Liming Gao 
> Cc: Hao A Wu 
> Cc: Ray Ni 
> Cc: Pedro Falcato 
> Cc: Michael Brown 
> Signed-off-by: Joe Lopez 
> ---
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index 157a0ada80..c2dc2018d6 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -1238,6 +1238,14 @@ 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.25.1
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110874): https://edk2.groups.io/g/devel/message/110874
Mute This Topic: https://groups.io/mt/102435564/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2023-11-06 Thread Joe L 
From: joelopez333 

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.

Fix:

- 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.

- Risk of reading a "write-only" register and causing a CA which leaves the 
device
  unresponsive. The expectation based on the PCI Base Spec v6.1 section 7.4 
is that
  all PCI Spec-defined registers will be readable, however, there may exist
  design-specific registers that fall into this category.

Cc: Leif Lindholm 
Cc: Ard Biesheuvel 
Cc: Sami Mujawar 
Cc: Jian J Wang 
Cc: Liming Gao 
Cc: Hao A Wu 
Cc: Ray Ni 
Cc: Pedro Falcato 
Cc: Michael Brown 
Signed-off-by: Joe Lopez 
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c 
b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index 157a0ada80..c2dc2018d6 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -1238,6 +1238,14 @@ 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.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110782): https://edk2.groups.io/g/devel/message/110782
Mute This Topic: https://groups.io/mt/102435564/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-