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

[Kinney, Michael D]

Leif,

Yes.  I think the single swap lib is sufficient for
the specific use case.

We can wait until there is a request to add support 
for a BE CPU to look at a more complete solution to
reusing the same module sources for all combinations
of CPU and I/O endianness.

Thanks,

Mike

> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Tuesday, April 17, 2018 6:27 AM
> To: Kinney, Michael D <michael.d.kin...@intel.com>;
> Gao, Liming <liming@intel.com>; Laszlo Ersek
> <ler...@redhat.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] MdePkg: add big-endian MMIO
> BaseBeIoLib
> 
> On Mon, Apr 16, 2018 at 10:42:26PM +0200, Laszlo Ersek
> wrote:
> > On 04/16/18 16:34, Michael Brown wrote:
> > > On 16/04/18 15:10, Kinney, Michael D wrote:
> > >> I agree that the opposite use case is a BE CPU
> > >> needing a LE operation.
> > >>
> > >> 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.
> 
> Yes. I'm thinking that is a good enough solution for
> this type of
> situation and I overcomplicated things. Apologies for
> that.
> 
> We are talking about the relatively unusual situation
> where an
> otherwise driver-compatible device can in some
> platforms be of a
> different endianness than in others.
> 
> So, Mike, Liming - would you be OK with a solution
> similar to
> https://www.mail-archive.com/edk2-
> de...@lists.01.org/msg36520.html.
> 
> /
> Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Leif Lindholm]

On Mon, Apr 16, 2018 at 10:42:26PM +0200, Laszlo Ersek wrote:
> On 04/16/18 16:34, Michael Brown wrote:
> > On 16/04/18 15:10, Kinney, Michael D wrote:
> >> I agree that the opposite use case is a BE CPU
> >> needing a LE operation.
> >>
> >> 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.

Yes. I'm thinking that is a good enough solution for this type of
situation and I overcomplicated things. Apologies for that.

We are talking about the relatively unusual situation where an
otherwise driver-compatible device can in some platforms be of a
different endianness than in others.

So, Mike, Liming - would you be OK with a solution similar to
https://www.mail-archive.com/edk2-devel@lists.01.org/msg36520.html.

/
Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Udit Kumar]

Hi Laszlo 

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, April 17, 2018 3:12 PM
> To: Udit Kumar <udit.ku...@nxp.com>; Leif Lindholm
> <leif.lindh...@linaro.org>
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; edk2-devel@lists.01.org;
> Gao, Liming <liming....@intel.com>
> Subject: Re: [edk2] [PATCH] MdePkg: add big-endian MMIO BaseBeIoLib
> 
> On 04/17/18 10:15, Udit Kumar wrote:
> > 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 ?
> 
> Apparently I've needlessly complicated things already. Feel free to proceed as
> you see fit.

You haven't complicated the things , rather helping to get better solution !!
For me, patch from Leif, seems to work on BE or LE CPU. 
With as it is patch, Apart from edk2 coding rules, one limitation  I see for BE 
CPU having BE IP device driver, swap will be done twice.

On lighter note, 
To keep things simple, I need to request hardware designer to stay away from BE 
IP in next SOC  

> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

On 04/17/18 10:24, Michael Brown wrote:
> On 17/04/18 09:01, Laszlo Ersek wrote:
>> 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().
> 
> I know.  I thought that suggesting a 40-character function name

Yes, I noticed that (and commented on it).

> and a
> runtime check in case the CPU endianness changed mid-execution would be
> sufficiently obviously ridiculous,

Linking BaseBeIoLib (and BaseLeIoLib) against both IoLib and IoSwapLib,
and then deciding with an "if" in the code which one to call, would not
expect the CPU to change endianness mid-execution. The controlling
expression of the "if" would be evaluable at compile time, and then
link-time optimization at the latest could fully eliminate one of IoLib
/ IoSwapLib. This was discussed last time the topic was alive:

2a1fa56f-98db-a1c1-d973-7e84cc7dc1fa@redhat.com">http://mid.mail-archive.com/2a1fa56f-98db-a1c1-d973-7e84cc7dc1fa@redhat.com

//
// at file scope
//
STATIC CONST UINT16 mOne = 1;

//
// at function scope
//
if (*(CONST UINT8 *) == 1) {

Keeping the "if" in genuine C source code provides better source code
coverage (for compiling anyway) than conditional compilation via macros
(this is a practice the SeaBIOS project follows as well).

> but I fear that it may have sounded
> too plausible for EDK2.

Please don't troll? I've spent some time on this because I was asked for
my opinion. You may find the edk2 coding practices ridiculous (and we're
totally not blind to their shortcomings either), and/or my thoughts on
the matter painfully naive, but trolling just wastes our time.

I guess I'm out of this thread.
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

On 04/17/18 10:15, Udit Kumar wrote:
> 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 ? 

Apparently I've needlessly complicated things already. Feel free to
proceed as you see fit.

Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Michael Brown]

On 17/04/18 09:01, Laszlo Ersek wrote:

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().


I know.  I thought that suggesting a 40-character function name and a 
runtime check in case the CPU endianness changed mid-execution would be 
sufficiently obviously ridiculous, but I fear that it may have sounded 
too plausible for EDK2.


Michael
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Udit Kumar]

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-boun...@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, April 17, 2018 1:03 AM
> To: Leif Lindholm <leif.lindh...@linaro.org>
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; edk2-devel@lists.01.org;
> Gao, Liming <liming....@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 +, 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 

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

[Laszlo Ersek]

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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Udit Kumar]

Hi Mike, 

Do we want to support BE CPUs, ?
If yes then Last discussion of Lazlo is valid, where driver don't need to take 
care of CPU endianness. 

Regards
Udit 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Kinney, Michael D
> Sent: Monday, April 16, 2018 7:41 PM
> To: Leif Lindholm <leif.lindh...@linaro.org>; Kinney, Michael D
> <michael.d.kin...@intel.com>
> Cc: edk2-devel@lists.01.org; Laszlo Ersek <ler...@redhat.com>; Gao, Liming
> <liming....@intel.com>
> Subject: Re: [edk2] [PATCH] MdePkg: add big-endian MMIO BaseBeIoLib
> 
> Leif,
> 
> I agree that the opposite use case is a BE CPU
> needing a LE operation.
> 
> 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".
> 
> The 4 cases
> 
> * LE CPU, LE I/O.  Use BaseIoLib
> * BE CPU, BE I/O.  Use BaseIoLib
> * LE CPU, BE I/O.  Use BaseIoSwapLib
> * BE CPU, LE I/O.  Use BaseIoSwapLib
> 
> Mike
> 
> 
> > -Original Message-
> > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > Sent: Monday, April 16, 2018 3:07 AM
> > To: Kinney, Michael D <michael.d.kin...@intel.com>
> > Cc: edk2-devel@lists.01.org; Laszlo Ersek
> > <ler...@redhat.com>; Gao, Liming <liming@intel.com>
> > Subject: Re: [edk2] [PATCH] MdePkg: add big-endian MMIO
> > BaseBeIoLib
> >
> > On Fri, Apr 13, 2018 at 11:32:35PM +, 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).
> >
> > /
> > Leif
> >
> > > Mike
> > >
> > >
> > > > -Original Message-
> > > > From: edk2-devel [mailto:edk2-devel-
> > > > boun...@lists.01.org] On Behalf Of Leif Lindholm
> > > > Sent: Friday, April 13, 2018 12:32 PM
> > > > To: Kinney, Michael D <michael.d.kin...@intel.com>
> > > > Cc: edk2-devel@lists.01.org; Laszlo Ersek
> > > > <ler...@redhat.com>; Gao, Liming
> > <liming@intel.com>
> > > > Subject: Re: [edk2] [PATCH] MdePkg: add big-endian
> > MMIO
> > > > BaseBeIoLib
> > > >
> > > > On Fri, Apr 13, 2018 at 07:24:06PM +, Kinney,
> > > > Michael D wrote:
> > > > > Hi Leif,
> > > > >
> > > > > I think we need to look at the names.  I see a
> > mix of
> > > > > "Be" and "Swap".  We should pick one and use it
> > > > > consistently.
> > > >
> > > > This was what I meant by the comments:
> > > > ---
> > > > This modified version introduces a single BeIoLib
> > > > instance, backed by
> > > > a source-file that could be used also for a
> > > > hypothetical LeIoLib.
> > > > There is no LeIoLib.h included though.
> > > >
> > > > While this is arguably overengineered, I do feel
> > > > reasonably strongly
> > > > that code should be named for what it does, not for
> > how
> > > > it is used,
> > > > and doing it this way lets me follow that rule.
> > > > ---
> > > >
> > > > Clearly this is open for discussion, but the above
> > is
> > > > my opinion and
> > > > the code intentionally reflects that.
> > > >
> > > > Regards,
>

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

[Michael Brown]

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.


Maybe we need some kind of wrapper library that provides an abstraction 
layer to automatically determine whether or not swapping is needed and 
then call the appropriate IoLib or IoSwapLib API function.  For example, 
the wrapper library could provide a function 
SwapIfNeededForBigEndianDeviceMmioRead16() which would perform a runtime 
check on each call to determine the current CPU endianness and then call 
MmioRead16() or SwapMmioRead16() as appropriate.


Michael
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

On 04/16/18 16:34, Michael Brown wrote:
> On 16/04/18 15:10, Kinney, Michael D wrote:
>> I agree that the opposite use case is a BE CPU
>> needing a LE operation.
>>
>> 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.

Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

On 04/16/18 12:07, Leif Lindholm wrote:
> On Fri, Apr 13, 2018 at 11:32:35PM +, 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 

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

[Michael Brown]

On 16/04/18 15:10, Kinney, Michael D wrote:

I agree that the opposite use case is a BE CPU
needing a LE operation.

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.


Michael
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Kinney, Michael D]

Leif,

I agree that the opposite use case is a BE CPU
needing a LE operation. 

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".

The 4 cases

* LE CPU, LE I/O.  Use BaseIoLib
* BE CPU, BE I/O.  Use BaseIoLib
* LE CPU, BE I/O.  Use BaseIoSwapLib
* BE CPU, LE I/O.  Use BaseIoSwapLib

Mike


> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Monday, April 16, 2018 3:07 AM
> To: Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: edk2-devel@lists.01.org; Laszlo Ersek
> <ler...@redhat.com>; Gao, Liming <liming....@intel.com>
> Subject: Re: [edk2] [PATCH] MdePkg: add big-endian MMIO
> BaseBeIoLib
> 
> On Fri, Apr 13, 2018 at 11:32:35PM +, 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).
> 
> /
> Leif
> 
> > Mike
> >
> >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-
> > > boun...@lists.01.org] On Behalf Of Leif Lindholm
> > > Sent: Friday, April 13, 2018 12:32 PM
> > > To: Kinney, Michael D <michael.d.kin...@intel.com>
> > > Cc: edk2-devel@lists.01.org; Laszlo Ersek
> > > <ler...@redhat.com>; Gao, Liming
> <liming@intel.com>
> > > Subject: Re: [edk2] [PATCH] MdePkg: add big-endian
> MMIO
> > > BaseBeIoLib
> > >
> > > On Fri, Apr 13, 2018 at 07:24:06PM +, Kinney,
> > > Michael D wrote:
> > > > Hi Leif,
> > > >
> > > > I think we need to look at the names.  I see a
> mix of
> > > > "Be" and "Swap".  We should pick one and use it
> > > > consistently.
> > >
> > > This was what I meant by the comments:
> > > ---
> > > This modified version introduces a single BeIoLib
> > > instance, backed by
> > > a source-file that could be used also for a
> > > hypothetical LeIoLib.
> > > There is no LeIoLib.h included though.
> > >
> > > While this is arguably overengineered, I do feel
> > > reasonably strongly
> > > that code should be named for what it does, not for
> how
> > > it is used,
> > > and doing it this way lets me follow that rule.
> > > ---
> > >
> > > Clearly this is open for discussion, but the above
> is
> > > my opinion and
> > > the code intentionally reflects that.
> > >
> > > Regards,
> > >
> > > Leif
> > >
> > > > Mike
> > > >
> > > > > -Original Message-
> > > > > From: Leif Lindholm
> > > [mailto:leif.lindh...@linaro.org]
> > > > > Sent: Friday, April 13, 2018 10:42 AM
> > > > > To: edk2-devel@lists.01.org
> > > > > Cc: Kinney, Michael D
> <michael.d.kin...@intel.com>;
> > > > > Gao, Liming <liming@intel.com>; Laszlo
> Ersek
> > > > > <ler...@redhat.com>; udit.ku...@nxp.com
> > > > > Subject: [PATCH] MdePkg: add big-endian MMIO
> > > > > BaseBeIoLib
> > > > >
> > > > > When performing MMIO to a destination of the
> > > opposite
> > > > > endianness to the
> > > > > executing processor, this library provides
> > > automatic
> > > > > byte order reversal
> > > > > on inputs and outputs.
> > > > >
> > > > > Contributed-under: TianoCore Contribution
> Agreement
> > > 1.1
> > > > > Signed-of

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

[Leif Lindholm]

On Fri, Apr 13, 2018 at 11:32:35PM +, 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).

/
Leif

> Mike
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-
> > boun...@lists.01.org] On Behalf Of Leif Lindholm
> > Sent: Friday, April 13, 2018 12:32 PM
> > To: Kinney, Michael D <michael.d.kin...@intel.com>
> > Cc: edk2-devel@lists.01.org; Laszlo Ersek
> > <ler...@redhat.com>; Gao, Liming <liming@intel.com>
> > Subject: Re: [edk2] [PATCH] MdePkg: add big-endian MMIO
> > BaseBeIoLib
> > 
> > On Fri, Apr 13, 2018 at 07:24:06PM +, Kinney,
> > Michael D wrote:
> > > Hi Leif,
> > >
> > > I think we need to look at the names.  I see a mix of
> > > "Be" and "Swap".  We should pick one and use it
> > > consistently.
> > 
> > This was what I meant by the comments:
> > ---
> > This modified version introduces a single BeIoLib
> > instance, backed by
> > a source-file that could be used also for a
> > hypothetical LeIoLib.
> > There is no LeIoLib.h included though.
> > 
> > While this is arguably overengineered, I do feel
> > reasonably strongly
> > that code should be named for what it does, not for how
> > it is used,
> > and doing it this way lets me follow that rule.
> > ---
> > 
> > Clearly this is open for discussion, but the above is
> > my opinion and
> > the code intentionally reflects that.
> > 
> > Regards,
> > 
> > Leif
> > 
> > > Mike
> > >
> > > > -Original Message-
> > > > From: Leif Lindholm
> > [mailto:leif.lindh...@linaro.org]
> > > > Sent: Friday, April 13, 2018 10:42 AM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>;
> > > > Gao, Liming <liming@intel.com>; Laszlo Ersek
> > > > <ler...@redhat.com>; udit.ku...@nxp.com
> > > > Subject: [PATCH] MdePkg: add big-endian MMIO
> > > > BaseBeIoLib
> > > >
> > > > When performing MMIO to a destination of the
> > opposite
> > > > endianness to the
> > > > executing processor, this library provides
> > automatic
> > > > byte order reversal
> > > > on inputs and outputs.
> > > >
> > > > Contributed-under: TianoCore Contribution Agreement
> > 1.1
> > > > Signed-off-by: Leif Lindholm
> > <leif.lindh...@linaro.org>
> > > > ---
> > > >
> > > > Udit, many apologies for this dragging out - back-
> > to-
> > > > back conferences,
> > > > holidays, and lots of catching up.
> > > >
> > > > This modified version introduces a single BeIoLib
> > > > instance, backed by
> > > > a source-file that could be used also for a
> > > > hypothetical LeIoLib.
> > > > There is no LeIoLib.h included though.
> > > >
> > > > While this is arguably overengineered, I do feel
> > > > reasonably strongly
> > > > that code should be named for what it does, not for
> > how
> > > > it is used,
> > > > and doing it this way lets me follow that rule.
> > > >
> > > > I have not duplicated the .uni file together with
> > the
> > > > .inf, since
> > > > this follows what is done in BaseIoLibIntrinsic.
> > > >
> > > >  MdePkg/Include/Library/BeIoLib.h   |
> > 376
> > > > +++
> > > >  MdePkg/Library/BaseIoLibSwap/BaseBeIoLib.inf   |
> > 48
> > > > +++
> > > >  MdePkg/Library/BaseIo

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

[Udit Kumar]

Thanks Leif 
This is really useful, We will re-spin our patches based on this 
Regards
Udit

> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Friday, April 13, 2018 11:12 PM
> To: edk2-devel@lists.01.org
> Cc: Michael D Kinney ; Liming Gao
> ; Laszlo Ersek ; Udit Kumar
> 
> Subject: [PATCH] MdePkg: add big-endian MMIO BaseBeIoLib
> 
> When performing MMIO to a destination of the opposite endianness to the
> executing processor, this library provides automatic byte order reversal on
> inputs and outputs.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Leif Lindholm 
> ---
> 
> Udit, many apologies for this dragging out - back-to-back conferences, 
> holidays,
> and lots of catching up.
> 
> This modified version introduces a single BeIoLib instance, backed by a 
> source-
> file that could be used also for a hypothetical LeIoLib.
> There is no LeIoLib.h included though.

I assume at present, there is no need of LeIoLib as specs itself supports LE 
only.

> While this is arguably overengineered, I do feel reasonably strongly that code
> should be named for what it does, not for how it is used, and doing it this 
> way
> lets me follow that rule.

You are being too futuristic  

Reviewed by Udit Kumar

> I have not duplicated the .uni file together with the .inf, since this 
> follows what
> is done in BaseIoLibIntrinsic.
> 
>  MdePkg/Include/Library/BeIoLib.h   | 376 +++
>  MdePkg/Library/BaseIoLibSwap/BaseBeIoLib.inf   |  48 +++
>  MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni |  23 ++
>  MdePkg/Library/BaseIoLibSwap/IoLibSwap.c   | 477
> +
>  MdePkg/MdePkg.dec  |   3 +
>  5 files changed, 927 insertions(+)
>  create mode 100644 MdePkg/Include/Library/BeIoLib.h  create mode 100644
> MdePkg/Library/BaseIoLibSwap/BaseBeIoLib.inf
>  create mode 100644 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni
>  create mode 100644 MdePkg/Library/BaseIoLibSwap/IoLibSwap.c
> 
> diff --git a/MdePkg/Include/Library/BeIoLib.h
> b/MdePkg/Include/Library/BeIoLib.h
> new file mode 100644
> index 00..5b2dc1a8e1
> --- /dev/null
> +++ b/MdePkg/Include/Library/BeIoLib.h
> @@ -0,0 +1,376 @@
> +/** @file
> +  Provide byte-swapping services to access MMIO registers.
> +
> +Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.
> +Copyright (c) 2017, AMD Incorporated. All rights reserved.
> +Copyright (c) 2018, Linaro ltd. All rights reserved.
> +
> +This program and the accompanying materials are licensed and made
> +available under the terms and conditions of the BSD License which
> +accompanies this distribution.  The full text of the license may be
> +found at
> +https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopens
> +ource.org%2Flicenses%2Fbsd-
> license.php=02%7C01%7Cudit.kumar%40nxp.
> +com%7C2f6c31eed3854caa00b408d5a165e4d7%7C686ea1d3bc2b4c6fa92cd9
> 9c5c3016
> +35%7C0%7C0%7C636592381374030897=c0FF7EKjnDdTqTMP%2BVDzjO
> BEhI5DE0F
> +jV8w5dOgIpoQ%3D=0
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS
> OR IMPLIED.
> +
> +**/
> +
> +#ifndef __BE_IO_LIB_H__
> +#define __BE_IO_LIB_H__
> +
> +/**
> +  Reads a 16-bit MMIO register of opposite endianness.
> +
> +  Reads the 16-bit MMIO register specified by Address.
> +  The 16-bit read value is returned in reversed byte order.
> +  This function must guarantee that all MMIO read and write  operations
> + are serialized.
> +
> +  @param  Address The MMIO register to read.
> +
> +  @return The value read.
> +
> +**/
> +UINT16
> +EFIAPI
> +BeMmioRead16 (
> +  IN  UINTN Address
> +  );
> +
> +/**
> +  Writes a 16-bit MMIO register of opposite endianness.
> +
> +  Writes the 16-bit MMIO register specified by Address with the
> + byte-reversed  version of the value specified by Value and returns the 
> original
> Value.
> +  This function must guarantee that all MMIO read and write  operations
> + are serialized.
> +
> +  @param  Address The MMIO register to write.
> +  @param  Value   The value to write to the MMIO register.
> +
> +  @return Value.
> +
> +**/
> +UINT16
> +EFIAPI
> +BeMmioWrite16 (
> +  IN  UINTN Address,
> +  IN  UINT16Value
> +  );
> +
> +/**
> +  Reads a 16-bit MMIO register of opposite endianness, performs a
> +bitwise OR,
> +  and writes the result back to the 16-bit MMIO register.
> +
> +  Reads the 16-bit MMIO register specified by Address, byte-reverses
> + the read  result, performs a bitwise OR between the read result and
> + the value specified  by OrData, byte-reverses the result, and writes
> + the result to the 16-bit MMIO  register specified by Address. The
> + pre-reversal value 

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

[Kinney, Michael D]

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?

Mike


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org] On Behalf Of Leif Lindholm
> Sent: Friday, April 13, 2018 12:32 PM
> To: Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: edk2-devel@lists.01.org; Laszlo Ersek
> <ler...@redhat.com>; Gao, Liming <liming....@intel.com>
> Subject: Re: [edk2] [PATCH] MdePkg: add big-endian MMIO
> BaseBeIoLib
> 
> On Fri, Apr 13, 2018 at 07:24:06PM +, Kinney,
> Michael D wrote:
> > Hi Leif,
> >
> > I think we need to look at the names.  I see a mix of
> > "Be" and "Swap".  We should pick one and use it
> > consistently.
> 
> This was what I meant by the comments:
> ---
> This modified version introduces a single BeIoLib
> instance, backed by
> a source-file that could be used also for a
> hypothetical LeIoLib.
> There is no LeIoLib.h included though.
> 
> While this is arguably overengineered, I do feel
> reasonably strongly
> that code should be named for what it does, not for how
> it is used,
> and doing it this way lets me follow that rule.
> ---
> 
> Clearly this is open for discussion, but the above is
> my opinion and
> the code intentionally reflects that.
> 
> Regards,
> 
> Leif
> 
> > Mike
> >
> > > -Original Message-
> > > From: Leif Lindholm
> [mailto:leif.lindh...@linaro.org]
> > > Sent: Friday, April 13, 2018 10:42 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>;
> > > Gao, Liming <liming@intel.com>; Laszlo Ersek
> > > <ler...@redhat.com>; udit.ku...@nxp.com
> > > Subject: [PATCH] MdePkg: add big-endian MMIO
> > > BaseBeIoLib
> > >
> > > When performing MMIO to a destination of the
> opposite
> > > endianness to the
> > > executing processor, this library provides
> automatic
> > > byte order reversal
> > > on inputs and outputs.
> > >
> > > Contributed-under: TianoCore Contribution Agreement
> 1.1
> > > Signed-off-by: Leif Lindholm
> <leif.lindh...@linaro.org>
> > > ---
> > >
> > > Udit, many apologies for this dragging out - back-
> to-
> > > back conferences,
> > > holidays, and lots of catching up.
> > >
> > > This modified version introduces a single BeIoLib
> > > instance, backed by
> > > a source-file that could be used also for a
> > > hypothetical LeIoLib.
> > > There is no LeIoLib.h included though.
> > >
> > > While this is arguably overengineered, I do feel
> > > reasonably strongly
> > > that code should be named for what it does, not for
> how
> > > it is used,
> > > and doing it this way lets me follow that rule.
> > >
> > > I have not duplicated the .uni file together with
> the
> > > .inf, since
> > > this follows what is done in BaseIoLibIntrinsic.
> > >
> > >  MdePkg/Include/Library/BeIoLib.h   |
> 376
> > > +++
> > >  MdePkg/Library/BaseIoLibSwap/BaseBeIoLib.inf   |
> 48
> > > +++
> > >  MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni |
> 23
> > > ++
> > >  MdePkg/Library/BaseIoLibSwap/IoLibSwap.c   |
> 477
> > > +
> > >  MdePkg/MdePkg.dec  |
> 3 +
> > >  5 files changed, 927 insertions(+)
> > >  create mode 100644
> MdePkg/Include/Library/BeIoLib.h
> > >  create mode 100644
> > > MdePkg/Library/BaseIoLibSwap/BaseBeIoLib.inf
> > >  create mode 100644
> > > MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni
> > >  create mode 100644
> > > MdePkg/Library/BaseIoLibSwap/IoLibSwap.c
> > >
> > > diff --git a/MdePkg/Include/Library/BeIoLib.h
> > > b/MdePkg/Include/Library/BeIoLib.h
> > > new file mode 100644
> > > index 00..5b2dc1a8e1
> > > --- /dev/null
> > > +++ b/MdePkg/Include/Library/BeIoLib.h
> > > @@ -0,0 +1,376 @@
> > > +/** @file
> > > +  Provide byte-swapping services to access MMIO
> > > registers.
> >

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

[Leif Lindholm]

On Fri, Apr 13, 2018 at 07:24:06PM +, Kinney, Michael D wrote:
> Hi Leif,
> 
> I think we need to look at the names.  I see a mix of
> "Be" and "Swap".  We should pick one and use it 
> consistently.

This was what I meant by the comments:
---
This modified version introduces a single BeIoLib instance, backed by
a source-file that could be used also for a hypothetical LeIoLib.
There is no LeIoLib.h included though.

While this is arguably overengineered, I do feel reasonably strongly
that code should be named for what it does, not for how it is used,
and doing it this way lets me follow that rule.
---

Clearly this is open for discussion, but the above is my opinion and
the code intentionally reflects that.

Regards,

Leif

> Mike
> 
> > -Original Message-
> > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > Sent: Friday, April 13, 2018 10:42 AM
> > To: edk2-devel@lists.01.org
> > Cc: Kinney, Michael D ;
> > Gao, Liming ; Laszlo Ersek
> > ; udit.ku...@nxp.com
> > Subject: [PATCH] MdePkg: add big-endian MMIO
> > BaseBeIoLib
> > 
> > When performing MMIO to a destination of the opposite
> > endianness to the
> > executing processor, this library provides automatic
> > byte order reversal
> > on inputs and outputs.
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Leif Lindholm 
> > ---
> > 
> > Udit, many apologies for this dragging out - back-to-
> > back conferences,
> > holidays, and lots of catching up.
> > 
> > This modified version introduces a single BeIoLib
> > instance, backed by
> > a source-file that could be used also for a
> > hypothetical LeIoLib.
> > There is no LeIoLib.h included though.
> > 
> > While this is arguably overengineered, I do feel
> > reasonably strongly
> > that code should be named for what it does, not for how
> > it is used,
> > and doing it this way lets me follow that rule.
> > 
> > I have not duplicated the .uni file together with the
> > .inf, since
> > this follows what is done in BaseIoLibIntrinsic.
> > 
> >  MdePkg/Include/Library/BeIoLib.h   | 376
> > +++
> >  MdePkg/Library/BaseIoLibSwap/BaseBeIoLib.inf   |  48
> > +++
> >  MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni |  23
> > ++
> >  MdePkg/Library/BaseIoLibSwap/IoLibSwap.c   | 477
> > +
> >  MdePkg/MdePkg.dec  |   3 +
> >  5 files changed, 927 insertions(+)
> >  create mode 100644 MdePkg/Include/Library/BeIoLib.h
> >  create mode 100644
> > MdePkg/Library/BaseIoLibSwap/BaseBeIoLib.inf
> >  create mode 100644
> > MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni
> >  create mode 100644
> > MdePkg/Library/BaseIoLibSwap/IoLibSwap.c
> > 
> > diff --git a/MdePkg/Include/Library/BeIoLib.h
> > b/MdePkg/Include/Library/BeIoLib.h
> > new file mode 100644
> > index 00..5b2dc1a8e1
> > --- /dev/null
> > +++ b/MdePkg/Include/Library/BeIoLib.h
> > @@ -0,0 +1,376 @@
> > +/** @file
> > +  Provide byte-swapping services to access MMIO
> > registers.
> > +
> > +Copyright (c) 2006 - 2012, Intel Corporation. All
> > rights reserved.
> > +Copyright (c) 2017, AMD Incorporated. All rights
> > reserved.
> > +Copyright (c) 2018, Linaro ltd. All rights
> > reserved.
> > +
> > +This program and the accompanying materials
> > +are licensed and made available under the terms and
> > conditions of the BSD License
> > +which accompanies this distribution.  The full text of
> > the license may be found at
> > +http://opensource.org/licenses/bsd-license.php
> > +
> > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN
> > "AS IS" BASIS,
> > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> > EITHER EXPRESS OR IMPLIED.
> > +
> > +**/
> > +
> > +#ifndef __BE_IO_LIB_H__
> > +#define __BE_IO_LIB_H__
> > +
> > +/**
> > +  Reads a 16-bit MMIO register of opposite endianness.
> > +
> > +  Reads the 16-bit MMIO register specified by Address.
> > +  The 16-bit read value is returned in reversed byte
> > order.
> > +  This function must guarantee that all MMIO read and
> > write
> > +  operations are serialized.
> > +
> > +  @param  Address The MMIO register to read.
> > +
> > +  @return The value read.
> > +
> > +**/
> > +UINT16
> > +EFIAPI
> > +BeMmioRead16 (
> > +  IN  UINTN Address
> > +  );
> > +
> > +/**
> > +  Writes a 16-bit MMIO register of opposite
> > endianness.
> > +
> > +  Writes the 16-bit MMIO register specified by Address
> > with the byte-reversed
> > +  version of the value specified by Value and returns
> > the original Value.
> > +  This function must guarantee that all MMIO read and
> > write
> > +  operations are serialized.
> > +
> > +  @param  Address The MMIO register to write.
> > +  @param  Value   The value to write to the MMIO
> > register.
> > +
> > +  @return Value.
> > +
> > +**/
> > +UINT16
> > +EFIAPI
> > +BeMmioWrite16 (
> > +  IN  UINTN   

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

[Kinney, Michael D]

Hi Leif,

I think we need to look at the names.  I see a mix of
"Be" and "Swap".  We should pick one and use it 
consistently.

Mike

> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Friday, April 13, 2018 10:42 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D ;
> Gao, Liming ; Laszlo Ersek
> ; udit.ku...@nxp.com
> Subject: [PATCH] MdePkg: add big-endian MMIO
> BaseBeIoLib
> 
> When performing MMIO to a destination of the opposite
> endianness to the
> executing processor, this library provides automatic
> byte order reversal
> on inputs and outputs.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Leif Lindholm 
> ---
> 
> Udit, many apologies for this dragging out - back-to-
> back conferences,
> holidays, and lots of catching up.
> 
> This modified version introduces a single BeIoLib
> instance, backed by
> a source-file that could be used also for a
> hypothetical LeIoLib.
> There is no LeIoLib.h included though.
> 
> While this is arguably overengineered, I do feel
> reasonably strongly
> that code should be named for what it does, not for how
> it is used,
> and doing it this way lets me follow that rule.
> 
> I have not duplicated the .uni file together with the
> .inf, since
> this follows what is done in BaseIoLibIntrinsic.
> 
>  MdePkg/Include/Library/BeIoLib.h   | 376
> +++
>  MdePkg/Library/BaseIoLibSwap/BaseBeIoLib.inf   |  48
> +++
>  MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni |  23
> ++
>  MdePkg/Library/BaseIoLibSwap/IoLibSwap.c   | 477
> +
>  MdePkg/MdePkg.dec  |   3 +
>  5 files changed, 927 insertions(+)
>  create mode 100644 MdePkg/Include/Library/BeIoLib.h
>  create mode 100644
> MdePkg/Library/BaseIoLibSwap/BaseBeIoLib.inf
>  create mode 100644
> MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni
>  create mode 100644
> MdePkg/Library/BaseIoLibSwap/IoLibSwap.c
> 
> diff --git a/MdePkg/Include/Library/BeIoLib.h
> b/MdePkg/Include/Library/BeIoLib.h
> new file mode 100644
> index 00..5b2dc1a8e1
> --- /dev/null
> +++ b/MdePkg/Include/Library/BeIoLib.h
> @@ -0,0 +1,376 @@
> +/** @file
> +  Provide byte-swapping services to access MMIO
> registers.
> +
> +Copyright (c) 2006 - 2012, Intel Corporation. All
> rights reserved.
> +Copyright (c) 2017, AMD Incorporated. All rights
> reserved.
> +Copyright (c) 2018, Linaro ltd. All rights
> reserved.
> +
> +This program and the accompanying materials
> +are licensed and made available under the terms and
> conditions of the BSD License
> +which accompanies this distribution.  The full text of
> the license may be found at
> +http://opensource.org/licenses/bsd-license.php
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN
> "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef __BE_IO_LIB_H__
> +#define __BE_IO_LIB_H__
> +
> +/**
> +  Reads a 16-bit MMIO register of opposite endianness.
> +
> +  Reads the 16-bit MMIO register specified by Address.
> +  The 16-bit read value is returned in reversed byte
> order.
> +  This function must guarantee that all MMIO read and
> write
> +  operations are serialized.
> +
> +  @param  Address The MMIO register to read.
> +
> +  @return The value read.
> +
> +**/
> +UINT16
> +EFIAPI
> +BeMmioRead16 (
> +  IN  UINTN Address
> +  );
> +
> +/**
> +  Writes a 16-bit MMIO register of opposite
> endianness.
> +
> +  Writes the 16-bit MMIO register specified by Address
> with the byte-reversed
> +  version of the value specified by Value and returns
> the original Value.
> +  This function must guarantee that all MMIO read and
> write
> +  operations are serialized.
> +
> +  @param  Address The MMIO register to write.
> +  @param  Value   The value to write to the MMIO
> register.
> +
> +  @return Value.
> +
> +**/
> +UINT16
> +EFIAPI
> +BeMmioWrite16 (
> +  IN  UINTN Address,
> +  IN  UINT16Value
> +  );
> +
> +/**
> +  Reads a 16-bit MMIO register of opposite endianness,
> performs a bitwise OR,
> +  and writes the result back to the 16-bit MMIO
> register.
> +
> +  Reads the 16-bit MMIO register specified by Address,
> byte-reverses the read
> +  result, performs a bitwise OR between the read
> result and the value specified
> +  by OrData, byte-reverses the result, and writes the
> result to the 16-bit MMIO
> +  register specified by Address. The pre-reversal
> value written to the MMIO
> +  register is returned.
> +  This function must guarantee that all MMIO read and
> write
> +  operations are serialized.
> +
> +  @param  Address The MMIO register to write.
> +  @param  OrData  The value to OR with the read value
> from the MMIO register.
> +
> +  @return The value written back to the MMIO 

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

[Leif Lindholm]

When performing MMIO to a destination of the opposite endianness to the
executing processor, this library provides automatic byte order reversal
on inputs and outputs.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Leif Lindholm 
---

Udit, many apologies for this dragging out - back-to-back conferences,
holidays, and lots of catching up.

This modified version introduces a single BeIoLib instance, backed by
a source-file that could be used also for a hypothetical LeIoLib.
There is no LeIoLib.h included though.

While this is arguably overengineered, I do feel reasonably strongly
that code should be named for what it does, not for how it is used,
and doing it this way lets me follow that rule.

I have not duplicated the .uni file together with the .inf, since
this follows what is done in BaseIoLibIntrinsic.

 MdePkg/Include/Library/BeIoLib.h   | 376 +++
 MdePkg/Library/BaseIoLibSwap/BaseBeIoLib.inf   |  48 +++
 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni |  23 ++
 MdePkg/Library/BaseIoLibSwap/IoLibSwap.c   | 477 +
 MdePkg/MdePkg.dec  |   3 +
 5 files changed, 927 insertions(+)
 create mode 100644 MdePkg/Include/Library/BeIoLib.h
 create mode 100644 MdePkg/Library/BaseIoLibSwap/BaseBeIoLib.inf
 create mode 100644 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni
 create mode 100644 MdePkg/Library/BaseIoLibSwap/IoLibSwap.c

diff --git a/MdePkg/Include/Library/BeIoLib.h b/MdePkg/Include/Library/BeIoLib.h
new file mode 100644
index 00..5b2dc1a8e1
--- /dev/null
+++ b/MdePkg/Include/Library/BeIoLib.h
@@ -0,0 +1,376 @@
+/** @file
+  Provide byte-swapping services to access MMIO registers.
+
+Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.
+Copyright (c) 2017, AMD Incorporated. All rights reserved.
+Copyright (c) 2018, Linaro ltd. All rights reserved.
+
+This program and the accompanying materials
+are licensed and made available under the terms and conditions of the BSD 
License
+which accompanies this distribution.  The full text of the license may be 
found at
+http://opensource.org/licenses/bsd-license.php
+
+THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __BE_IO_LIB_H__
+#define __BE_IO_LIB_H__
+
+/**
+  Reads a 16-bit MMIO register of opposite endianness.
+
+  Reads the 16-bit MMIO register specified by Address.
+  The 16-bit read value is returned in reversed byte order.
+  This function must guarantee that all MMIO read and write
+  operations are serialized.
+
+  @param  Address The MMIO register to read.
+
+  @return The value read.
+
+**/
+UINT16
+EFIAPI
+BeMmioRead16 (
+  IN  UINTN Address
+  );
+
+/**
+  Writes a 16-bit MMIO register of opposite endianness.
+
+  Writes the 16-bit MMIO register specified by Address with the byte-reversed
+  version of the value specified by Value and returns the original Value.
+  This function must guarantee that all MMIO read and write
+  operations are serialized.
+
+  @param  Address The MMIO register to write.
+  @param  Value   The value to write to the MMIO register.
+
+  @return Value.
+
+**/
+UINT16
+EFIAPI
+BeMmioWrite16 (
+  IN  UINTN Address,
+  IN  UINT16Value
+  );
+
+/**
+  Reads a 16-bit MMIO register of opposite endianness, performs a bitwise OR,
+  and writes the result back to the 16-bit MMIO register.
+
+  Reads the 16-bit MMIO register specified by Address, byte-reverses the read
+  result, performs a bitwise OR between the read result and the value specified
+  by OrData, byte-reverses the result, and writes the result to the 16-bit MMIO
+  register specified by Address. The pre-reversal value written to the MMIO
+  register is returned.
+  This function must guarantee that all MMIO read and write
+  operations are serialized.
+
+  @param  Address The MMIO register to write.
+  @param  OrData  The value to OR with the read value from the MMIO register.
+
+  @return The value written back to the MMIO register.
+
+**/
+UINT16
+EFIAPI
+BeMmioOr16 (
+  IN  UINTN Address,
+  IN  UINT16OrData
+  );
+
+/**
+  Reads a 16-bit MMIO register of opposite endianness, performs a bitwise AND,
+  and writes the result back to the 16-bit MMIO register.
+
+  Reads the 16-bit MMIO register specified by Address, byte-reverses the read
+  result, performs a bitwise AND between the read result and the value 
specified
+  by AndData, byte-reverses the result, and writes the result to the 16-bit 
MMIO
+  register specified by Address. The pre-reversal value written to the MMIO
+  register is returned.
+  This function must guarantee that all MMIO read and write
+  operations are serialized.
+
+  @param  Address The MMIO register to write.
+  @param  AndData The value to AND