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

["Pedro Falcato" <pedro.falcato@gmail.com>]

On Sat, Oct 21, 2023 at 6:33 PM Dhaval Sharma <dhaval@rivosinc.com> 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>
>
> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> ---
>
> Notes:
>     V1:
>     - 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 |   2 +
>  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c                | 159 +++++++++++++++++---
>  MdePkg/MdePkg.uni                                                  |   4 +
>  4 files changed, 154 insertions(+), 19 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..39a7fb963b49 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> @@ -56,3 +56,5 @@ [LibraryClasses]
>    BaseLib
>    DebugLib
>
> +[Pcd.RISCV64]
> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride  ## CONSUMES
> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> index 4eb18edb9aa7..6851970c9e16 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> @@ -1,7 +1,8 @@
>  /** @file
> -  RISC-V specific functionality for cache.
> +  Implement Risc-V Cache Management Operations

Why the change? You're effectively implementing cache management for
riscv, you're not exclusively using any sort of extension (such as
CMO).

>
>    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,111 @@
>  #include <Base.h>
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +
> +// TODO: This will be removed once RISC-V CPU HOB is available
> +#define RISCV_CACHE_BLOCK_SIZE         64
> +#define RISCV_CPU_FEATURE_CMO_BITMASK  0x1
> +
> +typedef enum {
> +  Clean,
> +  Flush,
> +  Invld,
> +} CACHE_OP;
> +
> +/**
> +Verify CBOs are supported by this HW
> +TODO: Use RISC-V CPU HOB once available.
> +
> +**/
> +STATIC
> +BOOLEAN
> +RiscVIsCMOEnabled (
> +  VOID
> +  )
> +{
> +  // TODO: Add check for CMO from CPU HOB.

Too many TODOs? One TODO at the top of the file (mentioning feature
detection, cache line size detection) should be enough. There's no
point in peppering these out throughout the file :)

> +  // 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 != Invld) && (Op != Flush) && (Op != Clean)) {
> +    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_INFO,
> +     "%a Performing Cache Management Operation %d \n", __func__, Op)
> +    );

nit: Can we pick a log style here? Like <something>: <log message>
In this case, "CacheOpCacheRange: Performing ...". It's just prettier
and more greppable.

> +
> +  do {
> +    switch (Op) {
> +      case Invld:
> +        RiscVCpuCacheInvalAsmCbo (Start);
> +        break;
> +      case Flush:
> +        RiscVCpuCacheFlushAsmCbo (Start);
> +        break;
> +      case Clean:
> +        RiscVCpuCacheCleanAsmCbo (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 entire I-cache. Hence using Fence instruction for now. P.S. Fence
nit: Invalidate *the* entire I-cache

Also, what do you mean? 1) you're calling CacheOpCacheRange for the range
2) please don't mix CBO and CMO. Too much terminology :)

Does Zicbom only operate on the dcache? Or does it also touch the
icache? As far as I'm aware, riscv does not require a dcache/icache
coherency (like ARM), but a quick stroll through the extension's spec
didn't lead me to much.
> +  instruction may or may not implement full I-cache invd functionality on all
> +  implementations.
>
>  **/
>  VOID
> @@ -56,12 +158,17 @@ InvalidateInstructionCacheRange (
>    IN UINTN  Length
>    )
>  {
> -  DEBUG (
> -    (DEBUG_WARN,
> -     "%a:RISC-V unsupported function.\n"
> -     "Invalidating the whole instruction cache instead.\n", __func__)
> -    );
> -  InvalidateInstructionCache ();
> +  if (RiscVIsCMOEnabled ()) {
> +    CacheOpCacheRange (Address, Length, Invld);
> +  } else {
> +    DEBUG (
> +      (DEBUG_WARN,
> +       "%a:RISC-V unsupported function.\n"
> +       "Invalidating the whole instruction cache instead.\n", __func__)
> +      );
nit: "%a: ", with a space, but i'd probably write this bit as
> +       "%a: Zicbom not enabled, invalidating the whole instruction cache instead.\n", __func__)

Given that this may be repeatedly called, can we just warn once (e.g
on a constructor) that CMO is disabled and leave this be? Since the
cache management is in fact being done correctly, but just in a slower
way.

> +    InvalidateInstructionCache ();
> +  }
> +
>    return Address;
>  }
>
> @@ -117,7 +224,12 @@ WriteBackInvalidateDataCacheRange (
>    IN      UINTN  Length
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  if (RiscVIsCMOEnabled ()) {
> +    CacheOpCacheRange (Address, Length, Flush);
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));

Ok, so here it may make sense to ASSERT, as you're no longer complying
to the function's behavior (and this is unsafe).

> +  }
> +
>    return Address;
>  }
>
> @@ -156,10 +268,7 @@ WriteBackDataCache (
>
>    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  Address The base address of the data cache lines to write back.
>    @param  Length  The number of bytes to write back from the data cache.
>
>    @return Address of cache written in main memory.
> @@ -172,7 +281,12 @@ WriteBackDataCacheRange (
>    IN      UINTN  Length
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  if (RiscVIsCMOEnabled ()) {
> +    CacheOpCacheRange (Address, Length, Clean);
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));

Same comment here WRT assert.
> +  }
> +
>    return Address;
>  }
>
> @@ -214,10 +328,7 @@ InvalidateDataCache (
>
>    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  Address The base address of the data cache lines to invalidate.
>    @param  Length  The number of bytes to invalidate from the data cache.
>
>    @return Address.
> @@ -230,6 +341,16 @@ InvalidateDataCacheRange (
>    IN      UINTN  Length
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  if (RiscVIsCMOEnabled ()) {
> +    CacheOpCacheRange (Address, Length, Invld);
> +  } else {
> +    DEBUG (
> +      (DEBUG_WARN,
> +       "%a:RISC-V unsupported function.\n"
> +       "Invalidating the whole Data cache instead.\n", __func__)
> +      );

Aaaand the same comment here WRT warning.

Pedro


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