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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

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