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

["Dhaval Sharma" <dhaval@rivosinc.com>]

[-- Attachment #1: Type: text/plain, Size: 12596 bytes --]

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 <pedro.falcato@gmail.com>
wrote:

> On Sat, Oct 21, 2023 at 6:33 PM Dhaval Sharma <dhaval@rivosinc.com> 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>
> > ---
> >
> > 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.<BR>
> > +  Copyright (c) 2023, Rivos Inc. All rights reserved.<BR>
> >
> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> >  **/
> > @@ -9,10 +10,111 @@
> >  #include <Base.h>
> >  #include <Library/BaseLib.h>
> >  #include <Library/DebugLib.h>
> > +#include <Library/PcdLib.h>
> > +
> > +// 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 <something>: <log message>
> 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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 17369 bytes --]