Hi Dhaval,
On 12/13/23 15:59, 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"
> + ));
> }
>
> /**
> @@ -117,7 +224,12 @@ WriteBackInvalidateDataCacheRange (
> IN UINTN Length
> )
> {
> - DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> + if (RiscVIsCMOEnabled ()) {
> + CacheOpCacheRange (Address, Length, CacheOpFlush);
> + } else {
> + ASSERT (FALSE);
> + }
> +
> return Address;
> }
This change (replacing the DEBUG with an ASSERT()) seems to be causing a
failure for some physical platforms:
https://bugzilla.tianocore.org/show_bug.cgi?id=4637
Can you please check that ticket?
(I'd have CC'd you on the ticket, but the CC search field returned no
hits for "Dhaval" -- I think you may not have an account in Bugzilla yet.)
If a platform has no support for CMO (and the platform DSC sets
PcdRiscVFeatureOverride accordingly), then
WriteBackInvalidateDataCacheRange() is effectively uncallable on that
platform. Is that intentional?
I think there are two possibilities (when RiscVIsCMOEnabled() returns
FALSE):
- the platform has "no need" for writing back and invalidating a range
of the data cache -- perhaps because it has no data cache in the CPUs at
all. In that case, a no-op is better (and still safe) than a failed
ASSERT().
- or else, the platform needs this kind of flushing and invalidation,
but the CPU just doesn't support the particular CMO / fine-grained
instruction. In that case, can we do something heavier-weight, but
functionally *sufficient*? A good example is in this same patch's
InvalidateDataCacheRange() function, which falls back to
InvalidateDataCache() if CMO is missing.
Now, I can see that WriteBackDataCache() *itself* is not supported
(regardless of CMO). Why is that? Does it make no sense on RISC-V
platforms? If that's the case, should it be a no-op instead?
Yangcheng: can you provide a backtrace? What outer call path led you to
the ASSERT() in WriteBackInvalidateDataCacheRange()?
Thanks!
Laszlo
>
> @@ -137,7 +249,7 @@ WriteBackDataCache (
> VOID
> )
> {
> - DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__));
> + ASSERT (FALSE);
> }
>
> /**
> @@ -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, CacheOpClean);
> + } else {
> + ASSERT (FALSE);
> + }
> +
> 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, CacheOpInvld);
> + } else {
> + DEBUG (
> + (DEBUG_VERBOSE,
> + "InvalidateDataCacheRange: Zicbom not supported.\n" \
> + "Invalidating the whole Data cache instead.\n") \
> + );
> + InvalidateDataCache ();
> + }
> +
> return Address;
> }
> diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
> index 5c1fa24065c7..73b5dd8f32cc 100644
> --- a/MdePkg/MdePkg.uni
> +++ b/MdePkg/MdePkg.uni
> @@ -287,6 +287,10 @@
>
> #string
> STR_gEfiMdePkgTokenSpaceGuid_PcdGuidedExtractHandlerTableAddress_HELP
> #language en-US "This value is used to set the available memory address to
> store Guided Extract Handlers. The required memory space is decided by the
> value of PcdMaximumGuidedExtractHandler."
>
> +#string STR_gEfiMdePkgTokenSpaceGuid_PcdRiscVFeatureOverride_PROMPT
> #language en-US "RISC-V Feature Override"
> +
> +#string STR_gEfiMdePkgTokenSpaceGuid_PcdRiscVFeatureOverride_HELP #language
> en-US "This value is used to override any RISC-V specific features supported
> by this PCD"
> +
> #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_PROMPT
> #language en-US "PCI Express Base Address"
>
> #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_HELP
> #language en-US "This value is used to set the base address of PCI express
> hierarchy."
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113390): https://edk2.groups.io/g/devel/message/113390
Mute This Topic: https://groups.io/mt/103150435/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
