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 176F3740040 for ; Thu, 7 Dec 2023 05:02:04 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=jjVJ0Sh07P4C1KKwnroGug6Uaf8FR3KXOcaIP9+N/Xs=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1701925323; v=1; b=GMyX6k/QdQmLPhiFVyf9SDA9r6U0luTONUXWVZAvr3p67bcUdvvQ/3OS26FAjZ2KRfnM7Iqe VdCz5Em/d8ccuErB6pzEyy37PVMQf3xh5R7uIgcwMuNU2CLWO6lvoYBhVbd5bLYrw3BEfGLIE4R zWYB0b9uq630D29ZP08Fx1Ls= X-Received: by 127.0.0.2 with SMTP id Jw66YY7687511xbyXi7MG25w; Wed, 06 Dec 2023 21:02:03 -0800 X-Received: from mail-qv1-f53.google.com (mail-qv1-f53.google.com [209.85.219.53]) by mx.groups.io with SMTP id smtpd.web10.77060.1701925322209524997 for ; Wed, 06 Dec 2023 21:02:02 -0800 X-Received: by mail-qv1-f53.google.com with SMTP id 6a1803df08f44-677fba00a49so4662436d6.1 for ; Wed, 06 Dec 2023 21:02:02 -0800 (PST) X-Gm-Message-State: gjlBe7LWWCjZFhB6yXVYwVAlx7686176AA= X-Google-Smtp-Source: AGHT+IFrnKF5p78QMeGLACyKpVDIQipqNZnip+eWxKqnYGFDS2qERLIB6XJyY2Fwsm1nTK99+u4JyemFaZQajzp8oDQ= X-Received: by 2002:a0c:d78d:0:b0:67a:c855:66aa with SMTP id z13-20020a0cd78d000000b0067ac85566aamr3310227qvi.17.1701925321185; Wed, 06 Dec 2023 21:02:01 -0800 (PST) MIME-Version: 1.0 References: <20231204082950.96914-1-dhaval@rivosinc.com> <20231204082950.96914-5-dhaval@rivosinc.com> In-Reply-To: From: "Dhaval Sharma" Date: Thu, 7 Dec 2023 10:31:48 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V To: Sunil V L Cc: devel@edk2.groups.io, Michael D Kinney , Liming Gao , Zhiguang Liu , Laszlo Ersek 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,dhaval@rivosinc.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="00000000000034310f060be45e72" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="GMyX6k/Q"; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --00000000000034310f060be45e72 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Comments inline: On Wed, Dec 6, 2023 at 7:50=E2=80=AFPM Sunil V L = 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 > > 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. > [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*|0x00000= 037 > > > > +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64] > > + # > > + # Configurability to override RISC-V CPU Features > > + # BIT 0 =3D Cache Management Operations. This bit is relevant only i= f > > + # previous stage has feature enabled and user wants to disable it. > > + # > > + > gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UINT6= 4|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.in= f > > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.in= f > > @@ -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? > [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 thi= s > size > anytime in the near future. > +#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) !=3D 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 instructi= on > > + 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 =3D=3D 0) { > > + return; > > + } > > + > > + if ((Op !=3D CacheOpInvld) && (Op !=3D CacheOpFlush) && (Op !=3D > CacheOpClean)) { > > + return; > > + } > > + > > + ASSERT ((Length - 1) <=3D (MAX_ADDRESS - (UINTN)Address)); > > + > > + CacheLineSize =3D RISCV_CACHE_BLOCK_SIZE; > > + > > + Start =3D (UINTN)Address; > > + // > > + // Calculate the cache line alignment > > + // > > + End =3D (Start + Length + (CacheLineSize - 1)) & ~(CacheLineSize = - > 1); > > + Start &=3D ~((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 :) > > [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 =3D Start + CacheLineSize; > > + } while (Start !=3D End); > > +} > > > > /** > > Invalidates the entire instruction cache in cache coherency domain o= f > 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 coherenc= y > 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 th= an > > - invalidating the specified range. If Length is 0, then no instructio= n > 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 th= e > 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? > 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 cach= e. > > > > @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 cach= e. > > > > @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 t= o > store Guided Extract Handlers. The required memory space is decided by th= e > 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 > --=20 Thanks! =3DD -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112158): https://edk2.groups.io/g/devel/message/112158 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --00000000000034310f060be45e72 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Comments inline:


=
On Wed, De= c 6, 2023 at 7:50=E2=80=AFPM 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 possibl= e
> 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:
>=C2=A0 =C2=A0 =C2=A0V9:
>=C2=A0 =C2=A0 =C2=A0- Fixed an issue with Instruction cache invalidatio= n. Use fence.i
>=C2=A0 =C2=A0 =C2=A0 =C2=A0instruction as CMO does not support i-cache = operations.
>=C2=A0 =C2=A0 =C2=A0V8:
>=C2=A0 =C2=A0 =C2=A0- Added note to convert PCD into RISC-V feature bit= map pointer
>=C2=A0 =C2=A0 =C2=A0- Modified function names to be more explicit about= cache ops
>=C2=A0 =C2=A0 =C2=A0- Added RB tag
>=C2=A0 =C2=A0 =C2=A0V7:
>=C2=A0 =C2=A0 =C2=A0- Added PcdLib
>=C2=A0 =C2=A0 =C2=A0- Restructure DEBUG message based on feedback on V6=
>=C2=A0 =C2=A0 =C2=A0- Make naming consistent to CMO, remove all CBO ref= erences
>=C2=A0 =C2=A0 =C2=A0- Add ASSERT for not supported functions instead of= plain debug message
>=C2=A0 =C2=A0 =C2=A0- Added RB tag
>=C2=A0 =C2=A0 =C2=A0V6:
>=C2=A0 =C2=A0 =C2=A0- Utilize cache management instructions if HW suppo= rts it
>=C2=A0 =C2=A0 =C2=A0 =C2=A0This patch is part of restructuring on top o= f 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 comm= its. But it makes sense, I can add an update to the cover letter.=C2=A0

>=C2=A0 MdePkg/MdePkg.dec=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 =C2=A08 + >=C2=A0 MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.i= nf |=C2=A0 =C2=A05 +
>=C2=A0 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c=C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 173 ++++++++++++++++----
>=C2=A0 MdePkg/MdePkg.uni=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 =C2=A04 + >=C2=A0 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, PcdsPatchableInModul= e.AARCH64]
>=C2=A0 =C2=A0 # @Prompt CPU Rng algorithm's GUID.
>=C2=A0 =C2=A0 gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x0= 0,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x0= 0}|VOID*|0x00000037
>=C2=A0
> +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
> +=C2=A0 #
> +=C2=A0 # Configurability to override RISC-V CPU Features
> +=C2=A0 # BIT 0 =3D Cache Management Operations. This bit is relevant = only if
> +=C2=A0 # previous stage has feature enabled and user wants to disable= it.
> +=C2=A0 #
> +=C2=A0 gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFF= FFFFF|UINT64|0x69
> +
>=C2=A0 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynam= icEx]
>=C2=A0 =C2=A0 ## This value is used to set the base address of PCI expr= ess hierarchy.
>=C2=A0 =C2=A0 # @Prompt PCI Express Base Address.
> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenan= ceLib.inf b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.= inf
> index 6fd9cbe5f6c9..601a38d6c109 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.i= nf
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.i= nf
> @@ -56,3 +56,8 @@ [LibraryClasses]
>=C2=A0 =C2=A0 BaseLib
>=C2=A0 =C2=A0 DebugLib
>=C2=A0
> +[LibraryClasses.RISCV64]
> +=C2=A0 PcdLib
> +
> +[Pcd.RISCV64]
> +=C2=A0 gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride=C2=A0 ## CONS= UMES
> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/Mde= Pkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> index ac2a3c23a249..cacc38eff4f4 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> @@ -2,6 +2,7 @@
>=C2=A0 =C2=A0 RISC-V specific functionality for cache.
>=C2=A0
>=C2=A0 =C2=A0 Copyright (c) 2020, Hewlett Packard Enterprise Developmen= t LP. All rights reserved.<BR>
> +=C2=A0 Copyright (c) 2023, Rivos Inc. All rights reserved.<BR><= br> >=C2=A0
>=C2=A0 =C2=A0 SPDX-License-Identifier: BSD-2-Clause-Patent
>=C2=A0 **/
> @@ -9,10 +10,117 @@
>=C2=A0 #include <Base.h>
>=C2=A0 #include <Library/BaseLib.h>
>=C2=A0 #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=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A064 Can we make this also as a PCD?
[Dhaval] Actually this define should go away once we have CPU HOB. So thoug= ht to keep it simple for now. Anyways there is no plan to change this size<= br>
=C2=A0 =C2=A0 anytime in the near future.=C2=A0
<= div>
> +#define RISCV_CPU_FEATURE_CMO_BITMASK=C2=A0 0x1
> +
> +typedef enum {
> +=C2=A0 CacheOpClean,
> +=C2=A0 CacheOpFlush,
> +=C2=A0 CacheOpInvld,
> +} CACHE_OP;
> +
> +/**
> +Verify CBOs are supported by this HW
> +TODO: Use RISC-V CPU HOB once available.
> +
> +**/
> +STATIC
> +BOOLEAN
> +RiscVIsCMOEnabled (
> +=C2=A0 VOID
> +=C2=A0 )
> +{
> +=C2=A0 // If CMO is disabled in HW, skip Override check
> +=C2=A0 // Otherwise this PCD can override settings
> +=C2=A0 return ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FE= ATURE_CMO_BITMASK) !=3D 0);
> +}
> +
> +/**
> +=C2=A0 Performs required opeartion on cache lines in the cache cohere= ncy domain
> +=C2=A0 of the calling CPU. If Address is not aligned on a cache line = boundary,
> +=C2=A0 then entire cache line containing Address is operated. If Addr= ess + Length
> +=C2=A0 is not aligned on a cache line boundary, then the entire cache= line
> +=C2=A0 containing Address + Length -1 is operated.
> +=C2=A0 If Length is greater than (MAX_ADDRESS - Address + 1), then AS= SERT().
> +=C2=A0 @param=C2=A0 Address The base address of the cache lines to > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 invalidate.
> +=C2=A0 @param=C2=A0 Length=C2=A0 The number of bytes to invalidate fr= om the instruction
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 cache.
> +=C2=A0 @param=C2=A0 Op=C2=A0 Type of CMO operation to be performed > +=C2=A0 @return Address.
> +
> +**/
> +STATIC
> +VOID
> +CacheOpCacheRange (
> +=C2=A0 IN VOID=C2=A0 =C2=A0 =C2=A0 *Address,
> +=C2=A0 IN UINTN=C2=A0 =C2=A0 =C2=A0Length,
> +=C2=A0 IN CACHE_OP=C2=A0 Op
> +=C2=A0 )
> +{
> +=C2=A0 UINTN=C2=A0 CacheLineSize;
> +=C2=A0 UINTN=C2=A0 Start;
> +=C2=A0 UINTN=C2=A0 End;
> +
> +=C2=A0 if (Length =3D=3D 0) {
> +=C2=A0 =C2=A0 return;
> +=C2=A0 }
> +
> +=C2=A0 if ((Op !=3D CacheOpInvld) && (Op !=3D CacheOpFlush) &= amp;& (Op !=3D CacheOpClean)) {
> +=C2=A0 =C2=A0 return;
> +=C2=A0 }
> +
> +=C2=A0 ASSERT ((Length - 1) <=3D (MAX_ADDRESS - (UINTN)Address));<= br> > +
> +=C2=A0 CacheLineSize =3D RISCV_CACHE_BLOCK_SIZE;
> +
> +=C2=A0 Start =3D (UINTN)Address;
> +=C2=A0 //
> +=C2=A0 // Calculate the cache line alignment
> +=C2=A0 //
> +=C2=A0 End=C2=A0 =C2=A0 =3D (Start + Length + (CacheLineSize - 1)) &a= mp; ~(CacheLineSize - 1);
> +=C2=A0 Start &=3D ~((UINTN)CacheLineSize - 1);
> +
> +=C2=A0 DEBUG (
> +=C2=A0 =C2=A0 (DEBUG_INFO,
> +=C2=A0 =C2=A0 =C2=A0"CacheOpCacheRange:\
Use __func__ ?

[Dhaval] Around patch 6 revi= ew there was a suggestion to define a function name which is more greppable= . Hence I had changed it from __func__ to this :)=C2=A0

[Dhaval]
> +=C2=A0 =C2=A0 =C2=A0Performing Cache Management Operation %d \n"= , Op)
> +=C2=A0 =C2=A0 );
> +
> +=C2=A0 do {
> +=C2=A0 =C2=A0 switch (Op) {
> +=C2=A0 =C2=A0 =C2=A0 case CacheOpInvld:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 RiscVCpuCacheInvalCmoAsm (Start);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +=C2=A0 =C2=A0 =C2=A0 case CacheOpFlush:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 RiscVCpuCacheFlushCmoAsm (Start);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +=C2=A0 =C2=A0 =C2=A0 case CacheOpClean:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 RiscVCpuCacheCleanCmoAsm (Start);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +=C2=A0 =C2=A0 =C2=A0 default:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +=C2=A0 =C2=A0 }
> +
> +=C2=A0 =C2=A0 Start =3D Start + CacheLineSize;
> +=C2=A0 } while (Start !=3D End);
> +}
>=C2=A0
>=C2=A0 /**
>=C2=A0 =C2=A0 Invalidates the entire instruction cache in cache coheren= cy domain of the
> -=C2=A0 calling CPU.
> +=C2=A0 calling CPU. Risc-V does not have currently an CBO implementat= ion which can
> +=C2=A0 invalidate the entire I-cache. Hence using Fence instruction f= or now. P.S.
> +=C2=A0 Fence instruction may or may not implement full I-cache invd f= unctionality
> +=C2=A0 on all implementations.
>=C2=A0
>=C2=A0 **/
>=C2=A0 VOID
> @@ -28,17 +136,10 @@ InvalidateInstructionCache (
>=C2=A0 =C2=A0 Invalidates a range of instruction cache lines in the cac= he coherency domain
>=C2=A0 =C2=A0 of the calling CPU.
>=C2=A0
> -=C2=A0 Invalidates the instruction cache lines specified by Address a= nd Length. If
> -=C2=A0 Address is not aligned on a cache line boundary, then entire i= nstruction
> -=C2=A0 cache line containing Address is invalidated. If Address + Len= gth is not
> -=C2=A0 aligned on a cache line boundary, then the entire instruction = cache line
> -=C2=A0 containing Address + Length -1 is invalidated. This function m= ay choose to
> -=C2=A0 invalidate the entire instruction cache if that is more effici= ent than
> -=C2=A0 invalidating the specified range. If Length is 0, then no inst= ruction cache
> -=C2=A0 lines are invalidated. Address is returned.
> -
> -=C2=A0 If Length is greater than (MAX_ADDRESS - Address + 1), then AS= SERT().
> -
> +=C2=A0 An operation from a CMO instruction is defined to operate only= on the copies of a cache block that are
> +=C2=A0 cached in the caches accessible by the explicit memory accesse= s performed by the set of coherent agents.
> +=C2=A0 In other words CMO operations are not applicable to instructio= n cache. Use fence.i instruction instead
> +=C2=A0 to achieve the same purpose.
Could you please keep the comments within 80 character?
[D= haval] will do.=C2=A0



>=C2=A0 =C2=A0 @param=C2=A0 Address The base address of the instruction = cache lines to
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 i= nvalidate. If the CPU is in a physical addressing mode, then
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 A= ddress is a physical address. If the CPU is in a virtual
> @@ -56,11 +157,7 @@ InvalidateInstructionCacheRange (
>=C2=A0 =C2=A0 IN UINTN=C2=A0 Length
>=C2=A0 =C2=A0 )
>=C2=A0 {
> -=C2=A0 DEBUG (
> -=C2=A0 =C2=A0 (DEBUG_WARN,
> -=C2=A0 =C2=A0 =C2=A0"%a:RISC-V unsupported function.\n"
> -=C2=A0 =C2=A0 =C2=A0"Invalidating the whole instruction cache in= stead.\n", __func__)
> -=C2=A0 =C2=A0 );
> +=C2=A0 DEBUG ((DEBUG_VERBOSE, "InvalidateInstructionCache:\n&quo= t;));
This change is not required.

[Dhaval] Are you saying that the earlier comment was = fine as it was?

>=C2=A0 =C2=A0 InvalidateInstructionCache ();
>=C2=A0 =C2=A0 return Address;
>=C2=A0 }
> @@ -81,7 +178,8 @@ WriteBackInvalidateDataCache (
>=C2=A0 =C2=A0 VOID
>=C2=A0 =C2=A0 )
>=C2=A0 {
> -=C2=A0 DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n&q= uot;, __func__));
> +=C2=A0 DEBUG ((DEBUG_ERROR, "WriteBackInvalidateDataCache:\
> +=C2=A0 RISC-V unsupported function.\n"));
This change is not required.

>=C2=A0 }
>=C2=A0
>=C2=A0 /**
> @@ -117,7 +215,12 @@ WriteBackInvalidateDataCacheRange (
>=C2=A0 =C2=A0 IN=C2=A0 =C2=A0 =C2=A0 UINTN=C2=A0 Length
>=C2=A0 =C2=A0 )
>=C2=A0 {
> -=C2=A0 DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n&q= uot;, __func__));
> +=C2=A0 if (RiscVIsCMOEnabled ()) {
> +=C2=A0 =C2=A0 CacheOpCacheRange (Address, Length, CacheOpFlush);
> +=C2=A0 } else {
> +=C2=A0 =C2=A0 ASSERT (FALSE);
> +=C2=A0 }
> +
>=C2=A0 =C2=A0 return Address;
>=C2=A0 }
>=C2=A0
> @@ -137,7 +240,7 @@ WriteBackDataCache (
>=C2=A0 =C2=A0 VOID
>=C2=A0 =C2=A0 )
>=C2=A0 {
> -=C2=A0 DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n&q= uot;, __func__));
> +=C2=A0 ASSERT (FALSE);
>=C2=A0 }
>=C2=A0
>=C2=A0 /**
> @@ -156,10 +259,7 @@ WriteBackDataCache (
>=C2=A0
>=C2=A0 =C2=A0 If Length is greater than (MAX_ADDRESS - Address + 1), th= en ASSERT().
>=C2=A0
> -=C2=A0 @param=C2=A0 Address The base address of the data cache lines = to write back. If
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 the CP= U is in a physical addressing mode, then Address is a
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 physic= al address. If the CPU is in a virtual addressing
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mode, = then Address is a virtual address.
> +=C2=A0 @param=C2=A0 Address The base address of the data cache lines = to write back.
>=C2=A0 =C2=A0 @param=C2=A0 Length=C2=A0 The number of bytes to write ba= ck from the data cache.
>=C2=A0
>=C2=A0 =C2=A0 @return Address of cache written in main memory.
> @@ -172,7 +272,12 @@ WriteBackDataCacheRange (
>=C2=A0 =C2=A0 IN=C2=A0 =C2=A0 =C2=A0 UINTN=C2=A0 Length
>=C2=A0 =C2=A0 )
>=C2=A0 {
> -=C2=A0 DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n&q= uot;, __func__));
> +=C2=A0 if (RiscVIsCMOEnabled ()) {
> +=C2=A0 =C2=A0 CacheOpCacheRange (Address, Length, CacheOpClean);
> +=C2=A0 } else {
> +=C2=A0 =C2=A0 ASSERT (FALSE);
> +=C2=A0 }
> +
>=C2=A0 =C2=A0 return Address;
>=C2=A0 }
>=C2=A0
> @@ -214,10 +319,7 @@ InvalidateDataCache (
>=C2=A0
>=C2=A0 =C2=A0 If Length is greater than (MAX_ADDRESS - Address + 1), th= en ASSERT().
>=C2=A0
> -=C2=A0 @param=C2=A0 Address The base address of the data cache lines = to invalidate. If
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 the CP= U is in a physical addressing mode, then Address is a
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 physic= al address. If the CPU is in a virtual addressing mode,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 then A= ddress is a virtual address.
> +=C2=A0 @param=C2=A0 Address The base address of the data cache lines = to invalidate.
>=C2=A0 =C2=A0 @param=C2=A0 Length=C2=A0 The number of bytes to invalida= te from the data cache.
>=C2=A0
>=C2=A0 =C2=A0 @return Address.
> @@ -230,6 +332,17 @@ InvalidateDataCacheRange (
>=C2=A0 =C2=A0 IN=C2=A0 =C2=A0 =C2=A0 UINTN=C2=A0 Length
>=C2=A0 =C2=A0 )
>=C2=A0 {
> -=C2=A0 DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n&q= uot;, __func__));
> +=C2=A0 if (RiscVIsCMOEnabled ()) {
> +=C2=A0 =C2=A0 CacheOpCacheRange (Address, Length, CacheOpInvld);
> +=C2=A0 } else {
> +=C2=A0 =C2=A0 DEBUG (
> +=C2=A0 =C2=A0 =C2=A0 (DEBUG_VERBOSE,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"InvalidateDataCacheRange:\
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0Zicbom not supported.\n" \
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"Invalidating the whole Data cache in= stead.\n")
> +=C2=A0 =C2=A0 =C2=A0 );
> +=C2=A0 =C2=A0 InvalidateDataCache ();
> +=C2=A0 }
> +
>=C2=A0 =C2=A0 return Address;
>=C2=A0 }
> 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 @@
>=C2=A0
>=C2=A0 #string STR_gEfiMdePkgTokenSpaceGuid_PcdGuidedExtractHandlerTabl= eAddress_HELP=C2=A0 #language en-US "This value is used to set the ava= ilable memory address to store Guided Extract Handlers. The required memory= space is decided by the value of PcdMaximumGuidedExtractHandler."
>=C2=A0
> +#string STR_gEfiMdePkgTokenSpaceGuid_PcdRiscVFeatureOverride_PROMPT= =C2=A0 #language en-US "RISC-V Feature Override"
> +
> +#string STR_gEfiMdePkgTokenSpaceGuid_PcdRiscVFeatureOverride_HELP=C2= =A0 #language en-US "This value is used to Override Any RISC-V specifi= c features supported by this PCD"
"override any" instead of "Override Any"?

Thanks,
Sunil


--
Thanks!
=3DD
_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#112158) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--00000000000034310f060be45e72--