[edk2-devel] [PATCH v9 0/5] Cache Management Operations Support For RISC-V

[edk2-devel] [PATCH v9 0/5] Cache Management Operations Support For RISC-V

[Dhaval Sharma]

Implementing code to support Cache Management Operations (CMO) defined by
RISC-V CMO instructions.https://github.com/riscv/riscv-CMOs
This is a re-write of original series v5.
The patchset contains 5 patches- created based on V5 feedback.
1. Restructuring of existing code and move instruction declarations into BaseLib
2. Renaming existing functions to denote type of instruction used to maanage cache.
   This is useful for further patches where more cache management instructions are added.
3. Add the new cache maintenance operations to BaseLib, including the
	 new assembly instruction encodings.
4. Update BaseCacheMaintenanceLib (utilizing the new BaseLib primitives)
5. Add platform level PCD to allow overriding of RISC-V features.

Code Link: https://github.com/tianocore/edk2/pull/5103

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Sunil V L <sunilvl@ventanamicro.com>
Cc: Andrei Warkentin <andrei.warkentin@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Daniel Schaefer <git@danielschaefer.me>


Dhaval (5):
  MdePkg: Move RISC-V Cache Management Declarations Into BaseLib
  MdePkg: Rename Cache Management Function To Clarify Fence Based Op
  MdePkg: Implement RISC-V Cache Management Operations
  MdePkg: Utilize Cache Management Operations Implementation For RISC-V
  OvmfPkg/RiscVVirt: Override for RV CPU Features

 MdePkg/MdePkg.dec                                                  |   8 +
 OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc                                |   1 +
 MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |   5 +
 MdePkg/Library/BaseLib/BaseLib.inf                                 |   2 +-
 MdePkg/Include/Library/BaseLib.h                                   |  53 ++++++
 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c                | 177 +++++++++++++++-----
 MdePkg/Include/RiscV64/RiscVasm.inc                                |  19 +++
 MdePkg/Library/BaseLib/RiscV64/FlushCache.S                        |  21 ---
 MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S                    |  38 +++++
 MdePkg/MdePkg.uni                                                  |   4 +
 10 files changed, 264 insertions(+), 64 deletions(-)
 create mode 100644 MdePkg/Include/RiscV64/RiscVasm.inc
 delete mode 100644 MdePkg/Library/BaseLib/RiscV64/FlushCache.S
 create mode 100644 MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S

-- 
2.39.2



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

[edk2-devel] [PATCH v9 1/5] MdePkg: Move RISC-V Cache Management Declarations Into BaseLib

[Dhaval Sharma]

The declarations for cache Management functions belong to BaseLib
instead of instance source file. This helps with further restructuring
of cache management code for RISC-V.

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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    V7:
    - Added RB tag
    V6:
    - Move cache management function declaration in baselib where it belongs

 MdePkg/Include/Library/BaseLib.h                    | 20 ++++++++++++++++++++
 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c | 20 --------------------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 5d7067ee854e..7142bbfa42f2 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -206,6 +206,26 @@ RiscVClearPendingTimerInterrupt (
   VOID
   );
 
+/**
+  RISC-V invalidate instruction cache.
+
+**/
+VOID
+EFIAPI
+RiscVInvalidateInstCacheAsm (
+  VOID
+  );
+
+/**
+  RISC-V invalidate data cache.
+
+**/
+VOID
+EFIAPI
+RiscVInvalidateDataCacheAsm (
+  VOID
+  );
+
 #endif // defined (MDE_CPU_RISCV64)
 
 #if defined (MDE_CPU_LOONGARCH64)
diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
index d08fb9f193ca..d5efcf49a4bf 100644
--- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
@@ -10,26 +10,6 @@
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 
-/**
-  RISC-V invalidate instruction cache.
-
-**/
-VOID
-EFIAPI
-RiscVInvalidateInstCacheAsm (
-  VOID
-  );
-
-/**
-  RISC-V invalidate data cache.
-
-**/
-VOID
-EFIAPI
-RiscVInvalidateDataCacheAsm (
-  VOID
-  );
-
 /**
   Invalidates the entire instruction cache in cache coherency domain of the
   calling CPU.
-- 
2.39.2



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

[edk2-devel] [PATCH v9 2/5] MdePkg: Rename Cache Management Function To Clarify Fence Based Op

[Dhaval Sharma]

There are different ways to manage cache on RISC-V Processors.
One way is to use fence instruction. Another way is to use CPU
specific cache management operation instructions ratified as
per RISC-V ISA specifications to be introduced in future
patches. Current method is fence instruction based, rename the
function accordingly to add that clarity.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Sunil V L <sunilvl@ventanamicro.com>
Cc: Daniel Schaefer <git@danielschaefer.me>
Cc: Laszlo Ersek <lersek@redhat.com>

Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    V8:
    - Update function name to udpate *asm* in the end
    V7:
    - Add RB tag
    V6:
    - As part of restructuring, adding cache instruction differentiation
      in function naming

 MdePkg/Include/Library/BaseLib.h                    | 4 ++--
 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c | 4 ++--
 MdePkg/Library/BaseLib/RiscV64/FlushCache.S         | 8 ++++----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 7142bbfa42f2..d80e27285424 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -212,7 +212,7 @@ RiscVClearPendingTimerInterrupt (
 **/
 VOID
 EFIAPI
-RiscVInvalidateInstCacheAsm (
+RiscVInvalidateInstCacheFenceAsm (
   VOID
   );
 
@@ -222,7 +222,7 @@ RiscVInvalidateInstCacheAsm (
 **/
 VOID
 EFIAPI
-RiscVInvalidateDataCacheAsm (
+RiscVInvalidateDataCacheFenceAsm (
   VOID
   );
 
diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
index d5efcf49a4bf..ac2a3c23a249 100644
--- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
@@ -21,7 +21,7 @@ InvalidateInstructionCache (
   VOID
   )
 {
-  RiscVInvalidateInstCacheAsm ();
+  RiscVInvalidateInstCacheFenceAsm ();
 }
 
 /**
@@ -193,7 +193,7 @@ InvalidateDataCache (
   VOID
   )
 {
-  RiscVInvalidateDataCacheAsm ();
+  RiscVInvalidateDataCacheFenceAsm ();
 }
 
 /**
diff --git a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S b/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
index 7c10fdd268af..8cfb85097996 100644
--- a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
+++ b/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
@@ -9,13 +9,13 @@
 //------------------------------------------------------------------------------
 
 .align 3
-ASM_GLOBAL ASM_PFX(RiscVInvalidateInstCacheAsm)
-ASM_GLOBAL ASM_PFX(RiscVInvalidateDataCacheAsm)
+ASM_GLOBAL ASM_PFX(RiscVInvalidateInstCacheFenceAsm)
+ASM_GLOBAL ASM_PFX(RiscVInvalidateDataCacheFenceAsm)
 
-ASM_PFX(RiscVInvalidateInstCacheAsm):
+ASM_PFX(RiscVInvalidateInstCacheFenceAsm):
     fence.i
     ret
 
-ASM_PFX(RiscVInvalidateDataCacheAsm):
+ASM_PFX(RiscVInvalidateDataCacheFenceAsm):
     fence
     ret
-- 
2.39.2



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

[edk2-devel] [PATCH v9 3/5] MdePkg: Implement RISC-V Cache Management Operations

[Dhaval Sharma]

Implement Cache Management Operations (CMO) defined by
RISC-V spec https://github.com/riscv/riscv-CMOs.

Notes:
1. CMO only supports block based Operations. Meaning cache
   flush/invd/clean Operations are not available for the entire
   range. In that case we fallback on fence.i instructions.
2. Operations are implemented using Opcodes to make them compiler
   independent. binutils 2.39+ compilers support CMO instructions.

Test:
1. Ensured correct instructions are refelecting in asm
2. Qemu implements basic support for CMO operations in that it allwos
   instructions without exceptions. Verified it works properly in
   that sense.
3. SG2042Pkg implements CMO-like instructions. It was verified that
   CpuFlushCpuDataCache works fine. This more of less
   confirms that framework is alright.
4. TODO: Once Silicon is available with exact instructions, we will
   further verify this.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Sunil V L <sunilvl@ventanamicro.com>
Cc: Daniel Schaefer <git@danielschaefer.me>
Cc: Laszlo Ersek <lersek@redhat.com>

Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Sunil V L <sunilvl@...>
Reviewed-by: Jingyu Li <jingyu.li01@...>
---

Notes:
    v8:
    - Add *asm* postfix to cmo functions
    - Add reviewed by tags
    V7:
    - Modify instruction names as per feedback from V6
    - Added RB
    V6:
    - Implement Cache management instructions in Baselib

 MdePkg/Library/BaseLib/BaseLib.inf                                |  2 +-
 MdePkg/Include/Library/BaseLib.h                                  | 33 ++++++++++++++++++++
 MdePkg/Include/RiscV64/RiscVasm.inc                               | 19 +++++++++++
 MdePkg/Library/BaseLib/RiscV64/{FlushCache.S => RiscVCacheMgmt.S} | 17 ++++++++++
 4 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
index 03c7b02e828b..53389389448c 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -400,7 +400,7 @@ [Sources.RISCV64]
   RiscV64/RiscVCpuBreakpoint.S      | GCC
   RiscV64/RiscVCpuPause.S           | GCC
   RiscV64/RiscVInterrupt.S          | GCC
-  RiscV64/FlushCache.S              | GCC
+  RiscV64/RiscVCacheMgmt.S          | GCC
   RiscV64/CpuScratch.S              | GCC
   RiscV64/ReadTimer.S               | GCC
   RiscV64/RiscVMmu.S                | GCC
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index d80e27285424..47424709cd72 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -226,6 +226,39 @@ RiscVInvalidateDataCacheFenceAsm (
   VOID
   );
 
+/**
+  RISC-V flush cache block. Atomically perform a clean operation
+  followed by an invalidate operation
+
+**/
+VOID
+EFIAPI
+RiscVCpuCacheFlushCmoAsm (
+  IN UINTN
+  );
+
+/**
+Perform a write transfer to another cache or to memory if the
+data in the copy of the cache block have been modified by a store
+operation
+
+**/
+VOID
+EFIAPI
+RiscVCpuCacheCleanCmoAsm (
+  IN UINTN
+  );
+
+/**
+Deallocate the copy of the cache block
+
+**/
+VOID
+EFIAPI
+RiscVCpuCacheInvalCmoAsm (
+  IN UINTN
+  );
+
 #endif // defined (MDE_CPU_RISCV64)
 
 #if defined (MDE_CPU_LOONGARCH64)
diff --git a/MdePkg/Include/RiscV64/RiscVasm.inc b/MdePkg/Include/RiscV64/RiscVasm.inc
new file mode 100644
index 000000000000..29de7358855c
--- /dev/null
+++ b/MdePkg/Include/RiscV64/RiscVasm.inc
@@ -0,0 +1,19 @@
+/*
+ *
+ * RISC-V cache operation encoding.
+ * Copyright (c) 2023, Rivos Inc. All rights reserved.<BR>
+ * SPDX-License-Identifier: BSD-2-Clause-Patent
+ *
+ */
+
+.macro RISCVCMOFLUSH
+    .word 0x25200f
+.endm
+
+.macro RISCVCMOINVALIDATE
+    .word 0x05200f
+.endm
+
+.macro RISCVCMOCLEAN
+    .word 0x15200f
+.endm
diff --git a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S b/MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S
similarity index 56%
rename from MdePkg/Library/BaseLib/RiscV64/FlushCache.S
rename to MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S
index 8cfb85097996..4752aa72d95e 100644
--- a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
+++ b/MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S
@@ -3,10 +3,12 @@
 // RISC-V cache operation.
 //
 // 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
 //
 //------------------------------------------------------------------------------
+.include "RiscVasm.inc"
 
 .align 3
 ASM_GLOBAL ASM_PFX(RiscVInvalidateInstCacheFenceAsm)
@@ -19,3 +21,18 @@ ASM_PFX(RiscVInvalidateInstCacheFenceAsm):
 ASM_PFX(RiscVInvalidateDataCacheFenceAsm):
     fence
     ret
+
+ASM_GLOBAL ASM_PFX (RiscVCpuCacheFlushCmoAsm)
+ASM_PFX (RiscVCpuCacheFlushCmoAsm):
+    RISCVCMOFLUSH
+    ret
+
+ASM_GLOBAL ASM_PFX (RiscVCpuCacheCleanCmoAsm)
+ASM_PFX (RiscVCpuCacheCleanCmoAsm):
+    RISCVCMOCLEAN
+    ret
+
+ASM_GLOBAL ASM_PFX (RiscVCpuCacheInvalCmoAsm)
+ASM_PFX (RiscVCpuCacheInvalCmoAsm):
+    RISCVCMOINVALIDATE
+    ret
-- 
2.39.2



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

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

[Dhaval Sharma]

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>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    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                | 173 ++++++++++++++++----
 MdePkg/MdePkg.uni                                                  |   4 +
 4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
 #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_INFO,
+     "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 +136,10 @@ 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
@@ -56,11 +157,7 @@ InvalidateInstructionCacheRange (
   IN UINTN  Length
   )
 {
-  DEBUG (
-    (DEBUG_WARN,
-     "%a:RISC-V unsupported function.\n"
-     "Invalidating the whole instruction cache instead.\n", __func__)
-    );
+  DEBUG ((DEBUG_VERBOSE, "InvalidateInstructionCache:\n"));
   InvalidateInstructionCache ();
   return Address;
 }
@@ -81,7 +178,8 @@ WriteBackInvalidateDataCache (
   VOID
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  DEBUG ((DEBUG_ERROR, "WriteBackInvalidateDataCache:\
+  RISC-V unsupported function.\n"));
 }
 
 /**
@@ -117,7 +215,12 @@ WriteBackInvalidateDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  if (RiscVIsCMOEnabled ()) {
+    CacheOpCacheRange (Address, Length, CacheOpFlush);
+  } else {
+    ASSERT (FALSE);
+  }
+
   return Address;
 }
 
@@ -137,7 +240,7 @@ WriteBackDataCache (
   VOID
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  ASSERT (FALSE);
 }
 
 /**
@@ -156,10 +259,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 +272,12 @@ WriteBackDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  if (RiscVIsCMOEnabled ()) {
+    CacheOpCacheRange (Address, Length, CacheOpClean);
+  } else {
+    ASSERT (FALSE);
+  }
+
   return Address;
 }
 
@@ -214,10 +319,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 +332,17 @@ InvalidateDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  if (RiscVIsCMOEnabled ()) {
+    CacheOpCacheRange (Address, Length, CacheOpInvld);
+  } else {
+    DEBUG (
+      (DEBUG_VERBOSE,
+       "InvalidateDataCacheRange:\
+       Zicbom not supported.\n" \
+       "Invalidating the whole Data cache instead.\n")
+      );
+    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 (#112022): https://edk2.groups.io/g/devel/message/112022
Mute This Topic: https://groups.io/mt/102967058/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v9 5/5] OvmfPkg/RiscVVirt: Override for RV CPU Features

[Dhaval Sharma]

This PCD provides a way for platform to override any
HW features that are default enabled by previous stages
of FW (like OpenSBI). For the case where previous/prev
stage has disabled the feature, this override is not
useful and its usage should be avoided.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Sunil V L <sunilvl@ventanamicro.com>
Cc: Andrei Warkentin <andrei.warkentin@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>

Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Andrei Warkentin <andrei.warkentin@...>
---

Notes:
    V8:
    - Added RV tag
    V7:
    - Added RB tag
    v6:
    - Modify PCD name according to changes made in Baselib implementation
    V5:
    - Introduce PCD for platform

 OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
index fe320525153f..5d66f7fe6ae6 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
+++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
@@ -203,6 +203,7 @@ [PcdsFeatureFlag]
   gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
 
 [PcdsFixedAtBuild.common]
+  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0
   gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|1000000
   gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|1000000
   gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0
-- 
2.39.2



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

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

[Sunil V L]

Hi Dhaval,

Thank you very much for fixing the issue with instruction cache
invalidation and confirming with the spec owner. Few minor comments
below.

On Mon, Dec 04, 2023 at 01:59:49PM +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>
> 
> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     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
> 
IMO, it is better to keep the change log in the cover letter. Since not
all patches may be CC'd to every one apart from the cover letter, it is
difficult to understand from the cover letter what has changed in the new
series.

>  MdePkg/MdePkg.dec                                                  |   8 +
>  MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |   5 +
>  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c                | 173 ++++++++++++++++----
>  MdePkg/MdePkg.uni                                                  |   4 +
>  4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
>  #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
Can we make this also as a PCD?

> +#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_INFO,
> +     "CacheOpCacheRange:\
Use __func__ ? 

> +     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 +136,10 @@ 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.
Could you please keep the comments within 80 character? 

>    @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
> @@ -56,11 +157,7 @@ InvalidateInstructionCacheRange (
>    IN UINTN  Length
>    )
>  {
> -  DEBUG (
> -    (DEBUG_WARN,
> -     "%a:RISC-V unsupported function.\n"
> -     "Invalidating the whole instruction cache instead.\n", __func__)
> -    );
> +  DEBUG ((DEBUG_VERBOSE, "InvalidateInstructionCache:\n"));
This change is not required.

>    InvalidateInstructionCache ();
>    return Address;
>  }
> @@ -81,7 +178,8 @@ WriteBackInvalidateDataCache (
>    VOID
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  DEBUG ((DEBUG_ERROR, "WriteBackInvalidateDataCache:\
> +  RISC-V unsupported function.\n"));
This change is not required.

>  }
>  
>  /**
> @@ -117,7 +215,12 @@ WriteBackInvalidateDataCacheRange (
>    IN      UINTN  Length
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  if (RiscVIsCMOEnabled ()) {
> +    CacheOpCacheRange (Address, Length, CacheOpFlush);
> +  } else {
> +    ASSERT (FALSE);
> +  }
> +
>    return Address;
>  }
>  
> @@ -137,7 +240,7 @@ WriteBackDataCache (
>    VOID
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  ASSERT (FALSE);
>  }
>  
>  /**
> @@ -156,10 +259,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 +272,12 @@ WriteBackDataCacheRange (
>    IN      UINTN  Length
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  if (RiscVIsCMOEnabled ()) {
> +    CacheOpCacheRange (Address, Length, CacheOpClean);
> +  } else {
> +    ASSERT (FALSE);
> +  }
> +
>    return Address;
>  }
>  
> @@ -214,10 +319,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 +332,17 @@ InvalidateDataCacheRange (
>    IN      UINTN  Length
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  if (RiscVIsCMOEnabled ()) {
> +    CacheOpCacheRange (Address, Length, CacheOpInvld);
> +  } else {
> +    DEBUG (
> +      (DEBUG_VERBOSE,
> +       "InvalidateDataCacheRange:\
> +       Zicbom not supported.\n" \
> +       "Invalidating the whole Data cache instead.\n")
> +      );
> +    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"
"override any" instead of "Override Any"?

Thanks,
Sunil


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

Re: [edk2-devel] [PATCH v9 5/5] OvmfPkg/RiscVVirt: Override for RV CPU Features

[Sunil V L]

Hi Dhaval,

Few minor comments.

1) Please use RISC-V instead of RV every where.

On Mon, Dec 04, 2023 at 01:59:50PM +0530, Dhaval wrote:
> This PCD provides a way for platform to override any
> HW features that are default enabled by previous stages
> of FW (like OpenSBI). For the case where previous/prev
> stage has disabled the feature, this override is not
> useful and its usage should be avoided.
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Sunil V L <sunilvl@ventanamicro.com>
> Cc: Andrei Warkentin <andrei.warkentin@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> 
> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Andrei Warkentin <andrei.warkentin@...>
> ---
> 
> Notes:
>     V8:
>     - Added RV tag
>     V7:
>     - Added RB tag
>     v6:
>     - Modify PCD name according to changes made in Baselib implementation
>     V5:
>     - Introduce PCD for platform
> 
>  OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> index fe320525153f..5d66f7fe6ae6 100644
> --- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> +++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> @@ -203,6 +203,7 @@ [PcdsFeatureFlag]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
>  
>  [PcdsFixedAtBuild.common]
> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0
Could you set this to 0xFFFFFFFFFFFFFFFE? Just disable only CMO?

Thanks,
Snil


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

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

[Dhaval Sharma]

[-- Attachment #1: Type: text/plain, Size: 15350 bytes --]

Comments inline:


On Wed, Dec 6, 2023 at 7:50 PM Sunil V L <sunilvl@ventanamicro.com> wrote:

> Hi Dhaval,
>
> Thank you very much for fixing the issue with instruction cache
> invalidation and confirming with the spec owner. Few minor comments
> below.
>
> On Mon, Dec 04, 2023 at 01:59:49PM +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>
> >
> > Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> > Acked-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >
> > Notes:
> >     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
> >
> IMO, it is better to keep the change log in the cover letter. Since not
> all patches may be CC'd to every one apart from the cover letter, it is
> difficult to understand from the cover letter what has changed in the new
> series.
>
[Dhaval] AFAIU notes are tied to specific commits. But it makes sense, I
can add an update to the cover letter.

>
> >  MdePkg/MdePkg.dec                                                  |
>  8 +
> >  MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |
>  5 +
> >  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c                |
> 173 ++++++++++++++++----
> >  MdePkg/MdePkg.uni                                                  |
>  4 +
> >  4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
> >  #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
> Can we make this also as a PCD?
> [Dhaval] Actually this define should go away once we have CPU HOB. So
> thought to keep it simple for now. Anyways there is no plan to change this
> size
>
    anytime in the near future.

> +#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_INFO,
> > +     "CacheOpCacheRange:\
> Use __func__ ?


[Dhaval] Around patch 6 review there was a suggestion to define a function
name which is more greppable. Hence I had changed it from __func__ to this
:)

>
> [Dhaval]
> > +     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 +136,10 @@ 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.
> Could you please keep the comments within 80 character?

[Dhaval] will do.


>
> >    @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
> > @@ -56,11 +157,7 @@ InvalidateInstructionCacheRange (
> >    IN UINTN  Length
> >    )
> >  {
> > -  DEBUG (
> > -    (DEBUG_WARN,
> > -     "%a:RISC-V unsupported function.\n"
> > -     "Invalidating the whole instruction cache instead.\n", __func__)
> > -    );
> > +  DEBUG ((DEBUG_VERBOSE, "InvalidateInstructionCache:\n"));
> This change is not required.
>
> [Dhaval] Are you saying that the earlier comment was fine as it was?

>    InvalidateInstructionCache ();
> >    return Address;
> >  }
> > @@ -81,7 +178,8 @@ WriteBackInvalidateDataCache (
> >    VOID
> >    )
> >  {
> > -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> > +  DEBUG ((DEBUG_ERROR, "WriteBackInvalidateDataCache:\
> > +  RISC-V unsupported function.\n"));
> This change is not required.
>
> >  }
> >
> >  /**
> > @@ -117,7 +215,12 @@ WriteBackInvalidateDataCacheRange (
> >    IN      UINTN  Length
> >    )
> >  {
> > -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> > +  if (RiscVIsCMOEnabled ()) {
> > +    CacheOpCacheRange (Address, Length, CacheOpFlush);
> > +  } else {
> > +    ASSERT (FALSE);
> > +  }
> > +
> >    return Address;
> >  }
> >
> > @@ -137,7 +240,7 @@ WriteBackDataCache (
> >    VOID
> >    )
> >  {
> > -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> > +  ASSERT (FALSE);
> >  }
> >
> >  /**
> > @@ -156,10 +259,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 +272,12 @@ WriteBackDataCacheRange (
> >    IN      UINTN  Length
> >    )
> >  {
> > -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> > +  if (RiscVIsCMOEnabled ()) {
> > +    CacheOpCacheRange (Address, Length, CacheOpClean);
> > +  } else {
> > +    ASSERT (FALSE);
> > +  }
> > +
> >    return Address;
> >  }
> >
> > @@ -214,10 +319,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 +332,17 @@ InvalidateDataCacheRange (
> >    IN      UINTN  Length
> >    )
> >  {
> > -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> > +  if (RiscVIsCMOEnabled ()) {
> > +    CacheOpCacheRange (Address, Length, CacheOpInvld);
> > +  } else {
> > +    DEBUG (
> > +      (DEBUG_VERBOSE,
> > +       "InvalidateDataCacheRange:\
> > +       Zicbom not supported.\n" \
> > +       "Invalidating the whole Data cache instead.\n")
> > +      );
> > +    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"
> "override any" instead of "Override Any"?
>
> Thanks,
> Sunil
>


-- 
Thanks!
=D


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



[-- Attachment #2: Type: text/html, Size: 19816 bytes --]

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

[Sunil V L]

On Thu, Dec 07, 2023 at 10:31:48AM +0530, Dhaval Sharma wrote:
> Comments inline:
> 
> 
> On Wed, Dec 6, 2023 at 7:50 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> 
> > Hi Dhaval,
> >
> > Thank you very much for fixing the issue with instruction cache
> > invalidation and confirming with the spec owner. Few minor comments
> > below.
> >
> > On Mon, Dec 04, 2023 at 01:59:49PM +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>
> > >
> > > Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> > > Acked-by: Laszlo Ersek <lersek@redhat.com>
> > > ---
> > >
> > > Notes:
> > >     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
> > >
> > IMO, it is better to keep the change log in the cover letter. Since not
> > all patches may be CC'd to every one apart from the cover letter, it is
> > difficult to understand from the cover letter what has changed in the new
> > series.
> >
> [Dhaval] AFAIU notes are tied to specific commits. But it makes sense, I
> can add an update to the cover letter.
> 
> >
> > >  MdePkg/MdePkg.dec                                                  |
> >  8 +
> > >  MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |
> >  5 +
> > >  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c                |
> > 173 ++++++++++++++++----
> > >  MdePkg/MdePkg.uni                                                  |
> >  4 +
> > >  4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
> > >  #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
> > Can we make this also as a PCD?
> > [Dhaval] Actually this define should go away once we have CPU HOB. So
> > thought to keep it simple for now. Anyways there is no plan to change this
> > size
> >
>     anytime in the near future.
> 
What do you mean by no plan to change this size? Isn't it a choice of
the platform? Given that this macro is in MdePkg, it will force people
to use the same size.

> > +#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_INFO,
> > > +     "CacheOpCacheRange:\
> > Use __func__ ?
> 
> 
> [Dhaval] Around patch 6 review there was a suggestion to define a function
> name which is more greppable. Hence I had changed it from __func__ to this
> :)
> 
What was the actual comment? I am not aware of any such guidelines and
__func__ is widely used.

> >
> > [Dhaval]
> > > +     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 +136,10 @@ 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.
> > Could you please keep the comments within 80 character?
> 
> [Dhaval] will do.
> 
> 
> >
> > >    @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
> > > @@ -56,11 +157,7 @@ InvalidateInstructionCacheRange (
> > >    IN UINTN  Length
> > >    )
> > >  {
> > > -  DEBUG (
> > > -    (DEBUG_WARN,
> > > -     "%a:RISC-V unsupported function.\n"
> > > -     "Invalidating the whole instruction cache instead.\n", __func__)
> > > -    );
> > > +  DEBUG ((DEBUG_VERBOSE, "InvalidateInstructionCache:\n"));
> > This change is not required.
> >
> > [Dhaval] Are you saying that the earlier comment was fine as it was?
>
Yes.
 
-Sunil


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

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

[Dhaval Sharma]

[-- Attachment #1: Type: text/plain, Size: 14397 bytes --]

Thanks for the review. My comments inline:

On Fri, Dec 8, 2023 at 9:58 AM Sunil V L <sunilvl@ventanamicro.com> wrote:

> On Thu, Dec 07, 2023 at 10:31:48AM +0530, Dhaval Sharma wrote:
> > Comments inline:
> >
> >
> > On Wed, Dec 6, 2023 at 7:50 PM Sunil V L <sunilvl@ventanamicro.com>
> wrote:
> >
> > > Hi Dhaval,
> > >
> > > Thank you very much for fixing the issue with instruction cache
> > > invalidation and confirming with the spec owner. Few minor comments
> > > below.
> > >
> > > On Mon, Dec 04, 2023 at 01:59:49PM +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>
> > > >
> > > > Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> > > > Acked-by: Laszlo Ersek <lersek@redhat.com>
> > > > ---
> > > >
> > > > Notes:
> > > >     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
> > > >
> > > IMO, it is better to keep the change log in the cover letter. Since not
> > > all patches may be CC'd to every one apart from the cover letter, it is
> > > difficult to understand from the cover letter what has changed in the
> new
> > > series.
> > >
> > [Dhaval] AFAIU notes are tied to specific commits. But it makes sense, I
> > can add an update to the cover letter.
> >
> > >
> > > >  MdePkg/MdePkg.dec                                                  |
> > >  8 +
> > > >  MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |
> > >  5 +
> > > >  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c                |
> > > 173 ++++++++++++++++----
> > > >  MdePkg/MdePkg.uni                                                  |
> > >  4 +
> > > >  4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
> > > >  #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
> > > Can we make this also as a PCD?
> > > [Dhaval] Actually this define should go away once we have CPU HOB. So
> > > thought to keep it simple for now. Anyways there is no plan to change
> this
> > > size
> > >
> >     anytime in the near future.
> >
> What do you mean by no plan to change this size? Isn't it a choice of
> the platform? Given that this macro is in MdePkg, it will force people
> to use the same size.


[Dhaval] My understanding is that this cache block size is fixed for RVA
profile which is where this code will be mostly used. Custom
implementations can change it if they want. Second, we are planning to
introduce a cache block through CPU HOB which when available will replace
this.
So I thought it is okay to defer this implementation and use a simpler
option.


> > > +#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_INFO,
> > > > +     "CacheOpCacheRange:\
> > > Use __func__ ?
> >
> >
> > [Dhaval] Around patch 6 review there was a suggestion to define a
> function
> > name which is more greppable. Hence I had changed it from __func__ to
> this
> > :)
> >
> What was the actual comment? I am not aware of any such guidelines and
> __func__ is widely used.
>
> This was Pedro's comment on V6.
> +  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.
My interpretation of this was removing __func__ and instead having some
relevant text would make it more searchable.
And it kind of did make sense to me. I know many places __func__ is used
but this is just a perspective.

> > >
> > > [Dhaval]
> > > > +     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 +136,10 @@ 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.
> > > Could you please keep the comments within 80 character?
> >
> > [Dhaval] will do.
> >
> >
> > >
> > > >    @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
> > > > @@ -56,11 +157,7 @@ InvalidateInstructionCacheRange (
> > > >    IN UINTN  Length
> > > >    )
> > > >  {
> > > > -  DEBUG (
> > > > -    (DEBUG_WARN,
> > > > -     "%a:RISC-V unsupported function.\n"
> > > > -     "Invalidating the whole instruction cache instead.\n",
> __func__)
> > > > -    );
> > > > +  DEBUG ((DEBUG_VERBOSE, "InvalidateInstructionCache:\n"));
> > > This change is not required.
> > >
> > > [Dhaval] Are you saying that the earlier comment was fine as it was?
> >
> Yes.
>
> The reason I modified the text was- "Supported" in a way gives sense that
this implementation does not support (and some other may support). But from
spec perspective it is not supposed to be supported using CBO. Kind of nit.
I am okay to bring earlier comment back if that is more readable.

> -Sunil
>


-- 
Thanks!
=D


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



[-- Attachment #2: Type: text/html, Size: 19925 bytes --]

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

[Sunil V L]

On Sun, Dec 10, 2023 at 07:51:12PM +0530, Dhaval Sharma wrote:
> Thanks for the review. My comments inline:
> 
> On Fri, Dec 8, 2023 at 9:58 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
> 
> > On Thu, Dec 07, 2023 at 10:31:48AM +0530, Dhaval Sharma wrote:
> > > Comments inline:
> > >
> > >
> > > On Wed, Dec 6, 2023 at 7:50 PM Sunil V L <sunilvl@ventanamicro.com>
> > wrote:
> > >
> > > > Hi Dhaval,
> > > >
> > > > Thank you very much for fixing the issue with instruction cache
> > > > invalidation and confirming with the spec owner. Few minor comments
> > > > below.
> > > >
> > > > On Mon, Dec 04, 2023 at 01:59:49PM +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>
> > > > >
> > > > > Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> > > > > Acked-by: Laszlo Ersek <lersek@redhat.com>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > >     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
> > > > >
> > > > IMO, it is better to keep the change log in the cover letter. Since not
> > > > all patches may be CC'd to every one apart from the cover letter, it is
> > > > difficult to understand from the cover letter what has changed in the
> > new
> > > > series.
> > > >
> > > [Dhaval] AFAIU notes are tied to specific commits. But it makes sense, I
> > > can add an update to the cover letter.
> > >
> > > >
> > > > >  MdePkg/MdePkg.dec                                                  |
> > > >  8 +
> > > > >  MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |
> > > >  5 +
> > > > >  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c                |
> > > > 173 ++++++++++++++++----
> > > > >  MdePkg/MdePkg.uni                                                  |
> > > >  4 +
> > > > >  4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
> > > > >  #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
> > > > Can we make this also as a PCD?
> > > > [Dhaval] Actually this define should go away once we have CPU HOB. So
> > > > thought to keep it simple for now. Anyways there is no plan to change
> > this
> > > > size
> > > >
> > >     anytime in the near future.
> > >
> > What do you mean by no plan to change this size? Isn't it a choice of
> > the platform? Given that this macro is in MdePkg, it will force people
> > to use the same size.
> 
> 
> [Dhaval] My understanding is that this cache block size is fixed for RVA
> profile which is where this code will be mostly used. Custom
> implementations can change it if they want. Second, we are planning to
> introduce a cache block through CPU HOB which when available will replace
> this.
> So I thought it is okay to defer this implementation and use a simpler
> option.
> 
Platforms may not be aligned with a profile. But I am fine if you want
to add the flexibility as part of CPU HOB.

> 
> > > > +#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_INFO,
> > > > > +     "CacheOpCacheRange:\
> > > > Use __func__ ?
> > >
> > >
> > > [Dhaval] Around patch 6 review there was a suggestion to define a
> > function
> > > name which is more greppable. Hence I had changed it from __func__ to
> > this
> > > :)
> > >
> > What was the actual comment? I am not aware of any such guidelines and
> > __func__ is widely used.
> >
> > This was Pedro's comment on V6.
> > +  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.
> My interpretation of this was removing __func__ and instead having some
> relevant text would make it more searchable.
> And it kind of did make sense to me. I know many places __func__ is used
> but this is just a perspective.
>
I think the comment meant to follow a standard logging format since
there was no ":" and a space in original change. I prefer __func__ over
this so that we don't need to update multiple lines in case function
name gets changed.
 
> > > >
> > > > [Dhaval]
> > > > > +     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 +136,10 @@ 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.
> > > > Could you please keep the comments within 80 character?
> > >
> > > [Dhaval] will do.
> > >
> > >
> > > >
> > > > >    @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
> > > > > @@ -56,11 +157,7 @@ InvalidateInstructionCacheRange (
> > > > >    IN UINTN  Length
> > > > >    )
> > > > >  {
> > > > > -  DEBUG (
> > > > > -    (DEBUG_WARN,
> > > > > -     "%a:RISC-V unsupported function.\n"
> > > > > -     "Invalidating the whole instruction cache instead.\n",
> > __func__)
> > > > > -    );
> > > > > +  DEBUG ((DEBUG_VERBOSE, "InvalidateInstructionCache:\n"));
> > > > This change is not required.
> > > >
> > > > [Dhaval] Are you saying that the earlier comment was fine as it was?
> > >
> > Yes.
> >
> > The reason I modified the text was- "Supported" in a way gives sense that
> this implementation does not support (and some other may support). But from
> spec perspective it is not supposed to be supported using CBO. Kind of nit.
> I am okay to bring earlier comment back if that is more readable.
>
I prefer to keep it unchanged. 

Thanks,
Sunil


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

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

[Pedro Falcato]

On Mon, Dec 11, 2023 at 1:12 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> On Sun, Dec 10, 2023 at 07:51:12PM +0530, Dhaval Sharma wrote:
[...]
> > nit: Can we pick a log style here? Like <something>: <log message>
> > In this case, "CacheOpCacheRange: Performing ...". It's just prettier
> > and more greppable.
> > My interpretation of this was removing __func__ and instead having some
> > relevant text would make it more searchable.
> > And it kind of did make sense to me. I know many places __func__ is used
> > but this is just a perspective.
> >
> I think the comment meant to follow a standard logging format since
> there was no ":" and a space in original change. I prefer __func__ over
> this so that we don't need to update multiple lines in case function
> name gets changed.

I definitely meant that __func__ should not be used for this as well.
You can't really search for an error message if you're doing
gratuitous printf formatting for no reason.
Linux even has a policy where user-facing strings (i.e logs) cannot
get broken up, even if you run out of line width.

PS: Dhaval, I gave you a bunch of feedback and you dropped me from
CCs. Please don't do that, I completely lost track of this patch set
:/

-- 
Pedro


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

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

[Sunil V L]

On Mon, Dec 11, 2023 at 03:09:19PM +0000, Pedro Falcato wrote:
> On Mon, Dec 11, 2023 at 1:12 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> >
> > On Sun, Dec 10, 2023 at 07:51:12PM +0530, Dhaval Sharma wrote:
> [...]
> > > nit: Can we pick a log style here? Like <something>: <log message>
> > > In this case, "CacheOpCacheRange: Performing ...". It's just prettier
> > > and more greppable.
> > > My interpretation of this was removing __func__ and instead having some
> > > relevant text would make it more searchable.
> > > And it kind of did make sense to me. I know many places __func__ is used
> > > but this is just a perspective.
> > >
> > I think the comment meant to follow a standard logging format since
> > there was no ":" and a space in original change. I prefer __func__ over
> > this so that we don't need to update multiple lines in case function
> > name gets changed.
> 
> I definitely meant that __func__ should not be used for this as well.
> You can't really search for an error message if you're doing
> gratuitous printf formatting for no reason.
> Linux even has a policy where user-facing strings (i.e logs) cannot
> get broken up, even if you run out of line width.
> 
Thanks Pedro. Do you mean __func__ should not be used at all in any
of logging? Or is there a case where it is allowed vs not allowed? 

Thanks,
Sunil


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

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

[Pedro Falcato]

On Mon, Dec 11, 2023 at 3:20 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> On Mon, Dec 11, 2023 at 03:09:19PM +0000, Pedro Falcato wrote:
> > On Mon, Dec 11, 2023 at 1:12 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> > >
> > > On Sun, Dec 10, 2023 at 07:51:12PM +0530, Dhaval Sharma wrote:
> > [...]
> > > > nit: Can we pick a log style here? Like <something>: <log message>
> > > > In this case, "CacheOpCacheRange: Performing ...". It's just prettier
> > > > and more greppable.
> > > > My interpretation of this was removing __func__ and instead having some
> > > > relevant text would make it more searchable.
> > > > And it kind of did make sense to me. I know many places __func__ is used
> > > > but this is just a perspective.
> > > >
> > > I think the comment meant to follow a standard logging format since
> > > there was no ":" and a space in original change. I prefer __func__ over
> > > this so that we don't need to update multiple lines in case function
> > > name gets changed.
> >
> > I definitely meant that __func__ should not be used for this as well.
> > You can't really search for an error message if you're doing
> > gratuitous printf formatting for no reason.
> > Linux even has a policy where user-facing strings (i.e logs) cannot
> > get broken up, even if you run out of line width.
> >
> Thanks Pedro. Do you mean __func__ should not be used at all in any
> of logging? Or is there a case where it is allowed vs not allowed?

My point is that we should aid people trying to do
    git grep <error message>

FWIW, Linux itself uses __func__ a bunch for logs. But I personally
dislike it, for the reasons stated above.

-- 
Pedro


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

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

[Pedro Falcato]

On Mon, Dec 4, 2023 at 8:30 AM 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>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
>     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                | 173 ++++++++++++++++----
>  MdePkg/MdePkg.uni                                                  |   4 +
>  4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
>  #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_INFO,
> +     "CacheOpCacheRange:\
> +     Performing Cache Management Operation %d \n", Op)
> +    );

Two comments:

1) You probably want DEBUG_VERBOSE here, if anything. Cache operations
may be too common/boring for an INFO log.
2) Your string splitting looks waaaaaaacky. That's not how you split
strings. See the differences: https://godbolt.org/z/64WjT9oKo
(If what you're doing actually results in normal-ish looking strings,
it may be due to a weird CPP corner case I'm not aware of. Not my
fault :P)

> +
> +  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 +136,10 @@ 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
> @@ -56,11 +157,7 @@ InvalidateInstructionCacheRange (
>    IN UINTN  Length
>    )
>  {
> -  DEBUG (
> -    (DEBUG_WARN,
> -     "%a:RISC-V unsupported function.\n"
> -     "Invalidating the whole instruction cache instead.\n", __func__)
> -    );
> +  DEBUG ((DEBUG_VERBOSE, "InvalidateInstructionCache:\n"));
>    InvalidateInstructionCache ();
>    return Address;
>  }
> @@ -81,7 +178,8 @@ WriteBackInvalidateDataCache (
>    VOID
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  DEBUG ((DEBUG_ERROR, "WriteBackInvalidateDataCache:\
> +  RISC-V unsupported function.\n"));

ASSERT(FALSE) here as well?

> @@ -230,6 +332,17 @@ InvalidateDataCacheRange (
>    IN      UINTN  Length
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  if (RiscVIsCMOEnabled ()) {
> +    CacheOpCacheRange (Address, Length, CacheOpInvld);
> +  } else {
> +    DEBUG (
> +      (DEBUG_VERBOSE,
> +       "InvalidateDataCacheRange:\
> +       Zicbom not supported.\n" \
> +       "Invalidating the whole Data cache instead.\n")
> +      );

I don't understand why the strings are getting so weirdly split (using
the \ and in such short chunks). Is this uncrustify?
This also applies to the above DEBUG()'s.

-- 
Pedro


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