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

[Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Friday, February 23, 2018 2:51 PM
> To: Pankaj Bansal <pankaj.bansal@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 09:40, Pankaj Bansal wrote:
> > Hi All
> >
> >> -----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
> >>       //
> >>     }
> >
> > I suggest this approach :
> >
> > 1. Add BeMmio* functions in existing IoLib. BeMmio* functions will swap
> the input before write and swap output after read and so on.
> >     Mmio* functions will not perform any byte swapping
> > 2. create second instance (a copy) of this IoLib for CPUs that are Big Endian.
> We can call it BigEndianIoLib.
> >      In this library Mmio* functions will swap the input before write and swap
> output after read and so on.
> >      BeMmio* functions will not perform any byte swapping.
> > 3. Include the instance of IoLib in dsc file based on cpu endianness that the
> platform wants to use. i.e.
> >     If BIG_ENDIAN == FALSE
> >        IoLib | ..\..\..\IoLib
> >    Else
> >       IoLib | ..\..\..\BigEndianIoLib
> > 4. The devices that are Big endian in platform will always call BeMmio*
> functions. They need not check CPU endianness.
> > 5. The devices that are Little endian in platform will always call Mmio*
> functions. They need not check CPU endianness.
> 
> This can work too, but there is a downside: a large number of IoLib
> instances exist in the tree already. If you add the BeMmio* functions to
> the existent IoLib class, you'll have to duplicate the implementation to
> all instances (identically, I think).
> 
> We've had this debate in the past. Back then it was about IoFifo
> routines. I argued for an IoFifo lib class. Ultimately the IoFifo
> routines were added to IoLib, and they had to be implemented for many
> more library instances than client code would have actually required.
> (See the series at 13a50a6fe1dc..2b631390f9f5.) In turn this runs the
> risk of adding untested code.
> 
> Regarding the instances for BE CPUs: the name should likely be
> BaseIoLibBigEndian or something similar. In lib instance names, the lib
> class name is usually prefixed with the firmware phases where the
> instance is usable, and hints about the implementation or constraints
> are added as a suffix.
> 
> Also, if you want to support BE CPUs with separate IoLib instances, I'm
> afraid that's going to lead to the combinatorial explosion that Leif
> characterized as "burst[ing] an aneurysm". I think using the (seemingly
> dynamic) "mOne" approach I suggested above, a smart compiler can
> eliminate all the branches at build time.
> 
> Anyway, I don't insist; I just commented on an RFC.
> 
The implementation suggested by you is pretty clean.

Just one concern on the naming convention, 

     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
       //
 }

If we keep it BeIoLib, then it is not justifying the case for BE CPUs.
I mean, for a BE CPU architecture, it look weird to call BeIoLib APIs when swap is needed.

So, i think, we should name it IoLibSwap.

> Laszlo 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.01.org%2Fmailman%2Flistinfo%2Fedk2-
> devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C932b04d97a2
> 648845d9208d57a9ece38%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C636549744880515349&sdata=SBrgvgbrsk3ViYygJdNIzy%2FuB19pWeSvHln
> IMX6ae90%3D&reserved=0