Re: [PATCH] MdePkg: add ARM/AARCH64 support to BaseCacheMaintenanceLib

["Gao, Liming" <liming.gao@intel.com>]

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 <leif.lindholm@linaro.org>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 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:edk2-devel-bounces@lists.01.org] On
> > Behalf Of Leif Lindholm
> > Sent: Wednesday, September 20, 2017 8:06 AM
> > To: Gao, Liming <liming.gao@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
> > devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > 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:leif.lindholm@linaro.org]
> > > > Sent: Wednesday, September 20, 2017 9:23 PM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
> > Liming <liming.gao@intel.com>; Ard Biesheuvel
> > > > <ard.biesheuvel@linaro.org>
> > > > 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 <leif.lindholm@linaro.org>
> > > > ---
> > > >
> > > > 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
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel