Re: [edk2-devel] [RFC] Ordering of Arm PCI ECAM and MMIO operations

["Ard Biesheuvel" <ardb@kernel.org>]

(cc Ray)

On Wed, 1 Nov 2023 at 03:09, Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> +CC ARM maintainers
>
> On Wed, Nov 1, 2023 at 12:40 AM Joe L <jlotwo@gmail.com> wrote:
> >
> > Hello,
> >
> > During experimentation on an AARCH64 platform with a PCIe endpoint that supports option ROM, it was found that the ordering of transactions between ECAM (Cfg-Write) and MMIO (Mem-Read) is not preserved by the CHI HN-I node. The observed sequence is as follows:
> > 1. EFI issues a Cfg-Write to endpoint Memory Space Enable bit to enable memory access
> > 2. EFI issues a Mem-Read to the endpoint Option ROM memory space and receives back 0xFFFF (unsupported request)
> > If the memory ordering between the two accesses is explicitly preserved via memory barrier (DMB instruction), 2. will return back valid data instead of UR. Analyzing the transactions with a protocol analyzer, we found that the Mem-Read was being issued before the completion for the Cfg-Write is received.
> >
> > On this system, the HN-I node is configured to separate the ECAM and MMIO into different SAM regions. Both regions are assigned Decice-nGnRnE attributes. According to this snippet from Arm "DEN0114 PCIe AMBA Integration Guide", the ordering of even Device-nGnRnE memory regions cannot be guaranteed if they belong to separate PCIe address spaces
> >
> > 4.8.2
> >
> > Multiple PCIe address spaces mapped as Device-nGnRnE or Device-nGnRE Arm memory model does not give any ordering guarantees between accesses to different Device-nGnRnE or Device-nGnRE peripherals. Additionally, there is no restriction on mapping various PCIe address spaces of the same PCIe function as different Device-nGnRnE or Device-nGnRE peripherals. Consequently, software cannot assume that program order will be maintained between accesses to two different PCIe address spaces, even though both spaces are mapped as Device-nGnRnE or Device-nGnRE. Therefore, for maximum software portability, ordering requirements between accesses to different PCIe address spaces must be handled explicitly in software using appropriate ordering instructions."
> >
> > We requested a comment from an Arm representative and received a similar response, confirming that a memory barrier is needed to ensure ordering between accesses to ECAM and MMIO regions (or between any two ranges that are assigned to a separate SAM address region)
> >
> > When they are to two different order regions, the read will not wait for the write to complete, and can return data before the write does anything. The HN-I only preserves ordering between reads and writes to the same Order Region (which implies the same Address Region). Likewise, the HN-I will only preserve ordering between multiple reads and between multiple writes within the same Order Region, and it accomplishes this by issuing the reads with the same ARID and the writes with the same AWID (i.e. it relies on the downstream device to follow AXI ordering rules). Issuing a CHI request with REQ.Order=EndpointOrder only guarantees ordering to the same “endpoint address range,” which the HN-I defines as an Order Region (within an Address Region).
> >
> > Our CMN TRM showcases an example where ECAM and MMIO are two different regions in the HN-I SAM. The implication is that we would expect a DSB between the ECAM write and MMIO read. I'm asking our Open Source Software group to confirm that standard PCIe software is generally expected to be aware of the need for a DSB--but my impression from talking to some of our hardware engineers is that that is indeed the expectation.
> >
> >
> > I am requesting that EDK2 consumes or produces a change to the current PciExpressLib that will ensure ordering on Arm architectures after Cfg-Writes which may or may not have side effects. For example, in MdePkg/Library/BasePciExpressLib/PciExpressLib.c,
> >
> > UINT8
> > EFIAPI
> > PciExpressWrite8 (
> >   IN      UINTN  Address,
> >   IN      UINT8  Value
> >   )
> > {
> >   ASSERT_INVALID_PCI_ADDRESS (Address);
> >   if (Address >= PcdPciExpressBaseSize ()) {
> >     return (UINT8)-1;
> >   }
> >
> >   return MmioWrite8 ((UINTN)GetPciExpressBaseAddress () + Address, Value);
> > }
> >
> > should become
> >
> >
> >
> > UINT8
> > EFIAPI
> > PciExpressWrite8 (
> >   IN      UINTN  Address,
> >   IN      UINT8  Value
> >   )
> > {
> >   ASSERT_INVALID_PCI_ADDRESS (Address);
> >   if (Address >= PcdPciExpressBaseSize ()) {
> >     return (UINT8)-1;
> >   }
> >
> >   UINT8 ReturnValue = MmioWrite8 ((UINTN)GetPciExpressBaseAddress () + Address, Value);
> >   #if defined (MDE_CPU_AARCH64)
> >      MemoryFence (); // DMB sy or DSB
> >   #endif
> >
> >   return ReturnValue;
> > }
> >
> > Please let me know your thoughts and if this is the correct implementation change needed to enforce memory ordering between separate address regions. I would also like to know the preferred memory barrier instruction in this case (DMB or DSB).
>
> Hrmmm, I think the current implementation of MmioRead/Write only
> offers TSO instead of "full serialization". For instance:
>
> //
> //  Reads an 8-bit MMIO register.
> //
> //  Reads the 8-bit MMIO register specified by Address. The 8-bit read value is
> //  returned. This function must guarantee that all MMIO read and write
> //  operations are serialized.
> //
> //  @param  Address The MMIO register to read.
> //
> //  @return The value read.
> //
> ASM_PFX(MmioRead8Internal):
>   AARCH64_BTI(c)
>   ldrb    w0, [x0]
>   dmb     ld
>   ret
>
> //
> //  Writes an 8-bit MMIO register.
> //
> //  Writes the 8-bit MMIO register specified by Address with the value specified
> //  by Value and returns Value. This function must guarantee that all MMIO read
> //  and write operations are serialized.
> //
> //  @param  Address The MMIO register to write.
> //  @param  Value   The value to write to the MMIO register.
> //
> ASM_PFX(MmioWrite8Internal):
>   AARCH64_BTI(c)
>   dmb     st
>   strb    w1, [x0]
>   ret
>
>
> So prior reads are ordered against reads/stores (through dmb ld after
> the load). Prior writes are ordered against writes only (through dmb
> st before the store). So one can easily devise a problem here where
>
>   dmb     st /* orders against prior writes */
>   strb    w1, [x0] /* write */
> [...]
>
>   ldrb    w0, [x0] /* load */
>   dmb     ld /* orders this load against later reads/stores, but not
> against prior writes (!!) */
>
> where the CPU can (AFAIK) freely reorder the write after the load in
> the ARM64 case.
>

Not if the memory type is device/strongly ordered, which is the
assumption for regions these accessors operate on. And even with
cached normal memory, only /other/ observers might see these take
effect in a different order.

The barriers here are supposed to manage concurrent accesses, which
are mostly from coherent masters other than secondary CPUs in the
context that EDK2 typically operates in.

For instance, the DMB ST in MmioWrite8Internal() is supposed to ensure
that any prior writes to a DMA buffer are not reordered with the MMIO
write that kicks of the DMA transaction.


> Do we want dmb st after the store instead?
>

I think we should separate the two cases here:

1) as per the architecture (as interpreted by the ARM architects), a
DSB is required to ensure that the side effects of enabling a MMIO BAR
in the PCI config space are sufficiently observable to memory accesses
to that BAR that appear after the PCI config space access in the
program.

2) the MMIO accessors are completely undocumented, and it would make
sense to put some of the (alleged) rationale in the source code for
posterity.


As for 1), I don't think dropping a MemoryFence() (which issues a DBM
not a DSB on arm64) in a shared source file somewhere is the right
approach here. I'd much prefer handling this in
RootBridgeIoPciAccess(), and using/introducing a generic helper other
than MemoryFence() to convey the existence of a potential side effect
that may turn entire regions of memory on or off (including ones that
the program/compiler may consider 'ordinary' memory)

On ARM/AARCH64, we already have DataSynchronizationBarrier () for this
purpose. Alternatively, we might introduce a PciExpressLib instance in
ArmPkg that uses this.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110473): https://edk2.groups.io/g/devel/message/110473
Mute This Topic: https://groups.io/mt/102310377/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-