Re: [edk2] [RFC PATCH] MdePkg: add byte-swapping MMIO BaseIoLibSwap
Hi Ard. > -Original Message- > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > Sent: Wednesday, February 21, 2018 11:39 PM > To: Leif Lindholm> Cc: edk2-devel@lists.01.org; Michael D Kinney > ; Liming Gao ; > Meenakshi Aggarwal ; Udit Kumar > ; Varun Sethi > Subject: Re: [RFC PATCH] MdePkg: add byte-swapping MMIO BaseIoLibSwap > > On 21 February 2018 at 15:25, Leif Lindholm > wrote: > > 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. > > > > As pointed out by Laszlo, 'opposite' is ill defined, as it is not a property > of > the device nor is it a property of the CPU/platform (even though UEFI is > likely to remain little endian for the foreseeable > future) Is this assumption, of part of UEFI specs ? > So BigEndianIoLib seems more appropriate as a library class, and it does > need to be a separate class so that a single driver can perform both LE and > BE MMIO (which, as was pointed out, is the case for any driver that drives a > BE MMIO peripheral but uses LE MMIO for serial port DEBUG output) Yes, We need two lib here, we can discuss on name. Thanks Udit > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Leif Lindholm > > --- > > > > As promised in the dark and distant past: > > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww > w > > .mail-archive.com%2Fedk2- > devel%40lists.01.org%2Fmsg33447.html=02% > > > 7C01%7Cudit.kumar%40nxp.com%7C503430fe81e544d1df8b08d57956346f%7 > C686ea > > > 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636548333521065509= > AR1g64t > > C4ag8V18MYGN5bimkXe7FQj%2BEYbzONr%2BLFXM%3D=0 > > > > This starts out as a clone of MdePkg/Include/Library/IoLib.h, > > implementing simple wrappers that depend on an IoLib instance to > > perform any actual accesses. > > > > Comments are mostly nonsense, since they've not been changed from > > their originals in IoLib.h. If people feel this library is a sensible > > approach, I'll fix that up before I send a v1. > > > > I have explicitly excluded: > > - non-MMIO accesses (could you really end up with mixed endianness in > > such a system?) > > - bitfields (would either require duplicating code or merging this all > > into the BaseIoLibIntrinsic module, which is already a bit on the > > bloated side) > > - buffers operations (let's avoid those until we find we need them) > > > > MdePkg/Include/Library/IoLibSwap.h | 395 > > If you create a library class, please declare it in the .dec file > > > MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.inf | 44 +++ > > MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni | 23 ++ > > MdePkg/Library/BaseIoLibSwap/IoLibSwap.c | 491 > + > > MdePkg/MdePkg.dec | 3 + > > MdePkg/MdePkg.dsc | 1 + > > 6 files changed, 957 insertions(+) > > create mode 100644 MdePkg/Include/Library/IoLibSwap.h > > create mode 100644 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.inf > > create mode 100644 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni > > create mode 100644 MdePkg/Library/BaseIoLibSwap/IoLibSwap.c > > > > diff --git a/MdePkg/Include/Library/IoLibSwap.h > > b/MdePkg/Include/Library/IoLibSwap.h > > new file mode 100644 > > index 00..f13c8d86e7 > > --- /dev/null > > +++ b/MdePkg/Include/Library/IoLibSwap.h > > @@ -0,0 +1,395 @@ > > +/** @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%2Fope > > +nsource.org%2Flicenses%2Fbsd- > license.php=02%7C01%7Cudit.kumar%40 > > > +nxp.com%7C503430fe81e544d1df8b08d57956346f%7C686ea1d3bc2b4c6fa9 > 2cd99c > > > +5c301635%7C0%7C0%7C636548333521065509=gW0hj%2FR0rTXl%2Bl > Jr6mAiu > > +aGSy7ZHoxOiy35WIRO26JE%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 __IO_LIB_SWAP_H__ > > +#define __IO_LIB_SWAP_H__ > > + > > +/** > > + Reads a 16-bit MMIO register. > > + > > + Reads the 16-bit MMIO register specified by Address. The 16-bit > > + read value is returned. This function
Re: [edk2] [RFC PATCH] MdePkg: add byte-swapping MMIO BaseIoLibSwap
On 21 February 2018 at 15:25, Leif Lindholmwrote: > 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. > As pointed out by Laszlo, 'opposite' is ill defined, as it is not a property of the device nor is it a property of the CPU/platform (even though UEFI is likely to remain little endian for the foreseeable future) So BigEndianIoLib seems more appropriate as a library class, and it does need to be a separate class so that a single driver can perform both LE and BE MMIO (which, as was pointed out, is the case for any driver that drives a BE MMIO peripheral but uses LE MMIO for serial port DEBUG output) > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Leif Lindholm > --- > > As promised in the dark and distant past: > https://www.mail-archive.com/edk2-devel@lists.01.org/msg33447.html > > This starts out as a clone of MdePkg/Include/Library/IoLib.h, > implementing simple wrappers that depend on an IoLib instance to perform > any actual accesses. > > Comments are mostly nonsense, since they've not been changed from their > originals in IoLib.h. If people feel this library is a sensible approach, > I'll fix that up before I send a v1. > > I have explicitly excluded: > - non-MMIO accesses (could you really end up with mixed endianness in > such a system?) > - bitfields (would either require duplicating code or merging this all > into the BaseIoLibIntrinsic module, which is already a bit on the > bloated side) > - buffers operations (let's avoid those until we find we need them) > > MdePkg/Include/Library/IoLibSwap.h | 395 If you create a library class, please declare it in the .dec file > MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.inf | 44 +++ > MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni | 23 ++ > MdePkg/Library/BaseIoLibSwap/IoLibSwap.c | 491 > + > MdePkg/MdePkg.dec | 3 + > MdePkg/MdePkg.dsc | 1 + > 6 files changed, 957 insertions(+) > create mode 100644 MdePkg/Include/Library/IoLibSwap.h > create mode 100644 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.inf > create mode 100644 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni > create mode 100644 MdePkg/Library/BaseIoLibSwap/IoLibSwap.c > > diff --git a/MdePkg/Include/Library/IoLibSwap.h > b/MdePkg/Include/Library/IoLibSwap.h > new file mode 100644 > index 00..f13c8d86e7 > --- /dev/null > +++ b/MdePkg/Include/Library/IoLibSwap.h > @@ -0,0 +1,395 @@ > +/** @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 __IO_LIB_SWAP_H__ > +#define __IO_LIB_SWAP_H__ > + > +/** > + Reads a 16-bit MMIO register. > + > + Reads the 16-bit MMIO register specified by Address. The 16-bit read value > is > + returned. This function must guarantee that all MMIO read and write > + operations are serialized. > + > + If 16-bit MMIO register operations are not supported, then ASSERT(). > + If Address is not aligned on a 16-bit boundary, then ASSERT(). > + > + @param Address The MMIO register to read. > + > + @return The value read. > + > +**/ > +UINT16 > +EFIAPI > +SwapMmioRead16 ( > + IN UINTN Address > + ); > + > +/** > + Writes a 16-bit MMIO register. > + > + Writes the 16-bit MMIO register specified by Address with the value > specified > + by Value and returns Value. This function must guarantee that all MMIO read > + and write operations are serialized. > + > + If 16-bit MMIO register operations are not supported, then ASSERT(). > + If Address is not aligned on a 16-bit boundary, then ASSERT(). > + > + @param Address The MMIO register to write. > + @param Value The value to write to the MMIO register. > + > + @return Value. > + > +**/ > +UINT16 > +EFIAPI > +SwapMmioWrite16 ( > + IN UINTN Address, > + IN UINT16Value > + ); > + > +/** > + Reads a 16-bit MMIO register, performs a bitwise OR, and writes the > + result back to the 16-bit MMIO register. > + > + Reads the 16-bit MMIO register specified by Address, performs a bitwise > + OR between the read result and
Re: [edk2] [RFC PATCH] MdePkg: add byte-swapping MMIO BaseIoLibSwap
On 02/21/18 16:25, Leif Lindholm wrote: > 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> --- > > As promised in the dark and distant past: > https://www.mail-archive.com/edk2-devel@lists.01.org/msg33447.html > > This starts out as a clone of MdePkg/Include/Library/IoLib.h, > implementing simple wrappers that depend on an IoLib instance to perform > any actual accesses. > > Comments are mostly nonsense, since they've not been changed from their > originals in IoLib.h. If people feel this library is a sensible approach, > I'll fix that up before I send a v1. > > I have explicitly excluded: > - non-MMIO accesses (could you really end up with mixed endianness in > such a system?) > - bitfields (would either require duplicating code or merging this all > into the BaseIoLibIntrinsic module, which is already a bit on the > bloated side) > - buffers operations (let's avoid those until we find we need them) > > MdePkg/Include/Library/IoLibSwap.h | 395 > MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.inf | 44 +++ > MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni | 23 ++ > MdePkg/Library/BaseIoLibSwap/IoLibSwap.c | 491 > + > MdePkg/MdePkg.dec | 3 + > MdePkg/MdePkg.dsc | 1 + > 6 files changed, 957 insertions(+) > create mode 100644 MdePkg/Include/Library/IoLibSwap.h > create mode 100644 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.inf > create mode 100644 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni > create mode 100644 MdePkg/Library/BaseIoLibSwap/IoLibSwap.c UEFI drivers are supposed to go through PciIo instances in order to access config space and MMIO registers of PCI devices (and I assume most devices are PCI), and the related PciIo member functions are LE-only. Thus, in such device drivers (for the exceptional(?) PCI device that has custom BE registers), the byte swapping will have to occur at the call sites (or at least higher-level wrapper functions). Which makes me think that this library is primarily for BE platform devices that are exposed via DXE drivers or (maybe) PEIMs. Is that corect? If so, can you please state it in the commit message? Also I would suggest calling the new functions "BE" or "BigEndian" or something similar -- once we turn this into a lib class, byte swapping becomes an implementation detail. The API should reflect the goal, which is "talking to BE platform devices". The @file comments should be updated similarly, IMO; "non-native endianness" should simply be "big endian", and the swapping language should be dropped -- for PI/UEFI, only LE is native (it is hard-coded by the spec(s), IIRC). ... Another superficial comment: in the INF file, there's a commented out DebugLib entry under [LibraryClasses]; I guess that's unintended. To clarify, I'm neutral on this library, I'd just like its documentation to be clear on when someone should consider using it. Thanks! Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [RFC PATCH] MdePkg: add byte-swapping MMIO BaseIoLibSwap
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--- As promised in the dark and distant past: https://www.mail-archive.com/edk2-devel@lists.01.org/msg33447.html This starts out as a clone of MdePkg/Include/Library/IoLib.h, implementing simple wrappers that depend on an IoLib instance to perform any actual accesses. Comments are mostly nonsense, since they've not been changed from their originals in IoLib.h. If people feel this library is a sensible approach, I'll fix that up before I send a v1. I have explicitly excluded: - non-MMIO accesses (could you really end up with mixed endianness in such a system?) - bitfields (would either require duplicating code or merging this all into the BaseIoLibIntrinsic module, which is already a bit on the bloated side) - buffers operations (let's avoid those until we find we need them) MdePkg/Include/Library/IoLibSwap.h | 395 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.inf | 44 +++ MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni | 23 ++ MdePkg/Library/BaseIoLibSwap/IoLibSwap.c | 491 + MdePkg/MdePkg.dec | 3 + MdePkg/MdePkg.dsc | 1 + 6 files changed, 957 insertions(+) create mode 100644 MdePkg/Include/Library/IoLibSwap.h create mode 100644 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.inf create mode 100644 MdePkg/Library/BaseIoLibSwap/BaseIoLibSwap.uni create mode 100644 MdePkg/Library/BaseIoLibSwap/IoLibSwap.c diff --git a/MdePkg/Include/Library/IoLibSwap.h b/MdePkg/Include/Library/IoLibSwap.h new file mode 100644 index 00..f13c8d86e7 --- /dev/null +++ b/MdePkg/Include/Library/IoLibSwap.h @@ -0,0 +1,395 @@ +/** @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 __IO_LIB_SWAP_H__ +#define __IO_LIB_SWAP_H__ + +/** + Reads a 16-bit MMIO register. + + Reads the 16-bit MMIO register specified by Address. The 16-bit read value is + returned. This function must guarantee that all MMIO read and write + operations are serialized. + + If 16-bit MMIO register operations are not supported, then ASSERT(). + If Address is not aligned on a 16-bit boundary, then ASSERT(). + + @param Address The MMIO register to read. + + @return The value read. + +**/ +UINT16 +EFIAPI +SwapMmioRead16 ( + IN UINTN Address + ); + +/** + Writes a 16-bit MMIO register. + + Writes the 16-bit MMIO register specified by Address with the value specified + by Value and returns Value. This function must guarantee that all MMIO read + and write operations are serialized. + + If 16-bit MMIO register operations are not supported, then ASSERT(). + If Address is not aligned on a 16-bit boundary, then ASSERT(). + + @param Address The MMIO register to write. + @param Value The value to write to the MMIO register. + + @return Value. + +**/ +UINT16 +EFIAPI +SwapMmioWrite16 ( + IN UINTN Address, + IN UINT16Value + ); + +/** + Reads a 16-bit MMIO register, performs a bitwise OR, and writes the + result back to the 16-bit MMIO register. + + Reads the 16-bit MMIO register specified by Address, performs a bitwise + OR between the read result and the value specified by OrData, and + writes the result to the 16-bit MMIO register specified by Address. The value + written to the MMIO register is returned. This function must guarantee that + all MMIO read and write operations are serialized. + + If 16-bit MMIO register operations are not supported, then ASSERT(). + If Address is not aligned on a 16-bit boundary, then ASSERT(). + + @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 +SwapMmioOr16 ( + IN UINTN Address, + IN UINT16OrData + ); + +/** + Reads a 16-bit MMIO register, performs a bitwise AND, and writes the result + back to the 16-bit MMIO register. + + Reads the 16-bit MMIO register specified by Address,