Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V

["Sunil V L" <sunilvl@ventanamicro.com>]

On Wed, Dec 13, 2023 at 08:29:30PM +0530, Dhaval Sharma wrote:
> Use newly defined cache management operations for RISC-V where possible
> It builds up on the support added for RISC-V cache management
> instructions in BaseLib.
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> 
> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
> ---
> 
> Notes:
>     V10:
>     - Fix formatting to keep comments within 80
>     - Replace RV with RISC-V
>     - Fix an issue with multi line comments
>     - Added assert to an unsupported function
>     - Minor case modification in str in .uni
> 
>     V9:
>     - Fixed an issue with Instruction cache invalidation. Use fence.i
>       instruction as CMO does not support i-cache operations.
>     V8:
>     - Added note to convert PCD into RISC-V feature bitmap pointer
>     - Modified function names to be more explicit about cache ops
>     - Added RB tag
>     V7:
>     - Added PcdLib
>     - Restructure DEBUG message based on feedback on V6
>     - Make naming consistent to CMO, remove all CBO references
>     - Add ASSERT for not supported functions instead of plain debug message
>     - Added RB tag
>     V6:
>     - Utilize cache management instructions if HW supports it
>       This patch is part of restructuring on top of v5
> 
>  MdePkg/MdePkg.dec                                                  |   8 +
>  MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |   5 +
>  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c                | 177 ++++++++++++++++----
>  MdePkg/MdePkg.uni                                                  |   4 +
>  4 files changed, 166 insertions(+), 28 deletions(-)
> 
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index ac54338089e8..fa92673ff633 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2399,6 +2399,14 @@ [PcdsFixedAtBuild.AARCH64, PcdsPatchableInModule.AARCH64]
>    # @Prompt CPU Rng algorithm's GUID.
>    gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00000037
>  
> +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
> +  #
> +  # Configurability to override RISC-V CPU Features
> +  # BIT 0 = Cache Management Operations. This bit is relevant only if
> +  # previous stage has feature enabled and user wants to disable it.
> +  #
> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UINT64|0x69
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    ## This value is used to set the base address of PCI express hierarchy.
>    # @Prompt PCI Express Base Address.
> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> index 6fd9cbe5f6c9..601a38d6c109 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> @@ -56,3 +56,8 @@ [LibraryClasses]
>    BaseLib
>    DebugLib
>  
> +[LibraryClasses.RISCV64]
> +  PcdLib
> +
> +[Pcd.RISCV64]
> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride  ## CONSUMES
> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> index ac2a3c23a249..7c53a17abbb5 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> @@ -2,6 +2,7 @@
>    RISC-V specific functionality for cache.
>  
>    Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +  Copyright (c) 2023, Rivos Inc. All rights reserved.<BR>
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
> @@ -9,10 +10,116 @@
>  #include <Base.h>
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +
> +//
> +// TODO: Grab cache block size and make Cache Management Operation
> +// enabling decision based on RISC-V CPU HOB in
> +// future when it is available and convert PcdRiscVFeatureOverride
> +// PCD to a pointer that contains pointer to bitmap structure
> +// which can be operated more elegantly.
> +//
> +#define RISCV_CACHE_BLOCK_SIZE         64
> +#define RISCV_CPU_FEATURE_CMO_BITMASK  0x1
> +
> +typedef enum {
> +  CacheOpClean,
> +  CacheOpFlush,
> +  CacheOpInvld,
> +} CACHE_OP;
> +
> +/**
> +Verify CBOs are supported by this HW
> +TODO: Use RISC-V CPU HOB once available.
> +
> +**/
> +STATIC
> +BOOLEAN
> +RiscVIsCMOEnabled (
> +  VOID
> +  )
> +{
> +  // If CMO is disabled in HW, skip Override check
> +  // Otherwise this PCD can override settings
> +  return ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_CMO_BITMASK) != 0);
> +}
> +
> +/**
> +  Performs required opeartion on cache lines in the cache coherency domain
> +  of the calling CPU. If Address is not aligned on a cache line boundary,
> +  then entire cache line containing Address is operated. If Address + Length
> +  is not aligned on a cache line boundary, then the entire cache line
> +  containing Address + Length -1 is operated.
> +  If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
> +  @param  Address The base address of the cache lines to
> +          invalidate.
> +  @param  Length  The number of bytes to invalidate from the instruction
> +          cache.
> +  @param  Op  Type of CMO operation to be performed
> +  @return Address.
> +
> +**/
> +STATIC
> +VOID
> +CacheOpCacheRange (
> +  IN VOID      *Address,
> +  IN UINTN     Length,
> +  IN CACHE_OP  Op
> +  )
> +{
> +  UINTN  CacheLineSize;
> +  UINTN  Start;
> +  UINTN  End;
> +
> +  if (Length == 0) {
> +    return;
> +  }
> +
> +  if ((Op != CacheOpInvld) && (Op != CacheOpFlush) && (Op != CacheOpClean)) {
> +    return;
> +  }
> +
> +  ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)Address));
> +
> +  CacheLineSize = RISCV_CACHE_BLOCK_SIZE;
> +
> +  Start = (UINTN)Address;
> +  //
> +  // Calculate the cache line alignment
> +  //
> +  End    = (Start + Length + (CacheLineSize - 1)) & ~(CacheLineSize - 1);
> +  Start &= ~((UINTN)CacheLineSize - 1);
> +
> +  DEBUG (
> +    (DEBUG_VERBOSE,
> +     "CacheOpCacheRange: Performing Cache Management Operation %d \n", Op)
> +    );
> +
> +  do {
> +    switch (Op) {
> +      case CacheOpInvld:
> +        RiscVCpuCacheInvalCmoAsm (Start);
> +        break;
> +      case CacheOpFlush:
> +        RiscVCpuCacheFlushCmoAsm (Start);
> +        break;
> +      case CacheOpClean:
> +        RiscVCpuCacheCleanCmoAsm (Start);
> +        break;
> +      default:
> +        break;
> +    }
> +
> +    Start = Start + CacheLineSize;
> +  } while (Start != End);
> +}
>  
>  /**
>    Invalidates the entire instruction cache in cache coherency domain of the
> -  calling CPU.
> +  calling CPU. Risc-V does not have currently an CBO implementation which can
> +  invalidate the entire I-cache. Hence using Fence instruction for now. P.S.
> +  Fence instruction may or may not implement full I-cache invd functionality
> +  on all implementations.
>  
>  **/
>  VOID
> @@ -28,17 +135,11 @@ InvalidateInstructionCache (
>    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().
> -
> +  An operation from a CMO instruction is defined to operate only on the copies
> +  of a cache block that are cached in the caches accessible by the explicit
> +  memory accesses performed by the set of coherent agents.In other words CMO
> +  operations are not applicable to instruction cache. Use fence.i instruction
> +  instead to achieve the same purpose.
>    @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
> @@ -57,9 +158,10 @@ InvalidateInstructionCacheRange (
>    )
>  {
>    DEBUG (
> -    (DEBUG_WARN,
> -     "%a:RISC-V unsupported function.\n"
> -     "Invalidating the whole instruction cache instead.\n", __func__)
> +    (DEBUG_VERBOSE,
> +     "InvalidateInstructionCacheRange: RISC-V unsupported function.\n"
> +     "Invalidating the whole instruction cache instead.\n"
> +    )
>      );
>    InvalidateInstructionCache ();
>    return Address;
> @@ -81,7 +183,12 @@ WriteBackInvalidateDataCache (
>    VOID
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  ASSERT (FALSE);
> +  DEBUG ((
> +    DEBUG_ERROR,
> +    "WriteBackInvalidateDataCache:" \
> +    "RISC-V unsupported function.\n"
I guess this formatting was pointed by Pedro earlier. This can be made
single line. Let me fix it before merging so that you don't need to send
another version.

Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112710): https://edk2.groups.io/g/devel/message/112710
Mute This Topic: https://groups.io/mt/103150435/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-