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

[Laszlo Ersek <lersek@redhat.com>]

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