[PATCH v1 0/2] WIP: Enable CMO support for RiscV64

[PATCH v1 0/2] WIP: Enable CMO support for RiscV64

[Dhaval Sharma]

Current implementation for cache management (instruction/data flush/invd)
depends on ifence instruction. All RV platforms may not use the same
method for cache management. Instead RV defines CMO Cache management
operations specification which consits of cbo.x instructions for cache
management. However it requires GCC12+ to enable the same. Need to decide
how cbo based implementation coexists with ifence based implementation
with GCC version dependency.

This patchset is primarily to review the same and decide path forward.
review branch: https://github.com/rivosinc/edk2/tree/dev_rv_cmo_v1

Dhaval Sharma (2):
  MdePkg/BaseCacheMaintenanceLib: Enable RISCV CMO
  OvmfPkg/RiscVVirt: Enable CMO support

 OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc                 |   9 ++
 MdePkg/Library/BaseLib/BaseLib.inf                  |   1 +
 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c | 126 ++++++++++++++++++--
 MdePkg/Library/BaseLib/RiscV64/RiscVCpuCache.S      |  23 ++++
 4 files changed, 152 insertions(+), 7 deletions(-)
 create mode 100644 MdePkg/Library/BaseLib/RiscV64/RiscVCpuCache.S

-- 
2.40.0.rc0.57.g454dfcbddf

[PATCH v1 1/2] MdePkg/BaseCacheMaintenanceLib: Enable RISCV CMO

[Dhaval Sharma]

Adding 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
2. Current implementation uses ifence instructions but it
maybe platform specific. Many platforms may not support cache
Operations based on ifence.
3. For now adding CMO on top of ifence as it is not considered
harmful.
4. This requires support for GCC12.2 onwards.

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: Sunil V L <sunilvl@ventanamicro.com>
Cc: Andrei Warkentin <andrei.warkentin@intel.com>
Cc: Daniel Schaefer <git@danielschaefer.me>
Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
---
 MdePkg/Library/BaseLib/BaseLib.inf                  |   1 +
 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c | 126 ++++++++++++++++++--
 MdePkg/Library/BaseLib/RiscV64/RiscVCpuCache.S      |  23 ++++
 3 files changed, 143 insertions(+), 7 deletions(-)

diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
index 3a48492b1a01..0d6d6b7414c8 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -398,6 +398,7 @@ [Sources.RISCV64]
   RiscV64/MemoryFence.S             | GCC
   RiscV64/RiscVSetJumpLongJump.S    | GCC
   RiscV64/RiscVCpuBreakpoint.S      | GCC
+  RiscV64/RiscVCpuCache.S           | GCC
   RiscV64/RiscVCpuPause.S           | GCC
   RiscV64/RiscVInterrupt.S          | GCC
   RiscV64/FlushCache.S              | GCC
diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
index d08fb9f193ca..8e88b8391a74 100644
--- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
@@ -10,9 +10,111 @@
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 
+/**
+  Use runtime discovery mechanism in future when avalable
+  through https://lists.riscv.org/g/tech-privileged/topic/83853282
+**/
+#define RV64_CACHE_BLOCK_SIZE   64
+
+typedef enum{
+  cln,
+  flsh,
+  invd,
+}CACHE_OP;
+
+/* Ideally we should do this through BaseLib.h by adding
+   Asm*CacheLine functions. This can be done after Initial
+   RV refactoring is complete. For now call functions directly
+*/
+VOID
+EFIAPI RiscVCpuCacheFlush (
+  UINTN
+  );
+
+VOID
+EFIAPI RiscVCpuCacheClean (
+  UINTN
+  );
+
+VOID
+EFIAPI RiscVCpuCacheInval (
+  UINTN
+  );
+
+/**
+  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. 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  Length  The number of bytes to invalidate from the instruction cache.
+
+  @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));
+
+  //
+  // Cache line size is 8 * Bits 15-08 of EBX returned from CPUID 01H
+  //
+  CacheLineSize = RV64_CACHE_BLOCK_SIZE;
+
+  Start = (UINTN)Address;
+  //
+  // Calculate the cache line alignment
+  //
+  End    = (Start + Length + (CacheLineSize - 1)) & ~(CacheLineSize - 1);
+  Start &= ~((UINTN)CacheLineSize - 1);
+
+  do {
+    switch (op) {
+      case invd:
+        RiscVCpuCacheInval(Start);
+        break;
+      case flsh:
+        RiscVCpuCacheFlush(Start);
+        break;
+      case cln:
+        RiscVCpuCacheClean(Start);
+        break;
+      default:
+        DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported operation\n"));
+        break;
+    }
+
+    Start = Start + CacheLineSize;
+  } while (Start != End);
+
+  return Address;
+}
+
 /**
   RISC-V invalidate instruction cache.
-
 **/
 VOID
 EFIAPI
@@ -22,7 +124,6 @@ RiscVInvalidateInstCacheAsm (
 
 /**
   RISC-V invalidate data cache.
-
 **/
 VOID
 EFIAPI
@@ -32,7 +133,9 @@ RiscVInvalidateDataCacheAsm (
 
 /**
   Invalidates the entire instruction cache in cache coherency domain of the
-  calling CPU.
+  calling CPU. This may not clear $IC on all RV implementations.
+  RV CMO only offers block operations as per spec. Entire cache invd will be
+  platform dependent implementation.
 
 **/
 VOID
@@ -77,11 +180,13 @@ InvalidateInstructionCacheRange (
   )
 {
   DEBUG (
-    (DEBUG_WARN,
+    (DEBUG_ERROR,
      "%a:RISC-V unsupported function.\n"
      "Invalidating the whole instruction cache instead.\n", __func__)
     );
   InvalidateInstructionCache ();
+  //RV does not support $I specific operation.
+  CacheOpCacheRange(Address, Length, invd);
   return Address;
 }
 
@@ -93,6 +198,8 @@ InvalidateInstructionCacheRange (
   of the calling CPU. This function guarantees that all dirty cache lines are
   written back to system memory, and also invalidates all the data cache lines
   in the cache coherency domain of the calling CPU.
+  RV CMO only offers block operations as per spec. Entire cache invd will be
+  platform dependent implementation.
 
 **/
 VOID
@@ -137,7 +244,7 @@ WriteBackInvalidateDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  CacheOpCacheRange(Address, Length, flsh);
   return Address;
 }
 
@@ -149,6 +256,8 @@ WriteBackInvalidateDataCacheRange (
   CPU. This function guarantees that all dirty cache lines are written back to
   system memory. This function may also invalidate all the data cache lines in
   the cache coherency domain of the calling CPU.
+  RV CMO only offers block operations as per spec. Entire cache invd will be
+  platform dependent implementation.
 
 **/
 VOID
@@ -192,7 +301,7 @@ WriteBackDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  CacheOpCacheRange(Address, Length, cln);
   return Address;
 }
 
@@ -205,6 +314,8 @@ WriteBackDataCacheRange (
   written back to system memory. It is typically used for cache diagnostics. If
   the CPU does not support invalidation of the entire data cache, then a write
   back and invalidate operation should be performed on the entire data cache.
+  RV CMO only offers block operations as per spec. Entire cache invd will be
+  platform dependent implementation.
 
 **/
 VOID
@@ -250,6 +361,7 @@ InvalidateDataCacheRange (
   IN      UINTN  Length
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
+  //RV does not support $D specific operation.
+  CacheOpCacheRange(Address, Length, invd);
   return Address;
 }
diff --git a/MdePkg/Library/BaseLib/RiscV64/RiscVCpuCache.S b/MdePkg/Library/BaseLib/RiscV64/RiscVCpuCache.S
new file mode 100644
index 000000000000..0913ed3e9221
--- /dev/null
+++ b/MdePkg/Library/BaseLib/RiscV64/RiscVCpuCache.S
@@ -0,0 +1,23 @@
+//------------------------------------------------------------------------------
+//
+// CpuPause for RISC-V
+//
+// Copyright (c) 2022, Rivos Inc. All rights reserved.<BR>
+//
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+//------------------------------------------------------------------------------
+ASM_GLOBAL ASM_PFX(RiscVCpuCacheFlush)
+ASM_PFX(RiscVCpuCacheFlush):
+  cbo.flush (a0)
+  ret
+
+ASM_GLOBAL ASM_PFX(RiscVCpuCacheClean)
+ASM_PFX(RiscVCpuCacheClean):
+  cbo.clean (a0)
+  ret
+
+ASM_GLOBAL ASM_PFX(RiscVCpuCacheInval)
+ASM_PFX(RiscVCpuCacheInval):
+  cbo.inval (a0)
+  ret
-- 
2.40.0.rc0.57.g454dfcbddf

Re: [PATCH v1 1/2] MdePkg/BaseCacheMaintenanceLib: Enable RISCV CMO

[Sunil V L]

Hi Dhaval,

Thank you for looking at CMO support!

General comments first:
1) Please have a cover letter patch and move some part of the commit
message to cover letter. Please CC all maintainers in the cover letter
also.
2) Please run BaseTools/Scripts/GetMaintainer.py and CC all maintainers.
3) Follow
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

Have you run CI tests?

On Fri, Mar 24, 2023 at 09:13:41PM +0530, Dhaval Sharma wrote:
> Adding 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
> 2. Current implementation uses ifence instructions but it
> maybe platform specific. Many platforms may not support cache
> Operations based on ifence.
fence.i? 

IMO, it is better to add a new library such as BaseRiscV64CMOLib and
included conditionally in the DSC for the platforms which support CMO.
BaseCacheMaintenanceLib will continue to have default fence.i
implementation. Is there an issue with this?

> 3. For now adding CMO on top of ifence as it is not considered
> harmful.
> 4. This requires support for GCC12.2 onwards.
>
Yeah, this is another challenge like zifencei_zicsr which we could
workaround and support both older and newer tool chain. But for CMO, 
I don't see any option but to support only GCC12.2+.

Thanks,
Sunil

Re: [PATCH v1 1/2] MdePkg/BaseCacheMaintenanceLib: Enable RISCV CMO

[Dhaval Sharma]

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

My comments inline:

On Mon, Mar 27, 2023 at 9:12 PM Sunil V L <sunilvl@ventanamicro.com> wrote:

> Hi Dhaval,
>
> Thank you for looking at CMO support!
>
> General comments first:
> 1) Please have a cover letter patch and move some part of the commit

message to cover letter. Please CC all maintainers in the cover letter
> also.
>

https://edk2.groups.io/g/devel/message/101795?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2Ccmo%2C20%2C2%2C0%2C97826395.
Is this the one you are looking for?


> 2) Please run BaseTools/Scripts/GetMaintainer.py and CC all maintainers.
>

Sure.

> 3) Follow
>
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
>
> Have you run CI tests?
>
I actually did run it but I believe the current edk2 CI is using a GCC5
based compiler. Hence it failed as it did not recognize cmo instructions as
expected. So I submitted this as WIP patch to sort that out first.
Do let me know if I can follow any other better process here.


> On Fri, Mar 24, 2023 at 09:13:41PM +0530, Dhaval Sharma wrote:
> > Adding 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
> > 2. Current implementation uses ifence instructions but it
> > maybe platform specific. Many platforms may not support cache
> > Operations based on ifence.
> fence.i?


Ack.

>
>
> IMO, it is better to add a new library such as BaseRiscV64CMOLib and
> included conditionally in the DSC for the platforms which support CMO.
> BaseCacheMaintenanceLib will continue to have default fence.i
> implementation. Is there an issue with this?
>
There are 2 libraries involved here. 1. BaseCacheMaintenanceLib. It is a
generic Lib for multiple archs. So yes it is possible to create another
Lib, but I was thinking if it is possible somehow to create a RV specific
Lib.
2. BaseLib which contains required .S files. For CBO I have added a
separate .S. Again this is generic Baselib for all Arch. So we need to be
able to differentiate in DSC now for both these libs. I am not sure if this
is the
best way to address this. I could try to do inline assembly within
CMOCachelib to address #2.


> > 3. For now adding CMO on top of ifence as it is not considered
> > harmful.
> > 4. This requires support for GCC12.2 onwards.
> >
> Yeah, this is another challenge like zifencei_zicsr which we could
> workaround and support both older and newer tool chain. But for CMO,
> I don't see any option but to support only GCC12.2+.
>

How do we support this in CI?


> Thanks,
> Sunil
>


-- 
Thanks!
=D

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

Re: [PATCH v1 1/2] MdePkg/BaseCacheMaintenanceLib: Enable RISCV CMO

[Sunil V L]

On Mon, Mar 27, 2023 at 11:29:07PM +0530, Dhaval Sharma wrote:
> My comments inline:
> 
> On Mon, Mar 27, 2023 at 9:12 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> 
> > Hi Dhaval,
> >
> > Thank you for looking at CMO support!
> >
> > General comments first:
> > 1) Please have a cover letter patch and move some part of the commit
> 
> message to cover letter. Please CC all maintainers in the cover letter
> > also.
> >
> 
> https://edk2.groups.io/g/devel/message/101795?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2Ccmo%2C20%2C2%2C0%2C97826395.
> Is this the one you are looking for?
> 
Yes, sorry I missed it due to mail filters.

> 
> > 2) Please run BaseTools/Scripts/GetMaintainer.py and CC all maintainers.
> >
> 
> Sure.
> 
> > 3) Follow
> >
> > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
> >
> > Have you run CI tests?
> >
> I actually did run it but I believe the current edk2 CI is using a GCC5
> based compiler. Hence it failed as it did not recognize cmo instructions as
> expected. So I submitted this as WIP patch to sort that out first.
> Do let me know if I can follow any other better process here.
> 
> 
> > On Fri, Mar 24, 2023 at 09:13:41PM +0530, Dhaval Sharma wrote:
> > > Adding 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
> > > 2. Current implementation uses ifence instructions but it
> > > maybe platform specific. Many platforms may not support cache
> > > Operations based on ifence.
> > fence.i?
> 
> 
> Ack.
> 
> >
> >
> > IMO, it is better to add a new library such as BaseRiscV64CMOLib and
> > included conditionally in the DSC for the platforms which support CMO.
> > BaseCacheMaintenanceLib will continue to have default fence.i
> > implementation. Is there an issue with this?
> >
> There are 2 libraries involved here. 1. BaseCacheMaintenanceLib. It is a
> generic Lib for multiple archs. So yes it is possible to create another
> Lib, but I was thinking if it is possible somehow to create a RV specific
> Lib.
> 2. BaseLib which contains required .S files. For CBO I have added a
> separate .S. Again this is generic Baselib for all Arch. So we need to be
> able to differentiate in DSC now for both these libs. I am not sure if this
> is the
> best way to address this. I could try to do inline assembly within
> CMOCachelib to address #2.
>
I was thinking single independent library of CacheMaintenanceLib class
for CMO exclusively. Let BaseLib/BaseCacheMaintenanceLib continue to use
the default fence.i implementation. The DSC for the platform can chose
between default vs CMO libraries.

> 
> > > 3. For now adding CMO on top of ifence as it is not considered
> > > harmful.
> > > 4. This requires support for GCC12.2 onwards.
> > >
> > Yeah, this is another challenge like zifencei_zicsr which we could
> > workaround and support both older and newer tool chain. But for CMO,
> > I don't see any option but to support only GCC12.2+.
> >
> 
> How do we support this in CI?
> 
Oliver has a patch to update CI image to GCC12. I think it is not yet
merged. But I have not checked whether it is 12.2. You can run CI
including that patch with yours and try.

https://edk2.groups.io/g/devel/message/101164

[PATCH v1 2/2] OvmfPkg/RiscVVirt: Enable CMO support

[Dhaval Sharma]

Cc: Sunil V L <sunilvl@ventanamicro.com>
Cc: Andrei Warkentin <andrei.warkentin@intel.com>
Cc: Daniel Schaefer <git@danielschaefer.me>
Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>

Add support for Cache Management Operations
---
 OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
index 28d9af4d79b9..16c591d94228 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
+++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
@@ -46,6 +46,12 @@ [Defines]
   DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = TRUE
   DEFINE NETWORK_ISCSI_ENABLE           = FALSE
 
+#
+# CMO support for RV. It depends on 2 factors. First support in compiler
+# GCC:Binutils 2.39 (GCC12.2+) is required.
+#
+  DEFINE RV_CMO_FEATURE_AVAILABLE = FALSE
+
 !if $(NETWORK_SNP_ENABLE) == TRUE
   !error "NETWORK_SNP_ENABLE is IA32/X64/EBC only"
 !endif
@@ -112,6 +118,9 @@ [LibraryClasses.common]
   TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf
 !endif
 
+!if $(RV_CMO_FEATURE_AVAILABLE) == TRUE
+   CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
+!endif
 [LibraryClasses.common.DXE_DRIVER]
   ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
   PciExpressLib|OvmfPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
-- 
2.40.0.rc0.57.g454dfcbddf

Re: [PATCH v1 2/2] OvmfPkg/RiscVVirt: Enable CMO support

[Sunil V L]

On Fri, Mar 24, 2023 at 09:13:42PM +0530, Dhaval Sharma wrote:
> Cc: Sunil V L <sunilvl@ventanamicro.com>
> Cc: Andrei Warkentin <andrei.warkentin@intel.com>
> Cc: Daniel Schaefer <git@danielschaefer.me>
> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> 
> Add support for Cache Management Operations
> ---
>  OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
> index 28d9af4d79b9..16c591d94228 100644
> --- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
> +++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
> @@ -46,6 +46,12 @@ [Defines]
>    DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = TRUE
>    DEFINE NETWORK_ISCSI_ENABLE           = FALSE
>  
> +#
> +# CMO support for RV. It depends on 2 factors. First support in compiler
> +# GCC:Binutils 2.39 (GCC12.2+) is required.
> +#
> +  DEFINE RV_CMO_FEATURE_AVAILABLE = FALSE
> +
>  !if $(NETWORK_SNP_ENABLE) == TRUE
>    !error "NETWORK_SNP_ENABLE is IA32/X64/EBC only"
>  !endif
> @@ -112,6 +118,9 @@ [LibraryClasses.common]
>    TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf
>  !endif
>  
> +!if $(RV_CMO_FEATURE_AVAILABLE) == TRUE
> +   CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> +!endif
Hi Dhaval,

I don't understand this change. BaseCacheMaintenanceLib is already
included for the platform. So, why do we need this?

Thanks,
Sunil