Replied inline. Most of the cases I have addressed in the new patch I submitted. On Wed, Oct 25, 2023 at 1:39 AM Pedro Falcato wrote: > On Sat, Oct 21, 2023 at 6:33 PM Dhaval Sharma wrote: > > > > Use newly defined cache management operations for RISC-V where possible > > It builds up on the support added for RISC-V cache management > > instructions in BaseLib. > > Cc: Michael D Kinney > > Cc: Liming Gao > > Cc: Zhiguang Liu > > Cc: Laszlo Ersek > > > > Signed-off-by: Dhaval Sharma > > --- > > > > Notes: > > V1: > > - Utilize cache management instructions if HW supports it > > This patch is part of restructuring on top of v5 > > > > MdePkg/MdePkg.dec | > 8 + > > MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf | > 2 + > > MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c | > 159 +++++++++++++++++--- > > MdePkg/MdePkg.uni | > 4 + > > 4 files changed, 154 insertions(+), 19 deletions(-) > > > > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec > > index ac54338089e8..fa92673ff633 100644 > > --- a/MdePkg/MdePkg.dec > > +++ b/MdePkg/MdePkg.dec > > @@ -2399,6 +2399,14 @@ [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 RISC-V CPU Features > > + # BIT 0 = Cache Management Operations. This bit is relevant only if > > + # previous stage has feature enabled and user wants to disable it. > > + # > > + > gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|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..39a7fb963b49 100644 > > --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf > > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf > > @@ -56,3 +56,5 @@ [LibraryClasses] > > BaseLib > > DebugLib > > > > +[Pcd.RISCV64] > > + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES > > diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c > b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c > > index 4eb18edb9aa7..6851970c9e16 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 > > Why the change? You're effectively implementing cache management for > riscv, you're not exclusively using any sort of extension (such as > CMO). > Done. I believe you meant to keep it a generic description. > > > > > 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,10 +10,111 @@ > > #include > > #include > > #include > > +#include > > + > > +// TODO: This will be removed once RISC-V CPU HOB is available > > +#define RISCV_CACHE_BLOCK_SIZE 64 > > +#define RISCV_CPU_FEATURE_CMO_BITMASK 0x1 > > + > > +typedef enum { > > + Clean, > > + Flush, > > + Invld, > > +} CACHE_OP; > > + > > +/** > > +Verify CBOs are supported by this HW > > +TODO: Use RISC-V CPU HOB once available. > > + > > +**/ > > +STATIC > > +BOOLEAN > > +RiscVIsCMOEnabled ( > > + VOID > > + ) > > +{ > > + // TODO: Add check for CMO from CPU HOB. > > Too many TODOs? One TODO at the top of the file (mentioning feature > detection, cache line size detection) should be enough. There's no > point in peppering these out throughout the file :) > > Done. > > + // If CMO is disabled in HW, skip Override check > > + // Otherwise this PCD can override settings > > + return ((PcdGet64 (PcdRiscVFeatureOverride) & > RISCV_CPU_FEATURE_CMO_BITMASK) != 0); > > +} > > + > > +/** > > + 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. > > + > > +**/ > > +STATIC > > +VOID > > +CacheOpCacheRange ( > > + IN VOID *Address, > > + IN UINTN Length, > > + IN CACHE_OP Op > > + ) > > +{ > > + UINTN CacheLineSize; > > + UINTN Start; > > + UINTN End; > > + > > + if (Length == 0) { > > + return; > > + } > > + > > + if ((Op != Invld) && (Op != Flush) && (Op != Clean)) { > > + return; > > + } > > + > > + ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)Address)); > > + > > + CacheLineSize = RISCV_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) > > + ); > > nit: Can we pick a log style here? Like : > In this case, "CacheOpCacheRange: Performing ...". It's just prettier > and more greppable. > > Done. > > + > > + do { > > + switch (Op) { > > + case Invld: > > + RiscVCpuCacheInvalAsmCbo (Start); > > + break; > > + case Flush: > > + RiscVCpuCacheFlushAsmCbo (Start); > > + break; > > + case Clean: > > + RiscVCpuCacheCleanAsmCbo (Start); > > + break; > > + default: > > + break; > > + } > > + > > + Start = Start + CacheLineSize; > > + } while (Start != End); > > +} > > > > /** > > 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 > nit: Invalidate *the* entire I-cache > > Done. > Also, what do you mean? 1) you're calling CacheOpCacheRange for the range > 2) please don't mix CBO and CMO. Too much terminology :) > > Modified naming to be consistent. All CMO now. > Does Zicbom only operate on the dcache? Or does it also touch the > icache? As far as I'm aware, riscv does not require a dcache/icache > coherency (like ARM), but a quick stroll through the extension's spec > didn't lead me to much. > My understanding is that Zicbom operates on both. It just needs an address to act on. > > + instruction may or may not implement full I-cache invd functionality > on all > > + implementations. > > > > **/ > > VOID > > @@ -56,12 +158,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 ()) { > > + CacheOpCacheRange (Address, Length, Invld); > > + } else { > > + DEBUG ( > > + (DEBUG_WARN, > > + "%a:RISC-V unsupported function.\n" > > + "Invalidating the whole instruction cache instead.\n", __func__) > > + ); > nit: "%a: ", with a space, but i'd probably write this bit as > > + "%a: Zicbom not enabled, invalidating the whole instruction > cache instead.\n", __func__) > > Done with minor modification. Given that this may be repeatedly called, can we just warn once (e.g > on a constructor) that CMO is disabled and leave this be? Since the > cache management is in fact being done correctly, but just in a slower > way. > > > + InvalidateInstructionCache (); > > + } > > + > > return Address; > > } > > > > @@ -117,7 +224,12 @@ WriteBackInvalidateDataCacheRange ( > > IN UINTN Length > > ) > > { > > - DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__)); > > + if (RiscVIsCMOEnabled ()) { > > + CacheOpCacheRange (Address, Length, Flush); > > + } else { > > + DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", > __func__)); > > Ok, so here it may make sense to ASSERT, as you're no longer complying > to the function's behavior (and this is unsafe). > > Done. > > + } > > + > > return Address; > > } > > > > @@ -156,10 +268,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. > > @@ -172,7 +281,12 @@ WriteBackDataCacheRange ( > > IN UINTN Length > > ) > > { > > - DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__)); > > + if (RiscVIsCMOEnabled ()) { > > + CacheOpCacheRange (Address, Length, Clean); > > + } else { > > + DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", > __func__)); > > Same comment here WRT assert. > Done. > > + } > > + > > return Address; > > } > > > > @@ -214,10 +328,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. > > @@ -230,6 +341,16 @@ InvalidateDataCacheRange ( > > IN UINTN Length > > ) > > { > > - DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__)); > > + if (RiscVIsCMOEnabled ()) { > > + CacheOpCacheRange (Address, Length, Invld); > > + } else { > > + DEBUG ( > > + (DEBUG_WARN, > > + "%a:RISC-V unsupported function.\n" > > + "Invalidating the whole Data cache instead.\n", __func__) > > + ); > > Aaaand the same comment here WRT warning. > > Done. > Pedro > -- Thanks! =D -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110268): https://edk2.groups.io/g/devel/message/110268 Mute This Topic: https://groups.io/mt/102103783/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-