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

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

V10:
- Fix formatting to keep comments within 80
- Replace RV with RISC-V
- Fix an issue with multi line comments
- Added assert to an unsupported function WriteBackInvalidateDataCache
- Only keep CMO feature bitmask bit to disabled. Unimplemented bits remain 1.
- Minor case modification in str in .uni

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: Laszlo Ersek <lersek@redhat.com>
Cc: Pedro Falcato <pedro.falcato@gmail.com>


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 RISC-V 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                | 181 +++++++++++++++-----
 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, 270 insertions(+), 62 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 (#112478): https://edk2.groups.io/g/devel/message/112478
Mute This Topic: https://groups.io/mt/103150381/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v10 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>
Cc: Pedro Falcato <pedro.falcato@gmail.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 728e89d2bf44..2c69c5f52877 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 (#112479): https://edk2.groups.io/g/devel/message/112479
Mute This Topic: https://groups.io/mt/103150382/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v10 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>
Cc: Pedro Falcato <pedro.falcato@gmail.com>

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

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

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

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



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

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

[Dhaval Sharma]

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

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

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

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

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

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

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

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



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

[edk2-devel] [PATCH v10 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>
Cc: Pedro Falcato <pedro.falcato@gmail.com>

Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>
---

Notes:
    V10:
    - Fix formatting to keep comments within 80
    - Replace RV with RISC-V
    - Fix an issue with multi line comments
    - Added assert to an unsupported function
    - Minor case modification in str in .uni

    V9:
    - Fixed an issue with Instruction cache invalidation. Use fence.i
      instruction as CMO does not support i-cache operations.
    V8:
    - Added note to convert PCD into RISC-V feature bitmap pointer
    - Modified function names to be more explicit about cache ops
    - Added RB tag
    V7:
    - Added PcdLib
    - Restructure DEBUG message based on feedback on V6
    - Make naming consistent to CMO, remove all CBO references
    - Add ASSERT for not supported functions instead of plain debug message
    - Added RB tag
    V6:
    - Utilize cache management instructions if HW supports it
      This patch is part of restructuring on top of v5

 MdePkg/MdePkg.dec                                                  |   8 +
 MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |   5 +
 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c                | 177 ++++++++++++++++----
 MdePkg/MdePkg.uni                                                  |   4 +
 4 files changed, 166 insertions(+), 28 deletions(-)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index ac54338089e8..fa92673ff633 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2399,6 +2399,14 @@ [PcdsFixedAtBuild.AARCH64, PcdsPatchableInModule.AARCH64]
   # @Prompt CPU Rng algorithm's GUID.
   gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00000037
 
+[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
+  #
+  # Configurability to override RISC-V CPU Features
+  # BIT 0 = Cache Management Operations. This bit is relevant only if
+  # previous stage has feature enabled and user wants to disable it.
+  #
+  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UINT64|0x69
+
 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## This value is used to set the base address of PCI express hierarchy.
   # @Prompt PCI Express Base Address.
diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
index 6fd9cbe5f6c9..601a38d6c109 100644
--- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
@@ -56,3 +56,8 @@ [LibraryClasses]
   BaseLib
   DebugLib
 
+[LibraryClasses.RISCV64]
+  PcdLib
+
+[Pcd.RISCV64]
+  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride  ## CONSUMES
diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
index ac2a3c23a249..7c53a17abbb5 100644
--- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
@@ -2,6 +2,7 @@
   RISC-V specific functionality for cache.
 
   Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
+  Copyright (c) 2023, Rivos Inc. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
@@ -9,10 +10,116 @@
 #include <Base.h>
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+
+//
+// TODO: Grab cache block size and make Cache Management Operation
+// enabling decision based on RISC-V CPU HOB in
+// future when it is available and convert PcdRiscVFeatureOverride
+// PCD to a pointer that contains pointer to bitmap structure
+// which can be operated more elegantly.
+//
+#define RISCV_CACHE_BLOCK_SIZE         64
+#define RISCV_CPU_FEATURE_CMO_BITMASK  0x1
+
+typedef enum {
+  CacheOpClean,
+  CacheOpFlush,
+  CacheOpInvld,
+} CACHE_OP;
+
+/**
+Verify CBOs are supported by this HW
+TODO: Use RISC-V CPU HOB once available.
+
+**/
+STATIC
+BOOLEAN
+RiscVIsCMOEnabled (
+  VOID
+  )
+{
+  // If CMO is disabled in HW, skip Override check
+  // Otherwise this PCD can override settings
+  return ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_CMO_BITMASK) != 0);
+}
+
+/**
+  Performs required opeartion on cache lines in the cache coherency domain
+  of the calling CPU. If Address is not aligned on a cache line boundary,
+  then entire cache line containing Address is operated. If Address + Length
+  is not aligned on a cache line boundary, then the entire cache line
+  containing Address + Length -1 is operated.
+  If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
+  @param  Address The base address of the cache lines to
+          invalidate.
+  @param  Length  The number of bytes to invalidate from the instruction
+          cache.
+  @param  Op  Type of CMO operation to be performed
+  @return Address.
+
+**/
+STATIC
+VOID
+CacheOpCacheRange (
+  IN VOID      *Address,
+  IN UINTN     Length,
+  IN CACHE_OP  Op
+  )
+{
+  UINTN  CacheLineSize;
+  UINTN  Start;
+  UINTN  End;
+
+  if (Length == 0) {
+    return;
+  }
+
+  if ((Op != CacheOpInvld) && (Op != CacheOpFlush) && (Op != CacheOpClean)) {
+    return;
+  }
+
+  ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)Address));
+
+  CacheLineSize = RISCV_CACHE_BLOCK_SIZE;
+
+  Start = (UINTN)Address;
+  //
+  // Calculate the cache line alignment
+  //
+  End    = (Start + Length + (CacheLineSize - 1)) & ~(CacheLineSize - 1);
+  Start &= ~((UINTN)CacheLineSize - 1);
+
+  DEBUG (
+    (DEBUG_VERBOSE,
+     "CacheOpCacheRange: Performing Cache Management Operation %d \n", Op)
+    );
+
+  do {
+    switch (Op) {
+      case CacheOpInvld:
+        RiscVCpuCacheInvalCmoAsm (Start);
+        break;
+      case CacheOpFlush:
+        RiscVCpuCacheFlushCmoAsm (Start);
+        break;
+      case CacheOpClean:
+        RiscVCpuCacheCleanCmoAsm (Start);
+        break;
+      default:
+        break;
+    }
+
+    Start = Start + CacheLineSize;
+  } while (Start != End);
+}
 
 /**
   Invalidates the entire instruction cache in cache coherency domain of the
-  calling CPU.
+  calling CPU. Risc-V does not have currently an CBO implementation which can
+  invalidate the entire I-cache. Hence using Fence instruction for now. P.S.
+  Fence instruction may or may not implement full I-cache invd functionality
+  on all implementations.
 
 **/
 VOID
@@ -28,17 +135,11 @@ InvalidateInstructionCache (
   Invalidates a range of instruction cache lines in the cache coherency domain
   of the calling CPU.
 
-  Invalidates the instruction cache lines specified by Address and Length. If
-  Address is not aligned on a cache line boundary, then entire instruction
-  cache line containing Address is invalidated. If Address + Length is not
-  aligned on a cache line boundary, then the entire instruction cache line
-  containing Address + Length -1 is invalidated. This function may choose to
-  invalidate the entire instruction cache if that is more efficient than
-  invalidating the specified range. If Length is 0, then no instruction cache
-  lines are invalidated. Address is returned.
-
-  If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
-
+  An operation from a CMO instruction is defined to operate only on the copies
+  of a cache block that are cached in the caches accessible by the explicit
+  memory accesses performed by the set of coherent agents.In other words CMO
+  operations are not applicable to instruction cache. Use fence.i instruction
+  instead to achieve the same purpose.
   @param  Address The base address of the instruction cache lines to
                   invalidate. If the CPU is in a physical addressing mode, then
                   Address is a physical address. If the CPU is in a virtual
@@ -57,9 +158,10 @@ InvalidateInstructionCacheRange (
   )
 {
   DEBUG (
-    (DEBUG_WARN,
-     "%a:RISC-V unsupported function.\n"
-     "Invalidating the whole instruction cache instead.\n", __func__)
+    (DEBUG_VERBOSE,
+     "InvalidateInstructionCacheRange: RISC-V unsupported function.\n"
+     "Invalidating the whole instruction cache instead.\n"
+    )
     );
   InvalidateInstructionCache ();
   return Address;
@@ -81,7 +183,12 @@ WriteBackInvalidateDataCache (
   VOID
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  ASSERT (FALSE);
+  DEBUG ((
+    DEBUG_ERROR,
+    "WriteBackInvalidateDataCache:" \
+    "RISC-V unsupported function.\n"
+    ));
 }
 
 /**
@@ -117,7 +224,12 @@ WriteBackInvalidateDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  if (RiscVIsCMOEnabled ()) {
+    CacheOpCacheRange (Address, Length, CacheOpFlush);
+  } else {
+    ASSERT (FALSE);
+  }
+
   return Address;
 }
 
@@ -137,7 +249,7 @@ WriteBackDataCache (
   VOID
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  ASSERT (FALSE);
 }
 
 /**
@@ -156,10 +268,7 @@ WriteBackDataCache (
 
   If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
 
-  @param  Address The base address of the data cache lines to write back. If
-                  the CPU is in a physical addressing mode, then Address is a
-                  physical address. If the CPU is in a virtual addressing
-                  mode, then Address is a virtual address.
+  @param  Address The base address of the data cache lines to write back.
   @param  Length  The number of bytes to write back from the data cache.
 
   @return Address of cache written in main memory.
@@ -172,7 +281,12 @@ WriteBackDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  if (RiscVIsCMOEnabled ()) {
+    CacheOpCacheRange (Address, Length, CacheOpClean);
+  } else {
+    ASSERT (FALSE);
+  }
+
   return Address;
 }
 
@@ -214,10 +328,7 @@ InvalidateDataCache (
 
   If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
 
-  @param  Address The base address of the data cache lines to invalidate. If
-                  the CPU is in a physical addressing mode, then Address is a
-                  physical address. If the CPU is in a virtual addressing mode,
-                  then Address is a virtual address.
+  @param  Address The base address of the data cache lines to invalidate.
   @param  Length  The number of bytes to invalidate from the data cache.
 
   @return Address.
@@ -230,6 +341,16 @@ InvalidateDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  if (RiscVIsCMOEnabled ()) {
+    CacheOpCacheRange (Address, Length, CacheOpInvld);
+  } else {
+    DEBUG (
+      (DEBUG_VERBOSE,
+       "InvalidateDataCacheRange: Zicbom not supported.\n" \
+       "Invalidating the whole Data cache instead.\n") \
+      );
+    InvalidateDataCache ();
+  }
+
   return Address;
 }
diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
index 5c1fa24065c7..73b5dd8f32cc 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 (#112482): https://edk2.groups.io/g/devel/message/112482
Mute This Topic: https://groups.io/mt/103150435/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v10 5/5] OvmfPkg/RiscVVirt: Override for RISC-V 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>
Cc: Pedro Falcato <pedro.falcato@gmail.com>

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

Notes:
    V10:
    - Only keep CMO feature bitmask bit to disabled. Unimplemented bits
      remain 1.
    V8:
    - Added RV tag
    V7:
    - Added RB tag
    v6:
    - Modify PCD name according to changes made in Baselib implementation
    V5:
    - Introduce PCD for platform

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

diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
index fe320525153f..a050f1ffc1d4 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|0xFFFFFFFE
   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 (#112483): https://edk2.groups.io/g/devel/message/112483
Mute This Topic: https://groups.io/mt/103150443/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sunil V L]

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

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


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

Re: [edk2-devel] [PATCH v10 5/5] OvmfPkg/RiscVVirt: Override for RISC-V CPU Features

[Sunil V L]

On Wed, Dec 13, 2023 at 08:29:31PM +0530, Dhaval wrote:
> This PCD provides a way for platform to override any
> HW features that are default enabled by previous stages
> of FW (like OpenSBI). For the case where previous/prev
> stage has disabled the feature, this override is not
> useful and its usage should be avoided.
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Sunil V L <sunilvl@ventanamicro.com>
> Cc: Andrei Warkentin <andrei.warkentin@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> 
> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Andrei Warkentin <andrei.warkentin@...>
> ---
> 
> Notes:
>     V10:
>     - Only keep CMO feature bitmask bit to disabled. Unimplemented bits
>       remain 1.
>     V8:
>     - Added RV tag
>     V7:
>     - Added RB tag
>     v6:
>     - Modify PCD name according to changes made in Baselib implementation
>     V5:
>     - Introduce PCD for platform
> 
>  OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> index fe320525153f..a050f1ffc1d4 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|0xFFFFFFFE
This needs to be a 64-bit value. Again, I will fix it before merging.

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


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

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

[Dhaval Sharma]

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

Thanks. Just to clarify, In the earlier formatting
"InvalidateDataCacheRange:\
> +       Zicbom not supported.\n" \
A missing " after *Range:\ was causing slightly skewed prints. After adding
this " it looks okay. So that is one change I had addressed.
But if keeping it in a single line works better please feel free to update.
And Thanks!


On Tue, Dec 19, 2023 at 12:59 PM Sunil V L <sunilvl@ventanamicro.com> wrote:

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


-- 
Thanks!
=D


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



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

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

[Sunil V L]

On Wed, Dec 13, 2023 at 08:29:26PM +0530, Dhaval 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.
> 
Merged as #5164

Thanks,
Sunil


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

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

[Laszlo Ersek]

Hi Dhaval,

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

This change (replacing the DEBUG with an ASSERT()) seems to be causing a
failure for some physical platforms:

https://bugzilla.tianocore.org/show_bug.cgi?id=4637

Can you please check that ticket?

(I'd have CC'd you on the ticket, but the CC search field returned no
hits for "Dhaval" -- I think you may not have an account in Bugzilla yet.)

If a platform has no support for CMO (and the platform DSC sets
PcdRiscVFeatureOverride accordingly), then
WriteBackInvalidateDataCacheRange() is effectively uncallable on that
platform. Is that intentional?

I think there are two possibilities (when RiscVIsCMOEnabled() returns
FALSE):

- the platform has "no need" for writing back and invalidating a range
of the data cache -- perhaps because it has no data cache in the CPUs at
all. In that case, a no-op is better (and still safe) than a failed
ASSERT().

- or else, the platform needs this kind of flushing and invalidation,
but the CPU just doesn't support the particular CMO / fine-grained
instruction. In that case, can we do something heavier-weight, but
functionally *sufficient*? A good example is in this same patch's
InvalidateDataCacheRange() function, which falls back to
InvalidateDataCache() if CMO is missing.

Now, I can see that WriteBackDataCache() *itself* is not supported
(regardless of CMO). Why is that? Does it make no sense on RISC-V
platforms? If that's the case, should it be a no-op instead?

Yangcheng: can you provide a backtrace? What outer call path led you to
the ASSERT() in WriteBackInvalidateDataCacheRange()?

Thanks!
Laszlo

>  
> @@ -137,7 +249,7 @@ WriteBackDataCache (
>    VOID
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  ASSERT (FALSE);
>  }
>  
>  /**
> @@ -156,10 +268,7 @@ WriteBackDataCache (
>  
>    If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
>  
> -  @param  Address The base address of the data cache lines to write back. If
> -                  the CPU is in a physical addressing mode, then Address is a
> -                  physical address. If the CPU is in a virtual addressing
> -                  mode, then Address is a virtual address.
> +  @param  Address The base address of the data cache lines to write back.
>    @param  Length  The number of bytes to write back from the data cache.
>  
>    @return Address of cache written in main memory.
> @@ -172,7 +281,12 @@ WriteBackDataCacheRange (
>    IN      UINTN  Length
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  if (RiscVIsCMOEnabled ()) {
> +    CacheOpCacheRange (Address, Length, CacheOpClean);
> +  } else {
> +    ASSERT (FALSE);
> +  }
> +
>    return Address;
>  }
>  
> @@ -214,10 +328,7 @@ InvalidateDataCache (
>  
>    If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
>  
> -  @param  Address The base address of the data cache lines to invalidate. If
> -                  the CPU is in a physical addressing mode, then Address is a
> -                  physical address. If the CPU is in a virtual addressing mode,
> -                  then Address is a virtual address.
> +  @param  Address The base address of the data cache lines to invalidate.
>    @param  Length  The number of bytes to invalidate from the data cache.
>  
>    @return Address.
> @@ -230,6 +341,16 @@ InvalidateDataCacheRange (
>    IN      UINTN  Length
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  if (RiscVIsCMOEnabled ()) {
> +    CacheOpCacheRange (Address, Length, CacheOpInvld);
> +  } else {
> +    DEBUG (
> +      (DEBUG_VERBOSE,
> +       "InvalidateDataCacheRange: Zicbom not supported.\n" \
> +       "Invalidating the whole Data cache instead.\n") \
> +      );
> +    InvalidateDataCache ();
> +  }
> +
>    return Address;
>  }
> diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
> index 5c1fa24065c7..73b5dd8f32cc 100644
> --- a/MdePkg/MdePkg.uni
> +++ b/MdePkg/MdePkg.uni
> @@ -287,6 +287,10 @@
>  
>  #string STR_gEfiMdePkgTokenSpaceGuid_PcdGuidedExtractHandlerTableAddress_HELP  #language en-US "This value is used to set the available memory address to store Guided Extract Handlers. The required memory space is decided by the value of PcdMaximumGuidedExtractHandler."
>  
> +#string STR_gEfiMdePkgTokenSpaceGuid_PcdRiscVFeatureOverride_PROMPT  #language en-US "RISC-V Feature Override"
> +
> +#string STR_gEfiMdePkgTokenSpaceGuid_PcdRiscVFeatureOverride_HELP  #language en-US "This value is used to override any RISC-V specific features supported by this PCD"
> +
>  #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_PROMPT  #language en-US "PCI Express Base Address"
>  
>  #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_HELP  #language en-US "This value is used to set the base address of PCI express hierarchy."



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113390): https://edk2.groups.io/g/devel/message/113390
Mute This Topic: https://groups.io/mt/103150435/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 v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V

[yorange]

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

Hi, all
I`m yangcheng, and I found this bug when using the *edk2-platforms/Silicon/Synopsys/DesignWare/Drivers/DwEmmcDxe* driver on my VisionFive2 development board, which does not support the *CMO* instruction set.  There are many functions in *DwEmmcDxe.c* that call Cache operations. For example, the *DwEmmcReadBlockData* function below calls *WriteBackDataCacheRange* ().

EFI_STATUS
*DwEmmcReadBlockData* (
IN EFI_MMC_HOST_PROTOCOL     *This,
IN EFI_LBA                    Lba,
IN UINTN                      Length,
IN UINT32*                   Buffer
)
{

.......

*WriteBackDataCacheRange* (gpIdmacDesc, DescPages * EFI_PAGE_SIZE);
StartDma (Length);

Status = SendCommand (mDwEmmcCommand, mDwEmmcArgument);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "Failed to read data, mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n", mDwEmmcCommand, mDwEmmcArgument, Status));
goto out;
}
out:
// Restore Tpl
gBS->RestoreTPL (Tpl);
return Status;
}

Initially, I didn't set *PcdRiscVFeatureOverride* , so I got an illegal instruction exception. Then I set my *PcdRiscVFeatureOverride* to *0xFFFFFFFFFFFFFFFE* to avoid using the *CMO* instruction set, and I got an *ASSERT*.

Most cross-platform driver codes may call Cache management operations. Modifying these driver codes may cost a lot, and I think we may need some better way than ASSERT.


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



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

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

[Dhaval Sharma]

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

Hi yangcheng/Pedro,
Thanks for bringing this up. I understand the issue and probably we could just keep it simple with a warning instead of an assert. But wanted to mention a couple of points:
1. I think initially even in my patchset it was DEBUG message but there was a comment to turn it into Assert and I kind of agreed to it thinking that Assert is also typically ignored in release mode and comes into effect during debug build.
2. It might be okay to keep it that way because at least in debug mode it brings it to a developer's notice that the functionality he/she intends to call is not implemented by underlying layer.
Whatever we decide, will be applicable to other places in this file as well. Pedro IMO, it would be better to have simple warnings instead of injecting NOPS as it at least notifies the user about actual underlying behavior.


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



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

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

[Pedro Falcato]

On Mon, Jan 8, 2024 at 4:23 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
>
> Hi yangcheng/Pedro,

+CC a bunch of relevant people

Hi, (FYI you did not CC me)

Looking at yangcheng's example:

  Status = PrepareDmaData (gpIdmacDesc, Length, Buffer); <-- We write
to the IDMAC desc
  if (EFI_ERROR (Status)) {
    goto out;
  }

  WriteBackDataCacheRange (gpIdmacDesc, DescPages * EFI_PAGE_SIZE);
<-- Make sure it's DMA-coherent
  StartDma (Length); <-- We've flushed the cache, everything is now in
DRAM and DMA-coherent, start DMA

which screams of "bad abstractions" because you don't actually need to
write data back, if the device and platform are DMA coherent.

So what we want here really depends. My local "Volume I: RISC-V
Unprivileged ISA V20191213" says, section A.5:

"Table A.5 provides a mapping of Linux memory ordering macros onto
RISC-V memory instructions.
The Linux fences dma rmb() and dma wmb() map onto FENCE R,R and FENCE
W,W, respectively,
since the RISC-V Unix Platform requires coherent DMA, but would be
mapped onto FENCE RI,RI
and FENCE WO,WO, respectively, on a platform with non-coherent DMA.
Platforms with non-
coherent DMA may also require a mechanism by which cache lines can be
flushed and/or invalidated.
Such mechanisms will be device-specific and/or standardized in a
future extension to the ISA."

The (current date) RISCV Platform Spec also says: "Memory accesses by
I/O masters can be coherent or non-coherent with respect to all
hart-related caches."
which is brilliantly useless.

so I think the best solution here is to:

1) Add a new PCD for platform DMA coherency, and test that on
WriteBackDataCacheRange's ASSERT (if (!Coherent) ASSERT() else
return;)
2) Add a more abstracting API that doesn't necessarily map to
WriteBackDataCache when all we wanted was to assert DMA coherency

but, alas, I've seen a lot less funky platforms than many of you, and
DMA/cache-coherency is not really my thing, so I'll defer to others..

-- 
Pedro


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

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

[yorange]

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

Hi Dhaval,
I can understand a little bit why ASSERT is used，If we can determine that Riscv's BaseCacheMaintenanceLib needs to depend on the CMO instruction set, then RISCV processor platforms that do not have the CMO instruction set should not use this library. They should use BaseCacheMaintenanceLibNull or other libraries instead. In fact, like the DwEmmcDxe driver I use on VisionFive2, doing nothing when it comes to Cache management will have no effect. . .
However, we may be able to add some DEBUG information before ASSERT to prompt developers to use the correct Cache management library. After all, many RISCV platforms use BaseCacheMaintenanceLib that previously only printed information that RISC-V unsupported function.


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



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

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

[Sunil V L]

On Mon, Jan 08, 2024 at 09:53:46PM +0000, Pedro Falcato wrote:
> On Mon, Jan 8, 2024 at 4:23 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
> >
> > Hi yangcheng/Pedro,
> 
> +CC a bunch of relevant people
> 
> Hi, (FYI you did not CC me)
> 
> Looking at yangcheng's example:
> 
>   Status = PrepareDmaData (gpIdmacDesc, Length, Buffer); <-- We write
> to the IDMAC desc
>   if (EFI_ERROR (Status)) {
>     goto out;
>   }
> 
>   WriteBackDataCacheRange (gpIdmacDesc, DescPages * EFI_PAGE_SIZE);
> <-- Make sure it's DMA-coherent
>   StartDma (Length); <-- We've flushed the cache, everything is now in
> DRAM and DMA-coherent, start DMA
> 
> which screams of "bad abstractions" because you don't actually need to
> write data back, if the device and platform are DMA coherent.
> 
> So what we want here really depends. My local "Volume I: RISC-V
> Unprivileged ISA V20191213" says, section A.5:
> 
> "Table A.5 provides a mapping of Linux memory ordering macros onto
> RISC-V memory instructions.
> The Linux fences dma rmb() and dma wmb() map onto FENCE R,R and FENCE
> W,W, respectively,
> since the RISC-V Unix Platform requires coherent DMA, but would be
> mapped onto FENCE RI,RI
> and FENCE WO,WO, respectively, on a platform with non-coherent DMA.
> Platforms with non-
> coherent DMA may also require a mechanism by which cache lines can be
> flushed and/or invalidated.
> Such mechanisms will be device-specific and/or standardized in a
> future extension to the ISA."
> 
> The (current date) RISCV Platform Spec also says: "Memory accesses by
> I/O masters can be coherent or non-coherent with respect to all
> hart-related caches."
> which is brilliantly useless.
> 
> so I think the best solution here is to:
> 
> 1) Add a new PCD for platform DMA coherency, and test that on
> WriteBackDataCacheRange's ASSERT (if (!Coherent) ASSERT() else
> return;)
> 2) Add a more abstracting API that doesn't necessarily map to
> WriteBackDataCache when all we wanted was to assert DMA coherency
> 
> but, alas, I've seen a lot less funky platforms than many of you, and
> DMA/cache-coherency is not really my thing, so I'll defer to others..
> 
My preference is just remove the assertion and add the debug verbose
message instead of changing drivers/introduce new interfaces. It is a
nop in linux as well if CMO is not present.

Thanks,
Sunil


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

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

[Andrei Warkentin]

For now, this is really something that ought to be hidden by DmaLib abstraction (Map/Unmap). This would allow the driver to be minimally aware of how the IP is integrated into the SoC.

A

> -----Original Message-----
> From: Sunil V L <sunilvl@ventanamicro.com>
> Sent: Monday, January 8, 2024 11:32 PM
> To: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: devel@edk2.groups.io; dhaval@rivosinc.com; yorange
> <yangcheng.work@foxmail.com>; Warkentin, Andrei
> <andrei.warkentin@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Leif Lindholm <quic_llindhol@quicinc.com>
> Subject: Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache
> Management Operations Implementation For RISC-V
> 
> On Mon, Jan 08, 2024 at 09:53:46PM +0000, Pedro Falcato wrote:
> > On Mon, Jan 8, 2024 at 4:23 PM Dhaval Sharma <dhaval@rivosinc.com>
> wrote:
> > >
> > > Hi yangcheng/Pedro,
> >
> > +CC a bunch of relevant people
> >
> > Hi, (FYI you did not CC me)
> >
> > Looking at yangcheng's example:
> >
> >   Status = PrepareDmaData (gpIdmacDesc, Length, Buffer); <-- We write
> > to the IDMAC desc
> >   if (EFI_ERROR (Status)) {
> >     goto out;
> >   }
> >
> >   WriteBackDataCacheRange (gpIdmacDesc, DescPages * EFI_PAGE_SIZE);
> > <-- Make sure it's DMA-coherent
> >   StartDma (Length); <-- We've flushed the cache, everything is now in
> > DRAM and DMA-coherent, start DMA
> >
> > which screams of "bad abstractions" because you don't actually need to
> > write data back, if the device and platform are DMA coherent.
> >
> > So what we want here really depends. My local "Volume I: RISC-V
> > Unprivileged ISA V20191213" says, section A.5:
> >
> > "Table A.5 provides a mapping of Linux memory ordering macros onto
> > RISC-V memory instructions.
> > The Linux fences dma rmb() and dma wmb() map onto FENCE R,R and FENCE
> > W,W, respectively, since the RISC-V Unix Platform requires coherent
> > DMA, but would be mapped onto FENCE RI,RI and FENCE WO,WO,
> > respectively, on a platform with non-coherent DMA.
> > Platforms with non-
> > coherent DMA may also require a mechanism by which cache lines can be
> > flushed and/or invalidated.
> > Such mechanisms will be device-specific and/or standardized in a
> > future extension to the ISA."
> >
> > The (current date) RISCV Platform Spec also says: "Memory accesses by
> > I/O masters can be coherent or non-coherent with respect to all
> > hart-related caches."
> > which is brilliantly useless.
> >
> > so I think the best solution here is to:
> >
> > 1) Add a new PCD for platform DMA coherency, and test that on
> > WriteBackDataCacheRange's ASSERT (if (!Coherent) ASSERT() else
> > return;)
> > 2) Add a more abstracting API that doesn't necessarily map to
> > WriteBackDataCache when all we wanted was to assert DMA coherency
> >
> > but, alas, I've seen a lot less funky platforms than many of you, and
> > DMA/cache-coherency is not really my thing, so I'll defer to others..
> >
> My preference is just remove the assertion and add the debug verbose
> message instead of changing drivers/introduce new interfaces. It is a nop in
> linux as well if CMO is not present.
> 
> Thanks,
> Sunil


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

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

[Dhaval Sharma]

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

Based on the above discussion, and some more thoughts I am thinking it is
okay to at least replace ASSERT from CMO code and let other platform code
place its own guards to avoid calling this code when it is known that
platform does not support such operations. If there are no objections to
this we can go ahead.

On Tue, Jan 9, 2024 at 9:50 PM Warkentin, Andrei <andrei.warkentin@intel.com>
wrote:

> For now, this is really something that ought to be hidden by DmaLib
> abstraction (Map/Unmap). This would allow the driver to be minimally aware
> of how the IP is integrated into the SoC.
>
> A
>
> > -----Original Message-----
> > From: Sunil V L <sunilvl@ventanamicro.com>
> > Sent: Monday, January 8, 2024 11:32 PM
> > To: Pedro Falcato <pedro.falcato@gmail.com>
> > Cc: devel@edk2.groups.io; dhaval@rivosinc.com; yorange
> > <yangcheng.work@foxmail.com>; Warkentin, Andrei
> > <andrei.warkentin@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org
> >;
> > Leif Lindholm <quic_llindhol@quicinc.com>
> > Subject: Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache
> > Management Operations Implementation For RISC-V
> >
> > On Mon, Jan 08, 2024 at 09:53:46PM +0000, Pedro Falcato wrote:
> > > On Mon, Jan 8, 2024 at 4:23 PM Dhaval Sharma <dhaval@rivosinc.com>
> > wrote:
> > > >
> > > > Hi yangcheng/Pedro,
> > >
> > > +CC a bunch of relevant people
> > >
> > > Hi, (FYI you did not CC me)
> > >
> > > Looking at yangcheng's example:
> > >
> > >   Status = PrepareDmaData (gpIdmacDesc, Length, Buffer); <-- We write
> > > to the IDMAC desc
> > >   if (EFI_ERROR (Status)) {
> > >     goto out;
> > >   }
> > >
> > >   WriteBackDataCacheRange (gpIdmacDesc, DescPages * EFI_PAGE_SIZE);
> > > <-- Make sure it's DMA-coherent
> > >   StartDma (Length); <-- We've flushed the cache, everything is now in
> > > DRAM and DMA-coherent, start DMA
> > >
> > > which screams of "bad abstractions" because you don't actually need to
> > > write data back, if the device and platform are DMA coherent.
> > >
> > > So what we want here really depends. My local "Volume I: RISC-V
> > > Unprivileged ISA V20191213" says, section A.5:
> > >
> > > "Table A.5 provides a mapping of Linux memory ordering macros onto
> > > RISC-V memory instructions.
> > > The Linux fences dma rmb() and dma wmb() map onto FENCE R,R and FENCE
> > > W,W, respectively, since the RISC-V Unix Platform requires coherent
> > > DMA, but would be mapped onto FENCE RI,RI and FENCE WO,WO,
> > > respectively, on a platform with non-coherent DMA.
> > > Platforms with non-
> > > coherent DMA may also require a mechanism by which cache lines can be
> > > flushed and/or invalidated.
> > > Such mechanisms will be device-specific and/or standardized in a
> > > future extension to the ISA."
> > >
> > > The (current date) RISCV Platform Spec also says: "Memory accesses by
> > > I/O masters can be coherent or non-coherent with respect to all
> > > hart-related caches."
> > > which is brilliantly useless.
> > >
> > > so I think the best solution here is to:
> > >
> > > 1) Add a new PCD for platform DMA coherency, and test that on
> > > WriteBackDataCacheRange's ASSERT (if (!Coherent) ASSERT() else
> > > return;)
> > > 2) Add a more abstracting API that doesn't necessarily map to
> > > WriteBackDataCache when all we wanted was to assert DMA coherency
> > >
> > > but, alas, I've seen a lot less funky platforms than many of you, and
> > > DMA/cache-coherency is not really my thing, so I'll defer to others..
> > >
> > My preference is just remove the assertion and add the debug verbose
> > message instead of changing drivers/introduce new interfaces. It is a
> nop in
> > linux as well if CMO is not present.
> >
> > Thanks,
> > Sunil
>


-- 
Thanks!
=D


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



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