Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V

["Sunil V L" <sunilvl@ventanamicro.com>]

On Sun, Dec 10, 2023 at 07:51:12PM +0530, Dhaval Sharma wrote:
> Thanks for the review. My comments inline:
> 
> On Fri, Dec 8, 2023 at 9:58 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
> 
> > 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 <sunilvl@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.kinney@intel.com>
> > > > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > > > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > >
> > > > > Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> > > > > Acked-by: Laszlo Ersek <lersek@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.
> 
> 
> [Dhaval] My understanding is that this cache block size is fixed for RVA
> profile which is where this code will be mostly used. Custom
> implementations can change it if they want. Second, we are planning to
> introduce a cache block through CPU HOB which when available will replace
> this.
> So I thought it is okay to defer this implementation and use a simpler
> option.
> 
Platforms may not be aligned with a profile. But I am fine if you want
to add the flexibility as part of CPU HOB.

> 
> > > > +#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.
> >
> > This was Pedro's comment on V6.
> > +  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 <something>: <log message>
> In this case, "CacheOpCacheRange: Performing ...". It's just prettier
> and more greppable.
> My interpretation of this was removing __func__ and instead having some
> relevant text would make it more searchable.
> And it kind of did make sense to me. I know many places __func__ is used
> but this is just a perspective.
>
I think the comment meant to follow a standard logging format since
there was no ":" and a space in original change. I prefer __func__ over
this so that we don't need to update multiple lines in case function
name gets changed.
 
> > > >
> > > > [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.
> >
> > The reason I modified the text was- "Supported" in a way gives sense that
> this implementation does not support (and some other may support). But from
> spec perspective it is not supposed to be supported using CBO. Kind of nit.
> I am okay to bring earlier comment back if that is more readable.
>
I prefer to keep it unchanged. 

Thanks,
Sunil


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112297): https://edk2.groups.io/g/devel/message/112297
Mute This Topic: https://groups.io/mt/102967058/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-