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

[edk2-devel] [PATCH v7 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.

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                | 172 ++++++++++++++++----
 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, 269 insertions(+), 54 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 (#110262): https://edk2.groups.io/g/devel/message/110262
Mute This Topic: https://groups.io/mt/102256459/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v7 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 (#110263): https://edk2.groups.io/g/devel/message/110263
Mute This Topic: https://groups.io/mt/102256462/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v7 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:
    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..d4b56a9601da 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -212,7 +212,7 @@ RiscVClearPendingTimerInterrupt (
 **/
 VOID
 EFIAPI
-RiscVInvalidateInstCacheAsm (
+RiscVInvalidateInstCacheAsmFence (
   VOID
   );
 
@@ -222,7 +222,7 @@ RiscVInvalidateInstCacheAsm (
 **/
 VOID
 EFIAPI
-RiscVInvalidateDataCacheAsm (
+RiscVInvalidateDataCacheAsmFence (
   VOID
   );
 
diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
index d5efcf49a4bf..4eb18edb9aa7 100644
--- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
@@ -21,7 +21,7 @@ InvalidateInstructionCache (
   VOID
   )
 {
-  RiscVInvalidateInstCacheAsm ();
+  RiscVInvalidateInstCacheAsmFence ();
 }
 
 /**
@@ -193,7 +193,7 @@ InvalidateDataCache (
   VOID
   )
 {
-  RiscVInvalidateDataCacheAsm ();
+  RiscVInvalidateDataCacheAsmFence ();
 }
 
 /**
diff --git a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S b/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
index 7c10fdd268af..e0eea0b5fb25 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(RiscVInvalidateInstCacheAsmFence)
+ASM_GLOBAL ASM_PFX(RiscVInvalidateDataCacheAsmFence)
 
-ASM_PFX(RiscVInvalidateInstCacheAsm):
+ASM_PFX(RiscVInvalidateInstCacheAsmFence):
     fence.i
     ret
 
-ASM_PFX(RiscVInvalidateDataCacheAsm):
+ASM_PFX(RiscVInvalidateDataCacheAsmFence):
     fence
     ret
-- 
2.39.2



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

[edk2-devel] [PATCH v7 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. Not able to verify actual instruction in HW as Qemu ignores
   any actual cache operations.

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:
    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 d4b56a9601da..c42cc165dc82 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -226,6 +226,39 @@ RiscVInvalidateDataCacheAsmFence (
   VOID
   );
 
+/**
+  RISC-V flush cache block. Atomically perform a clean operation
+  followed by an invalidate operation
+
+**/
+VOID
+EFIAPI
+RiscVCpuCacheFlushAsmCmo (
+  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
+RiscVCpuCacheCleanAsmCmo (
+  IN UINTN
+  );
+
+/**
+Deallocate the copy of the cache block
+
+**/
+VOID
+EFIAPI
+RiscVCpuCacheInvalAsmCmo (
+  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 e0eea0b5fb25..3c7be3229e3b 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(RiscVInvalidateInstCacheAsmFence)
@@ -19,3 +21,18 @@ ASM_PFX(RiscVInvalidateInstCacheAsmFence):
 ASM_PFX(RiscVInvalidateDataCacheAsmFence):
     fence
     ret
+
+ASM_GLOBAL ASM_PFX (RiscVCpuCacheFlushAsmCmo)
+ASM_PFX (RiscVCpuCacheFlushAsmCmo):
+    RISCVCMOFLUSH
+    ret
+
+ASM_GLOBAL ASM_PFX (RiscVCpuCacheCleanAsmCmo)
+ASM_PFX (RiscVCpuCacheCleanAsmCmo):
+    RISCVCMOCLEAN
+    ret
+
+ASM_GLOBAL ASM_PFX (RiscVCpuCacheInvalAsmCmo)
+ASM_PFX (RiscVCpuCacheInvalAsmCmo):
+    RISCVCMOINVALIDATE
+    ret
-- 
2.39.2



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

[edk2-devel] [PATCH v7 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:
    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                | 168 +++++++++++++++++---
 MdePkg/MdePkg.uni                                                  |   4 +
 4 files changed, 165 insertions(+), 20 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 4eb18edb9aa7..5b3104afb67e 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,115 @@
 #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.
+//
+#define RISCV_CACHE_BLOCK_SIZE         64
+#define RISCV_CPU_FEATURE_CMO_BITMASK  0x1
+
+typedef enum {
+  Clean,
+  Flush,
+  Invld,
+} CACHE_OP;
+
+/**
+Verify CBOs are supported by this HW
+TODO: Use RISC-V CPU HOB once available.
+
+**/
+STATIC
+BOOLEAN
+RiscVIsCMOEnabled (
+  VOID
+  )
+{
+  // If CMO is disabled in HW, skip Override check
+  // Otherwise this PCD can override settings
+  return ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_CMO_BITMASK) != 0);
+}
+
+/**
+  Performs required opeartion on cache lines in the cache coherency domain
+  of the calling CPU. If Address is not aligned on a cache line boundary,
+  then entire cache line containing Address is operated. If Address + Length
+  is not aligned on a cache line boundary, then the entire cache line
+  containing Address + Length -1 is operated.
+  If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
+  @param  Address The base address of the cache lines to
+          invalidate.
+  @param  Length  The number of bytes to invalidate from the instruction
+          cache.
+  @param  Op  Type of CMO operation to be performed
+  @return Address.
+
+**/
+STATIC
+VOID
+CacheOpCacheRange (
+  IN VOID      *Address,
+  IN UINTN     Length,
+  IN CACHE_OP  Op
+  )
+{
+  UINTN  CacheLineSize;
+  UINTN  Start;
+  UINTN  End;
+
+  if (Length == 0) {
+    return;
+  }
+
+  if ((Op != Invld) && (Op != Flush) && (Op != Clean)) {
+    return;
+  }
+
+  ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)Address));
+
+  CacheLineSize = RISCV_CACHE_BLOCK_SIZE;
+
+  Start = (UINTN)Address;
+  //
+  // Calculate the cache line alignment
+  //
+  End    = (Start + Length + (CacheLineSize - 1)) & ~(CacheLineSize - 1);
+  Start &= ~((UINTN)CacheLineSize - 1);
+
+  DEBUG (
+    (DEBUG_INFO,
+     "CacheOpCacheRange:\
+     Performing Cache Management Operation %d \n", Op)
+    );
+
+  do {
+    switch (Op) {
+      case Invld:
+        RiscVCpuCacheInvalAsmCmo (Start);
+        break;
+      case Flush:
+        RiscVCpuCacheFlushAsmCmo (Start);
+        break;
+      case Clean:
+        RiscVCpuCacheCleanAsmCmo (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
@@ -56,12 +162,18 @@ InvalidateInstructionCacheRange (
   IN UINTN  Length
   )
 {
-  DEBUG (
-    (DEBUG_WARN,
-     "%a:RISC-V unsupported function.\n"
-     "Invalidating the whole instruction cache instead.\n", __func__)
-    );
-  InvalidateInstructionCache ();
+  if (RiscVIsCMOEnabled ()) {
+    CacheOpCacheRange (Address, Length, Invld);
+  } else {
+    DEBUG (
+      (DEBUG_VERBOSE,
+       "InvalidateInstructionCacheRange:\
+        Zicbom not supported.\n" \
+       "Invalidating the whole instruction cache instead.\n")
+      );
+    InvalidateInstructionCache ();
+  }
+
   return Address;
 }
 
@@ -81,7 +193,8 @@ WriteBackInvalidateDataCache (
   VOID
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  DEBUG ((DEBUG_ERROR, "WriteBackInvalidateDataCache:\
+  RISC-V unsupported function.\n"));
 }
 
 /**
@@ -117,7 +230,12 @@ WriteBackInvalidateDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  if (RiscVIsCMOEnabled ()) {
+    CacheOpCacheRange (Address, Length, Flush);
+  } else {
+    ASSERT (FALSE);
+  }
+
   return Address;
 }
 
@@ -137,7 +255,7 @@ WriteBackDataCache (
   VOID
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  ASSERT (FALSE);
 }
 
 /**
@@ -156,10 +274,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 +287,12 @@ WriteBackDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  if (RiscVIsCMOEnabled ()) {
+    CacheOpCacheRange (Address, Length, Clean);
+  } else {
+    ASSERT (FALSE);
+  }
+
   return Address;
 }
 
@@ -214,10 +334,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 +347,17 @@ InvalidateDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  if (RiscVIsCMOEnabled ()) {
+    CacheOpCacheRange (Address, Length, Invld);
+  } 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 (#110266): https://edk2.groups.io/g/devel/message/110266
Mute This Topic: https://groups.io/mt/102256468/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v7 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>
---

Notes:
    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 (#110267): https://edk2.groups.io/g/devel/message/110267
Mute This Topic: https://groups.io/mt/102256471/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Pedro Falcato]

On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
>
> Use newly defined cache management operations for RISC-V where possible
> It builds up on the support added for RISC-V cache management
> instructions in BaseLib.
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
>
> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
>     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                | 168 +++++++++++++++++---
>  MdePkg/MdePkg.uni                                                  |   4 +
>  4 files changed, 165 insertions(+), 20 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 4eb18edb9aa7..5b3104afb67e 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,115 @@
>  #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.
> +//
> +#define RISCV_CACHE_BLOCK_SIZE         64
> +#define RISCV_CPU_FEATURE_CMO_BITMASK  0x1
> +
> +typedef enum {
> +  Clean,
> +  Flush,
> +  Invld,
> +} CACHE_OP;

small nit: You may want to do something like CACHE_OP_{CLEAN, FLUSH,
INVID}. but since this is a file-local enum I don't consider this
merge-blocking.

-- 
Pedro


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

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

[Pedro Falcato]

On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
>
> 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

nit: reflecting

> 2. Not able to verify actual instruction in HW as Qemu ignores
>    any actual cache operations.

Do you have no way to test this in hardware? Since Rivos is a RISCV
vendor and all ;)
I don't like inviting the idea of merging CPU architectural changes
without actually testing them in something resembling real silicon
(i.e QEMU KVM is _fine_, QEMU TCG really isn't).

-- 
Pedro


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

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

[Pedro Falcato]

On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
>
> 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.
>

With or without nits fixed:

Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

But I would *really* prefer it if you could test this on a real board
with this extension.

-- 
Pedro


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

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

[Laszlo Ersek]

On 10/29/23 20:12, Pedro Falcato wrote:
> On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
>>
>> 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
> 
> nit: reflecting
> 
>> 2. Not able to verify actual instruction in HW as Qemu ignores
>>    any actual cache operations.
> 
> Do you have no way to test this in hardware? Since Rivos is a RISCV
> vendor and all ;)
> I don't like inviting the idea of merging CPU architectural changes
> without actually testing them in something resembling real silicon
> (i.e QEMU KVM is _fine_, QEMU TCG really isn't).
> 

Hopefully I'm not drawing an incorrect parallel here, but, as I recall
arm64 enablement in 2014, nearly all initial enablement in RHEL occurred
on software emulators (ARM Foundation Model, ARM FVP, then QEMU TCG).
You need to start somewhere. In particular, qemu-system-aarch64 was a
huge step forward (performance-wise) once it *existed*, relative to the
Foundation Model / FVP, even though qemu-system-aarch64 wouldn't emulate
CPU caches (IIRC).

Laszlo



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

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

[Laszlo Ersek]

On 10/29/23 20:07, Pedro Falcato wrote:
> On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
>>
>> Use newly defined cache management operations for RISC-V where possible
>> It builds up on the support added for RISC-V cache management
>> instructions in BaseLib.
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>>
>> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     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                | 168 +++++++++++++++++---
>>  MdePkg/MdePkg.uni                                                  |   4 +
>>  4 files changed, 165 insertions(+), 20 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 4eb18edb9aa7..5b3104afb67e 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,115 @@
>>  #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.
>> +//
>> +#define RISCV_CACHE_BLOCK_SIZE         64
>> +#define RISCV_CPU_FEATURE_CMO_BITMASK  0x1
>> +
>> +typedef enum {
>> +  Clean,
>> +  Flush,
>> +  Invld,
>> +} CACHE_OP;
> 
> small nit: You may want to do something like CACHE_OP_{CLEAN, FLUSH,
> INVID}. but since this is a file-local enum I don't consider this
> merge-blocking.
> 

Agree with you on the prefix, but not on the uppercase spelling; edk2
(and UEFI) uses CamelCase enumeration constants. Compare: at the very
top of "MdePkg/Include/Uefi/UefiSpec.h", we have EFI_ALLOCATE_TYPE with
constants like AllocateAnyPages.

Laszlo



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

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

[Sunil V L]

On Sun, Oct 29, 2023 at 08:16:11PM +0530, Dhaval wrote:
> 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. Not able to verify actual instruction in HW as Qemu ignores
>    any actual cache operations.
> 
> 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:
>     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 d4b56a9601da..c42cc165dc82 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -226,6 +226,39 @@ RiscVInvalidateDataCacheAsmFence (
>    VOID
>    );
>  
> +/**
> +  RISC-V flush cache block. Atomically perform a clean operation
> +  followed by an invalidate operation
> +
> +**/
> +VOID
> +EFIAPI
> +RiscVCpuCacheFlushAsmCmo (

NIT: I would keep Asm at the end for these interface names.

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

Thanks,
Sunil


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

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

[Sunil V L]

On Sun, Oct 29, 2023 at 08:16:12PM +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:
>     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                | 168 +++++++++++++++++---
>  MdePkg/MdePkg.uni                                                  |   4 +
>  4 files changed, 165 insertions(+), 20 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.
NIT: I am wondering whether PcdRiscVCpuFeatureDisable is better so that
it is explicit.

> +  #
> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UINT64|0x69
> +
Instead of this, can default value match only those features which are
enabled by default for qemu virt machine? That way, I think we can avoid
having this PCD defined again in RiscVVirt.

>  [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 4eb18edb9aa7..5b3104afb67e 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,115 @@
>  #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.
> +//
> +#define RISCV_CACHE_BLOCK_SIZE         64
> +#define RISCV_CPU_FEATURE_CMO_BITMASK  0x1
> +
Can we define these bits in the header file so that the definitions can
be used by multiple modules?

Thanks,
Sunil


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

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

[Sunil V L]

On Mon, Oct 30, 2023 at 04:48:18PM +0530, Sunil V L wrote:
> On Sun, Oct 29, 2023 at 08:16:12PM +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:
> >     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                | 168 +++++++++++++++++---
> >  MdePkg/MdePkg.uni                                                  |   4 +
> >  4 files changed, 165 insertions(+), 20 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.
> NIT: I am wondering whether PcdRiscVCpuFeatureDisable is better so that
> it is explicit.
> 
> > +  #
> > +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UINT64|0x69
> > +
> Instead of this, can default value match only those features which are
> enabled by default for qemu virt machine? That way, I think we can avoid
> having this PCD defined again in RiscVVirt.
> 
Sorry, I take back. This is common for all platforms. So, we can't take
qemu as reference.

Thanks,
Sunil


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

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

[Sunil V L]

On Mon, Oct 30, 2023 at 10:38:21AM +0100, Laszlo Ersek wrote:
> On 10/29/23 20:12, Pedro Falcato wrote:
> > On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
> >>
> >> 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
> > 
> > nit: reflecting
> > 
> >> 2. Not able to verify actual instruction in HW as Qemu ignores
> >>    any actual cache operations.
> > 
> > Do you have no way to test this in hardware? Since Rivos is a RISCV
> > vendor and all ;)
> > I don't like inviting the idea of merging CPU architectural changes
> > without actually testing them in something resembling real silicon
> > (i.e QEMU KVM is _fine_, QEMU TCG really isn't).
> > 
> 
> Hopefully I'm not drawing an incorrect parallel here, but, as I recall
> arm64 enablement in 2014, nearly all initial enablement in RHEL occurred
> on software emulators (ARM Foundation Model, ARM FVP, then QEMU TCG).
> You need to start somewhere. In particular, qemu-system-aarch64 was a
> huge step forward (performance-wise) once it *existed*, relative to the
> Foundation Model / FVP, even though qemu-system-aarch64 wouldn't emulate
> CPU caches (IIRC).
> 
I agree. As per my knowledge, we don't have a publicly available silicon
implementing these features as of today. So, we are taking the approach
of how linux merged these features when the code adhered to the spec. It
will be great for downstream to get these patches merged.

Thanks,
Sunil


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

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

[Pedro Falcato]

On Mon, Oct 30, 2023 at 9:38 AM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 10/29/23 20:12, Pedro Falcato wrote:
> > On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
> >>
> >> 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
> >
> > nit: reflecting
> >
> >> 2. Not able to verify actual instruction in HW as Qemu ignores
> >>    any actual cache operations.
> >
> > Do you have no way to test this in hardware? Since Rivos is a RISCV
> > vendor and all ;)
> > I don't like inviting the idea of merging CPU architectural changes
> > without actually testing them in something resembling real silicon
> > (i.e QEMU KVM is _fine_, QEMU TCG really isn't).
> >
>
> Hopefully I'm not drawing an incorrect parallel here, but, as I recall
> arm64 enablement in 2014, nearly all initial enablement in RHEL occurred
> on software emulators (ARM Foundation Model, ARM FVP, then QEMU TCG).
> You need to start somewhere. In particular, qemu-system-aarch64 was a
> huge step forward (performance-wise) once it *existed*, relative to the
> Foundation Model / FVP, even though qemu-system-aarch64 wouldn't emulate
> CPU caches (IIRC).

Right. I don't know how faithful those early ARM simulators were, but
QEMU TCG is not very faithful and uarch details *can* slip through the
cracks.
In arm64 it's easy to miss a dsb or a isb if you're not extra careful
(or read the ARM ARM wrong).

RISCV has a bunch of fun gotchas too. For instance, did you know you
need to flush the TLB using sfence.vma even when only mapping a page?
This "small" detail results in boot failures on real hardware (such as
the visionfive 2), but is completely silent in QEMU TCG.

So this is why I would much prefer a test on real silicon. It's hard
to prove correctness when all you have is QEMU's spotty simulation
(rightfully so, it's not a simulator).

--
Pedro


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

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

[Andrei Warkentin]

Looks fine to me. While it's not necessary to add a default value PCD to every platform DSC, it might me a good idea to do so for "example" platforms like RiscVVirt, so that folks who borrow code are aware of the feature?

Reviewed-by: Andrei Warkentin <andrei.warkentin@intel.com> 

> -----Original Message-----
> From: Dhaval <dhaval@rivosinc.com>
> Sent: Sunday, October 29, 2023 9:46 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Yao, Jiewen
> <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Sunil V L <sunilvl@ventanamicro.com>;
> Warkentin, Andrei <andrei.warkentin@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: [PATCH v7 5/5] OvmfPkg/RiscVVirt: Override for RV CPU Features
> 
> 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>
> ---
> 
> Notes:
>     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 (#110362): https://edk2.groups.io/g/devel/message/110362
Mute This Topic: https://groups.io/mt/102256471/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Andrei Warkentin]

Hi Dhaval,

Do you mind sharing the repo with the full patch set? Like a github link?

A

> -----Original Message-----
> From: Dhaval <dhaval@rivosinc.com>
> Sent: Sunday, October 29, 2023 9:46 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Yao, Jiewen
> <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Sunil V L <sunilvl@ventanamicro.com>;
> Warkentin, Andrei <andrei.warkentin@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> <zhiguang.liu@intel.com>; Daniel Schaefer <git@danielschaefer.me>
> Subject: [PATCH v7 0/5] Cache Management Operations Support For RISC-V
> 
> 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.
> 
> 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                | 172
> ++++++++++++++++----
>  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, 269 insertions(+), 54 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 (#110363): https://edk2.groups.io/g/devel/message/110363
Mute This Topic: https://groups.io/mt/102256459/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Dhaval Sharma]

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

Here we go. https://github.com/tianocore/edk2/pull/4974


On Tue, Oct 31, 2023 at 9:46 AM Warkentin, Andrei <
andrei.warkentin@intel.com> wrote:

> Hi Dhaval,
>
> Do you mind sharing the repo with the full patch set? Like a github link?
>
> A
>
> > -----Original Message-----
> > From: Dhaval <dhaval@rivosinc.com>
> > Sent: Sunday, October 29, 2023 9:46 AM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Gerd
> > Hoffmann <kraxel@redhat.com>; Sunil V L <sunilvl@ventanamicro.com>;
> > Warkentin, Andrei <andrei.warkentin@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> > Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> > <zhiguang.liu@intel.com>; Daniel Schaefer <git@danielschaefer.me>
> > Subject: [PATCH v7 0/5] Cache Management Operations Support For RISC-V
> >
> > 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.
> >
> > 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                | 172
> > ++++++++++++++++----
> >  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, 269 insertions(+), 54 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
>
>

-- 
Thanks!
=D


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110364): https://edk2.groups.io/g/devel/message/110364
Mute This Topic: https://groups.io/mt/102256459/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: 7329 bytes --]

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

[Dhaval Sharma]

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

Thanks. This PCD is for Virt platform only. Or maybe I am missing the point.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110369): https://edk2.groups.io/g/devel/message/110369
Mute This Topic: https://groups.io/mt/102256471/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: 856 bytes --]

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

[Dhaval Sharma]

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

NIT: I am wondering whether PcdRiscVCpuFeatureDisable is better so that
it is explicit.
[Dhaval] Well setting it to 1 would mean feature is enabled. Do it would be confusing to see PcdRiscVCpuFeatureDisable == 1 means feature is enabled.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110370): https://edk2.groups.io/g/devel/message/110370
Mute This Topic: https://groups.io/mt/102256468/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: 1231 bytes --]

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

[Dhaval Sharma]

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

Can we define these bits in the header file so that the definitions can
be used by multiple modules?
[Dhaval] I could put it un Baselib.h (MDE_CPU_RISCV64) but sounds like right now BaseLib.h is free of such #defines. If you think it is still better would do it. I do not have any preference.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110372): https://edk2.groups.io/g/devel/message/110372
Mute This Topic: https://groups.io/mt/102256468/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: 1228 bytes --]

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

[Jingyu Li via groups.io]

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

> 
> 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. Not able to verify actual instruction in HW as Qemu ignores
> any actual cache operations.
> 
> Cc: Michael D Kinney <michael.d.kinney@...>
> Cc: Liming Gao <gaoliming@...>
> Cc: Zhiguang Liu <zhiguang.liu@...>
> Cc: Sunil V L <sunilvl@...>
> Cc: Daniel Schaefer <git@...>
> Cc: Laszlo Ersek <lersek@...>
> 
> Signed-off-by: Dhaval Sharma <dhaval@...>
> Reviewed-by: Laszlo Ersek <lersek@...>
> ---
> 

I verified this CMO framework on an actual HW platform.

SW:
edk2: https://github.com/rivosinc/edk2/tree/dev-rv-cmo-v7 branch: dev-rv-cmo-v7
edk2-platforms: https://github.com/sophgo/edk2-platforms branch: sg2042-dev

HW:
Milk-V Pioneer Box, a developer motherboard based on SG2042 with 64-Core T-HEAD C920.

Attention:
The T-HEAD C920 implemented its own CMO Extension and is different from the standard CMO Extension.

Test steps:
1. Modified the opcodes in RiscVasm.inc to accommodate the C920 CMO feature.
diff --git a/MdePkg/Include/RiscV64/RiscVasm.inc b/MdePkg/Include/RiscV64/RiscVasm.inc
index 29de735885..5df85fdb31 100644
--- a/MdePkg/Include/RiscV64/RiscVasm.inc
+++ b/MdePkg/Include/RiscV64/RiscVasm.inc
@@ -7,13 +7,13 @@
*/

.macro RISCVCMOFLUSH
-    .word 0x25200f
+    .long 0x0275000b^M
.endm

.macro RISCVCMOINVALIDATE
-    .word 0x05200f
+    .long 0x0265000b^M
.endm

.macro RISCVCMOCLEAN
-    .word 0x15200f
+    .long 0x0275000b^M
.endm

2. We enable the CMO during the PCIe devices with DMA access to the memory, just focus on the implementation of CpuFlushCpuDataCache based on the EFI_CPU_ARCH_PROTOCOL. Except for PCIe, in other words, except for the cpu->FlushDataCache, we do not use CMO. And the PCIe inbound only relates to datacache.clean and datacache.invalidate.
diff --git a/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c b/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c
index 2af3b62234..cf50bc5f92 100644
--- a/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c
@@ -9,6 +9,8 @@
**/

#include "CpuDxe.h"
+#include <Library/CacheMaintenanceLib.h>^M
+#include <Library/PcdLib.h>^M

//
// Global Variables
@@ -59,7 +61,7 @@ EFI_CPU_ARCH_PROTOCOL  gCpu = {
CpuGetTimerValue,
CpuSetMemoryAttributes,
1,                          // NumberOfTimers
-  4                           // DmaBufferAlignment
+  64                           // DmaBufferAlignment^M
};

//
@@ -90,6 +92,21 @@ CpuFlushCpuDataCache (
IN EFI_CPU_FLUSH_TYPE     FlushType
)
{
+  PatchPcdSet64 (PcdRiscVFeatureOverride, 0x1);^M
+  switch (FlushType) {^M
+    case EfiCpuFlushTypeWriteBack:^M
+      WriteBackDataCacheRange ((VOID *)(UINTN)Start, (UINTN)Length);^M
+      break;^M
+    case EfiCpuFlushTypeInvalidate:^M
+      InvalidateInstructionCacheRange ((VOID *)(UINTN)Start, (UINTN)Length);^M
+      break;^M
+    case EfiCpuFlushTypeWriteBackInvalidate:^M
+      WriteBackInvalidateDataCacheRange ((VOID *)(UINTN)Start, (UINTN)Length);^M
+      break;^M
+    default:^M
+      return EFI_INVALID_PARAMETER;^M
+  }^M
+^M
return EFI_SUCCESS;
}

diff --git a/Platform/Sophgo/SG2042_EVB_Board/SG2042.dsc b/Platform/Sophgo/SG2042_EVB_Board/SG2042.dsc
index 51ff89678c..e2e44ad619 100644
--- a/Platform/Sophgo/SG2042_EVB_Board/SG2042.dsc
+++ b/Platform/Sophgo/SG2042_EVB_Board/SG2042.dsc
@@ -389,6 +389,7 @@

[PcdsPatchableInModule]
gSophgoSG2042PlatformPkgTokenSpaceGuid.PcdSG2042PhyAddrToVirAddr|0
+  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0

################################################################################
#
@@ -500,7 +501,7 @@
# RISC-V Core module
#
UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
-  Silicon/Sophgo/SG2042Pkg/Override/UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf
+  UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf
MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf

MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf

diff --git a/Platform/Sophgo/SG2042_EVB_Board/SG2042.fdf b/Platform/Sophgo/SG2042_EVB_Board/SG2042.fdf
index 844fc3eac0..9cbb1d3f65 100644
--- a/Platform/Sophgo/SG2042_EVB_Board/SG2042.fdf
+++ b/Platform/Sophgo/SG2042_EVB_Board/SG2042.fdf
@@ -77,7 +77,7 @@ INF  Silicon/Sophgo/SG2042Pkg/Drivers/SdHostDxe/SdHostDxe.inf

# RISC-V Core Drivers
INF  UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
-INF  Silicon/Sophgo/SG2042Pkg/Override/UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf
+INF  UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf

INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf

3. Now the PCIe devices are in work order on PioneerBox. The CMO instructions are executed as expected.
Reviewed-by: Jingyu Li <jingyu.li01@sophgo.com>

Thanks,
Jingyu


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110428): https://edk2.groups.io/g/devel/message/110428
Mute This Topic: https://groups.io/mt/102256466/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: 11245 bytes --]

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

[Sunil V L]

On Mon, Oct 30, 2023 at 11:24:15PM -0700, Dhaval Sharma wrote:
> Can we define these bits in the header file so that the definitions can
> be used by multiple modules?
> [Dhaval] I could put it un Baselib.h (MDE_CPU_RISCV64) but sounds like right now BaseLib.h is free of such #defines. If you think it is still better would do it. I do not have any preference.
Right, fine then.

Thanks,
Sunil


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

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

[Dhaval Sharma]

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

I am posting an update on behalf of Jingyu as he had trouble with posting.
CC'ing him here:
In summary what we have verified so far:

   1. I have verified that instructions/op codes are okay. I have also
   verified on Qemu that functionally it seems to be calling correct
   instructions. Ensured with negative test cases that any other op codes do
   cause exceptions as expected.
   2. Jingyu was able to verify the CpuFlushCpuDataCache function with this
   framework (he had to use custom op code based on his soc implementation) on
   SG2042. There is one issue that he is debugging now which is related to
   other cache instructions and he will get back with more data. P.S. SG2042
   does not implement the exact same CMO opcodes but equivalent ones. So this
   experiment is just an additional data point that helps verify the framework
   and not CMO itself.
   3. In general it sounds like framework flows are alright and as long as
   instructions do their job as claimed in the spec, it is lower risk.

Guess this is what we have so far. If it makes sense to everyone, we could
go ahead with merging with this *feature disabled by default* after Jingyu
provides clarity reg failures on SG2042 platform. Otherwise we can wait
until newer Si is available where these exact instructions can be tested
and then upstreamed.

[From Jingyu]
I verified this CMO framework on an actual HW platform.

SW:
edk2: https://github.com/rivosinc/edk2/tree/dev-rv-cmo-v7 branch:
dev-rv-cmo-v7
edk2-platforms: https://github.com/sophgo/edk2-platforms  branch: sg2042-dev

HW:
Milk-V Pioneer Box, a developer motherboard based on SG2042 with 64-Core
T-HEAD C920.

Attention:
The T-HEAD C920 implemented its own CMO Extension and is different from the
standard CMO Extension.

Test steps:
1. Modified the opcodes in RiscVasm.inc to accommodate the C920 CMO feature.
diff --git a/MdePkg/Include/RiscV64/RiscVasm.inc
b/MdePkg/Include/RiscV64/RiscVasm.inc
index 29de735885..5df85fdb31 100644
--- a/MdePkg/Include/RiscV64/RiscVasm.inc
+++ b/MdePkg/Include/RiscV64/RiscVasm.inc
@@ -7,13 +7,13 @@
  */

 .macro RISCVCMOFLUSH
-    .word 0x25200f
+    .long 0x0275000b^M
 .endm

 .macro RISCVCMOINVALIDATE
-    .word 0x05200f
+    .long 0x0265000b^M
 .endm

 .macro RISCVCMOCLEAN
-    .word 0x15200f
+    .long 0x0275000b^M
 .endm

2. We enable the CMO during the PCIe devices with DMA access to the memory,
just focus on the implementation of CpuFlushCpuDataCache based on the
EFI_CPU_ARCH_PROTOCOL. Except for PCIe, in other words, except for the
cpu->FlushDataCache, we do not use CMO. And the PCIe inbound only relates
to datacache.clean and datacache.invalidate.
diff --git a/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c
b/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c
index 2af3b62234..cf50bc5f92 100644
--- a/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c
@@ -9,6 +9,8 @@
 **/

 #include "CpuDxe.h"
+#include <Library/CacheMaintenanceLib.h>^M
+#include <Library/PcdLib.h>^M

 //
 // Global Variables
@@ -59,7 +61,7 @@ EFI_CPU_ARCH_PROTOCOL  gCpu = {
   CpuGetTimerValue,
   CpuSetMemoryAttributes,
   1,                          // NumberOfTimers
-  4                           // DmaBufferAlignment
+  64                           // DmaBufferAlignment^M
 };

 //
@@ -90,6 +92,21 @@ CpuFlushCpuDataCache (
   IN EFI_CPU_FLUSH_TYPE     FlushType
   )
 {
+  PatchPcdSet64 (PcdRiscVFeatureOverride, 0x1);^M
+  switch (FlushType) {^M
+    case EfiCpuFlushTypeWriteBack:^M
+      WriteBackDataCacheRange ((VOID *)(UINTN)Start, (UINTN)Length);^M
+      break;^M
+    case EfiCpuFlushTypeInvalidate:^M
+      InvalidateInstructionCacheRange ((VOID *)(UINTN)Start,
(UINTN)Length);^M
+      break;^M
+    case EfiCpuFlushTypeWriteBackInvalidate:^M
+      WriteBackInvalidateDataCacheRange ((VOID *)(UINTN)Start,
(UINTN)Length);^M
+      break;^M
+    default:^M
+      return EFI_INVALID_PARAMETER;^M
+  }^M
+^M
   return EFI_SUCCESS;
 }

diff --git a/Platform/Sophgo/SG2042_EVB_Board/SG2042.dsc
b/Platform/Sophgo/SG2042_EVB_Board/SG2042.dsc
index 51ff89678c..e2e44ad619 100644
--- a/Platform/Sophgo/SG2042_EVB_Board/SG2042.dsc
+++ b/Platform/Sophgo/SG2042_EVB_Board/SG2042.dsc
@@ -389,6 +389,7 @@

 [PcdsPatchableInModule]
   gSophgoSG2042PlatformPkgTokenSpaceGuid.PcdSG2042PhyAddrToVirAddr|0
+  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0

 ################################################################################
 #
@@ -500,7 +501,7 @@
   # RISC-V Core module
   #
   UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
-
Silicon/Sophgo/SG2042Pkg/Override/UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf
+  UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf
   MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf

   MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf

diff --git a/Platform/Sophgo/SG2042_EVB_Board/SG2042.fdf
b/Platform/Sophgo/SG2042_EVB_Board/SG2042.fdf
index 844fc3eac0..9cbb1d3f65 100644
--- a/Platform/Sophgo/SG2042_EVB_Board/SG2042.fdf
+++ b/Platform/Sophgo/SG2042_EVB_Board/SG2042.fdf
@@ -77,7 +77,7 @@ INF
Silicon/Sophgo/SG2042Pkg/Drivers/SdHostDxe/SdHostDxe.inf

 # RISC-V Core Drivers
 INF  UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
-INF
Silicon/Sophgo/SG2042Pkg/Override/UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf
+INF  UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf

 INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
 INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf

3. Now the PCIe devices are in work order on PioneerBox. The CMO
instructions are executed as expected.

Reviewed-by: Jingyu Li <jingyu.li01@sophgo.com>

On Mon, Oct 30, 2023 at 10:07 PM Pedro Falcato <pedro.falcato@gmail.com>
wrote:

> On Mon, Oct 30, 2023 at 9:38 AM Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > On 10/29/23 20:12, Pedro Falcato wrote:
> > > On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma <dhaval@rivosinc.com>
> wrote:
> > >>
> > >> 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
> > >
> > > nit: reflecting
> > >
> > >> 2. Not able to verify actual instruction in HW as Qemu ignores
> > >>    any actual cache operations.
> > >
> > > Do you have no way to test this in hardware? Since Rivos is a RISCV
> > > vendor and all ;)
> > > I don't like inviting the idea of merging CPU architectural changes
> > > without actually testing them in something resembling real silicon
> > > (i.e QEMU KVM is _fine_, QEMU TCG really isn't).
> > >
> >
> > Hopefully I'm not drawing an incorrect parallel here, but, as I recall
> > arm64 enablement in 2014, nearly all initial enablement in RHEL occurred
> > on software emulators (ARM Foundation Model, ARM FVP, then QEMU TCG).
> > You need to start somewhere. In particular, qemu-system-aarch64 was a
> > huge step forward (performance-wise) once it *existed*, relative to the
> > Foundation Model / FVP, even though qemu-system-aarch64 wouldn't emulate
> > CPU caches (IIRC).
>
> Right. I don't know how faithful those early ARM simulators were, but
> QEMU TCG is not very faithful and uarch details *can* slip through the
> cracks.
> In arm64 it's easy to miss a dsb or a isb if you're not extra careful
> (or read the ARM ARM wrong).
>
> RISCV has a bunch of fun gotchas too. For instance, did you know you
> need to flush the TLB using sfence.vma even when only mapping a page?
> This "small" detail results in boot failures on real hardware (such as
> the visionfive 2), but is completely silent in QEMU TCG.
>
> So this is why I would much prefer a test on real silicon. It's hard
> to prove correctness when all you have is QEMU's spotty simulation
> (rightfully so, it's not a simulator).
>
> --
> Pedro
>


-- 
Thanks!
=D


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110392): https://edk2.groups.io/g/devel/message/110392
Mute This Topic: https://groups.io/mt/102256466/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: 11699 bytes --]

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

[Laszlo Ersek]

On 10/30/23 12:18, Sunil V L wrote:
> On Sun, Oct 29, 2023 at 08:16:12PM +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:
>>     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                | 168 +++++++++++++++++---
>>  MdePkg/MdePkg.uni                                                  |   4 +
>>  4 files changed, 165 insertions(+), 20 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.
> NIT: I am wondering whether PcdRiscVCpuFeatureDisable is better so that
> it is explicit.
> 
>> +  #
>> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UINT64|0x69
>> +
> Instead of this, can default value match only those features which are
> enabled by default for qemu virt machine? That way, I think we can avoid
> having this PCD defined again in RiscVVirt.

I proposed the all-bits-one value.

First, PcdRiscVFeatureOverride is in MdePkg, which is much more general
than QEMU.

Second, the all-bits-one pattern means that the MdePkg.dec default does
not try to disable any features enabled by earlier components in the
boot process, *even such features* that it does not know about
specifically (i.e., those for which edk2 does not define a specific
feature bit).

IMO, for any "feature negotiation" bitmask, there are two choices:

- clear all bits except those that we know about (and know how to use),

- use all-bits-one.

The first option is good if unknown (future) features are expected to
*break* us.

The second option is good if we are unaffected by extra (future)
features, and we want to keep everything enabled for the runtime OS that
comes after us.

I understood that we had case #2 here.

If we have case#1 instead, then we should indeed limit the bitmask to
those features that *core edk2* knows about. But even in that case, I
think MdePkg.dec should be as permissive as possible, and any
QEMU-specific limitation should go into the OVMF DSC file.

> 
>>  [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 4eb18edb9aa7..5b3104afb67e 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,115 @@
>>  #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.
>> +//
>> +#define RISCV_CACHE_BLOCK_SIZE         64
>> +#define RISCV_CPU_FEATURE_CMO_BITMASK  0x1
>> +
> Can we define these bits in the header file so that the definitions can
> be used by multiple modules?

I noticed this too, but didn't call it out, because the DEC file
sufficiently explained the bit meaning(s).

Laszlo



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

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

[Laszlo Ersek]

On 10/30/23 12:22, Sunil V L wrote:
> On Mon, Oct 30, 2023 at 04:48:18PM +0530, Sunil V L wrote:
>> On Sun, Oct 29, 2023 at 08:16:12PM +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:
>>>     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                | 168 +++++++++++++++++---
>>>  MdePkg/MdePkg.uni                                                  |   4 +
>>>  4 files changed, 165 insertions(+), 20 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.
>> NIT: I am wondering whether PcdRiscVCpuFeatureDisable is better so that
>> it is explicit.
>>
>>> +  #
>>> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UINT64|0x69
>>> +
>> Instead of this, can default value match only those features which are
>> enabled by default for qemu virt machine? That way, I think we can avoid
>> having this PCD defined again in RiscVVirt.
>>
> Sorry, I take back. This is common for all platforms. So, we can't take
> qemu as reference.

yes, and sorry that I'm only seeing your addition now!
Laszlo



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

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

[Laszlo Ersek]

On 10/31/23 10:55, Dhaval Sharma wrote:
> I am posting an update on behalf of Jingyu as he had trouble with
> posting.

... that should be rectified now



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

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

[Andrei Warkentin]

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

I think I misunderstood the intent. Reviewing the full patchset, it seems this is necessary to avoid using the new CMO path in the Virt platform (since the default value is all FFs). Shouldn’t the default Pcd value here be all 0’s – i.e. CMO or any other feature use becomes “opt in” instead of “opt out”?

It also seems that encoding the meaning inside the bit positions is a bit… obscure. Have you considered storing a pointer to a struct with bitfields instead? You could then change the logic to be something like “If PcdPtrValue != NULL && ((struct cast *) PcdPtrValue)->LegibleFieldName”. I think this would do wonders for code maintainability. The cost of course is in having to initialize the Pcd now at runtime, and the additional dereference, but that seems like a low cost all things considered.

From: Dhaval Sharma <dhaval@rivosinc.com>
Sent: Tuesday, October 31, 2023 1:13 AM
To: Warkentin, Andrei <andrei.warkentin@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH v7 5/5] OvmfPkg/RiscVVirt: Override for RV CPU Features

Thanks. This PCD is for Virt platform only. Or maybe I am missing the point.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110435): https://edk2.groups.io/g/devel/message/110435
Mute This Topic: https://groups.io/mt/102256471/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: 3899 bytes --]

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

[Pedro Falcato]

On Tue, Oct 31, 2023 at 9:55 AM Dhaval Sharma <dhaval@rivosinc.com> wrote:
>
> I am posting an update on behalf of Jingyu as he had trouble with posting. CC'ing him here:
> In summary what we have verified so far:
>
> I have verified that instructions/op codes are okay. I have also verified on Qemu that functionally it seems to be calling correct instructions. Ensured with negative test cases that any other op codes do cause exceptions as expected.
> Jingyu was able to verify the CpuFlushCpuDataCache function with this framework (he had to use custom op code based on his soc implementation) on SG2042. There is one issue that he is debugging now which is related to other cache instructions and he will get back with more data. P.S. SG2042 does not implement the exact same CMO opcodes but equivalent ones. So this experiment is just an additional data point that helps verify the framework and not CMO itself.
> In general it sounds like framework flows are alright and as long as instructions do their job as claimed in the spec, it is lower risk.
>
> Guess this is what we have so far. If it makes sense to everyone, we could go ahead with merging with this *feature disabled by default* after Jingyu provides clarity reg failures on SG2042 platform. Otherwise we can wait until newer Si is available where these exact instructions can be tested and then upstreamed.

Thanks!

To be clear, I wasn't NAKing it, I even gave you my Rb. I just think
we should be extra careful sending arch-related changes that haven't
been tested on real HW, because hardware tends to be tricky :)

-- 
Pedro


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

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

[Jingyu Li via groups.io]

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

On Tue, Oct 31, 2023 at 05:55 PM, Dhaval Sharma wrote:

> 
> I am posting an update on behalf of Jingyu as he had trouble with posting.
> CC'ing him here:
> In summary what we have verified so far:
> * I have verified that instructions/op codes are okay. I have also
> verified on Qemu that functionally it seems to be calling correct
> instructions. Ensured with negative test cases that any other op codes do
> cause exceptions as expected.
> * Jingyu was able to verify the CpuFlushCpuDataCache function with this
> framework (he had to use custom op code based on his soc implementation)
> on SG2042. There is one issue that he is debugging now which is related to
> other cache instructions and he will get back with more data. P.S. SG2042
> does not implement the exact same CMO opcodes but equivalent ones. So this
> experiment is just an additional data point that helps verify the
> framework and not CMO itself.
> * In general it sounds like framework flows are alright and as long as
> instructions do their job as claimed in the spec, it is lower risk.
> Guess this is what we have so far. If it makes sense to everyone, we could
> go ahead with merging with this *feature disabled by default* after Jingyu
> provides clarity reg failures on SG2042 platform. Otherwise we can wait
> until newer Si is available where these exact instructions can be tested
> and then upstreamed.
> 
> [From Jingyu]
> I verified this CMO framework on an actual HW platform.
> 
> SW:
> edk2: https://github.com/rivosinc/edk2/tree/dev-rv-cmo-v7 branch:
> dev-rv-cmo-v7
> edk2-platforms: https://github.com/sophgo/edk2-platforms branch: sg2042-dev
> 
> 
> HW:
> Milk-V Pioneer Box, a developer motherboard based on SG2042 with 64-Core
> T-HEAD C920.
> 
> Attention:
> The T-HEAD C920 implemented its own CMO Extension and is different from
> the standard CMO Extension.
> 
> Test steps:
> 1. Modified the opcodes in RiscVasm.inc to accommodate the C920 CMO
> feature.

Update the test status.
The InvalidateInstructionCacheRange and the InvalidateDataCacheRange execute the same instruction "cbo.inval", but the T-HEAD C920 executes different instructions for the two functions. Please see the form below for details.
I replaced ".long 0x02a5000b" with "fence.i", which solved unexpected exceptions that occurred yesterday.

RISC-V standard CMO Extension

C920 CMO Extension

WriteBackInvalidateDataCache

WriteBackInvalidateDataCacheRange

cbo. flush

".long 0x02b5000b"

WriteBackDataCache

WriteBackDataCacheRange

cbo. clean

".long 0x02b5000b"

InvalidateDataCache

fence

fence

InvalidateDataCacheRange

cbo. inval

".long 0x02a5000b"

InvalidateInstructionCache

fence. i

fence. i

InvalidateInstructionCacheRange

cbo. inval

fence. i

Now, I enable CMO in the entire edk2 phase, not just during PCIe inbound. No exceptions found so far. Hope to give you some reference.

Update the patches:
diff --git a/MdePkg/Include/RiscV64/RiscVasm.inc b/MdePkg/Include/RiscV64/RiscVasm.inc
index 29de735885..c2e573eb3d 100644
--- a/MdePkg/Include/RiscV64/RiscVasm.inc
+++ b/MdePkg/Include/RiscV64/RiscVasm.inc
@@ -7,13 +7,13 @@
*/

.macro RISCVCMOFLUSH
-    .word 0x25200f
+    .long 0x02b5000b^M
.endm

.macro RISCVCMOINVALIDATE
-    .word 0x05200f
+    .long 0x02a5000b^M
.endm

.macro RISCVCMOCLEAN
-    .word 0x15200f
+    .long 0x02b5000b^M
.endm

diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
index 5b3104afb6..ee85d0548c 100644
--- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
@@ -89,7 +89,7 @@ CacheOpCacheRange (
Start &= ~((UINTN)CacheLineSize - 1);

DEBUG (
-    (DEBUG_INFO,
+    (DEBUG_VERBOSE,^M
"CacheOpCacheRange:\
Performing Cache Management Operation %d \n", Op)
);
@@ -163,7 +163,8 @@ InvalidateInstructionCacheRange (
)
{
if (RiscVIsCMOEnabled ()) {
-    CacheOpCacheRange (Address, Length, Invld);
+    // CacheOpCacheRange (Address, Length, Invld);^M
+    InvalidateInstructionCache ();^M
} else {
DEBUG (
(DEBUG_VERBOSE,

diff --git a/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c b/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c
index 2af3b62234..824667bc87 100644
--- a/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c
@@ -9,6 +9,7 @@
**/

#include "CpuDxe.h"
+#include <Library/CacheMaintenanceLib.h>^M

//
// Global Variables
@@ -59,7 +60,7 @@ EFI_CPU_ARCH_PROTOCOL  gCpu = {
CpuGetTimerValue,
CpuSetMemoryAttributes,
1,                          // NumberOfTimers
-  4                           // DmaBufferAlignment
+  64                          // DmaBufferAlignment^M
};

//
@@ -90,6 +91,20 @@ CpuFlushCpuDataCache (
IN EFI_CPU_FLUSH_TYPE     FlushType
)
{
+  switch (FlushType) {^M
+    case EfiCpuFlushTypeWriteBack:^M
+      WriteBackDataCacheRange ((VOID *)(UINTN)Start, (UINTN)Length);^M
+      break;^M
+    case EfiCpuFlushTypeInvalidate:^M
+      InvalidateDataCacheRange ((VOID *)(UINTN)Start, (UINTN)Length);^M
+      break;^M
+    case EfiCpuFlushTypeWriteBackInvalidate:^M
+      WriteBackInvalidateDataCacheRange ((VOID *)(UINTN)Start, (UINTN)Length);^M
+      break;^M
+    default:^M
+      return EFI_INVALID_PARAMETER;^M
+  }^M
+^M
return EFI_SUCCESS;
}

diff --git a/Platform/Sophgo/SG2042_EVB_Board/SG2042.fdf b/Platform/Sophgo/SG2042_EVB_Board/SG2042.fdf
index 844fc3eac0..9cbb1d3f65 100644
--- a/Platform/Sophgo/SG2042_EVB_Board/SG2042.fdf
+++ b/Platform/Sophgo/SG2042_EVB_Board/SG2042.fdf
@@ -77,7 +77,7 @@ INF  Silicon/Sophgo/SG2042Pkg/Drivers/SdHostDxe/SdHostDxe.inf

# RISC-V Core Drivers
INF  UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
-INF  Silicon/Sophgo/SG2042Pkg/Override/UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf
+INF  UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf

INF  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf
INF  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf

diff --git a/Platform/Sophgo/SG2042_EVB_Board/SG2042.dsc b/Platform/Sophgo/SG2042_EVB_Board/SG2042.dsc
index 51ff89678c..bc3f96e21b 100644
--- a/Platform/Sophgo/SG2042_EVB_Board/SG2042.dsc
+++ b/Platform/Sophgo/SG2042_EVB_Board/SG2042.dsc
@@ -389,6 +389,7 @@

[PcdsPatchableInModule]
gSophgoSG2042PlatformPkgTokenSpaceGuid.PcdSG2042PhyAddrToVirAddr|0
+  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0x1

################################################################################
#
@@ -500,7 +501,7 @@
# RISC-V Core module
#
UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
-  Silicon/Sophgo/SG2042Pkg/Override/UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf
+  UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf
MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf

MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf

Thanks,
Jingyu


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110470): https://edk2.groups.io/g/devel/message/110470
Mute This Topic: https://groups.io/mt/102256466/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: 18013 bytes --]

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

[Dhaval Sharma]

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

Hi Andrei,
Are you suggesting:

   1. We have a generic PCD to store the address of RVFeatures bitfield.
   2. It gets populated at some point during initialization with let's say
   some kind of global variable address which keeps this bitfield.
   3. SEC/DXE phases deref this address and use where needed?

Is there a reference I can take a look at?

Assuming my above understanding is correct, the whole idea was to keep
implementation based on simple PCD such that it can be used easily as an
override mechanism. The reason to keep it "enabled" by default in MDE is
that if m-mode decides to keep it disabled it is anyways going to remain
that way. So practically this PCD is going to be useful only in cases where
the user wants to "Override Disable". LMK if you still think we should
modify the implementation.

=D

On Tue, Oct 31, 2023 at 10:31 PM Warkentin, Andrei <
andrei.warkentin@intel.com> wrote:

> I think I misunderstood the intent. Reviewing the full patchset, it seems
> this is necessary to avoid using the new CMO path in the Virt platform
> (since the default value is all FFs). Shouldn’t the default Pcd value here
> be all 0’s – i.e. CMO or any other feature use becomes “opt in” instead of
> “opt out”?
>
>
>
> It also seems that encoding the meaning inside the bit positions is a bit…
> obscure. Have you considered storing a pointer to a struct with bitfields
> instead? You could then change the logic to be something like “If
> PcdPtrValue != NULL && ((struct cast *) PcdPtrValue)->LegibleFieldName”. I
> think this would do wonders for code maintainability. The cost of course is
> in having to initialize the Pcd now at runtime, and the additional
> dereference, but that seems like a low cost all things considered.
>
>
>
> *From:* Dhaval Sharma <dhaval@rivosinc.com>
> *Sent:* Tuesday, October 31, 2023 1:13 AM
> *To:* Warkentin, Andrei <andrei.warkentin@intel.com>; devel@edk2.groups.io
> *Subject:* Re: [edk2-devel] [PATCH v7 5/5] OvmfPkg/RiscVVirt: Override
> for RV CPU Features
>
>
>
> Thanks. This PCD is for Virt platform only. Or maybe I am missing the
> point.
>


-- 
Thanks!
=D


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110487): https://edk2.groups.io/g/devel/message/110487
Mute This Topic: https://groups.io/mt/102256471/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: 4313 bytes --]

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

[Andrei Warkentin]

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

Hi Dhaval,

Yes, that is what I’m suggesting. But I’m not telling you that your current implementation needs to be adjusted, but I am providing my feedback, that IMHO continued /use and evolution/ of the added PCD may to be messy because it’s untyped. I already gave you a Reviewed-by, so you’re welcome to do whatever you want with the additional feedback.

As far as an example goes… see:
  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo|{0xFF}|VOID*|0x00010067
  …
  DeviceInfo = (PCI_UART_DEVICE_INFO *)PcdGetPtr (PcdSerialPciDeviceInfo);

So a PCD can be of type VOID *.

A

From: Dhaval Sharma <dhaval@rivosinc.com>
Sent: Wednesday, November 1, 2023 12:05 PM
To: Warkentin, Andrei <andrei.warkentin@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH v7 5/5] OvmfPkg/RiscVVirt: Override for RV CPU Features

Hi Andrei,
Are you suggesting:

  1.  We have a generic PCD to store the address of RVFeatures bitfield.
  2.  It gets populated at some point during initialization with let's say some kind of global variable address which keeps this bitfield.
  3.  SEC/DXE phases deref this address and use where needed?
Is there a reference I can take a look at?

Assuming my above understanding is correct, the whole idea was to keep implementation based on simple PCD such that it can be used easily as an override mechanism. The reason to keep it "enabled" by default in MDE is that if m-mode decides to keep it disabled it is anyways going to remain that way. So practically this PCD is going to be useful only in cases where the user wants to "Override Disable". LMK if you still think we should modify the implementation.

=D

On Tue, Oct 31, 2023 at 10:31 PM Warkentin, Andrei <andrei.warkentin@intel.com<mailto:andrei.warkentin@intel.com>> wrote:
I think I misunderstood the intent. Reviewing the full patchset, it seems this is necessary to avoid using the new CMO path in the Virt platform (since the default value is all FFs). Shouldn’t the default Pcd value here be all 0’s – i.e. CMO or any other feature use becomes “opt in” instead of “opt out”?

It also seems that encoding the meaning inside the bit positions is a bit… obscure. Have you considered storing a pointer to a struct with bitfields instead? You could then change the logic to be something like “If PcdPtrValue != NULL && ((struct cast *) PcdPtrValue)->LegibleFieldName”. I think this would do wonders for code maintainability. The cost of course is in having to initialize the Pcd now at runtime, and the additional dereference, but that seems like a low cost all things considered.

From: Dhaval Sharma <dhaval@rivosinc.com<mailto:dhaval@rivosinc.com>>
Sent: Tuesday, October 31, 2023 1:13 AM
To: Warkentin, Andrei <andrei.warkentin@intel.com<mailto:andrei.warkentin@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH v7 5/5] OvmfPkg/RiscVVirt: Override for RV CPU Features

Thanks. This PCD is for Virt platform only. Or maybe I am missing the point.


--
Thanks!
=D


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110491): https://edk2.groups.io/g/devel/message/110491
Mute This Topic: https://groups.io/mt/102256471/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: 8735 bytes --]