[edk2-devel] [PATCH v5 0/2] MdePkg:Implement RISCV CMO

[edk2-devel] [PATCH v5 0/2] MdePkg:Implement RISCV CMO

[Dhaval Sharma]

Implementing code to support Cache Management Operations (CMO) defined by RV spec https://github.com/riscv/riscv-CMOs

Notes:
CMO only supports block based Operations. Meaning complete cache flush/invd/clean Operations are not available. In that case we fallback on fence.i instructions.
Rely on the fact that platform init has initialized CMO and this implementation just checks if it is enabled.
In order to avoid compiler dependency injecting byte code. Code branch https://github.com/rivosinc/edk2/tree/dev_rv_cmo_v5

Test:
Ensured correct instructions are refelecting in asm
Able to boot platform with RiscVVirtQemu config
Not able to verify actual instruction in HW as Qemu ignores any actual cache operations.

Dhaval (2):
  MdePkg:Implement RISCV CMO
  OvmfPkg/RiscVVirt: Override for RV CPU Features

 MdePkg/MdePkg.dec                                                  |   7 +
 OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc                                |   2 +
 MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |   3 +-
 MdePkg/Library/BaseLib/BaseLib.inf                                 |   2 +-
 MdePkg/Include/Register/RiscV64/RiscVEncoding.h                    |   6 +
 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c                | 203 +++++++++++++++++---
 MdePkg/Library/BaseLib/RiscV64/FlushCache.S                        |  21 --
 MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S                    |  38 ++++
 8 files changed, 236 insertions(+), 46 deletions(-)
 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 (#109677): https://edk2.groups.io/g/devel/message/109677
Mute This Topic: https://groups.io/mt/102016147/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v5 1/2] MdePkg:Implement RISCV CMO

[Dhaval Sharma]

Implementing code to support Cache Management Operations
(CMO) defined by RV spec https://github.com/riscv/riscv-CMOs

Notes:
1. CMO only supports block based Operations. Meaning complete
   cache flush/invd/clean Operations are not available. In that case
   we fallback on fence.i instructions.
2. Rely on the fact that platform init has initialized CMO and this
   implementation just checks if it is enabled.
3. In order to avoid compiler dependency injecting byte code.

Test:
1. Ensured correct instructions are refelecting in asm
2. Able to boot platform with RiscVVirtQemu config
3. Not able to verify actual instruction in HW as Qemu ignores
any actual cache operations.

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>
Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
---

Notes:
    v5:
    - Addressed comments from v4
    - Use #defines instead of numbers in cache instruction encoding
    - Addressed function naming issues from previous patch
    - Added new PCD to override RV CPU features
    - Removed code that relied on ENVCFG registers
    - Fixing typos in comments

 MdePkg/MdePkg.dec                                                  |   7 +
 MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |   3 +-
 MdePkg/Library/BaseLib/BaseLib.inf                                 |   2 +-
 MdePkg/Include/Register/RiscV64/RiscVEncoding.h                    |   6 +
 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c                | 203 +++++++++++++++++---
 MdePkg/Library/BaseLib/RiscV64/FlushCache.S                        |  21 --
 MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S                    |  38 ++++
 7 files changed, 234 insertions(+), 46 deletions(-)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index ac54338089e8..2d06cf46b1ca 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2399,6 +2399,13 @@ [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 RV CPU Features
+  # BIT 0 = CMO
+  #
+  gEfiMdePkgTokenSpaceGuid.PcdRVFeatureOverride|0x1|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..037a0b49800a 100644
--- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
@@ -55,4 +55,5 @@ [Packages]
 [LibraryClasses]
   BaseLib
   DebugLib
-
+[Pcd]
+  gEfiMdePkgTokenSpaceGuid.PcdRVFeatureOverride
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/Register/RiscV64/RiscVEncoding.h b/MdePkg/Include/Register/RiscV64/RiscVEncoding.h
index 2bde8db478ff..5d6dcab12f74 100644
--- a/MdePkg/Include/Register/RiscV64/RiscVEncoding.h
+++ b/MdePkg/Include/Register/RiscV64/RiscVEncoding.h
@@ -117,4 +117,10 @@
 #define CAUSE_VIRTUAL_INST_FAULT        0x16
 #define CAUSE_STORE_GUEST_PAGE_FAULT    0x17
 
+#define CPU_FLUSH_CMO_ASM  0x0025200f
+
+#define CPU_CLEAN_CMO_ASM  0x0015200f
+
+#define CPU_INVLD_CMO_ASM  0x0005200f
+
 #endif
diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
index d08fb9f193ca..bd8794e1d818 100644
--- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
@@ -1,7 +1,8 @@
 /** @file
-  RISC-V specific functionality for cache.
+  Implement Risc-V Cache Management Operations
 
   Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
+  Copyright (c) 2023, Rivos Inc. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
@@ -9,6 +10,17 @@
 #include <Base.h>
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+
+// TODO: This will be removed once RISC-V CPU HOB is available
+#define RV64_CACHE_BLOCK_SIZE       64
+#define RV_CPU_FEATURE_CMO_BITMASK  0x1
+
+typedef enum {
+  Clean,
+  Flush,
+  Invld,
+} CACHE_OP;
 
 /**
   RISC-V invalidate instruction cache.
@@ -16,7 +28,7 @@
 **/
 VOID
 EFIAPI
-RiscVInvalidateInstCacheAsm (
+RiscVInvalidateInstCacheAsmFence (
   VOID
   );
 
@@ -26,13 +38,134 @@ RiscVInvalidateInstCacheAsm (
 **/
 VOID
 EFIAPI
-RiscVInvalidateDataCacheAsm (
+RiscVInvalidateDataCacheAsmFence (
   VOID
   );
 
+/**
+  RISC-V flush cache block. Atomically perform a clean operation
+  followed by an invalidate operation
+
+**/
+VOID
+EFIAPI
+RiscVCpuCacheFlushAsmCbo (
+  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
+RiscVCpuCacheCleanAsmCbo (
+  UINTN
+  );
+
+/**
+Deallocate the copy of the cache block
+
+**/
+VOID
+EFIAPI
+RiscVCpuCacheInvalAsmCbo (
+  UINTN
+  );
+
+/**
+Verify CBOs are supported by this HW
+TODO: Use RISC-V CPU HOB once available.
+
+**/
+UINT64
+RiscvIsCMOEnabled (
+  VOID
+  )
+{
+  // TODO: Add check for CMO from CPU HOB.
+  // If CMO is disabled in HW, skip Override check
+  // Otherwise this PCD can override settings
+  return (PcdGet64 (PcdRVFeatureOverride) & RV_CPU_FEATURE_CMO_BITMASK);
+}
+
+/**
+  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.
+
+**/
+VOID *
+EFIAPI
+CacheOpCacheRange (
+  IN VOID      *Address,
+  IN UINTN     Length,
+  IN CACHE_OP  Op
+  )
+{
+  UINTN  CacheLineSize;
+  UINTN  Start;
+  UINTN  End;
+
+  if (Length == 0) {
+    return Address;
+  }
+
+  ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)Address));
+
+  CacheLineSize = RV64_CACHE_BLOCK_SIZE;
+
+  Start = (UINTN)Address;
+  //
+  // Calculate the cache line alignment
+  //
+  End    = (Start + Length + (CacheLineSize - 1)) & ~(CacheLineSize - 1);
+  Start &= ~((UINTN)CacheLineSize - 1);
+
+  DEBUG (
+    (DEBUG_INFO,
+     "%a Performing Cache Management Operation %d \n", __func__, Op)
+    );
+
+  do {
+    switch (Op) {
+      case Invld:
+        RiscVCpuCacheInvalAsmCbo (Start);
+        break;
+      case Flush:
+        RiscVCpuCacheFlushAsmCbo (Start);
+        break;
+      case Clean:
+        RiscVCpuCacheCleanAsmCbo (Start);
+        break;
+      default:
+        DEBUG ((DEBUG_ERROR, "RISC-V unsupported operation\n"));
+        break;
+    }
+
+    Start = Start + CacheLineSize;
+  } while (Start != End);
+
+  return Address;
+}
+
 /**
   Invalidates the entire instruction cache in cache coherency domain of the
-  calling CPU.
+  calling CPU. Risc-V does not have currently an CBO implementation which can
+  invalidate entire I-cache. Hence using Fence instruction for now. P.S. Fence
+  instruction may or may not implement full I-cache invd functionality on all
+  implementations.
 
 **/
 VOID
@@ -41,7 +174,7 @@ InvalidateInstructionCache (
   VOID
   )
 {
-  RiscVInvalidateInstCacheAsm ();
+  RiscVInvalidateInstCacheAsmFence ();
 }
 
 /**
@@ -76,12 +209,17 @@ InvalidateInstructionCacheRange (
   IN UINTN  Length
   )
 {
-  DEBUG (
-    (DEBUG_WARN,
-     "%a:RISC-V unsupported function.\n"
-     "Invalidating the whole instruction cache instead.\n", __func__)
-    );
-  InvalidateInstructionCache ();
+  if (RiscvIsCMOEnabled () != 0) {
+    CacheOpCacheRange (Address, Length, Invld);
+  } else {
+    DEBUG (
+      (DEBUG_WARN,
+       "%a:RISC-V unsupported function.\n"
+       "Invalidating the whole instruction cache instead.\n", __func__)
+      );
+    InvalidateInstructionCache ();
+  }
+
   return Address;
 }
 
@@ -137,7 +275,12 @@ WriteBackInvalidateDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  if (RiscvIsCMOEnabled () != 0) {
+    CacheOpCacheRange (Address, Length, Flush);
+  } else {
+    DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  }
+
   return Address;
 }
 
@@ -176,10 +319,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.
@@ -192,7 +332,12 @@ WriteBackDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  if (RiscvIsCMOEnabled () != 0) {
+    CacheOpCacheRange (Address, Length, Clean);
+  } else {
+    DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  }
+
   return Address;
 }
 
@@ -213,7 +358,12 @@ InvalidateDataCache (
   VOID
   )
 {
-  RiscVInvalidateDataCacheAsm ();
+  DEBUG (
+    (DEBUG_WARN,
+     "%a:RISC-V unsupported function.\n"
+     "Invalidating the whole Data cache instead.\n", __func__)
+    );
+  RiscVInvalidateDataCacheAsmFence ();
 }
 
 /**
@@ -234,10 +384,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.
@@ -250,6 +397,16 @@ InvalidateDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  if (RiscvIsCMOEnabled () != 0) {
+    CacheOpCacheRange (Address, Length, Invld);
+  } else {
+    DEBUG (
+      (DEBUG_WARN,
+       "%a:RISC-V unsupported function.\n"
+       "Invalidating the whole Data cache instead.\n", __func__)
+      );
+    InvalidateDataCache ();
+  }
+
   return Address;
 }
diff --git a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S b/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
deleted file mode 100644
index 7c10fdd268af..000000000000
--- a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
+++ /dev/null
@@ -1,21 +0,0 @@
-//------------------------------------------------------------------------------
-//
-// RISC-V cache operation.
-//
-// Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
-//
-// SPDX-License-Identifier: BSD-2-Clause-Patent
-//
-//------------------------------------------------------------------------------
-
-.align 3
-ASM_GLOBAL ASM_PFX(RiscVInvalidateInstCacheAsm)
-ASM_GLOBAL ASM_PFX(RiscVInvalidateDataCacheAsm)
-
-ASM_PFX(RiscVInvalidateInstCacheAsm):
-    fence.i
-    ret
-
-ASM_PFX(RiscVInvalidateDataCacheAsm):
-    fence
-    ret
diff --git a/MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S b/MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S
new file mode 100644
index 000000000000..f9b79446b56a
--- /dev/null
+++ b/MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S
@@ -0,0 +1,38 @@
+//------------------------------------------------------------------------------
+//
+// RISC-V cache operation.
+//
+// Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
+// Copyright (c) 2022, Rivos Inc. All rights reserved.<BR>
+//
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+//------------------------------------------------------------------------------
+#include <Register/RiscV64/RiscVImpl.h>
+
+.align 3
+ASM_GLOBAL ASM_PFX(RiscVInvalidateInstCacheAsmFence)
+ASM_GLOBAL ASM_PFX(RiscVInvalidateDataCacheAsmFence)
+
+ASM_PFX(RiscVInvalidateInstCacheAsmFence):
+    fence.i
+    ret
+
+ASM_PFX(RiscVInvalidateDataCacheAsmFence):
+    fence
+    ret
+
+ASM_GLOBAL ASM_PFX (RiscVCpuCacheFlushAsmCbo)
+ASM_PFX (RiscVCpuCacheFlushAsmCbo):
+  .long CPU_FLUSH_CMO_ASM
+  ret
+
+ASM_GLOBAL ASM_PFX (RiscVCpuCacheCleanAsmCbo)
+ASM_PFX (RiscVCpuCacheCleanAsmCbo):
+  .long CPU_CLEAN_CMO_ASM
+  ret
+
+ASM_GLOBAL ASM_PFX (RiscVCpuCacheInvalAsmCbo)
+ASM_PFX (RiscVCpuCacheInvalAsmCbo):
+  .long CPU_INVLD_CMO_ASM
+  ret
-- 
2.39.2



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

[edk2-devel] [PATCH v5 2/2] 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.

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

Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
---
 OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
index fe320525153f..8b5e010316ba 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
+++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
@@ -203,6 +203,8 @@ [PcdsFeatureFlag]
   gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
 
 [PcdsFixedAtBuild.common]
+  gEfiMdePkgTokenSpaceGuid.PcdRVFeatureOverride|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 (#109679): https://edk2.groups.io/g/devel/message/109679
Mute This Topic: https://groups.io/mt/102016149/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v5 1/2] MdePkg:Implement RISCV CMO

[Laszlo Ersek]

On 10/17/23 14:17, Dhaval Sharma wrote:
> Implementing code to support Cache Management Operations
> (CMO) defined by RV spec https://github.com/riscv/riscv-CMOs
>
> Notes:
> 1. CMO only supports block based Operations. Meaning complete
>    cache flush/invd/clean Operations are not available. In that case
>    we fallback on fence.i instructions.
> 2. Rely on the fact that platform init has initialized CMO and this
>    implementation just checks if it is enabled.
> 3. In order to avoid compiler dependency injecting byte code.
>
> Test:
> 1. Ensured correct instructions are refelecting in asm
> 2. Able to boot platform with RiscVVirtQemu config
> 3. Not able to verify actual instruction in HW as Qemu ignores
> any actual cache operations.
>
> 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>
> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> ---
>
> Notes:
>     v5:
>     - Addressed comments from v4
>     - Use #defines instead of numbers in cache instruction encoding
>     - Addressed function naming issues from previous patch
>     - Added new PCD to override RV CPU features
>     - Removed code that relied on ENVCFG registers
>     - Fixing typos in comments
>
>  MdePkg/MdePkg.dec                                                  |   7 +
>  MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |   3 +-
>  MdePkg/Library/BaseLib/BaseLib.inf                                 |   2 +-
>  MdePkg/Include/Register/RiscV64/RiscVEncoding.h                    |   6 +
>  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c                | 203 +++++++++++++++++---
>  MdePkg/Library/BaseLib/RiscV64/FlushCache.S                        |  21 --
>  MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S                    |  38 ++++
>  7 files changed, 234 insertions(+), 46 deletions(-)

This is the first version of the series that I see, so I apologize in
advance if I touch on ground that's already been covered.

(1) Sorry, but this patch is a mess. It needs to be split into four
separate patches, in v6.

(1a) v6 patch#1:

I find that there is a preexistent problem, namely from the following,
earlier commits:

- 7601b251fd5c ("MdePkg/BaseLib: BaseLib for RISCV64 architecture",
2020-05-07)

- 38e72aa87725 ("MdePkg/BaseCacheMaintenanceLib: RISC-V cache
maintenance implementation.", 2020-05-07)

These commits were incorrectly structured. They added the assembly
language function definitions RiscVInvalidateInstCacheAsm() and
RiscVInvalidateDataCacheAsm() to BaseLib (which is fine). However, the
*declarations* for those functions didn't go into <BaseLib.h>, but were
buried in the library *instance* source file
"MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c".

Both of those functions should have been declared in <BaseLib.h>, inside
an

  #if defined (MDE_CPU_RISCV64)
  #endif

block.

Note that <BaseLib.h> is permitted (and supposed) to contain
processor-specific *function declarations*. It already contains a bunch
of such function declarations; one example is PatchInstructionX86().

Therefore, please correct this earlier mistake in v6 patch #1 -- move
the declarations of RiscVInvalidateInstCacheAsm() and
RiscVInvalidateDataCacheAsm() from
"MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c" to <BaseLib.h>,
into a MDE_CPU_RISCV64-dependent block.

(1b) v6 patch#2:

Renaming RiscVInvalidateDataCacheAsm() to
RiscVInvalidateDataCacheAsmFence(), and renaming
RiscVInvalidateInstCacheAsm() to RiscVInvalidateInstCacheAsmFence(),
should be isolated to v6 patch#2.

Said patch should contian *nothing else* but the rename -- plus any
comment additions that relate to the new (more exact) function names.
The tree must compile both before and after the patch.

(1c) v6 patch#3:

Adding the new cache maintenance operations to BaseLib, including the
new assembly instruction encodings.

This patch should contain the *file rename* as well (FlushCache.S ->
RiscVCacheMgmt.S), because the new operations are what generalize the
file from just flushing to management.

(1d) v6 patch#4:

Updating BaseCacheMaintenanceLib (utilizing the new BaseLib primitives).

More comments below:

> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index ac54338089e8..2d06cf46b1ca 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2399,6 +2399,13 @@ [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 RV CPU Features
> +  # BIT 0 = CMO
> +  #
> +  gEfiMdePkgTokenSpaceGuid.PcdRVFeatureOverride|0x1|UINT64|0x69
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    ## This value is used to set the base address of PCI express hierarchy.
>    # @Prompt PCI Express Base Address.

(2) This belongs to v6 patch#4, because only BaseCacheMaintenanceLib
needs the PCD.

(3) "CMO" should be expanded as "cache management operations".

(4) The whole PCD is insufficiently documented. This comment should
include the documentation from the commit message of *v5* patch#2 (i.e.,
that any bit that is clear in this bitmask is supposed to clear the
feature configuration inherited from earlier components such as OpenSBI,
but any bit set will not re-enable, only preserve, previously enabled
features.)

(5) Accordingly, the default value of the PCD should be
0xFFFFFFFFFFFFFFFF (all bits one -- "inherit everything"), arguably.

(6) The "MdePkg/MdePkg.uni" file should be kept in sync with
"MdePkg/MdePkg.dec"; any PCD should be documented in both.


> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> index 6fd9cbe5f6c9..037a0b49800a 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> @@ -55,4 +55,5 @@ [Packages]
>  [LibraryClasses]
>    BaseLib
>    DebugLib
> -
> +[Pcd]
> +  gEfiMdePkgTokenSpaceGuid.PcdRVFeatureOverride

(7) Belongs to v6 patch#4.

(8) Please consider appending the "## CONSUMES" hint.


> 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

(9) Belongs to v6 patch#3.


> diff --git a/MdePkg/Include/Register/RiscV64/RiscVEncoding.h b/MdePkg/Include/Register/RiscV64/RiscVEncoding.h
> index 2bde8db478ff..5d6dcab12f74 100644
> --- a/MdePkg/Include/Register/RiscV64/RiscVEncoding.h
> +++ b/MdePkg/Include/Register/RiscV64/RiscVEncoding.h
> @@ -117,4 +117,10 @@
>  #define CAUSE_VIRTUAL_INST_FAULT        0x16
>  #define CAUSE_STORE_GUEST_PAGE_FAULT    0x17
>
> +#define CPU_FLUSH_CMO_ASM  0x0025200f
> +
> +#define CPU_CLEAN_CMO_ASM  0x0015200f
> +
> +#define CPU_INVLD_CMO_ASM  0x0005200f
> +
>  #endif

(10) Belongs to v6 patch#3.

(11) I agree that we should use symbolic names rather than magic
constants, but raw encodings of machine instructions don't belong into a
C header file.

Instead, please refer to the <MdePkg/Include/X64/Nasm.inc> file as an
example; see the PVALIDATE and RMPADJUST instruction encodings. We
should follow the same pattern with these RISC-V instructions too, if
possible, even if we don't use NASM for building RISC-V assembly code.
(So just call the *.inc file something else.)

(12) Also, filing a feature request (about these instructions) for the
GNU Assembler, and pasting the URLs into the new assembly include file
(following the PVALIDATE example above) would be welcome.


> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> index d08fb9f193ca..bd8794e1d818 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> @@ -1,7 +1,8 @@
>  /** @file
> -  RISC-V specific functionality for cache.
> +  Implement Risc-V Cache Management Operations
>
>    Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +  Copyright (c) 2023, Rivos Inc. All rights reserved.<BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
> @@ -9,6 +10,17 @@
>  #include <Base.h>
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +
> +// TODO: This will be removed once RISC-V CPU HOB is available
> +#define RV64_CACHE_BLOCK_SIZE       64
> +#define RV_CPU_FEATURE_CMO_BITMASK  0x1
> +
> +typedef enum {
> +  Clean,
> +  Flush,
> +  Invld,
> +} CACHE_OP;
>
>  /**
>    RISC-V invalidate instruction cache.
> @@ -16,7 +28,7 @@
>  **/
>  VOID
>  EFIAPI
> -RiscVInvalidateInstCacheAsm (
> +RiscVInvalidateInstCacheAsmFence (
>    VOID
>    );
>
> @@ -26,13 +38,134 @@ RiscVInvalidateInstCacheAsm (
>  **/
>  VOID
>  EFIAPI
> -RiscVInvalidateDataCacheAsm (
> +RiscVInvalidateDataCacheAsmFence (
>    VOID
>    );
>

(13) As stated above, these two interfaces don't belong here. In v6
patch#1, they should be moved to <BaseLib.h>, and in v6 patch#2, they
should be renamed.

> +/**
> +  RISC-V flush cache block. Atomically perform a clean operation
> +  followed by an invalidate operation
> +
> +**/
> +VOID
> +EFIAPI
> +RiscVCpuCacheFlushAsmCbo (
> +  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
> +RiscVCpuCacheCleanAsmCbo (
> +  UINTN
> +  );
> +
> +/**
> +Deallocate the copy of the cache block
> +
> +**/
> +VOID
> +EFIAPI
> +RiscVCpuCacheInvalAsmCbo (
> +  UINTN
> +  );
> +

(14) As stated above, these function declarations don't belong here.
They should be introduced in v6 patch#3 to BaseLib. (And the
BaseCacheMaintenanceLib additions belong in v6 patch#4.)


> +/**
> +Verify CBOs are supported by this HW
> +TODO: Use RISC-V CPU HOB once available.
> +
> +**/


(15) I believe this un-indented comment will not pass ECC Check /
uncrustify. Did you submit a pull request just for triggering CI?


> +UINT64
> +RiscvIsCMOEnabled (
> +  VOID
> +  )
> +{
> +  // TODO: Add check for CMO from CPU HOB.
> +  // If CMO is disabled in HW, skip Override check
> +  // Otherwise this PCD can override settings
> +  return (PcdGet64 (PcdRVFeatureOverride) & RV_CPU_FEATURE_CMO_BITMASK);
> +}

(16) The name of the function suggests the return type should be
BOOLEAN.

(17) Consequently, the comparison against zero should be performed here,
not at the call sites.

(18) IIUC, this function should be STATIC. It's not a public library
interface.


> +
> +/**
> +  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.
> +
> +**/
> +VOID *
> +EFIAPI
> +CacheOpCacheRange (

(19) Should be STATIC, and should *not* be EFIAPI. (Not a public
interface.)


> +  IN VOID      *Address,
> +  IN UINTN     Length,
> +  IN CACHE_OP  Op
> +  )
> +{
> +  UINTN  CacheLineSize;
> +  UINTN  Start;
> +  UINTN  End;
> +
> +  if (Length == 0) {
> +    return Address;
> +  }
> +
> +  ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)Address));
> +
> +  CacheLineSize = RV64_CACHE_BLOCK_SIZE;
> +
> +  Start = (UINTN)Address;
> +  //
> +  // Calculate the cache line alignment
> +  //
> +  End    = (Start + Length + (CacheLineSize - 1)) & ~(CacheLineSize - 1);
> +  Start &= ~((UINTN)CacheLineSize - 1);
> +
> +  DEBUG (
> +    (DEBUG_INFO,
> +     "%a Performing Cache Management Operation %d \n", __func__, Op)
> +    );

(20) This will definitely not pass uncrustify.


> +
> +  do {
> +    switch (Op) {
> +      case Invld:
> +        RiscVCpuCacheInvalAsmCbo (Start);
> +        break;
> +      case Flush:
> +        RiscVCpuCacheFlushAsmCbo (Start);
> +        break;
> +      case Clean:
> +        RiscVCpuCacheCleanAsmCbo (Start);
> +        break;
> +      default:
> +        DEBUG ((DEBUG_ERROR, "RISC-V unsupported operation\n"));
> +        break;

(21) Logging this error for every cache line of the requested range does
not seem useful. I suggest checking Op before the loop.


> +    }
> +
> +    Start = Start + CacheLineSize;
> +  } while (Start != End);
> +
> +  return Address;
> +}
> +
>  /**
>    Invalidates the entire instruction cache in cache coherency domain of the
> -  calling CPU.
> +  calling CPU. Risc-V does not have currently an CBO implementation which can
> +  invalidate entire I-cache. Hence using Fence instruction for now. P.S. Fence
> +  instruction may or may not implement full I-cache invd functionality on all
> +  implementations.
>
>  **/
>  VOID
> @@ -41,7 +174,7 @@ InvalidateInstructionCache (
>    VOID
>    )
>  {
> -  RiscVInvalidateInstCacheAsm ();
> +  RiscVInvalidateInstCacheAsmFence ();
>  }
>
>  /**

(22) As stated above, the API renames -- together with the updated
leading comments -- belong in v6 patch#2.


> @@ -76,12 +209,17 @@ InvalidateInstructionCacheRange (
>    IN UINTN  Length
>    )
>  {
> -  DEBUG (
> -    (DEBUG_WARN,
> -     "%a:RISC-V unsupported function.\n"
> -     "Invalidating the whole instruction cache instead.\n", __func__)
> -    );
> -  InvalidateInstructionCache ();
> +  if (RiscvIsCMOEnabled () != 0) {
> +    CacheOpCacheRange (Address, Length, Invld);
> +  } else {
> +    DEBUG (
> +      (DEBUG_WARN,
> +       "%a:RISC-V unsupported function.\n"
> +       "Invalidating the whole instruction cache instead.\n", __func__)
> +      );
> +    InvalidateInstructionCache ();
> +  }
> +
>    return Address;
>  }
>
> @@ -137,7 +275,12 @@ WriteBackInvalidateDataCacheRange (
>    IN      UINTN  Length
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  if (RiscvIsCMOEnabled () != 0) {
> +    CacheOpCacheRange (Address, Length, Flush);
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  }
> +
>    return Address;
>  }
>
> @@ -176,10 +319,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.
> @@ -192,7 +332,12 @@ WriteBackDataCacheRange (
>    IN      UINTN  Length
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  if (RiscvIsCMOEnabled () != 0) {
> +    CacheOpCacheRange (Address, Length, Clean);
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  }
> +
>    return Address;
>  }
>
> @@ -213,7 +358,12 @@ InvalidateDataCache (
>    VOID
>    )
>  {
> -  RiscVInvalidateDataCacheAsm ();
> +  DEBUG (
> +    (DEBUG_WARN,
> +     "%a:RISC-V unsupported function.\n"
> +     "Invalidating the whole Data cache instead.\n", __func__)
> +    );
> +  RiscVInvalidateDataCacheAsmFence ();
>  }
>
>  /**

(23) As stated above, the API renames -- together with the updated
leading comments -- belong in v6 patch#2.

(24) The DEBUG message seems bogus; invalidating the whole I-Cache *is*
what is being requested here.


> @@ -234,10 +384,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.
> @@ -250,6 +397,16 @@ InvalidateDataCacheRange (
>    IN      UINTN  Length
>    )
>  {
> -  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> +  if (RiscvIsCMOEnabled () != 0) {
> +    CacheOpCacheRange (Address, Length, Invld);
> +  } else {
> +    DEBUG (
> +      (DEBUG_WARN,
> +       "%a:RISC-V unsupported function.\n"
> +       "Invalidating the whole Data cache instead.\n", __func__)
> +      );
> +    InvalidateDataCache ();
> +  }
> +
>    return Address;
>  }
> diff --git a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S b/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
> deleted file mode 100644
> index 7c10fdd268af..000000000000
> --- a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -//------------------------------------------------------------------------------
> -//
> -// RISC-V cache operation.
> -//
> -// Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> -//
> -// SPDX-License-Identifier: BSD-2-Clause-Patent
> -//
> -//------------------------------------------------------------------------------
> -
> -.align 3
> -ASM_GLOBAL ASM_PFX(RiscVInvalidateInstCacheAsm)
> -ASM_GLOBAL ASM_PFX(RiscVInvalidateDataCacheAsm)
> -
> -ASM_PFX(RiscVInvalidateInstCacheAsm):
> -    fence.i
> -    ret
> -
> -ASM_PFX(RiscVInvalidateDataCacheAsm):
> -    fence
> -    ret
> diff --git a/MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S b/MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S
> new file mode 100644
> index 000000000000..f9b79446b56a
> --- /dev/null
> +++ b/MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S
> @@ -0,0 +1,38 @@
> +//------------------------------------------------------------------------------
> +//
> +// RISC-V cache operation.
> +//
> +// Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +// Copyright (c) 2022, Rivos Inc. All rights reserved.<BR>
> +//
> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +//------------------------------------------------------------------------------
> +#include <Register/RiscV64/RiscVImpl.h>
> +
> +.align 3
> +ASM_GLOBAL ASM_PFX(RiscVInvalidateInstCacheAsmFence)
> +ASM_GLOBAL ASM_PFX(RiscVInvalidateDataCacheAsmFence)
> +
> +ASM_PFX(RiscVInvalidateInstCacheAsmFence):
> +    fence.i
> +    ret
> +
> +ASM_PFX(RiscVInvalidateDataCacheAsmFence):
> +    fence
> +    ret
> +
> +ASM_GLOBAL ASM_PFX (RiscVCpuCacheFlushAsmCbo)
> +ASM_PFX (RiscVCpuCacheFlushAsmCbo):
> +  .long CPU_FLUSH_CMO_ASM
> +  ret
> +
> +ASM_GLOBAL ASM_PFX (RiscVCpuCacheCleanAsmCbo)
> +ASM_PFX (RiscVCpuCacheCleanAsmCbo):
> +  .long CPU_CLEAN_CMO_ASM
> +  ret
> +
> +ASM_GLOBAL ASM_PFX (RiscVCpuCacheInvalAsmCbo)
> +ASM_PFX (RiscVCpuCacheInvalAsmCbo):
> +  .long CPU_INVLD_CMO_ASM
> +  ret

(25) The *API* renames belong to v6 patch#2.

(26) The new APIs, plus the *file* rename, belong to v6 patch#3.

(27) Please use the assembler macros from point (11) -- the macros also
belong to v6 patch#3.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109686): https://edk2.groups.io/g/devel/message/109686
Mute This Topic: https://groups.io/mt/102016148/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 v5 2/2] OvmfPkg/RiscVVirt: Override for RV CPU Features

[Laszlo Ersek]

On 10/17/23 14:17, Dhaval Sharma 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.
> 
> Ard Biesheuvel <ardb+tianocore@kernel.org>
> Jiewen Yao <jiewen.yao@intel.com>
> Jordan Justen <jordan.l.justen@intel.com>
> Gerd Hoffmann <kraxel@redhat.com>
> Sunil V L <sunilvl@ventanamicro.com>
> Andrei Warkentin <andrei.warkentin@intel.com>
> 
> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> ---
>  OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> index fe320525153f..8b5e010316ba 100644
> --- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> +++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> @@ -203,6 +203,8 @@ [PcdsFeatureFlag]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
>  
>  [PcdsFixedAtBuild.common]
> +  gEfiMdePkgTokenSpaceGuid.PcdRVFeatureOverride|0
> +
>    gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|1000000
>    gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|1000000
>    gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0

(1) This patch should be v6 patch#5.

(2) The commit message belongs *as documentation* into v6 patch#4 -- the
BaseCacheMaintenanceLib update.

(3) The commit message on *this* patch (i.e., v6 patch#5) should explain
*why* we want to disable the cache management operations on RiscVVirt.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109687): https://edk2.groups.io/g/devel/message/109687
Mute This Topic: https://groups.io/mt/102016149/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 v5 1/2] MdePkg:Implement RISCV CMO

[Laszlo Ersek]

On 10/17/23 16:22, Laszlo Ersek wrote:
> On 10/17/23 14:17, Dhaval Sharma wrote:
>> Implementing code to support Cache Management Operations
>> (CMO) defined by RV spec https://github.com/riscv/riscv-CMOs

(28) Please do not abbreviate RISC-V as "RV". It's incredibly confusing.

(29) Inconsistent spelling in the patch subject: "RISCV CMO". Should be

  RISC-V cache management operations

>> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
>> index ac54338089e8..2d06cf46b1ca 100644
>> --- a/MdePkg/MdePkg.dec
>> +++ b/MdePkg/MdePkg.dec
>> @@ -2399,6 +2399,13 @@ [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 RV CPU Features

(30) ditto; should be RISC-V

>> +  # BIT 0 = CMO
>> +  #
>> +  gEfiMdePkgTokenSpaceGuid.PcdRVFeatureOverride|0x1|UINT64|0x69

(31) ditto, should be PcdRiscVFeatureOverride

>> +#define RV64_CACHE_BLOCK_SIZE       64
>> +#define RV_CPU_FEATURE_CMO_BITMASK  0x1

(32) Total inconsistency, RV64_ versus RV_.

Edk2 only supports RISCV64, so "64" in the prefix is meaningless. Both
prefixes should be RISCV_.

>> +RiscvIsCMOEnabled (

(33) Should be RiscVIsCMOEnabled (upper case V).

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109688): https://edk2.groups.io/g/devel/message/109688
Mute This Topic: https://groups.io/mt/102016148/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 v5 2/2] OvmfPkg/RiscVVirt: Override for RV CPU Features

[Laszlo Ersek]

On 10/17/23 14:17, Dhaval Sharma 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.
> 
> Ard Biesheuvel <ardb+tianocore@kernel.org>
> Jiewen Yao <jiewen.yao@intel.com>
> Jordan Justen <jordan.l.justen@intel.com>
> Gerd Hoffmann <kraxel@redhat.com>
> Sunil V L <sunilvl@ventanamicro.com>
> Andrei Warkentin <andrei.warkentin@intel.com>

(4) You forgot to prepend "Cc:".

(5) The cover letter (0/2 email here) should contain all the Cc: tags
from the patches' commit messages, so that whoever gets at least one
patch CC'd from the series also get the cover letter for the series.

Thanks
Laszlo

> 
> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> ---
>  OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> index fe320525153f..8b5e010316ba 100644
> --- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> +++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> @@ -203,6 +203,8 @@ [PcdsFeatureFlag]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
>  
>  [PcdsFixedAtBuild.common]
> +  gEfiMdePkgTokenSpaceGuid.PcdRVFeatureOverride|0
> +
>    gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|1000000
>    gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|1000000
>    gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109689): https://edk2.groups.io/g/devel/message/109689
Mute This Topic: https://groups.io/mt/102016149/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 v5 2/2] OvmfPkg/RiscVVirt: Override for RV CPU Features

[Dhaval Sharma]

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

Hi Laszlo,
Thanks a lot for your feedback. I have modified my next patchset addressing
most of the comments. Summary below. But *before I submit the final
version* I wanted to seek clarification on a few things mentioned below
with [Dhaval]. Current PR I am planning to submit:
https://github.com/tianocore/edk2/pull/4928

I am summarizing all comments for better readability:
(1) Split into four separate patches, in v6.                       *Done*.-
with some comments below
    1a. Fix previous error from earlier patch that had declaration outside
baselib.h
    1b. Renaming RiscVInvalidateDataCacheAsm() to
RiscVInvalidateDataCacheAsmFence() etc.
    1c. Adding the new cache maintenance operations to BaseLib, including
the
          new assembly instruction encodings.
    1d. Updating BaseCacheMaintenanceLib (utilizing the new BaseLib
primitives).
    1e. I have added another one for RiscvVirt platform kind of an override
as 5th patch.
(2)  This belongs to v6 patch#4, because only BaseCacheMaintenanceLib needs
the PCD.        *Done*
(3)  "CMO" should be expanded as "cache management operations".       *Done*
(4)  The whole PCD is insufficiently documented.      *Done*
(5)  Accordingly, the default value of the PCD should be
0xFFFFFFFFFFFFFFFF.  *Done*
(6)  The "MdePkg/MdePkg.uni" file should be kept in sync with dec.
*Done*.  [Dhaval] Is this used beyond setup options? For some PCDs I do not
find an entry in uni.
(7)  Belongs to v6 patch#4. *Done*
(8)  Please consider appending the "## CONSUMES" hint. *Done*
(9)  Belongs to v6 patch#3. *Done*
(10) Belongs to v6 patch#3. *Done*
(11) I agree that we should use symbolic names rather than magic constants,
but raw encodings of machine instructions don't belong into a
     C header file. [Dhaval] This bytecode was introduced thinking what if
all compilers do not support it. but given the default compiler in edk2 GCC
12 supports it
     we can eliminate this byte encoding completely to make it easy and
simple to consume for others.
(12) Also, filing a feature request (about these instructions). As per (11)
it is already available.
(13) As stated above, these two interfaces don't belong here. *Done*
(14) As stated above, these function declarations don't belong here. *Done*
(15) I believe this un-indented comment will not pass ECC Check /
     uncrustify. [Dhaval] I attach my stuart_build logs. I do not see
specific errors.https://github.com/tianocore/edk2/pull/4928 Pull request
all passed. Am I missing something?
(16-17-18) The name of the function suggests the return type should be
BOOLEAN. *Done*
(19) Should be STATIC, and should *not* be EFIAPI. (Not a public
interface.) *Done*
(20) This will definitely not pass uncrustify. If you are talking about bad
indent seen on Ops and Length- it is fixed. *Done*
(21) Logging this error for every cache line of the requested range does
not seem useful. I suggest checking Op before the loop. *Done*
(22) As stated above, the API renames -- together with the updated leading
comments -- belong in v6 patch#2. *Done*
(23) As stated above, the API renames -- together with the updated leading
comments -- belong in v6 patch#2. *Done*
(24) The DEBUG message seems bogus; invalidating the whole I-Cache *is*
what is being requested here. *Done*
(25-26) The *API* renames belong to v6 patch#2. & The new APIs, plus the
*file* rename, belong to v6 patch#3. *Done*
(27) Please use the assembler macros from point (11). Please see (11)
(28-29-30-31) Please do not abbreviate RISC-V as "RV". It's incredibly
confusing.  Inconsistent spelling in the patch subject: "RISCV CMO". ditto;
should be RISC-V.ditto, should be PcdRiscVFeatureOverride *Done*
(32-33) Total inconsistency, RV64_ versus RV_.  Should be RiscVIsCMOEnabled
(upper case V). *Done*

On Tue, Oct 17, 2023 at 8:09 PM Laszlo Ersek <lersek@redhat.com> wrote:

> On 10/17/23 14:17, Dhaval Sharma 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.
> >
> > Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Jiewen Yao <jiewen.yao@intel.com>
> > Jordan Justen <jordan.l.justen@intel.com>
> > Gerd Hoffmann <kraxel@redhat.com>
> > Sunil V L <sunilvl@ventanamicro.com>
> > Andrei Warkentin <andrei.warkentin@intel.com>
>
> (4) You forgot to prepend "Cc:".
>
> (5) The cover letter (0/2 email here) should contain all the Cc: tags
> from the patches' commit messages, so that whoever gets at least one
> patch CC'd from the series also get the cover letter for the series.
>
> Thanks
> Laszlo
>
> >
> > Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> > ---
> >  OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> > index fe320525153f..8b5e010316ba 100644
> > --- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> > +++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> > @@ -203,6 +203,8 @@ [PcdsFeatureFlag]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
> >
> >  [PcdsFixedAtBuild.common]
> > +  gEfiMdePkgTokenSpaceGuid.PcdRVFeatureOverride|0
> > +
> >    gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|1000000
> >    gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|1000000
> >    gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0
>
>

-- 
Thanks!
=D


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

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

[Laszlo Ersek]

On 10/19/23 08:48, Dhaval Sharma wrote:
> Hi Laszlo,
> Thanks a lot for your feedback. I have modified my next patchset
> addressing most of the comments. Summary below. But *before I submit the
> final version* I wanted to seek clarification on a few things mentioned
> below with [Dhaval]. Current PR I am planning to
> submit: https://github.com/tianocore/edk2/pull/4928
> <https://github.com/tianocore/edk2/pull/4928>
> 
> I am summarizing all comments for better readability:
> (1) Split into four separate patches, in v6.                     
>  *Done*.- with some comments below
>     1a. Fix previous error from earlier patch that had declaration
> outside baselib.h
>     1b. Renaming RiscVInvalidateDataCacheAsm() to
> RiscVInvalidateDataCacheAsmFence() etc.
>     1c. Adding the new cache maintenance operations to BaseLib,
> including the
>           new assembly instruction encodings.
>     1d. Updating BaseCacheMaintenanceLib (utilizing the new BaseLib
> primitives).
>     1e. I have added another one for RiscvVirt platform kind of an
> override as 5th patch.
> (2)  This belongs to v6 patch#4, because only BaseCacheMaintenanceLib
> needs the PCD.        *Done*
> (3)  "CMO" should be expanded as "cache management operations".       *Done*
> (4)  The whole PCD is insufficiently documented.      *Done*
> (5)  Accordingly, the default value of the PCD should be
> 0xFFFFFFFFFFFFFFFF.  *Done*
> (6)  The "MdePkg/MdePkg.uni" file should be kept in sync with dec.   
> *Done*.  [Dhaval] Is this used beyond setup options? For some PCDs I do
> not find an entry in uni.

This is best cleared with the MdePkg maintainers. Some packages don't
include *.uni files at all, but in those that do, my memories are that
we always keep the PCDs in sync between *.dec and *.uni. If I remember
correctly, the *.uni files are also used by the UEFI Packaging Tool
(UPT). I recommend modifying the UNI as well.

> (7)  Belongs to v6 patch#4. *Done*     
> (8)  Please consider appending the "## CONSUMES" hint. *Done*
> (9)  Belongs to v6 patch#3. *Done*
> (10) Belongs to v6 patch#3. *Done*
> (11) I agree that we should use symbolic names rather than
> magic constants, but raw encodings of machine instructions don't belong
> into a
>      C header file. [Dhaval] This bytecode was introduced thinking what
> if all compilers do not support it. but given the default compiler in
> edk2 GCC 12 supports it
>      we can eliminate this byte encoding completely to make it easy and
> simple to consume for others.

To be honest, I can't determine the minimum expected gcc version for
edk2. "BaseTools/Conf/tools_def.template" states a minimum version for
NASM, for example, but I can't find a similar gcc requirement there.

gcc-12 does work for me personally, because my riscv cross-compiler is
"riscv64-linux-gnu-gcc (GCC) 12.1.1 20220507 (Red Hat Cross 12.1.1-1)".

If the CI environment that builds these patches also provides gcc-12+,
then I figure you should be set.

> (12) Also, filing a feature request (about these instructions). As per
> (11) it is already available.
> (13) As stated above, these two interfaces don't belong here. *Done*
> (14) As stated above, these function declarations don't belong here. *Done*
> (15) I believe this un-indented comment will not pass ECC Check /
>      uncrustify. [Dhaval] I attach my stuart_build logs. I do not see
> specific errors.https://github.com/tianocore/edk2/pull/4928
> <https://github.com/tianocore/edk2/pull/4928> Pull request all passed.
> Am I missing something?

Probably not.

We usually indent the function-top comment bodies by two spaces, but if
neither uncrustify nor ECC complain, then I won't insist.

Thanks
Laszlo

> (16-17-18) The name of the function suggests the return type should be
> BOOLEAN. *Done*
> (19) Should be STATIC, and should *not* be EFIAPI. (Not a public
> interface.) *Done*
> (20) This will definitely not pass uncrustify. If you are talking about
> bad indent seen on Ops and Length- it is fixed. *Done*
> (21) Logging this error for every cache line of the requested range does
> not seem useful. I suggest checking Op before the loop. *Done*
> (22) As stated above, the API renames -- together with the updated
> leading comments -- belong in v6 patch#2. *Done*
> (23) As stated above, the API renames -- together with the updated
> leading comments -- belong in v6 patch#2. *Done*
> (24) The DEBUG message seems bogus; invalidating the whole I-Cache *is*
> what is being requested here. *Done*
> (25-26) The *API* renames belong to v6 patch#2. & The new APIs, plus the
> *file* rename, belong to v6 patch#3. *Done*
> (27) Please use the assembler macros from point (11). Please see (11)
> (28-29-30-31) Please do not abbreviate RISC-V as "RV". It's incredibly
> confusing.  Inconsistent spelling in the patch subject: "RISCV CMO".
> ditto; should be RISC-V.ditto, should be PcdRiscVFeatureOverride *Done*
> (32-33) Total inconsistency, RV64_ versus RV_.  Should be
> RiscVIsCMOEnabled (upper case V). *Done*
> 
> On Tue, Oct 17, 2023 at 8:09 PM Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>> wrote:
> 
>     On 10/17/23 14:17, Dhaval Sharma 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.
>     >
>     > Ard Biesheuvel <ardb+tianocore@kernel.org
>     <mailto:ardb%2Btianocore@kernel.org>>
>     > Jiewen Yao <jiewen.yao@intel.com <mailto:jiewen.yao@intel.com>>
>     > Jordan Justen <jordan.l.justen@intel.com
>     <mailto:jordan.l.justen@intel.com>>
>     > Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>>
>     > Sunil V L <sunilvl@ventanamicro.com <mailto:sunilvl@ventanamicro.com>>
>     > Andrei Warkentin <andrei.warkentin@intel.com
>     <mailto:andrei.warkentin@intel.com>>
> 
>     (4) You forgot to prepend "Cc:".
> 
>     (5) The cover letter (0/2 email here) should contain all the Cc: tags
>     from the patches' commit messages, so that whoever gets at least one
>     patch CC'd from the series also get the cover letter for the series.
> 
>     Thanks
>     Laszlo
> 
>     >
>     > Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com
>     <mailto:dhaval@rivosinc.com>>
>     > ---
>     >  OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 2 ++
>     >  1 file changed, 2 insertions(+)
>     >
>     > diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
>     b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
>     > index fe320525153f..8b5e010316ba 100644
>     > --- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
>     > +++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
>     > @@ -203,6 +203,8 @@ [PcdsFeatureFlag]
>     >    gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
>     > 
>     >  [PcdsFixedAtBuild.common]
>     > +  gEfiMdePkgTokenSpaceGuid.PcdRVFeatureOverride|0
>     > +
>     >    gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|1000000
>     >    gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|1000000
>     >    gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0
> 
> 
> 
> -- 
> Thanks!
> =D



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109797): https://edk2.groups.io/g/devel/message/109797
Mute This Topic: https://groups.io/mt/102016149/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 v5 2/2] OvmfPkg/RiscVVirt: Override for RV CPU Features

[Laszlo Ersek]

On 10/19/23 11:22, Laszlo Ersek wrote:
> On 10/19/23 08:48, Dhaval Sharma wrote:

>> (11) I agree that we should use symbolic names rather than
>> magic constants, but raw encodings of machine instructions don't belong
>> into a
>>      C header file. [Dhaval] This bytecode was introduced thinking what
>> if all compilers do not support it. but given the default compiler in
>> edk2 GCC 12 supports it
>>      we can eliminate this byte encoding completely to make it easy and
>> simple to consume for others.
> 
> To be honest, I can't determine the minimum expected gcc version for
> edk2. "BaseTools/Conf/tools_def.template" states a minimum version for
> NASM, for example, but I can't find a similar gcc requirement there.
> 
> gcc-12 does work for me personally, because my riscv cross-compiler is
> "riscv64-linux-gnu-gcc (GCC) 12.1.1 20220507 (Red Hat Cross 12.1.1-1)".
> 
> If the CI environment that builds these patches also provides gcc-12+,
> then I figure you should be set.

Wait, for the assembly language source files, what matters is the
binutils version, not the gcc version. Mine is "GNU assembler version
2.38-3.el9" (from "binutils-riscv64-linux-gnu-2.38-3.el9.x86_64").

Is that sufficient for the instuctions in question?

(More generally -- what version does our CI env expect / provide?)

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109802): https://edk2.groups.io/g/devel/message/109802
Mute This Topic: https://groups.io/mt/102016149/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 v5 2/2] OvmfPkg/RiscVVirt: Override for RV CPU Features

[Dhaval Sharma]

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

It was introduced in 2.39 it seems. GCC 12 onwards contains this binutils
version as per my understanding. This version was released quite long back.
I can double check by submitting it through edk2 CI to ensure it works.
Current CI version is already GCC 12.

On Thu, Oct 19, 2023 at 5:47 PM Laszlo Ersek <lersek@redhat.com> wrote:

> On 10/19/23 11:22, Laszlo Ersek wrote:
> > On 10/19/23 08:48, Dhaval Sharma wrote:
>
> >> (11) I agree that we should use symbolic names rather than
> >> magic constants, but raw encodings of machine instructions don't belong
> >> into a
> >>      C header file. [Dhaval] This bytecode was introduced thinking what
> >> if all compilers do not support it. but given the default compiler in
> >> edk2 GCC 12 supports it
> >>      we can eliminate this byte encoding completely to make it easy and
> >> simple to consume for others.
> >
> > To be honest, I can't determine the minimum expected gcc version for
> > edk2. "BaseTools/Conf/tools_def.template" states a minimum version for
> > NASM, for example, but I can't find a similar gcc requirement there.
> >
> > gcc-12 does work for me personally, because my riscv cross-compiler is
> > "riscv64-linux-gnu-gcc (GCC) 12.1.1 20220507 (Red Hat Cross 12.1.1-1)".
> >
> > If the CI environment that builds these patches also provides gcc-12+,
> > then I figure you should be set.
>
> Wait, for the assembly language source files, what matters is the
> binutils version, not the gcc version. Mine is "GNU assembler version
> 2.38-3.el9" (from "binutils-riscv64-linux-gnu-2.38-3.el9.x86_64").
>
> Is that sufficient for the instuctions in question?
>
> (More generally -- what version does our CI env expect / provide?)
>
> Thanks
> Laszlo
>
>

-- 
Thanks!
=D


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

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

[Laszlo Ersek]

On 10/19/23 16:37, Dhaval Sharma wrote:
> It was introduced in 2.39 it seems.

Hm, then the macros should still be added please; RHEL9/EPEL9 only offer
binutils 2.38.

> GCC 12 onwards contains this
> binutils version as per my understanding.

No, gcc doesn't "contain" binutils. They are separate packages, and
their versions are not tightly locked together.

> This version was released
> quite long back. I can double check by submitting it through edk2 CI to
> ensure it works. Current CI version is already GCC 12.

It's the binutils version that matters for this.

(And even if CI has gcc-12, I'd still ask for the macros, because RHEL9
/ EPEL9 don't seem to have a new enough binutils.)

Thanks
Laszlo

> 
> On Thu, Oct 19, 2023 at 5:47 PM Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>> wrote:
> 
>     On 10/19/23 11:22, Laszlo Ersek wrote:
>     > On 10/19/23 08:48, Dhaval Sharma wrote:
> 
>     >> (11) I agree that we should use symbolic names rather than
>     >> magic constants, but raw encodings of machine instructions don't
>     belong
>     >> into a
>     >>      C header file. [Dhaval] This bytecode was introduced
>     thinking what
>     >> if all compilers do not support it. but given the default compiler in
>     >> edk2 GCC 12 supports it
>     >>      we can eliminate this byte encoding completely to make it
>     easy and
>     >> simple to consume for others.
>     >
>     > To be honest, I can't determine the minimum expected gcc version for
>     > edk2. "BaseTools/Conf/tools_def.template" states a minimum version for
>     > NASM, for example, but I can't find a similar gcc requirement there.
>     >
>     > gcc-12 does work for me personally, because my riscv cross-compiler is
>     > "riscv64-linux-gnu-gcc (GCC) 12.1.1 20220507 (Red Hat Cross
>     12.1.1-1)".
>     >
>     > If the CI environment that builds these patches also provides gcc-12+,
>     > then I figure you should be set.
> 
>     Wait, for the assembly language source files, what matters is the
>     binutils version, not the gcc version. Mine is "GNU assembler version
>     2.38-3.el9" (from "binutils-riscv64-linux-gnu-2.38-3.el9.x86_64").
> 
>     Is that sufficient for the instuctions in question?
> 
>     (More generally -- what version does our CI env expect / provide?)
> 
>     Thanks
>     Laszlo
> 
> 
> 
> -- 
> Thanks!
> =D



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