From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id B01AAD811B1 for ; Wed, 6 Dec 2023 14:20:55 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=9dwdNf6EqzWsecgRlRkYK/gbCfYJPFveSgh0Lv9epjo=; c=relaxed/simple; d=groups.io; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Disposition; s=20140610; t=1701872454; v=1; b=e4ufU4Sv03xVJG1h2UGOeWql086zXK9wteGG1C623jOccokYeMFMWhDvStoQH6BDaMASdDHT 78L1ARhaX4DEawQAla9VV7HfbrwZZU8TkOvgwWsJRM75APusxuufdisVVbY61XO1LlkXONhCQ1A JDqwQH3ByiMUZ+APVukK+CFY= X-Received: by 127.0.0.2 with SMTP id 6oifYY7687511xqXTCeJJlcN; Wed, 06 Dec 2023 06:20:54 -0800 X-Received: from mail-pl1-f180.google.com (mail-pl1-f180.google.com [209.85.214.180]) by mx.groups.io with SMTP id smtpd.web10.32777.1701872453656607092 for ; Wed, 06 Dec 2023 06:20:53 -0800 X-Received: by mail-pl1-f180.google.com with SMTP id d9443c01a7336-1cfc9c4acb6so35126605ad.0 for ; Wed, 06 Dec 2023 06:20:53 -0800 (PST) X-Gm-Message-State: GF9SrONB71Gtfb1eRWhxhe8Bx7686176AA= X-Google-Smtp-Source: AGHT+IEBD3YWB1PMz934sOvWwUZ0geOEI0sfu6DyXA49/WEdOxcNLvVJbJ/Vpu9PqLHUR8yAZ7P7Ng== X-Received: by 2002:a17:902:e745:b0:1d0:9475:1416 with SMTP id p5-20020a170902e74500b001d094751416mr824537plf.14.1701872450676; Wed, 06 Dec 2023 06:20:50 -0800 (PST) X-Received: from sunil-laptop ([106.51.188.200]) by smtp.gmail.com with ESMTPSA id h9-20020a170902b94900b001c61acd5bd2sm2397944pls.112.2023.12.06.06.20.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 06 Dec 2023 06:20:50 -0800 (PST) Date: Wed, 6 Dec 2023 19:50:44 +0530 From: "Sunil V L" To: devel@edk2.groups.io, dhaval@rivosinc.com Cc: Michael D Kinney , Liming Gao , Zhiguang Liu , Laszlo Ersek Subject: Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V Message-ID: References: <20231204082950.96914-1-dhaval@rivosinc.com> <20231204082950.96914-5-dhaval@rivosinc.com> MIME-Version: 1.0 In-Reply-To: <20231204082950.96914-5-dhaval@rivosinc.com> Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,sunilvl@ventanamicro.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=e4ufU4Sv; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=none 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 > Cc: Liming Gao > Cc: Zhiguang Liu > Cc: Laszlo Ersek > > Signed-off-by: Dhaval Sharma > Acked-by: Laszlo Ersek > --- > > 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. > 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.
> + Copyright (c) 2023, Rivos Inc. All rights reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent > **/ > @@ -9,10 +10,117 @@ > #include > #include > #include > +#include > + > +// > +// 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? > +#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__ ? > + 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? > @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. > InvalidateInstructionCache (); > return Address; > } > @@ -81,7 +178,8 @@ WriteBackInvalidateDataCache ( > VOID > ) > { > - DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__)); > + DEBUG ((DEBUG_ERROR, "WriteBackInvalidateDataCache:\ > + RISC-V unsupported function.\n")); This change is not required. > } > > /** > @@ -117,7 +215,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; > } > > @@ -137,7 +240,7 @@ WriteBackDataCache ( > VOID > ) > { > - DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__)); > + ASSERT (FALSE); > } > > /** > @@ -156,10 +259,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 +272,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 +319,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 +332,17 @@ 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..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" "override any" instead of "Override Any"? Thanks, Sunil -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112126): https://edk2.groups.io/g/devel/message/112126 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] -=-=-=-=-=-=-=-=-=-=-=-