From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 6A781740038 for ; Mon, 6 Nov 2023 06:55:10 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=nzwrcWi+BvydEXrlJluc9oD8tsdKPGOcgSvW4kBQFQc=; c=relaxed/simple; d=groups.io; h=Subject:To:From:User-Agent:MIME-Version:Date:References:In-Reply-To:Message-ID:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1699253709; v=1; b=HSNQw8GKui2E0R89A7FzQZeb+/b0iYYQ0QaHooJzEA9L6psCt6W6W2X71ITAWo5Sc8ZHMH2t 4o0SDIH/4UzOo8LrJEJiwlC6wtKYDg6jNGcZWxjdUDowA0QAYigSXG0nJddJsGnFfcTICgkQqzo 9pAGhpcupBSzyGR+hW59iOHI= X-Received: by 127.0.0.2 with SMTP id TsUQYY7687511xVqhRjtcpgN; Sun, 05 Nov 2023 22:55:09 -0800 Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridgeDxe: Add readback after final Cfg-Write To: Michael Brown ,devel@edk2.groups.io From: "Joe L" X-Originating-Location: Seattle, Washington, US (97.126.119.134) X-Originating-Platform: Windows Chrome 119 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Sun, 05 Nov 2023 22:55:08 -0800 References: <0102018b961decc1-e615fc66-39cb-4826-9f61-bdabeb43e73e-000000@eu-west-1.amazonses.com> In-Reply-To: <0102018b961decc1-e615fc66-39cb-4826-9f61-bdabeb43e73e-000000@eu-west-1.amazonses.com> Message-ID: <17154.1699253708520490104@groups.io> Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,jlotwo@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: fYShRrgDRFmXdLINpGb14x6Ax7686176AA= Content-Type: multipart/alternative; boundary="4iHEP5cKOUEsSnGuPaYY" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=HSNQw8GK; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --4iHEP5cKOUEsSnGuPaYY Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable >=20 > (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. >=20 Laszlo, please forgive me if this is the wrong way to reply (I am new to th= e patch process on edk2, should I instead send a new [PATCH v2] with change= s 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 regio= n, 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 subs= equent 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 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 | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeM= odulePkg/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 th= e last write +=C2=A0 // before subsequent memory operations can be issued. + // + if (!Read) { + PciSegmentRead8 (Address - InStride); + } + return EFI_SUCCESS; } >=20 >=20 >=20 > (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: >=20 >=20 >=20 Is a separate thread (other than this patch thread) needed to ensure that t= he BZ is created for this issue? >=20 > (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 (quot= ing Michael Brown in the previous thread) >=20 >=20 >=20 > 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): >=20 > HwInit - read-only, no read side effects >=20 > RO - read-only, no read side effects >=20 > RW - read-write, no read side effects >=20 > RW1C - write 1 to clear bits, no read side effects >=20 > ROS - read-only, no read side effects >=20 > RWS - read-write, no read side effects >=20 > RW1CS - write 1 to clear bits, no read side effects >=20 > RsvdP - read-write, no read side effects >=20 > RsvdZ - read-write, no read side effects >=20 > So, unless newer versions of the PCIe specification have allowed for the > existence of configuration register types with read side effects, then th= e > approach of always reading back from ECAM seems to be safe for any > conforming PCIe device. >=20 >=20 >=20 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 compl= eted on ARM systems, given that the entirety of the devices ECAM space is m= apped into the same contiguous Address Region and the address region has st= rongly ordered memory attributes ie device-nGnRnE. The alternative solution to the "potential reordering of ECAM/MMIO reads an= d writes" is a memory barrier (Data Synchronization Barrier or DSB) instruc= tion placed at the end of RootBridgeIoPciAccess() in place of the readback = originally proposed by this patch. This would ensure that the processor wil= l 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 DS= B would ensure ordering on Arm systems but the readback is an architectural= ly-agnostic solution. Thanks, Joe -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --4iHEP5cKOUEsSnGuPaYY Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable
(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 instea= d send a new [PATCH v2] with changes based on your feedback?)

Including the problem report and reasoni= ng 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 me= mory space reads (MMIO). ARM AMBA CHI
only guarantees ordering for rea= ds and writes within a single address region,
however, on some systems= MMIO and ECAM may be split into separate
address regions.
A prob= lem may arise when an ECAM write is issued a completion before a subsequent=
MMIO read is issued and receives a completion.
For example, a ty= pical PCI software flow is the following:
    1. ECAM write = to device command register to enable memory space
    2. MMI= O read from device memory space for which access was enabled
  &n= bsp;     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 th= e memory attributes device-nGnRnE.

- Add a read after the final PCI Configuration = space write
in RootBridgeIoPciAccess.
- When co= nfiguration space is strongly ordered, this ensures
that program execution cannot continue until the completion
is received for the previous Cfg-Write, which may have si= de-effects.

Cc: Leif Lindholm <quic_llindhol@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Sami Mujawar <sami.mujawar@...>
= Cc: Jian J Wang <jian.j.wang@...>
Cc: Limi= ng Gao <gaoliming@...>
Cc: Hao A Wu <ha= o.a.wu@...>
Cc: Ray Ni <ray.ni@...>
Cc: Pedro Falcato <pedro.falcato@...><= br style=3D"white-space-collapse: preserve;" />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/PciRootB= ridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c<= br style=3D"white-space-collapse: preserve;" />index 157a0ada80..4bc774b574 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBr= idgeIo.c
@@ -1238,6 +1238,13 @@ RootBridgeIoPci= Access (
}
}

+ //
+ // Perform readback after write to confirm completion was receive= d for the last write
+  // before subsequent memory operat= ions can be issued.
+ //
+ i= f (!Read) {
+ PciSegmentRead8 (Address - InStrid= e);
+ }
+
return EFI_SUCCESS;
}

 

(2) This is a significant change; please file a new tianocore BZ ab= out
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 t= he BZ is created for this issue?

(3) I seem to u= nderstand that the outcome of the discusson thus far is
that reading back any config space register should be without side<= /span>
effects. (In turn, this should be documented in = the comment and the
<= span style=3D"white-space-collapse: preserve;">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 a= ssigned one of the attributes (quoting Michael Brown in the previous thread= )

If reads are not allowed to have side effects (e.g. read-clear regi= sters) 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, n= o read side effects
<= br style=3D"white-space-collapse: preserve;" /> RW1C - write 1 to clear bits, no read side effects

RO= S - read-only, no read side effects

RWS - read-write, no read side effect= s


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 a= lways reading back from ECAM seems to be safe for any conforming PCIe devic= e.


It is my understanding as well tha= t 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 sy= stems, 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 barri= er (Data Synchronization Barrier or DSB) instruction placed at the end of&n= bsp;RootBridgeIoPciAccess()= in place of the readback originally proposed by this patch. This would ens= ure 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-o= rdered attributes). The DSB would ensure ordering on Arm systems but the re= adback is an architecturally-agnostic solution.

 

Thanks,
Joe

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#110735) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--4iHEP5cKOUEsSnGuPaYY--