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

[Laszlo Ersek <lersek@redhat.com>]

On 04/17/18 00:14, Michael Brown wrote:
> On 16/04/18 21:42, Laszlo Ersek wrote:
>> On 04/16/18 16:34, Michael Brown wrote:
>>> On 16/04/18 15:10, Kinney, Michael D wrote:
>>>> I think we only need a single lib class and lib
>>>> Instance that does the byte swap and we should
>>>> not use Le or Be in any of the names of the class,
>>>> instance, or APIs.  Just "Swap".
>>>
>>> I may have misunderstood, but wouldn't using "Swap" within the API
>>> names effectively encode knowledge of the endianness of the _build_
>>> platform into the source code?  This would prevent the same source
>>> code being built for both little-endian and big-endian CPUs.
>>
>> Under this scenario, all drivers meant to be portable to both byte
>> orders would have to:
>> - link against both IoLib and IoSwapLib,
>> - determine at device binding time, from CPU endianness and device
>>   endianness combined, whether swapping was needed for that device,
>> - call the IoLib or IoSwapLib APIs through wrapper functions, or
>>   function pointers.
>
> Given that "all drivers meant to be portable to both byte orders"
> would include almost the complete set of PCI device drivers, that
> sounds like an incredibly large amount of unnecessarily duplicated
> boilerplate code.

I don't think any of the library classes under discussion are suitable
for UEFI drivers that bind PCI devices. Such drivers should use the
PciIo protocol members to access registers in MMIO BARs.

Instead I think these lib classes target DXE (= platform) drivers, plus
modules that run in earlier phases (PEI and maybe SEC).

> Maybe we need some kind of wrapper library that provides an
> abstraction layer to automatically determine whether or not swapping
> is needed

I agree.

> and then call the appropriate IoLib or IoSwapLib API function.  For
> example, the wrapper library could provide a function
> SwapIfNeededForBigEndianDeviceMmioRead16()

(A 40-character function name; that would fit edk2 well :) )

> which would perform a runtime check on each call to determine the
> current CPU endianness and then call MmioRead16() or SwapMmioRead16()
> as appropriate.

Right, that would be a suitable Base implementation, requiring no
writeable global variables.

The thing is, the BeIoLib and LeIoLib classes are already good for this
-- they can be implemented as you suggest. So no need to call the
function SwapIfNeededForBigEndianDeviceMmioRead16(), just call it
BeMmioRead16().

Earlier I suggested the BaseBeIoLib and BaseBeIoLibSwap instances,
through which the platform DSC would select swapping-or-not at build
time. But that doesn't necessarily has to happen statically. We could
have just one BaseBeIoLib instance that linked against both IoLib and
IoSwapLib, and determined dynamically which one to delegate work to, on
every call.

Thanks
Laszlo