Thanks. Just to clarify, In the earlier formatting
"InvalidateDataCacheRange:\
> + Zicbom not supported.\n" \
A missing " after *Range:\ was causing slightly skewed prints. After adding
this " it looks okay. So that is one change I had addressed.
But if keeping it in a single line works better please feel free to update.
And Thanks!
On Tue, Dec 19, 2023 at 12:59 PM Sunil V L <suni...@ventanamicro.com> wrote:
> On Wed, Dec 13, 2023 at 08:29:30PM +0530, 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 <michael.d.kin...@intel.com>
> > Cc: Liming Gao <gaolim...@byosoft.com.cn>
> > Cc: Zhiguang Liu <zhiguang....@intel.com>
> > Cc: Laszlo Ersek <ler...@redhat.com>
> > Cc: Pedro Falcato <pedro.falc...@gmail.com>
> >
> > Signed-off-by: Dhaval Sharma <dha...@rivosinc.com>
> > Acked-by: Laszlo Ersek <ler...@redhat.com>
> > Reviewed-by: Pedro Falcato <pedro.falc...@gmail.com>
> > ---
> >
> > Notes:
> > V10:
> > - Fix formatting to keep comments within 80
> > - Replace RV with RISC-V
> > - Fix an issue with multi line comments
> > - Added assert to an unsupported function
> > - Minor case modification in str in .uni
> >
> > V9:
> > - Fixed an issue with Instruction cache invalidation. Use fence.i
> > instruction as CMO does not support i-cache operations.
> > V8:
> > - Added note to convert PCD into RISC-V feature bitmap pointer
> > - Modified function names to be more explicit about cache ops
> > - Added RB tag
> > V7:
> > - Added PcdLib
> > - Restructure DEBUG message based on feedback on V6
> > - Make naming consistent to CMO, remove all CBO references
> > - Add ASSERT for not supported functions instead of plain debug
> message
> > - Added RB tag
> > V6:
> > - 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 |
> 5 +
> > MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c |
> 177 ++++++++++++++++----
> > MdePkg/MdePkg.uni |
> 4 +
> > 4 files changed, 166 insertions(+), 28 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..601a38d6c109 100644
> > --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> > @@ -56,3 +56,8 @@ [LibraryClasses]
> > BaseLib
> > DebugLib
> >
> > +[LibraryClasses.RISCV64]
> > + PcdLib
> > +
> > +[Pcd.RISCV64]
> > + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES
> > diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > index ac2a3c23a249..7c53a17abbb5 100644
> > --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > @@ -2,6 +2,7 @@
> > RISC-V specific functionality for cache.
> >
> > 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
> > **/
> > @@ -9,10 +10,116 @@
> > #include <Base.h>
> > #include <Library/BaseLib.h>
> > #include <Library/DebugLib.h>
> > +#include <Library/PcdLib.h>
> > +
> > +//
> > +// TODO: Grab cache block size and make Cache Management Operation
> > +// enabling decision based on RISC-V CPU HOB in
> > +// future when it is available and convert PcdRiscVFeatureOverride
> > +// PCD to a pointer that contains pointer to bitmap structure
> > +// which can be operated more elegantly.
> > +//
> > +#define RISCV_CACHE_BLOCK_SIZE 64
> > +#define RISCV_CPU_FEATURE_CMO_BITMASK 0x1
> > +
> > +typedef enum {
> > + CacheOpClean,
> > + CacheOpFlush,
> > + CacheOpInvld,
> > +} CACHE_OP;
> > +
> > +/**
> > +Verify CBOs are supported by this HW
> > +TODO: Use RISC-V CPU HOB once available.
> > +
> > +**/
> > +STATIC
> > +BOOLEAN
> > +RiscVIsCMOEnabled (
> > + VOID
> > + )
> > +{
> > + // 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 != CacheOpInvld) && (Op != CacheOpFlush) && (Op !=
> CacheOpClean)) {
> > + 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_VERBOSE,
> > + "CacheOpCacheRange: Performing Cache Management Operation %d \n",
> Op)
> > + );
> > +
> > + do {
> > + switch (Op) {
> > + case CacheOpInvld:
> > + RiscVCpuCacheInvalCmoAsm (Start);
> > + break;
> > + case CacheOpFlush:
> > + RiscVCpuCacheFlushCmoAsm (Start);
> > + break;
> > + case CacheOpClean:
> > + RiscVCpuCacheCleanCmoAsm (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 the 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
> > @@ -28,17 +135,11 @@ InvalidateInstructionCache (
> > Invalidates a range of instruction cache lines in the cache coherency
> domain
> > of the calling CPU.
> >
> > - Invalidates the instruction cache lines specified by Address and
> Length. If
> > - Address is not aligned on a cache line boundary, then entire
> instruction
> > - cache line containing Address is invalidated. If Address + Length is
> not
> > - aligned on a cache line boundary, then the entire instruction cache
> line
> > - containing Address + Length -1 is invalidated. This function may
> choose to
> > - invalidate the entire instruction cache if that is more efficient than
> > - invalidating the specified range. If Length is 0, then no instruction
> cache
> > - lines are invalidated. Address is returned.
> > -
> > - If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
> > -
> > + An operation from a CMO instruction is defined to operate only on the
> copies
> > + of a cache block that are cached in the caches accessible by the
> explicit
> > + memory accesses performed by the set of coherent agents.In other
> words CMO
> > + operations are not applicable to instruction cache. Use fence.i
> instruction
> > + instead to achieve the same purpose.
> > @param Address The base address of the instruction 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
> > @@ -57,9 +158,10 @@ InvalidateInstructionCacheRange (
> > )
> > {
> > DEBUG (
> > - (DEBUG_WARN,
> > - "%a:RISC-V unsupported function.\n"
> > - "Invalidating the whole instruction cache instead.\n", __func__)
> > + (DEBUG_VERBOSE,
> > + "InvalidateInstructionCacheRange: RISC-V unsupported function.\n"
> > + "Invalidating the whole instruction cache instead.\n"
> > + )
> > );
> > InvalidateInstructionCache ();
> > return Address;
> > @@ -81,7 +183,12 @@ WriteBackInvalidateDataCache (
> > VOID
> > )
> > {
> > - DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> > + ASSERT (FALSE);
> > + DEBUG ((
> > + DEBUG_ERROR,
> > + "WriteBackInvalidateDataCache:" \
> > + "RISC-V unsupported function.\n"
> I guess this formatting was pointed by Pedro earlier. This can be made
> single line. Let me fix it before merging so that you don't need to send
> another version.
>
> Reviewed-by: Sunil V L <suni...@ventanamicro.com>
>
--
Thanks!
=D
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112713): https://edk2.groups.io/g/devel/message/112713
Mute This Topic: https://groups.io/mt/103150435/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
