Leif: I see current ArmLib in ArmPkg. It is not clean enough. ArmLib.h still includes Chipset/ArmV7 header. Its function has no function header. When you work the patch to move them to BaseLib, please make sure they align to BaseLib style.
Thanks Liming > -----Original Message----- > From: Kinney, Michael D > Sent: Thursday, September 21, 2017 1:32 AM > To: Leif Lindholm <[email protected]>; Gao, Liming > <[email protected]>; Kinney, Michael D <[email protected]> > Cc: [email protected]; Ard Biesheuvel <[email protected]> > Subject: RE: [edk2] [PATCH] MdePkg: add ARM/AARCH64 support to > BaseCacheMaintenanceLib > > Leif, > > Adding the ARM specific functions to the BaseLib to provide access > to ARM specific CPU registers/instructions makes sense and is > what has already been done for IA32/X64/IPF/EBC. > > Put the arch specific content in BaseLib.h inside #if clauses. > > #if defined (MDE_CPU_ARM) > > #if defined (MDE_CPU_AARCH64) > > Or for content shared by ARM and AARCH64 put in: > > #if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64) > > I see a few ARM and AARCH64 elements are already in BaseLib.h. > > Best regards, > > Mike > > > -----Original Message----- > > From: edk2-devel [mailto:[email protected]] On > > Behalf Of Leif Lindholm > > Sent: Wednesday, September 20, 2017 8:06 AM > > To: Gao, Liming <[email protected]> > > Cc: Kinney, Michael D <[email protected]>; edk2- > > [email protected]; Ard Biesheuvel <[email protected]> > > Subject: Re: [edk2] [PATCH] MdePkg: add ARM/AARCH64 support to > > BaseCacheMaintenanceLib > > > > Hi Liming, > > > > I understand the purity argument, but the situation (without > > this > > patch) is that: > > 1) There is a non-functional BaseCacheMaintenanceLib > > ARM/AARCH64 > > implementation with misleading information in MdePkg. > > 2) ARM/AARCH64-based needs to include a different cache > > maintenance > > library than all other architectures. And they all need to > > include > > the same one. > > > > 2 is an issue for the logical conclusion of the RFC series I am > > about > > to post for creating common include files for "boilerplate" > > bits of > > .dsf/.fdf files. > > > > An alternative option would be to move ArmLib into MdePkg? > > A casual glance suggests to me that the corresponding X86 > > features > > (like AsmWbinvd) are exposed via BaseLib. Would you see any > > issues > > with merging the ArmLib functionality into BaseLib? > > > > Regards, > > > > Leif > > > > On Wed, Sep 20, 2017 at 02:45:30PM +0000, Gao, Liming wrote: > > > Leif: > > > This change lets MdePkg BaseCacheMaintenanceLib depend on > > ArmPkg > > > ArmLib. But, MdePkg is the basic package. It should not > > depend on > > > other package. I suggest to add this ARM specific > > > BaseCacheMaintenanceLib library instance into ArmPkg. > > > > > > Thanks > > > Liming > > > > -----Original Message----- > > > > From: Leif Lindholm [mailto:[email protected]] > > > > Sent: Wednesday, September 20, 2017 9:23 PM > > > > To: [email protected] > > > > Cc: Kinney, Michael D <[email protected]>; Gao, > > Liming <[email protected]>; Ard Biesheuvel > > > > <[email protected]> > > > > Subject: [PATCH] MdePkg: add ARM/AARCH64 support to > > BaseCacheMaintenanceLib > > > > > > > > ARM platforms have been using a separately located library > > in ArmPkg for > > > > high-level cache maintenance calls. Resolve this anomaly by > > overwriting > > > > ArmCache.c with the contents of > > > > > > ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c, > > and add > > > > the ArmLib dependency for the affected architectures. > > > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > Signed-off-by: Leif Lindholm <[email protected]> > > > > --- > > > > > > > > The intent is to delete the ArmPkg version once no upstream > > platforms > > > > are using it. > > > > > > > > MdePkg/Library/BaseCacheMaintenanceLib/ArmCache.c | 222 > > +++++---------------- > > > > .../BaseCacheMaintenanceLib.inf | 2 + > > > > 2 files changed, 55 insertions(+), 169 deletions(-) > > > > > > > > diff --git > > a/MdePkg/Library/BaseCacheMaintenanceLib/ArmCache.c > > b/MdePkg/Library/BaseCacheMaintenanceLib/ArmCache.c > > > > index 79c84a0982..0759e38cd4 100644 > > > > --- a/MdePkg/Library/BaseCacheMaintenanceLib/ArmCache.c > > > > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/ArmCache.c > > > > @@ -1,67 +1,63 @@ > > > > /** @file > > > > - Cache Maintenance Functions. These functions vary by ARM > > architecture so the MdePkg > > > > - versions are null functions used to make sure things > > will compile. > > > > > > > > - Copyright (c) 2006 - 2009, Intel Corporation. All rights > > reserved.<BR> > > > > - Portions copyright (c) 2008 - 2009, Apple Inc. All > > rights reserved.<BR> > > > > + Copyright (c) 2008 - 2009, Apple Inc. All rights > > reserved.<BR> > > > > + Copyright (c) 2011 - 2014, ARM Limited. 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. > > > > + 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. > > > > > > > > **/ > > > > - > > > > -// > > > > -// Include common header file for this module. > > > > -// > > > > #include <Base.h> > > > > +#include <Library/ArmLib.h> > > > > #include <Library/DebugLib.h> > > > > +#include <Library/PcdLib.h> > > > > > > > > -/** > > > > - Invalidates the entire instruction cache in cache > > coherency domain of the > > > > - calling CPU. > > > > - > > > > - Invalidates the entire instruction cache in cache > > coherency domain of the > > > > - calling CPU. > > > > +STATIC > > > > +VOID > > > > +CacheRangeOperation ( > > > > + IN VOID *Start, > > > > + IN UINTN Length, > > > > + IN LINE_OPERATION LineOperation, > > > > + IN UINTN LineLength > > > > + ) > > > > +{ > > > > + UINTN ArmCacheLineAlignmentMask = LineLength - 1; > > > > + > > > > + // Align address (rounding down) > > > > + UINTN AlignedAddress = (UINTN)Start - ((UINTN)Start & > > ArmCacheLineAlignmentMask); > > > > + UINTN EndAddress = (UINTN)Start + Length; > > > > + > > > > + // Perform the line operation on an address in each > > cache line > > > > + while (AlignedAddress < EndAddress) { > > > > + LineOperation(AlignedAddress); > > > > + AlignedAddress += LineLength; > > > > + } > > > > + ArmDataSynchronizationBarrier (); > > > > +} > > > > > > > > -**/ > > > > VOID > > > > EFIAPI > > > > InvalidateInstructionCache ( > > > > VOID > > > > ) > > > > { > > > > - ASSERT(FALSE); > > > > + ASSERT (FALSE); > > > > } > > > > > > > > -/** > > > > - Invalidates a range of instruction cache lines in the > > cache coherency domain > > > > - of the calling CPU. > > > > - > > > > - Invalidates the instruction cache lines specified by > > Address and Length. If > > > > - Address is not aligned on a cache line boundary, then > > entire instruction > > > > - cache line containing Address is invalidated. If Address > > + Length is not > > > > - aligned on a cache line boundary, then the entire > > instruction cache line > > > > - containing Address + Length -1 is invalidated. This > > function may choose to > > > > - invalidate the entire instruction cache if that is more > > efficient than > > > > - invalidating the specified range. If Length is 0, then > > no instruction cache > > > > - lines are invalidated. Address is returned. > > > > - > > > > - If Length is greater than (MAX_ADDRESS - Address + 1), > > then ASSERT(). > > > > - > > > > - @param Address The base address of the instruction > > cache lines to > > > > - invalidate. If the CPU is in a physical > > addressing mode, then > > > > - Address is a physical address. If the > > CPU is in a virtual > > > > - addressing mode, then Address is a > > virtual address. > > > > - > > > > - @param Length The number of bytes to invalidate from > > the instruction cache. > > > > - > > > > - @return Address > > > > +VOID > > > > +EFIAPI > > > > +InvalidateDataCache ( > > > > + VOID > > > > + ) > > > > +{ > > > > + ASSERT (FALSE); > > > > +} > > > > > > > > -**/ > > > > VOID * > > > > EFIAPI > > > > InvalidateInstructionCacheRange ( > > > > @@ -69,56 +65,26 @@ InvalidateInstructionCacheRange ( > > > > IN UINTN Length > > > > ) > > > > { > > > > - ASSERT (Length <= MAX_ADDRESS - (UINTN)Address + 1); > > > > - ASSERT(FALSE); > > > > - return Address; > > > > -} > > > > + CacheRangeOperation (Address, Length, > > ArmCleanDataCacheEntryToPoUByMVA, > > > > + ArmDataCacheLineLength ()); > > > > + CacheRangeOperation (Address, Length, > > > > + ArmInvalidateInstructionCacheEntryToPoUByMVA, > > > > + ArmInstructionCacheLineLength ()); > > > > > > > > -/** > > > > - Writes back and invalidates the entire data cache in > > cache coherency domain > > > > - of the calling CPU. > > > > + ArmInstructionSynchronizationBarrier (); > > > > > > > > - Writes Back and Invalidates the entire data cache in > > cache coherency domain > > > > - of the calling CPU. This function guarantees that all > > dirty cache lines are > > > > - written back to system memory, and also invalidates all > > the data cache lines > > > > - in the cache coherency domain of the calling CPU. > > > > + return Address; > > > > +} > > > > > > > > -**/ > > > > VOID > > > > EFIAPI > > > > WriteBackInvalidateDataCache ( > > > > VOID > > > > ) > > > > { > > > > - ASSERT(FALSE); > > > > + ASSERT (FALSE); > > > > } > > > > > > > > -/** > > > > - Writes back and invalidates a range of data cache lines > > in the cache > > > > - coherency domain of the calling CPU. > > > > - > > > > - Writes back and invalidates the data cache lines > > specified by Address and > > > > - Length. If Address is not aligned on a cache line > > boundary, then entire data > > > > - cache line containing Address is written back and > > invalidated. If Address + > > > > - Length is not aligned on a cache line boundary, then the > > entire data cache > > > > - line containing Address + Length -1 is written back and > > invalidated. This > > > > - function may choose to write back and invalidate the > > entire data cache if > > > > - that is more efficient than writing back and > > invalidating the specified > > > > - range. If Length is 0, then no data cache lines are > > written back and > > > > - invalidated. Address is returned. > > > > - > > > > - If Length is greater than (MAX_ADDRESS - Address + 1), > > then ASSERT(). > > > > - > > > > - @param Address The base address of the data cache lines > > to write back and > > > > - invalidate. If the CPU is in a physical > > addressing mode, then > > > > - Address is a physical address. If the > > CPU is in a virtual > > > > - addressing mode, then Address is a > > virtual address. > > > > - @param Length The number of bytes to write back and > > invalidate from the > > > > - data cache. > > > > - > > > > - @return Address > > > > - > > > > -**/ > > > > VOID * > > > > EFIAPI > > > > WriteBackInvalidateDataCacheRange ( > > > > @@ -126,55 +92,20 @@ WriteBackInvalidateDataCacheRange ( > > > > IN UINTN Length > > > > ) > > > > { > > > > - ASSERT (Length <= MAX_ADDRESS - (UINTN)Address + 1); > > > > - ASSERT(FALSE); > > > > + CacheRangeOperation(Address, Length, > > ArmCleanInvalidateDataCacheEntryByMVA, > > > > + ArmDataCacheLineLength ()); > > > > return Address; > > > > } > > > > > > > > -/** > > > > - Writes back the entire data cache in cache coherency > > domain of the calling > > > > - CPU. > > > > - > > > > - Writes back the entire data cache in cache coherency > > domain of the calling > > > > - CPU. This function guarantees that all dirty cache lines > > are written back to > > > > - system memory. This function may also invalidate all the > > data cache lines in > > > > - the cache coherency domain of the calling CPU. > > > > - > > > > -**/ > > > > VOID > > > > EFIAPI > > > > WriteBackDataCache ( > > > > VOID > > > > ) > > > > { > > > > - ASSERT(FALSE); > > > > + ASSERT (FALSE); > > > > } > > > > > > > > -/** > > > > - Writes back a range of data cache lines in the cache > > coherency domain of the > > > > - calling CPU. > > > > - > > > > - Writes back the data cache lines specified by Address > > and Length. If Address > > > > - is not aligned on a cache line boundary, then entire > > data cache line > > > > - containing Address is written back. If Address + Length > > is not aligned on a > > > > - cache line boundary, then the entire data cache line > > containing Address + > > > > - Length -1 is written back. This function may choose to > > write back the entire > > > > - data cache if that is more efficient than writing back > > the specified range. > > > > - If Length is 0, then no data cache lines are written > > back. This function may > > > > - also invalidate all the data cache lines in the > > specified range of the cache > > > > - coherency domain of the calling CPU. Address is > > returned. > > > > - > > > > - If Length is greater than (MAX_ADDRESS - Address + 1), > > then ASSERT(). > > > > - > > > > - @param Address The base address of the data cache lines > > to write back. If > > > > - the CPU is in a physical addressing > > mode, then Address is a > > > > - physical address. If the CPU is in a > > virtual addressing > > > > - mode, then Address is a virtual address. > > > > - @param Length The number of bytes to write back from > > the data cache. > > > > - > > > > - @return Address > > > > - > > > > -**/ > > > > VOID * > > > > EFIAPI > > > > WriteBackDataCacheRange ( > > > > @@ -182,58 +113,11 @@ WriteBackDataCacheRange ( > > > > IN UINTN Length > > > > ) > > > > { > > > > - ASSERT (Length <= MAX_ADDRESS - (UINTN)Address + 1); > > > > - ASSERT(FALSE); > > > > + CacheRangeOperation(Address, Length, > > ArmCleanDataCacheEntryByMVA, > > > > + ArmDataCacheLineLength ()); > > > > return Address; > > > > } > > > > > > > > -/** > > > > - Invalidates the entire data cache in cache coherency > > domain of the calling > > > > - CPU. > > > > - > > > > - Invalidates the entire data cache in cache coherency > > domain of the calling > > > > - CPU. This function must be used with care because dirty > > cache lines are not > > > > - written back to system memory. It is typically used for > > cache diagnostics. If > > > > - the CPU does not support invalidation of the entire data > > cache, then a write > > > > - back and invalidate operation should be performed on the > > entire data cache. > > > > - > > > > -**/ > > > > -VOID > > > > -EFIAPI > > > > -InvalidateDataCache ( > > > > - VOID > > > > - ) > > > > -{ > > > > - ASSERT(FALSE); > > > > -} > > > > - > > > > -/** > > > > - Invalidates a range of data cache lines in the cache > > coherency domain of the > > > > - calling CPU. > > > > - > > > > - Invalidates the data cache lines specified by Address > > and Length. If Address > > > > - is not aligned on a cache line boundary, then entire > > data cache line > > > > - containing Address is invalidated. If Address + Length > > is not aligned on a > > > > - cache line boundary, then the entire data cache line > > containing Address + > > > > - Length -1 is invalidated. This function must never > > invalidate any cache lines > > > > - outside the specified range. If Length is 0, then no > > data cache lines are > > > > - invalidated. Address is returned. This function must be > > used with care > > > > - because dirty cache lines are not written back to system > > memory. It is > > > > - typically used for cache diagnostics. If the CPU does > > not support > > > > - invalidation of a data cache range, then a write back > > and invalidate > > > > - operation should be performed on the data cache range. > > > > - > > > > - If Length is greater than (MAX_ADDRESS - Address + 1), > > then ASSERT(). > > > > - > > > > - @param Address The base address of the data cache lines > > to invalidate. If > > > > - the CPU is in a physical addressing > > mode, then Address is a > > > > - physical address. If the CPU is in a > > virtual addressing mode, > > > > - then Address is a virtual address. > > > > - @param Length The number of bytes to invalidate from > > the data cache. > > > > - > > > > - @return Address > > > > - > > > > -**/ > > > > VOID * > > > > EFIAPI > > > > InvalidateDataCacheRange ( > > > > @@ -241,7 +125,7 @@ InvalidateDataCacheRange ( > > > > IN UINTN Length > > > > ) > > > > { > > > > - ASSERT (Length <= MAX_ADDRESS - (UINTN)Address + 1); > > > > - ASSERT(FALSE); > > > > + CacheRangeOperation(Address, Length, > > ArmInvalidateDataCacheEntryByMVA, > > > > + ArmDataCacheLineLength ()); > > > > return Address; > > > > } > > > > diff --git > > a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLi > > b.inf > > > > > > b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLi > > b.inf > > > > index d659161f33..7440a0062b 100644 > > > > --- > > a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLi > > b.inf > > > > +++ > > b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLi > > b.inf > > > > @@ -59,3 +59,5 @@ > > > > [LibraryClasses.Ipf] > > > > PalLib > > > > > > > > +[LibraryClasses.ARM,LibraryClasses.AARCH64] > > > > + ArmLib > > > > -- > > > > 2.11.0 > > > > > _______________________________________________ > > edk2-devel mailing list > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
