Re: [PATCH edk2-platforms 01/39] Silicon/NXP: Add support for Big Endian Mmio APIs

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Feb 23, 2018 at 11:47:28AM +0100, Laszlo Ersek wrote:
> >> I think the solution that saves the most on the *source* code size
> >> is:
> >> - introduce the BeIoLib class
> >> - duplicate the MMIO functions from BaseIoLibIntrinsic to the one
> >>   BeIoLib instance that we introduce
> >> - modify the MMIO functions in *both* lib instances (original LE, and
> >>   new BE), like this:
> >>
> >>   - If the CPU architecture is known to be bound to a single
> >>     endianness, then hardcode the appropriate operation. This can be
> >>     done with preprocessor macros, or with the architecture support
> >>     of INF files / separate source files. For example, on IA32 and
> >>     X64, the IoLib instance should work transparently,
> >>     unconditionally, and the BeIoLib instance should byte-swap,
> >>     unconditionally.
> >>
> >>   - On other CPU arches, all the wider-than-byte MMIO functions, in
> >>     *both* lib instances should do something like this:
> >>
> >>     //
> >>     // at file scope
> >>     //
> >>     STATIC CONST UINT16 mOne = 1;
> >>
> >>     //
> >>     // at function scope
> >>     //
> >>     if (*(CONST UINT8 *)&mOne == 1) {
> >>       //
> >>       // CPU in LE mode:
> >>       // - work transparently in the IoLib instance
> >>       // - byte-swap in the BeIoLib instance
> >>       //
> >>     } else {
> >>       //
> >>       // CPU in BE mode:
> >>       // - byte-swap in the IoLib instance
> >>       // - work transparently in the BeIoLib instance
> >>       //
> >>     }
> >
> > You meant, sort of cpu_to_le and cpu_to_be sort of APIs
> 
> I'm lost. I don't know how to put it any clearer. Let me try with actual
> code.
> 
> (a) Suggested for "MdePkg/Library/BaseIoLibIntrinsic/IoLib.c", which is
> used on IA32 and X64, therefore CPU byte order is little endian only:
> 
> > UINT32
> > EFIAPI
> > MmioWrite32 (
> >   IN      UINTN                     Address,
> >   IN      UINT32                    Value
> >   )
> > {
> >   ASSERT ((Address & 3) == 0);
> >
> >   MemoryFence ();
> >   *(volatile UINT32*)Address = Value;
> >   MemoryFence ();
> >
> >   return Value;
> > }
> 
> In other words, no change to the current implementation.
> 
> 
> (b) Suggested for "MdePkg/Library/BaseBeIoLibIntrinsic/IoLib.c", also to
> be used on IA32 and X64. Because the CPU byte order is LE only, this
> variant will byte-swap unconditionally.
> 
> > UINT32
> > EFIAPI
> > BeMmioWrite32 (
> >   IN      UINTN                     Address,
> >   IN      UINT32                    Value
> >   )
> > {
> >   ASSERT ((Address & 3) == 0);
> >
> >   MemoryFence ();
> >   *(volatile UINT32*)Address = SwapBytes32 (Value);
> >   MemoryFence ();
> >
> >   return Value;
> > }
> 
> (c) Suggested for "MdePkg/Library/BaseIoLibIntrinsic/IoLibArm.c", which
> is used on ARM and AARCH64. And here I'll assume that the CPU byte order
> on those can be either LE or BE.

We badly need to reduce the number of architecture-specific libraries
for doing mmio accesses from C code, rather than increasing them.

So, I realise I've confused the situation somewhat here by talking
about big-endian CPUs. We are not looking to support big-endian CPUs
today. All I wanted was to make sure we don't unneccesarily build in
assumptions in the codebase about things that could change in the
future with fairly minor changes to the specifications.

What I do see as an absolute is that a single _UEFI_ architecture
could never be more than one possible endianness. I.e., if someone
wanted to (and this _really_ isn't me being *nudge* *nudge*, *wink*
*wink*) bring in a BE-port of AArch64, that wouldn't be AARCH64, that
would be AARCH64BE.

Which also means I don't think there is any need for any sort of
runtime detection of this.

Your proposal of a set of function-pointers in the driver being mapped
to appropriate device endianness on initialization seems sufficient to
resolve the situation posed. And the situation feels sufficiently
esoteric to motivate that level of clunkiness.

Regards,

Leif