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 49E79740032 for ; Sun, 10 Dec 2023 14:21:27 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=37M/CLhWz1icYObFAKJghHZFtZa4dLO0SaEntQ5+CN8=; 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=1702218086; v=1; b=ipXtcP1RvNTCvoKrxaKHQmguKUaAZ1zCu1s47qjgjNmrtWA8Xr5ZBSaI/Kbqg5tZxr9tKr16 EYHheJJ6qbxwYdOOB5oov41KqNowtBV543zqLhBO377z1sV6wEzz5TveuUYIrCt3jWB9PbAsxON tcVnN73Kx/qZ8MJwNECY4og0= X-Received: by 127.0.0.2 with SMTP id owqFYY7687511xUR8B1daTeP; Sun, 10 Dec 2023 06:21:26 -0800 X-Received: from mail-qv1-f43.google.com (mail-qv1-f43.google.com [209.85.219.43]) by mx.groups.io with SMTP id smtpd.web10.54960.1702218084898712023 for ; Sun, 10 Dec 2023 06:21:25 -0800 X-Received: by mail-qv1-f43.google.com with SMTP id 6a1803df08f44-67ab41956f8so14917886d6.2 for ; Sun, 10 Dec 2023 06:21:24 -0800 (PST) X-Gm-Message-State: AuO9Lm4DYryVkg0PgSoh2RScx7686176AA= X-Google-Smtp-Source: AGHT+IEI0SOBNGnJNYFhVfuDtLcPoEJp3/Ey06+eUuKMr087xa8sey+XjFl0yHu/pW6AWvhlrFRri4F8rFQGeyPhwes= X-Received: by 2002:a0c:e907:0:b0:67e:aa3b:5ca6 with SMTP id a7-20020a0ce907000000b0067eaa3b5ca6mr1543722qvo.111.1702218083846; Sun, 10 Dec 2023 06:21:23 -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: Sun, 10 Dec 2023 19:51:12 +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="00000000000037e460060c28885b" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=ipXtcP1R; 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 --00000000000037e460060c28885b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Thanks for the review. My comments inline: On Fri, Dec 8, 2023 at 9:58=E2=80=AFAM Sunil V L = wrote: > On Thu, Dec 07, 2023 at 10:31:48AM +0530, Dhaval Sharma wrote: > > 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 n= ot > > > 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 on= ly > if > > > > + # previous stage has feature enabled and user wants to disable i= t. > > > > + # > > > > + > > > > 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.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. A= ll > > > 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 > this > > > size > > > > > anytime in the near future. > > > What do you mean by no plan to change this size? Isn't it a choice of > the platform? Given that this macro is in MdePkg, it will force people > to use the same size. [Dhaval] My understanding is that this cache block size is fixed for RVA profile which is where this code will be mostly used. Custom implementations can change it if they want. Second, we are planning to introduce a cache block through CPU HOB which when available will replace this. So I thought it is okay to defer this implementation and use a simpler option. > > > +#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 coherenc= y > > > domain > > > > + of the calling CPU. If Address is not aligned on a cache line > > > boundary, > > > > + then entire cache line containing Address is operated. If Addres= s > + > > > 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 =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)) & ~(CacheLineS= ize > - > > > 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 > > :) > > > What was the actual comment? I am not aware of any such guidelines and > __func__ is widely used. > > This was Pedro's comment on V6. > + End =3D (Start + Length + (CacheLineSize - 1)) & ~(CacheLineSize - = 1); > + Start &=3D ~((UINTN)CacheLineSize - 1); > + > + DEBUG ( > + (DEBUG_INFO, > + "%a Performing Cache Management Operation %d \n", __func__, Op) > + ); nit: Can we pick a log style here? Like : In this case, "CacheOpCacheRange: Performing ...". It's just prettier and more greppable. My interpretation of this was removing __func__ and instead having some relevant text would make it more searchable. And it kind of did make sense to me. I know many places __func__ is used but this is just a perspective. > > > > > > [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 of > > > the > > > > - calling CPU. > > > > + calling CPU. Risc-V does not have currently an CBO implementatio= n > > > 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 + Lengt= h > 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 efficien= t > 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 o= n > 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? > > > > [Dhaval] will do. > > > > > > > > > > > @param Address The base address of the instruction cache lines = to > > > > invalidate. If the CPU is in a physical addressi= ng > > > 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? > > > Yes. > > The reason I modified the text was- "Supported" in a way gives sense that this implementation does not support (and some other may support). But from spec perspective it is not supposed to be supported using CBO. Kind of nit. I am okay to bring earlier comment back if that is more readable. > -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 (#112263): https://edk2.groups.io/g/devel/message/112263 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- --00000000000037e460060c28885b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks for the review. My comments inline= :

On Fri, Dec 8, 2023 at 9:58=E2=80=AFAM Sunil V L <sunilvl@ventanamicro.com> w= rote:
On Thu, De= c 07, 2023 at 10:31:48AM +0530, Dhaval Sharma wrote:
> Comments inline:
>
>
> On Wed, Dec 6, 2023 at 7:50=E2=80=AFPM Sunil V L <sunilvl@ventanamicro.com&g= t; wrote:
>
> > Hi Dhaval,
> >
> > Thank you very much for fixing the issue with instruction cache > > invalidation and confirming with the spec owner. Few minor commen= ts
> > below.
> >
> > On Mon, Dec 04, 2023 at 01:59:49PM +0530, Dhaval Sharma wrote: > > > Use newly defined cache management operations for RISC-V whe= re possible
> > > It builds up on the support added for RISC-V cache managemen= t
> > > 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 i= nvalidation. Use fence.i
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0instruction as CMO does not suppor= t i-cache operations.
> > >=C2=A0 =C2=A0 =C2=A0V8:
> > >=C2=A0 =C2=A0 =C2=A0- Added note to convert PCD into RISC-V f= eature bitmap pointer
> > >=C2=A0 =C2=A0 =C2=A0- Modified function names to be more expl= icit 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 feed= back on V6
> > >=C2=A0 =C2=A0 =C2=A0- Make naming consistent to CMO, remove a= ll CBO references
> > >=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 i= f HW supports it
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0This patch is part of restructurin= g on top of v5
> > >
> > IMO, it is better to keep the change log in the cover letter. Sin= ce not
> > all patches may be CC'd to every one apart from the cover let= ter, 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.
>
> >
> > >=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 8 +
> > >=C2=A0 MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMainte= nanceLib.inf |
> >=C2=A0 5 +
> > >=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 4 +
> > >=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,
> > PcdsPatchableInModule.AARCH64]
> > >=C2=A0 =C2=A0 # @Prompt CPU Rng algorithm's GUID.
> > >
> > gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0= x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*= |0x00000037
> > >
> > > +[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 #
> > > +
> > gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFF= FF|UINT64|0x69
> > > +
> > >=C2=A0 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic,= PcdsDynamicEx]
> > >=C2=A0 =C2=A0 ## This value is used to set the base address o= f PCI express
> > hierarchy.
> > >=C2=A0 =C2=A0 # @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/BaseCacheMainte= nanceLib.inf
> > > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMainte= nanceLib.inf
> > > @@ -56,3 +56,8 @@ [LibraryClasses]
> > >=C2=A0 =C2=A0 BaseLib
> > >=C2=A0 =C2=A0 DebugLib
> > >
> > > +[LibraryClasses.RISCV64]
> > > +=C2=A0 PcdLib
> > > +
> > > +[Pcd.RISCV64]
> > > +=C2=A0 gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride=C2= =A0 ## CONSUMES
> > > diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCac= he.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 @@
> > >=C2=A0 =C2=A0 RISC-V specific functionality for cache.
> > >
> > >=C2=A0 =C2=A0 Copyright (c) 2020, Hewlett Packard Enterprise = Development LP. All
> > rights reserved.<BR>
> > > +=C2=A0 Copyright (c) 2023, Rivos Inc. All rights reserved.&= lt;BR>
> > >
> > >=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 Op= eration
> > > +// enabling decision based on RISC-V CPU HOB in
> > > +// future when it is available and convert PcdRiscVFeatureO= verride
> > > +// PCD to a pointer that contains pointer to bitmap structu= re
> > > +// 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
> > thought to keep it simple for now. Anyways there is no plan to ch= ange this
> > size
> >
>=C2=A0 =C2=A0 =C2=A0anytime in the near future.
>
What do you mean by no plan to change this size? Isn't it a choice of the platform? Given that this macro is in MdePkg, it will force people
to use the same size.
=C2=A0
[Dhaval] My underst= anding is that this cache block size is fixed for RVA profile which is wher= e this code will=C2=A0be mostly used. Custom implementations can change it = if they want. Second, we are planning to introduce a cache block through CP= U HOB which when available will replace this.
So I thought it is = okay to defer this implementation and use a simpler option.
=C2= =A0
> > +#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_FEATURE_CMO_BITMASK) !=3D 0);
> > > +}
> > > +
> > > +/**
> > > +=C2=A0 Performs required opeartion on cache lines in the ca= che coherency
> > domain
> > > +=C2=A0 of the calling CPU. If Address is not aligned on a c= ache line
> > boundary,
> > > +=C2=A0 then entire cache line containing Address is operate= d. If Address +
> > Length
> > > +=C2=A0 is not aligned on a cache line boundary, then the en= tire cache line
> > > +=C2=A0 containing Address + Length -1 is operated.
> > > +=C2=A0 If Length is greater than (MAX_ADDRESS - Address + 1= ), then ASSERT().
> > > +=C2=A0 @param=C2=A0 Address The base address of the cache l= ines 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 inv= alidate from 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 pe= rformed
> > > +=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 Cache= OpFlush) && (Op !=3D
> > CacheOpClean)) {
> > > +=C2=A0 =C2=A0 return;
> > > +=C2=A0 }
> > > +
> > > +=C2=A0 ASSERT ((Length - 1) <=3D (MAX_ADDRESS - (UINTN)A= ddress));
> > > +
> > > +=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 + (CacheLineSiz= e - 1)) & ~(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 review there was a suggestion to define a func= tion
> name which is more greppable. Hence I had changed it from __func__ to = this
> :)
>
What was the actual comment? I am not aware of any such guidelines and
__func__ is widely used.

This was Pedro's comment on V6.
> +=C2=A0 End=C2=A0 =C2=A0= =3D (Start + Length + (CacheLineSize - 1)) & ~(CacheLineSize - 1);
= > +=C2=A0 Start &=3D ~((UINTN)CacheLineSize - 1);
> +
> = +=C2=A0 DEBUG (
> +=C2=A0 =C2=A0 (DEBUG_INFO,
> +=C2=A0 =C2=A0 = =C2=A0"%a Performing Cache Management Operation %d \n", __func__,= Op)
> +=C2=A0 =C2=A0 );

nit: Can we pick a log st= yle here? Like <something>: <log message>
In this case, &quo= t;CacheOpCacheRange: Performing ...". It's just prettier
a= nd more greppable.
My interpretation of this was removing __func_= _ and instead having some relevant text would make it more searchable.
And it kind of did make sense to me. I know many places __func__ is u= sed but this is just a perspective.=C2=A0=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 Invalidates the entire instruction cache in cac= he coherency domain of
> > the
> > > -=C2=A0 calling CPU.
> > > +=C2=A0 calling CPU. Risc-V does not have currently an CBO i= mplementation
> > which can
> > > +=C2=A0 invalidate the entire I-cache. Hence using Fence ins= truction for now.
> > P.S.
> > > +=C2=A0 Fence instruction may or may not implement full I-ca= che invd
> > functionality
> > > +=C2=A0 on all implementations.
> > >
> > >=C2=A0 **/
> > >=C2=A0 VOID
> > > @@ -28,17 +136,10 @@ InvalidateInstructionCache (
> > >=C2=A0 =C2=A0 Invalidates a range of instruction cache lines = in the cache coherency
> > domain
> > >=C2=A0 =C2=A0 of the calling CPU.
> > >
> > > -=C2=A0 Invalidates the instruction cache lines specified by= Address and
> > Length. If
> > > -=C2=A0 Address is not aligned on a cache line boundary, the= n entire
> > instruction
> > > -=C2=A0 cache line containing Address is invalidated. If Add= ress + Length is
> > not
> > > -=C2=A0 aligned on a cache line boundary, then the entire in= struction cache
> > line
> > > -=C2=A0 containing Address + Length -1 is invalidated. This = function may
> > choose to
> > > -=C2=A0 invalidate the entire instruction cache if that is m= ore efficient than
> > > -=C2=A0 invalidating the specified range. If Length is 0, th= en no instruction
> > cache
> > > -=C2=A0 lines are invalidated. Address is returned.
> > > -
> > > -=C2=A0 If Length is greater than (MAX_ADDRESS - Address + 1= ), then ASSERT().
> > > -
> > > +=C2=A0 An operation from a CMO instruction is defined to op= erate only on the
> > copies of a cache block that are
> > > +=C2=A0 cached in the caches accessible by the explicit memo= ry accesses
> > performed by the set of coherent agents.
> > > +=C2=A0 In other words CMO operations are not applicable to = instruction
> > cache. Use fence.i instruction instead
> > > +=C2=A0 to achieve the same purpose.
> > Could you please keep the comments within 80 character?
>
> [Dhaval] will do.
>
>
> >
> > >=C2=A0 =C2=A0 @param=C2=A0 Address The base address of the in= struction cache lines to
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 invalidate. 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 Address 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 instructio= n cache instead.\n", __func__)
> > > -=C2=A0 =C2=A0 );
> > > +=C2=A0 DEBUG ((DEBUG_VERBOSE, "InvalidateInstructionCa= che:\n"));
> > This change is not required.
> >
> > [Dhaval] Are you saying that the earlier comment was fine as it w= as?
>
Yes.

The reason I modified the text was- "Supported&q= uot; in a way gives sense that this implementation does not support (and so= me other may support). But from spec perspective it is not supposed to be s= upported using CBO. Kind of nit. I am okay to bring earlier comment back if= that is more readable.=C2=A0=C2=A0
-Sunil


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

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

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

_._,_._,_
--00000000000037e460060c28885b--