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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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

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 <jlotwo@gmail.com>
Sent: Tuesday, November 7, 2023 10:59 AM
To: devel@edk2.groups.io
Cc: Leif Lindholm <quic_llindhol@quicinc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Gao, Liming <gaoliming@byosoft.com.cn>; Michael Brown <mcb30@ipxe.org>; Pedro Falcato <pedro.falcato@gmail.com>; Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Sami Mujawar <sami.mujawar@arm.com>; lersek@redhat.com; Kinney, Michael D <michael.d.kinney@intel.com>
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 <jlotwo@gmail.com<mailto:jlotwo@gmail.com>> wrote:
++ CC'd


On Mon, Nov 6, 2023 at 6:02 PM Joe Lopez <jlotwo@gmail.com<mailto:jlotwo@gmail.com>> wrote:
From: joelopez333 <jlotwo@gmail.com<mailto:jlotwo@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 <quic_llindhol@quicinc.com<mailto:quic_llindhol@quicinc.com>>
    Cc: Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb%2Btianocore@kernel.org>>
    Cc: Sami Mujawar <sami.mujawar@arm.com<mailto:sami.mujawar@arm.com>>
    Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
    Cc: Liming Gao <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
    Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
    Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
    Cc: Pedro Falcato <pedro.falcato@gmail.com<mailto:pedro.falcato@gmail.com>>
    Cc: Michael Brown <mcb30@ipxe.org<mailto:mcb30@ipxe.org>>
    Signed-off-by: Joe Lopez <jlotwo2@gmail.com<mailto:jlotwo2@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



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