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
 
   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.
+  // 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)
+    );
+
+  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
+  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__)
+      );
+    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__));
+  }
+
   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__));
+  }
+
   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__)
+      );
+    InvalidateDataCache ();
+  }
+
   return Address;
 }
diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
index 5c1fa24065c7..f49c33191054 100644
--- a/MdePkg/MdePkg.uni
+++ b/MdePkg/MdePkg.uni
@@ -287,6 +287,10 @@
 
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdGuidedExtractHandlerTableAddress_HELP  #language en-US "This value is used to set the available memory address to store Guided Extract Handlers. The required memory space is decided by the value of PcdMaximumGuidedExtractHandler."
 
+#string STR_gEfiMdePkgTokenSpaceGuid_PcdRiscVFeatureOverride_PROMPT  #language en-US "RISC-V Feature Override"
+
+#string STR_gEfiMdePkgTokenSpaceGuid_PcdRiscVFeatureOverride_HELP  #language en-US "This value is used to Override Any RISC-V specific features supported by this PCD"
+
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_PROMPT  #language en-US "PCI Express Base Address"
 
 #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_HELP  #language en-US "This value is used to set the base address of PCI express hierarchy."
-- 
2.39.2



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

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Replied inline. Most of the cases I have addressed in the new patch I
submitted.

On Wed, Oct 25, 2023 at 1:39 AM Pedro Falcato <pedro.falcato@gmail.com>
wrote:

> 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).
>
Done. I believe you meant to keep it a generic description.

>
> >
> >    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 :)
>
> Done.

> > +  // 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.
>
> Done.

> > +
> > +  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
>
> Done.

> Also, what do you mean? 1) you're calling CacheOpCacheRange for the range
> 2) please don't mix CBO and CMO. Too much terminology :)
>
> Modified naming to be consistent. All CMO now.


> 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.
>
My understanding is that Zicbom operates on both. It just needs an address
to act on.


> > +  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__)
>
> Done with minor modification.

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).
>
>  Done.

> > +  }
> > +
> >    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.
>
Done.

> > +  }
> > +
> >    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.
>
> Done.

> Pedro
>


-- 
Thanks!
=D


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

On 10/21/23 19:33, 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>
> 
> 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
>  
>    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>

One nit: given that we're introducing a PcdLib dependency here, we
should also add the following to the library instance INF file:

[LibraryClasses.RISCV64]
  PcdLib

I've not deeply verified the implementation in this patch, but
structurally it looks OK to me. So you can add:

Acked-by: Laszlo Ersek <lersek@redhat.com>

to the next version (with the LibraryClasses addition).

Thanks
Laszlo


> +
> +// 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.
> +  // 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)
> +    );
> +
> +  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
> +  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__)
> +      );
> +    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__));
> +  }
> +
>    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__));
> +  }
> +
>    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__)
> +      );
> +    InvalidateDataCache ();
> +  }
> +
>    return Address;
>  }
> diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
> index 5c1fa24065c7..f49c33191054 100644
> --- a/MdePkg/MdePkg.uni
> +++ b/MdePkg/MdePkg.uni
> @@ -287,6 +287,10 @@
>  
>  #string STR_gEfiMdePkgTokenSpaceGuid_PcdGuidedExtractHandlerTableAddress_HELP  #language en-US "This value is used to set the available memory address to store Guided Extract Handlers. The required memory space is decided by the value of PcdMaximumGuidedExtractHandler."
>  
> +#string STR_gEfiMdePkgTokenSpaceGuid_PcdRiscVFeatureOverride_PROMPT  #language en-US "RISC-V Feature Override"
> +
> +#string STR_gEfiMdePkgTokenSpaceGuid_PcdRiscVFeatureOverride_HELP  #language en-US "This value is used to Override Any RISC-V specific features supported by this PCD"
> +
>  #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_PROMPT  #language en-US "PCI Express Base Address"
>  
>  #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_HELP  #language en-US "This value is used to set the base address of PCI express hierarchy."



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110005): https://edk2.groups.io/g/devel/message/110005
Mute This Topic: https://groups.io/mt/102103783/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-