Thanks for your feedback. 1. Reg coding style, I will remove _ and resubmit but somehow PR CI seemed to pass for me (https://github.com/tianocore/edk2/pull/4636). 2. For size and ext discovery should I wait until your ext discovery patch is merged? 3. Thanks for catching the issue with SENVCFG. Will fix it in the next revision after #2 is addressed.
On Mon, Jul 24, 2023 at 10:03 AM Sunil V L <suni...@ventanamicro.com> wrote: > Hi Dhaval, > > On Thu, Jul 13, 2023 at 03:03:31PM +0530, Dhaval wrote: > > From: Dhaval Sharma <dha...@rivosinc.com> > > > > 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+tianoc...@kernel.org> > > Cc: Jiewen Yao <jiewen....@intel.com> > > Cc: Jordan Justen <jordan.l.jus...@intel.com> > > Cc: Gerd Hoffmann <kra...@redhat.com> > > Cc: Sunil V L <suni...@ventanamicro.com> > > Cc: Andrei Warkentin <andrei.warken...@intel.com> > > Signed-off-by: Dhaval Sharma <dha...@rivosinc.com> > > --- > > > > Notes: > > v4: > > - Removed CMO specific directory in Base Lib > > - Implemented compiler independent code for CMO > > - Merged CMO implementation with fence.i > > - Added logic to confirm CMO is enabled > > > > MdePkg/Library/BaseLib/BaseLib.inf | 2 +- > > MdePkg/Include/Register/RiscV64/RiscVEncoding.h | 4 + > > MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c | 200 > ++++++++++++++++++-- > > MdePkg/Library/BaseLib/RiscV64/FlushCache.S | 21 -- > > MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S | 64 +++++++ > > 5 files changed, 254 insertions(+), 37 deletions(-) > > > > 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 5c2989b797bf..ea1493578bd5 100644 > > --- a/MdePkg/Include/Register/RiscV64/RiscVEncoding.h > > +++ b/MdePkg/Include/Register/RiscV64/RiscVEncoding.h > > @@ -85,6 +85,10 @@ > > /* Supervisor Configuration */ > > #define CSR_SENVCFG 0x10a > > > > +/* Defined CBO bits*/ > > +#define SENVCFG_CBCFE 0x40UL > > +#define SENVCFG_CBIE 0x30UL > > + > > /* Supervisor Trap Handling */ > > #define CSR_SSCRATCH 0x140 > > #define CSR_SEPC 0x141 > > diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c > b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c > > index d08fb9f193ca..8b853e5b69fa 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 > > **/ > > @@ -10,13 +11,21 @@ > > #include <Library/BaseLib.h> > > #include <Library/DebugLib.h> > > > > +#define RV64_CACHE_BLOCK_SIZE 64 > > + > Can we avoid hard coding this? We can get it from DT. > > > +typedef enum { > > + Clean, > > + Flush, > > + Invld, > > +} CACHE_OP; > > + > > /** > > RISC-V invalidate instruction cache. > > > > **/ > > VOID > > EFIAPI > > -RiscVInvalidateInstCacheAsm ( > > +RiscVInvalidateInstCacheAsm_Fence ( > This is not EDK2 coding style... and other similar functions below. > > > VOID > > ); > > > > @@ -26,13 +35,144 @@ RiscVInvalidateInstCacheAsm ( > > **/ > > VOID > > EFIAPI > > -RiscVInvalidateDataCacheAsm ( > > +RiscVInvalidateDataCacheAsm_Fence ( > > VOID > > ); > > > > +/** > > + RISC-V flush cache block. Atomically perform a clean operation > > + followed by an invalidate operation > > + > > +**/ > > +VOID > > +EFIAPI > > +RiscVCpuCacheFlushAsm_Cbo ( > > + 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 > > +RiscVCpuCacheCleanAsm_Cbo ( > > + UINTN > > + ); > > + > > +/** > > +Deallocate the copy of the cache block > > + > > +**/ > > +VOID > > +EFIAPI > > +RiscVCpuCacheInvalAsm_Cbo ( > > + UINTN > > + ); > > + > > +/** > > +Verify CBOs are supported by this HW > > +CBCFE == Cache Block Clean and Flush instruction Enable > > +CBIE == Cache Block Invalidate instruction Enable > > + > > +**/ > > +UINTN > > +RiscvIsCbcfeEnabledAsm ( > > + VOID > > + ); > > + > > +UINTN > > +RiscvIsCbiEnabledAsm ( > > + VOID > > + ); > > + > > +/** > > + 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. > > + > > + @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)); > > + > > + // > > + // Cache line size is 8 * Bits 15-08 of EBX returned from CPUID 01H > > + // > This comments is invalid for RISC-V. > > > + 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: > > + RiscVCpuCacheInvalAsm_Cbo (Start); > > + break; > > + case Flush: > > + RiscVCpuCacheFlushAsm_Cbo (Start); > > + break; > > + case Clean: > > + RiscVCpuCacheCleanAsm_Cbo (Start); > > + break; > > + default: > > + DEBUG ((DEBUG_ERROR, "%a: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 +181,7 @@ InvalidateInstructionCache ( > > VOID > > ) > > { > > - RiscVInvalidateInstCacheAsm (); > > + RiscVInvalidateInstCacheAsm_Fence (); > > } > > > > /** > > @@ -76,12 +216,17 @@ InvalidateInstructionCacheRange ( > > IN UINTN Length > > ) > > { > > - DEBUG ( > > - (DEBUG_WARN, > > - "%a:RISC-V unsupported function.\n" > > - "Invalidating the whole instruction cache instead.\n", __func__) > > - ); > > - InvalidateInstructionCache (); > > + if (RiscvIsCbiEnabledAsm () == RETURN_SUCCESS) { > > + 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 +282,12 @@ WriteBackInvalidateDataCacheRange ( > > IN UINTN Length > > ) > > { > > - DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__)); > > + if (RiscvIsCbcfeEnabledAsm () == RETURN_SUCCESS) { > > + CacheOpCacheRange (Address, Length, Flush); > > + } else { > > + DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", > __func__)); > > + } > > + > > return Address; > > } > > > > @@ -192,7 +342,12 @@ WriteBackDataCacheRange ( > > IN UINTN Length > > ) > > { > > - DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__)); > > + if (RiscvIsCbcfeEnabledAsm () == RETURN_SUCCESS) { > > + CacheOpCacheRange (Address, Length, Clean); > > + } else { > > + DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", > __func__)); > > + } > > + > > return Address; > > } > > > > @@ -213,7 +368,12 @@ InvalidateDataCache ( > > VOID > > ) > > { > > - RiscVInvalidateDataCacheAsm (); > > + DEBUG ( > > + (DEBUG_WARN, > > + "%a:RISC-V unsupported function.\n" > > + "Invalidating the whole instruction cache instead.\n", __func__) > Data cache? > > + ); > > + RiscVInvalidateDataCacheAsm_Fence (); > > } > > > > /** > > @@ -250,6 +410,16 @@ InvalidateDataCacheRange ( > > IN UINTN Length > > ) > > { > > - DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__)); > > + if (RiscvIsCbiEnabledAsm () == RETURN_SUCCESS) { > > + CacheOpCacheRange (Address, Length, Invld); > > + } else { > > + DEBUG ( > > + (DEBUG_WARN, > > + "%a:RISC-V unsupported function.\n" > > + "Invalidating the whole instruction cache instead.\n", __func__) > Data cache? > > > + ); > > + 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 > Have you done git mv first to rename the file so that git history is > maintained? > > > diff --git a/MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S > b/MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S > > new file mode 100644 > > index 000000000000..ecf391632221 > > --- /dev/null > > +++ b/MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S > > @@ -0,0 +1,64 @@ > > > +//------------------------------------------------------------------------------ > > +// > > +// 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(RiscVInvalidateInstCacheAsm_Fence) > > +ASM_GLOBAL ASM_PFX(RiscVInvalidateDataCacheAsm_Fence) > > + > > +ASM_PFX(RiscVInvalidateInstCacheAsm_Fence): > > + fence.i > > + ret > > + > > +ASM_PFX(RiscVInvalidateDataCacheAsm_Fence): > > + fence > > + ret > > + > > +ASM_GLOBAL ASM_PFX (RiscVCpuCacheFlushAsm_Cbo) > > +ASM_PFX (RiscVCpuCacheFlushAsm_Cbo): > > + > > + .long 0x0025200f > Can we have macros instead of magic number? > > > + ret > > + > > +ASM_GLOBAL ASM_PFX (RiscVCpuCacheCleanAsm_Cbo) > > +ASM_PFX (RiscVCpuCacheCleanAsm_Cbo): > > + .long 0x0015200f > > + ret > > + > > +ASM_GLOBAL ASM_PFX (RiscVCpuCacheInvalAsm_Cbo) > > +ASM_PFX (RiscVCpuCacheInvalAsm_Cbo): > > + .long 0x0005200f > > + ret > > + > > +ASM_GLOBAL ASM_PFX(RiscvIsCbiEnabledAsm) > > +ASM_PFX(RiscvIsCbiEnabledAsm): > > + li a0, 3 > > + csrr a1, CSR_SENVCFG > > + and a1, a1, SENVCFG_CBIE > > This is not correct. SENVCFG is only for U-mode. It can not be relied > upon in S-mode. We have to use extension discovery itself. Same comment > for CBCFE also. > > Thanks, > Sunil > > > + beqz a1, skip > > + > > + and a1, a1, 0x10 > > + beqz a1, skip > > + > > + mv a0, x0 > > +skip: > > + ret > > + > > +ASM_GLOBAL ASM_PFX(RiscvIsCbcfeEnabledAsm) > > +ASM_PFX(RiscvIsCbcfeEnabledAsm): > > + li a0, 3 > > + csrr a1, CSR_SENVCFG > > + and a1, a1, SENVCFG_CBCFE > > + beqz a1, next > > + > > + mv a0, x0 > > +next: > > + ret > > -- > > 2.34.1 > > > -- Thanks! =D -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107292): https://edk2.groups.io/g/devel/message/107292 Mute This Topic: https://groups.io/mt/100117169/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
