Re: [edk2-devel] [PATCH v5 1/2] MdePkg:Implement RISCV CMO
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*|0x0037
>>
>> +[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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 1/2] MdePkg:Implement RISCV CMO
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
> Cc: Jiewen Yao
> Cc: Jordan Justen
> Cc: Gerd Hoffmann
> Cc: Sunil V L
> Cc: Andrei Warkentin
> Signed-off-by: Dhaval Sharma
> ---
>
> 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 , but were
buried in the library *instance* source file
"MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c".
Both of those functions should have been declared in , inside
an
#if defined (MDE_CPU_RISCV64)
#endif
block.
Note that 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 ,
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*|0x0037
>
> +[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
[edk2-devel] [PATCH v5 1/2] MdePkg:Implement RISCV CMO
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
Cc: Jiewen Yao
Cc: Jordan Justen
Cc: Gerd Hoffmann
Cc: Sunil V L
Cc: Andrei Warkentin
Signed-off-by: Dhaval Sharma
---
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*|0x0037
+[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_FAULT0x16
#define CAUSE_STORE_GUEST_PAGE_FAULT0x17
+#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.
+ Copyright (c) 2023, Rivos Inc. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -9,6 +10,17 @@
#include
#include
#include
+#include
+
+// 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
