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

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

Test:
1. Ensured correct instructions are refelecting in asm
2. Not able to verify actual instruction in HW as Qemu ignores
   any actual cache operations.

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

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

Notes:
    V7:
    - Modify instruction names as per feedback from V6
    - Added RB
    V6:
    - Implement Cache management instructions in Baselib

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

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



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

On Sun, Oct 29, 2023 at 08:16:11PM +0530, Dhaval wrote:
> Implement Cache Management Operations (CMO) defined by
> RISC-V spec https://github.com/riscv/riscv-CMOs.
> 
> Notes:
> 1. CMO only supports block based Operations. Meaning cache
>    flush/invd/clean Operations are not available for the entire
>    range. In that case we fallback on fence.i instructions.
> 2. Operations are implemented using Opcodes to make them compiler
>    independent. binutils 2.39+ compilers support CMO instructions.
> 
> Test:
> 1. Ensured correct instructions are refelecting in asm
> 2. Not able to verify actual instruction in HW as Qemu ignores
>    any actual cache operations.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Sunil V L <sunilvl@ventanamicro.com>
> Cc: Daniel Schaefer <git@danielschaefer.me>
> Cc: Laszlo Ersek <lersek@redhat.com>
> 
> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     V7:
>     - Modify instruction names as per feedback from V6
>     - Added RB
>     V6:
>     - Implement Cache management instructions in Baselib
> 
>  MdePkg/Library/BaseLib/BaseLib.inf                                |  2 +-
>  MdePkg/Include/Library/BaseLib.h                                  | 33 ++++++++++++++++++++
>  MdePkg/Include/RiscV64/RiscVasm.inc                               | 19 +++++++++++
>  MdePkg/Library/BaseLib/RiscV64/{FlushCache.S => RiscVCacheMgmt.S} | 17 ++++++++++
>  4 files changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
> index 03c7b02e828b..53389389448c 100644
> --- a/MdePkg/Library/BaseLib/BaseLib.inf
> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> @@ -400,7 +400,7 @@ [Sources.RISCV64]
>    RiscV64/RiscVCpuBreakpoint.S      | GCC
>    RiscV64/RiscVCpuPause.S           | GCC
>    RiscV64/RiscVInterrupt.S          | GCC
> -  RiscV64/FlushCache.S              | GCC
> +  RiscV64/RiscVCacheMgmt.S          | GCC
>    RiscV64/CpuScratch.S              | GCC
>    RiscV64/ReadTimer.S               | GCC
>    RiscV64/RiscVMmu.S                | GCC
> diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
> index d4b56a9601da..c42cc165dc82 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -226,6 +226,39 @@ RiscVInvalidateDataCacheAsmFence (
>    VOID
>    );
>  
> +/**
> +  RISC-V flush cache block. Atomically perform a clean operation
> +  followed by an invalidate operation
> +
> +**/
> +VOID
> +EFIAPI
> +RiscVCpuCacheFlushAsmCmo (

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

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

Thanks,
Sunil


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

On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
>
> Implement Cache Management Operations (CMO) defined by
> RISC-V spec https://github.com/riscv/riscv-CMOs.
>
> Notes:
> 1. CMO only supports block based Operations. Meaning cache
>    flush/invd/clean Operations are not available for the entire
>    range. In that case we fallback on fence.i instructions.
> 2. Operations are implemented using Opcodes to make them compiler
>    independent. binutils 2.39+ compilers support CMO instructions.
>
> Test:
> 1. Ensured correct instructions are refelecting in asm

nit: reflecting

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

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

-- 
Pedro


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

On 10/29/23 20:12, Pedro Falcato wrote:
> On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
>>
>> Implement Cache Management Operations (CMO) defined by
>> RISC-V spec https://github.com/riscv/riscv-CMOs.
>>
>> Notes:
>> 1. CMO only supports block based Operations. Meaning cache
>>    flush/invd/clean Operations are not available for the entire
>>    range. In that case we fallback on fence.i instructions.
>> 2. Operations are implemented using Opcodes to make them compiler
>>    independent. binutils 2.39+ compilers support CMO instructions.
>>
>> Test:
>> 1. Ensured correct instructions are refelecting in asm
> 
> nit: reflecting
> 
>> 2. Not able to verify actual instruction in HW as Qemu ignores
>>    any actual cache operations.
> 
> Do you have no way to test this in hardware? Since Rivos is a RISCV
> vendor and all ;)
> I don't like inviting the idea of merging CPU architectural changes
> without actually testing them in something resembling real silicon
> (i.e QEMU KVM is _fine_, QEMU TCG really isn't).
> 

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

Laszlo



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

On Mon, Oct 30, 2023 at 9:38 AM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 10/29/23 20:12, Pedro Falcato wrote:
> > On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
> >>
> >> Implement Cache Management Operations (CMO) defined by
> >> RISC-V spec https://github.com/riscv/riscv-CMOs.
> >>
> >> Notes:
> >> 1. CMO only supports block based Operations. Meaning cache
> >>    flush/invd/clean Operations are not available for the entire
> >>    range. In that case we fallback on fence.i instructions.
> >> 2. Operations are implemented using Opcodes to make them compiler
> >>    independent. binutils 2.39+ compilers support CMO instructions.
> >>
> >> Test:
> >> 1. Ensured correct instructions are refelecting in asm
> >
> > nit: reflecting
> >
> >> 2. Not able to verify actual instruction in HW as Qemu ignores
> >>    any actual cache operations.
> >
> > Do you have no way to test this in hardware? Since Rivos is a RISCV
> > vendor and all ;)
> > I don't like inviting the idea of merging CPU architectural changes
> > without actually testing them in something resembling real silicon
> > (i.e QEMU KVM is _fine_, QEMU TCG really isn't).
> >
>
> Hopefully I'm not drawing an incorrect parallel here, but, as I recall
> arm64 enablement in 2014, nearly all initial enablement in RHEL occurred
> on software emulators (ARM Foundation Model, ARM FVP, then QEMU TCG).
> You need to start somewhere. In particular, qemu-system-aarch64 was a
> huge step forward (performance-wise) once it *existed*, relative to the
> Foundation Model / FVP, even though qemu-system-aarch64 wouldn't emulate
> CPU caches (IIRC).

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

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

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

--
Pedro


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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

   MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf

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

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

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

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

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

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

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


-- 
Thanks!
=D


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

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

Thanks!

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

-- 
Pedro


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

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

... that should be rectified now



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

On Mon, Oct 30, 2023 at 10:38:21AM +0100, Laszlo Ersek wrote:
> On 10/29/23 20:12, Pedro Falcato wrote:
> > On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma <dhaval@rivosinc.com> wrote:
> >>
> >> Implement Cache Management Operations (CMO) defined by
> >> RISC-V spec https://github.com/riscv/riscv-CMOs.
> >>
> >> Notes:
> >> 1. CMO only supports block based Operations. Meaning cache
> >>    flush/invd/clean Operations are not available for the entire
> >>    range. In that case we fallback on fence.i instructions.
> >> 2. Operations are implemented using Opcodes to make them compiler
> >>    independent. binutils 2.39+ compilers support CMO instructions.
> >>
> >> Test:
> >> 1. Ensured correct instructions are refelecting in asm
> > 
> > nit: reflecting
> > 
> >> 2. Not able to verify actual instruction in HW as Qemu ignores
> >>    any actual cache operations.
> > 
> > Do you have no way to test this in hardware? Since Rivos is a RISCV
> > vendor and all ;)
> > I don't like inviting the idea of merging CPU architectural changes
> > without actually testing them in something resembling real silicon
> > (i.e QEMU KVM is _fine_, QEMU TCG really isn't).
> > 
> 
> Hopefully I'm not drawing an incorrect parallel here, but, as I recall
> arm64 enablement in 2014, nearly all initial enablement in RHEL occurred
> on software emulators (ARM Foundation Model, ARM FVP, then QEMU TCG).
> You need to start somewhere. In particular, qemu-system-aarch64 was a
> huge step forward (performance-wise) once it *existed*, relative to the
> Foundation Model / FVP, even though qemu-system-aarch64 wouldn't emulate
> CPU caches (IIRC).
> 
I agree. As per my knowledge, we don't have a publicly available silicon
implementing these features as of today. So, we are taking the approach
of how linux merged these features when the code adhered to the spec. It
will be great for downstream to get these patches merged.

Thanks,
Sunil


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