Re: [PATCH] MdePkg: add big-endian MMIO BaseBeIoLib

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

Hi Laszlo, 

Considering all possible option is best to have in code 😊 
But IMO, We are running UEFI on LE CPU only, Not sure someone is running on BE. AFAIK even specs says LE. 

> (c) assuming we introduce a CPU with BE byte order, the same driver
>     source will work (for both LE and BE devices), only the lib
>     instances will have to be switched around. (This might even happen
>     dynamically, via function pointers.)

I assume , when we say CPU in BE , this means we are talking here new CPU architecture
and new SOC.  So I expect some changes in hardware IP as well.
If not then, I see still this working with patch of Leif 

Like 
CPU (LE,) Driver (LE) Uses Mmio
CPU (LE,) Driver (BE) Uses BeMmio (Mmio with swap)

CPU (BE,) Driver (LE) Uses Mmio (Does read and Swap) <-- This will be new Mmio Lib for particular architecture 
CPU (BE,) Driver (BE) Uses BeMmio (Mmio with swap)  <-- Swap of swap will make same value 

With this, I see driver code is same irrespective of CPU arch 

thoughts ? 

Thanks 
Udit 

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, April 17, 2018 1:03 AM
> To: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org;
> Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH] MdePkg: add big-endian MMIO BaseBeIoLib
> 
> On 04/16/18 12:07, Leif Lindholm wrote:
> > On Fri, Apr 13, 2018 at 11:32:35PM +0000, Kinney, Michael D wrote:
> >> Leif,
> >>
> >> I am curious why a Swap class/instances is not sufficient.
> >>
> >> Currently EDK II follows the UEFI/PI specs, which for all supported
> >> CPU architectures use little endian ABI. The BaseIoLib follows the
> >> endianness of the CPU.  If UEFI/PI added a CPU that was big endian, I
> >> would expect BaseIoLib when built for that CPU would perform big
> >> endian operations.
> >>
> >> Am I missing something?
> >
> > If you did add a big-endian CPU, you could then find yourself in the
> > exact opposite situation and require a little-endian i/o access
> > library. Which would be implemented exactly as the contents of
> > IoLibSwap.c.
> >
> > The header file necessarily needs to be endianness-specific, and if
> > the coding style had permitted functions in header files, my automatic
> > reaction would have been to make all of these static inline helper
> > functions (even with the code duplication).
> 
> First, to remind myself of the previous discussion (and please correct me if I
> remember incorrectly): whether swapping is needed or not depends on both
> CPU byte order and device byte order. However, the library API names that
> device drivers use should reflect device byte order *only* (regardless of CPU
> byte order). This way,
> 
> (a) developers that write device drivers can focus on the devices,
>     regardless of what CPU the driver is compiled for,
> 
> (b) library classes for both LE and BE devices can be used together in
>     the same driver module,
> 
> (c) assuming we introduce a CPU with BE byte order, the same driver
>     source will work (for both LE and BE devices), only the lib
>     instances will have to be switched around. (This might even happen
>     dynamically, via function pointers.)
> 
> Now, after staring at this patch long and hard, here's my understanding.
> Crucially, you take the IoLib class to mean "it doesn't swap". You don't take it to
> mean "it talks to LE devices". This is evident from the source file "IoLibSwap.c",
> where you add the swapping on top of IoLib.
> That's fine, but it will have consequences:
> 
> (1) We need a separate library class called IoSwapLib (not IoLibSwap),
>     implemented under "MdePkg/Library/BaseIoSwapLib/BaseIoSwapLib.c",
>     and without the preprocessor trickery. There should be one library
>     instance only, adding nothing but byte swapping on top of IoLib.
> 
> (2) We need separate library classes called BeIoLib and LeIoLib. These
>     provide the ultimate APIs that I describe near the top. In total we
>     should have four library instances that are explicit about device
>     endianness. This means four INF files, and a single shared C source
>     file, *with* the preprocessor trickery.
> 
> (2.1) BaseBeIoLib.inf: implements the BeIoLib functions on top of IoLib,
>       that is, without swapping -- this means that it is suitable for
>       talking to BE devices on BE CPUs.
> 
> (2.2) BaseBeIoLibSwap.inf: implements the BeIoLib functions on top of
>       IoSwapLib -- talks to BE devices on LE CPUs.
> 
> (2.3) BaseLeIoLib.inf: implements LeIoLib functions on top of IoLib --
>       talks to LE devices on LE CPUs.
> 
> (2.4) BaseLeIoLibSwap.inf: implements LeIoLib functions on top of
>       IoSwapLib -- talks to LE devices on BE CPUs.
> 
> IMO, it's fine if you only want to add the BeIoLib class now, with its
> BaseBeIoLibSwap instance only. But the IoSwapLib and BeIoLib classes must
> exist separately nonetheless. And that's because your currently proposed C
> source file is unable to express case (2.1): you can generate the "Be" prefix
> alright, but the internals will always swap, and do that on top of IoLib (which
> *never* swaps). So you will end up swapping once in the Be*() functions, which
> is wrong for talking to BE devices on BE CPUs.
> 
> I guess -- relaxing my initial point a bit -- it's also OK if you do not introduce the
> BeIoLib class at all (let alone LeIoLib), saying that drivers are generally expected
> to compute at startup whether they need to byte-swap or not (considering both
> CPU and device byte order), and then they need to flip a few function pointers
> between IoLib versus IoSwapLib for all further use. In that case however the
> patch should not say "Be" or "Le" in any lib class, instance, or API names, at all.
> 
> Some other random comments I have:
> 
> (3) You mention that no UNI file is being duplicated, but I do see one.
> 
> (4) Please consider adopting
> 
> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FLaszlo%27s-unkempt-git-
> guide-for-edk2-contributors-and-maintainers%23contrib-
> 10&data=02%7C01%7Cudit.kumar%40nxp.com%7Ceca0e342996e407c59d808d
> 5a3d0d820%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636595039
> 784808084&sdata=nZLasYufnJxc9hUh7hjDOWafNwRcDs9XltT1tJqL7KM%3D&re
> served=0
> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FLaszlo%27s-unkempt-git-
> guide-for-edk2-contributors-and-maintainers%23contrib-
> 23&data=02%7C01%7Cudit.kumar%40nxp.com%7Ceca0e342996e407c59d808d
> 5a3d0d820%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636595039
> 784808084&sdata=KEYZKW3495HYMOxd9%2B7gsARWHU7cMhx5BEER7NgwF%
> 2Bg%3D&reserved=0
> 
>     so that patches roughly advance from "abstract" to "concrete".
> 
> (5) You skipped the following family of functions:
>     MmioBitField(Read|Write|Or|And|AndThenOr)(16|32|64). I don't think
>     that's right: the mask values at the least are expressed in host
>     byte order, and they'll need conversion. An earlier (independent)
>     discussion was at:
> 
> 
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.mai
> l-archive.com%2F48d7d1a9-503f-2a72-803d-
> f63a8bc67e9c%40redhat.com&data=02%7C01%7Cudit.kumar%40nxp.com%7Ce
> ca0e342996e407c59d808d5a3d0d820%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636595039784808084&sdata=s38kDSxqShQa2MYx03%2BeDW
> Xrx1LpJhQ2Y4WvyLPfgDA%3D&reserved=0
> 
> (6) Please don't use "__" and "_C" prefixes for identifiers (see
>     __CONCATENATE and _CONCATENATE); according to the C standard, "All
>     identifiers that begin with an underscore and either an uppercase
>     letter or another underscore are always reserved for any use".
> 
>     I know Linux uses "__" prefixes liberally; that doesn't make them
>     any less wrong :)
> 
> ... Obviously I don't insist on these patches being implemented "my way"; I'm
> stating my opinion because you CC'd me :) (Thanks for that!)
> 
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01
> .org%2Fmailman%2Flistinfo%2Fedk2-
> devel&data=02%7C01%7Cudit.kumar%40nxp.com%7Ceca0e342996e407c59d80
> 8d5a3d0d820%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6365950
> 39784808084&sdata=g2ZgKbxt7JD%2BK2SuTK4WsPBaD7VLaskW2RYI210nVBc%
> 3D&reserved=0