On Thu, Dec 07, 2023 at 10:31:48AM +0530, Dhaval Sharma wrote:
> Comments inline:
>
>
> On Wed, Dec 6, 2023 at 7:50 PM Sunil V L <suni...@ventanamicro.com> wrote:
>
> > Hi Dhaval,
> >
> > Thank you very much for fixing the issue with instruction cache
> > invalidation and confirming with the spec owner. Few minor comments
> > below.
> >
> > On Mon, Dec 04, 2023 at 01:59:49PM +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>
> > >
> > > Signed-off-by: Dhaval Sharma <dha...@rivosinc.com>
> > > Acked-by: Laszlo Ersek <ler...@redhat.com>
> > > ---
> > >
> > > Notes:
> > > 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
> > >
> > IMO, it is better to keep the change log in the cover letter. Since not
> > all patches may be CC'd to every one apart from the cover letter, it is
> > difficult to understand from the cover letter what has changed in the new
> > series.
> >
> [Dhaval] AFAIU notes are tied to specific commits. But it makes sense, I
> can add an update to the cover letter.
>
> >
> > > MdePkg/MdePkg.dec |
> > 8 +
> > > MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |
> > 5 +
> > > MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c |
> > 173 ++++++++++++++++----
> > > MdePkg/MdePkg.uni |
> > 4 +
> > > 4 files changed, 160 insertions(+), 30 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..cacc38eff4f4 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,117 @@
> > > #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
> > Can we make this also as a PCD?
> > [Dhaval] Actually this define should go away once we have CPU HOB. So
> > thought to keep it simple for now. Anyways there is no plan to change this
> > size
> >
> anytime in the near future.
>
What do you mean by no plan to change this size? Isn't it a choice of
the platform? Given that this macro is in MdePkg, it will force people
to use the same size.
> > +#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_INFO,
> > > + "CacheOpCacheRange:\
> > Use __func__ ?
>
>
> [Dhaval] Around patch 6 review there was a suggestion to define a function
> name which is more greppable. Hence I had changed it from __func__ to this
> :)
>
What was the actual comment? I am not aware of any such guidelines and
__func__ is widely used.
> >
> > [Dhaval]
> > > + 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 +136,10 @@ 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.
> > Could you please keep the comments within 80 character?
>
> [Dhaval] will do.
>
>
> >
> > > @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
> > > @@ -56,11 +157,7 @@ InvalidateInstructionCacheRange (
> > > IN UINTN Length
> > > )
> > > {
> > > - DEBUG (
> > > - (DEBUG_WARN,
> > > - "%a:RISC-V unsupported function.\n"
> > > - "Invalidating the whole instruction cache instead.\n", __func__)
> > > - );
> > > + DEBUG ((DEBUG_VERBOSE, "InvalidateInstructionCache:\n"));
> > This change is not required.
> >
> > [Dhaval] Are you saying that the earlier comment was fine as it was?
>
Yes.
-Sunil
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112207): https://edk2.groups.io/g/devel/message/112207
Mute This Topic: https://groups.io/mt/102967058/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
