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

[Udit Kumar <udit.kumar@nxp.com>]

Hi Laszlo/Leif,

For short term, is this ok to keep this lib under Silicon/NXP, here we are assuming  
CPU will always on be LE mode whereas IP can vary between LE/BE mode ?

For long term, we can discuss on APIs/name of Lib/Function name etc
We will update our code, as per agreement.

For me, Suggested approach is ok as well to keep CPU endianness in ARM package.
but need views from Ard/Leif here.

Thanks
Udit

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, February 23, 2018 4:17 PM
> To: Udit Kumar <udit.kumar@nxp.com>; Leif Lindholm
> <leif.lindholm@linaro.org>
> Cc: michael.d.kinney@intel.com; edk2-devel@lists.01.org;
> ard.biesheuvel@linaro.org
> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add support
> for Big Endian Mmio APIs
> 
> On 02/23/18 11:25, Udit Kumar wrote:
> >
> >
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> >> Of Laszlo Ersek
> >> Sent: Thursday, February 22, 2018 7:26 PM
> >> To: Leif Lindholm <leif.lindholm@linaro.org>
> >> Cc: michael.d.kinney@intel.com; edk2-devel@lists.01.org;
> >> ard.biesheuvel@linaro.org
> >> Subject: Re: [edk2] [PATCH edk2-platforms 01/39] Silicon/NXP: Add
> >> support for Big Endian Mmio APIs
> >>
> >> On 02/22/18 12:52, Leif Lindholm wrote:
> >>> On Thu, Feb 22, 2018 at 09:34:05AM +0100, Laszlo Ersek wrote:
> >>
> >>>>> But that brings back the complication as to how we have a driver
> >>>>> that needs an LE IO library to write output, and a BE IO library
> >>>>> to manipulate the hardware.
> >>>>
> >>>> Can you please explain the "write output" use case more precisely?
> >>>>
> >>>> My thinking would be this:
> >>>>
> >>>> - Use the IoLib class directly for "writing output" in little
> >>>> endian byte order (which is still unclear to me sorry).
> >>>
> >>> If the IoLib class is mapped to a an instance that byte-swaps
> >>> (hereto referred to as BeIoLib if IoLibSwap is unsuitable), would we
> >>> not then end up mapping the non-swapping, currently implemented in
> >>> BaseLibIoIntrinsic, variant as BeIoLib? Or if not, do we end up
> >>> needing to duplicated all IoLib implementation .infs to provide an
> >>> IoLib and a BeIoLib for each?
> >>>
> >>> It's at that point I burst an aneurysm.
> >>> Am I overthinking/underthinking this?
> >>
> >> We need two library classes, one for talking to LE devices and
> >> another to BE devices. These should be usable in a given module at
> >> the same time, as Ard says.
> >>
> >> Both library classes need to work on both LE and BE CPUs (working
> >> from your suggestion that UEFI might grow BE CPU support at some
> >> point). Whether that is implemented by dumb, separate library
> >> instances (yielding in total 2*2=4 library instances), or by smart,
> >> CPU-endianness-agnostic library instances (in total, 2), is a
> >> different question.
> >>
> >> Note that such "smarts" could be less than trivial to implement:
> >> - check CPU endianness in each library API?
> >> - or check in the lib constructor only, and flip some function
> >>   pointers?
> >> - use a dynamic PCD for caching CPU endianness?
> >> - use a HOB for the same?
> >> - use a lib global variable (for caching only on the module level)?
> >>
> >> 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.
> 
> > UINT32
> > EFIAPI
> > MmioWrite32 (
> >   IN      UINTN                     Address,
> >   IN      UINT32                    Value
> >   )
> > {
> >   ASSERT ((Address & 3) == 0);
> >   *(volatile UINT32*)Address = (*(CONST UINT8 *)&mOne == 1) ?
> >                                Value :
> >                                SwapBytes32 (Value);
> >   return Value;
> > }
> 
> 
> (d) Suggested for "MdePkg/Library/BaseBeIoLibIntrinsic/BeIoLibArm.c",
> which is to be used on ARM and AARCH64. And here I'll assume that the
> CPU byte order on those can be either LE or BE.
> 
> > UINT32
> > EFIAPI
> > BeMmioWrite32 (
> >   IN      UINTN                     Address,
> >   IN      UINT32                    Value
> >   )
> > {
> >   ASSERT ((Address & 3) == 0);
> >   *(volatile UINT32*)Address = (*(CONST UINT8 *)&mOne == 1) ?
> >                                SwapBytes32 (Value) :
> >                                Value;
> >   return Value;
> > }


> Laszlo