From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, dhaval@rivosinc.com
Cc: Michael D Kinney <michael.d.kinney@intel.com>,
Liming Gao <gaoliming@byosoft.com.cn>,
Zhiguang Liu <zhiguang.liu@intel.com>
Subject: Re: [edk2-devel] [PATCH v6 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
Date: Tue, 24 Oct 2023 14:45:35 +0200 [thread overview]
Message-ID: <03852129-ac51-86da-ffdb-b5106febb909@redhat.com> (raw)
In-Reply-To: <20231021173314.19363-5-dhaval@rivosinc.com>
On 10/21/23 19:33, 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>
> ---
>
> 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
>
> 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>
One nit: given that we're introducing a PcdLib dependency here, we
should also add the following to the library instance INF file:
[LibraryClasses.RISCV64]
PcdLib
I've not deeply verified the implementation in this patch, but
structurally it looks OK to me. So you can add:
Acked-by: Laszlo Ersek <lersek@redhat.com>
to the next version (with the LibraryClasses addition).
Thanks
Laszlo
> +
> +// 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.
> + // 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)
> + );
> +
> + 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
> + 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__)
> + );
> + 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__));
> + }
> +
> 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__));
> + }
> +
> 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__)
> + );
> + InvalidateDataCache ();
> + }
> +
> return Address;
> }
> diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
> index 5c1fa24065c7..f49c33191054 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 (#110005): https://edk2.groups.io/g/devel/message/110005
Mute This Topic: https://groups.io/mt/102103783/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2023-10-24 12:45 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-21 17:33 [edk2-devel] [PATCH v6 0/5] Cache Management Operations Support For RISC-V Dhaval Sharma
2023-10-21 17:33 ` [edk2-devel] [PATCH v6 1/5] MdePkg: Move RISC-V Cache Management Declarations Into BaseLib Dhaval Sharma
2023-10-24 12:32 ` Laszlo Ersek
2023-10-21 17:33 ` [edk2-devel] [PATCH v6 2/5] MdePkg: Rename Cache Management Function To Clarify Fence Based Op Dhaval Sharma
2023-10-24 12:34 ` Laszlo Ersek
2023-10-21 17:33 ` [edk2-devel] [PATCH v6 3/5] MdePkg: Implement RISC-V Cache Management Operations Dhaval Sharma
2023-10-24 12:39 ` Laszlo Ersek
2023-10-21 17:33 ` [edk2-devel] [PATCH v6 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V Dhaval Sharma
2023-10-24 12:45 ` Laszlo Ersek [this message]
2023-10-24 20:09 ` Pedro Falcato
2023-10-29 14:48 ` Dhaval Sharma
2023-10-21 17:33 ` [edk2-devel] [PATCH v6 5/5] OvmfPkg/RiscVVirt: Override for RV CPU Features Dhaval Sharma
2023-10-24 12:45 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=03852129-ac51-86da-ffdb-b5106febb909@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox