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

[Laszlo Ersek <lersek@redhat.com>]

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://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-10
      https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23

    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:

    http://mid.mail-archive.com/48d7d1a9-503f-2a72-803d-f63a8bc67e9c@redhat.com

(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