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 4C9BEAC0D6C for ; Sun, 29 Oct 2023 14:48:15 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=YTgo26lCsqaAMnw40nJKGnH74RTDTJheZclIk2iLWq4=; 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=1698590894; v=1; b=QeWKXzCzWt4CPsIBQL2ln9QXiy7QthpdaadfSfgJiFzW1E8bVtiehUlaD2HtuKeFldFoKoiS yJpXj5ughoh6/n9KQvtsrg95OiF3fGqEGUHgy0z/N0WAWPg7KfETMRsLheLUvLNa9UKa4BKYhp0 D1djSHxCe8+7gJiihl6FFLiY= X-Received: by 127.0.0.2 with SMTP id BXBGYY7687511xWhAfTAdAs6; Sun, 29 Oct 2023 07:48:14 -0700 X-Received: from mail-yw1-f173.google.com (mail-yw1-f173.google.com [209.85.128.173]) by mx.groups.io with SMTP id smtpd.web11.73670.1698590893240624810 for ; Sun, 29 Oct 2023 07:48:13 -0700 X-Received: by mail-yw1-f173.google.com with SMTP id 00721157ae682-5a7ac4c3666so31119977b3.3 for ; Sun, 29 Oct 2023 07:48:13 -0700 (PDT) X-Gm-Message-State: jmfwJNWvaamDcvbF1wPRSHDLx7686176AA= X-Google-Smtp-Source: AGHT+IFIrvHd+pK2j+Y7p25l3jDj0sNeKktalxP5xd/cgh8mi4Jfr8AcEmM15S9WW+6sZStGFsJCbCYzcX5L3FxnLJ8= X-Received: by 2002:a05:690c:72d:b0:583:d9dd:37fd with SMTP id bt13-20020a05690c072d00b00583d9dd37fdmr7045199ywb.31.1698590892405; Sun, 29 Oct 2023 07:48:12 -0700 (PDT) MIME-Version: 1.0 References: <20231021173314.19363-1-dhaval@rivosinc.com> <20231021173314.19363-5-dhaval@rivosinc.com> In-Reply-To: From: "Dhaval Sharma" Date: Sun, 29 Oct 2023 20:18:01 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v6 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V To: Pedro Falcato 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="000000000000c2cca80608dc0296" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=QeWKXzCz; 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 --000000000000c2cca80608dc0296 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Replied inline. Most of the cases I have addressed in the new patch I submitted. On Wed, Oct 25, 2023 at 1:39=E2=80=AFAM Pedro Falcato wrote: > On Sat, Oct 21, 2023 at 6:33=E2=80=AFPM 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 > > --- > > > > 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*|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..39a7fb963b49 100644 > > --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.in= f > > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.in= f > > @@ -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 > > Why the change? You're effectively implementing cache management for > riscv, you're not exclusively using any sort of extension (such as > CMO). > Done. I believe you meant to keep it a generic description. > > > > > 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,111 @@ > > #include > > #include > > #include > > +#include > > + > > +// 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. > > Too many TODOs? One TODO at the top of the file (mentioning feature > detection, cache line size detection) should be enough. There's no > point in peppering these out throughout the file :) > > Done. > > + // 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 Invld) && (Op !=3D Flush) && (Op !=3D Clean)) { > > + 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, > > + "%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. > > Done. > > + > > + do { > > + switch (Op) { > > + case Invld: > > + RiscVCpuCacheInvalAsmCbo (Start); > > + break; > > + case Flush: > > + RiscVCpuCacheFlushAsmCbo (Start); > > + break; > > + case Clean: > > + RiscVCpuCacheCleanAsmCbo (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 entire I-cache. Hence using Fence instruction for now. > P.S. Fence > nit: Invalidate *the* entire I-cache > > Done. > Also, what do you mean? 1) you're calling CacheOpCacheRange for the range > 2) please don't mix CBO and CMO. Too much terminology :) > > Modified naming to be consistent. All CMO now. > Does Zicbom only operate on the dcache? Or does it also touch the > icache? As far as I'm aware, riscv does not require a dcache/icache > coherency (like ARM), but a quick stroll through the extension's spec > didn't lead me to much. > My understanding is that Zicbom operates on both. It just needs an address to act on. > > + 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__= ) > > + ); > nit: "%a: ", with a space, but i'd probably write this bit as > > + "%a: Zicbom not enabled, invalidating the whole instruction > cache instead.\n", __func__) > > Done with minor modification. Given that this may be repeatedly called, can we just warn once (e.g > on a constructor) that CMO is disabled and leave this be? Since the > cache management is in fact being done correctly, but just in a slower > way. > > > + 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__)); > > Ok, so here it may make sense to ASSERT, as you're no longer complying > to the function's behavior (and this is unsafe). > > Done. > > + } > > + > > 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 cach= e. > > > > @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__)); > > Same comment here WRT assert. > Done. > > + } > > + > > 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 cach= e. > > > > @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__) > > + ); > > Aaaand the same comment here WRT warning. > > Done. > Pedro > --=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 (#110268): https://edk2.groups.io/g/devel/message/110268 Mute This Topic: https://groups.io/mt/102103783/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- --000000000000c2cca80608dc0296 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Replied inline. Most of the cases I have addressed in= the new patch I submitted.

On Wed, Oct 25, 2023 at 1:39=E2=80=AFAM Pedro Fa= lcato <pedr= o.falcato@gmail.com> wrote:
On Sat, Oct 21, 2023 at 6:33=E2=80=AFPM Dhaval Sharma &l= t;dhaval@rivosinc.= com> 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>
> ---
>
> Notes:
>=C2=A0 =C2=A0 =C2=A0V1:
>=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
>
>=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=A02 +
>=C2=A0 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c=C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 159 +++++++++++++++++---
>=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, 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, 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
>
> +[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..39a7fb963b49 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.i= nf
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.i= nf
> @@ -56,3 +56,5 @@ [LibraryClasses]
>=C2=A0 =C2=A0 BaseLib
>=C2=A0 =C2=A0 DebugLib
>
> +[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 4eb18edb9aa7..6851970c9e16 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> @@ -1,7 +1,8 @@
>=C2=A0 /** @file
> -=C2=A0 RISC-V specific functionality for cache.
> +=C2=A0 Implement Risc-V Cache Management Operations

Why the change? You're effectively implementing cache management for riscv, you're not exclusively using any sort of extension (such as
CMO).
Done. I believe you meant to keep it a generic d= escription.=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 SPDX-License-Identifier: BSD-2-Clause-Patent
>=C2=A0 **/
> @@ -9,10 +10,111 @@
>=C2=A0 #include <Base.h>
>=C2=A0 #include <Library/BaseLib.h>
>=C2=A0 #include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +
> +// TODO: This will be removed once RISC-V CPU HOB is available
> +#define RISCV_CACHE_BLOCK_SIZE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A064 > +#define RISCV_CPU_FEATURE_CMO_BITMASK=C2=A0 0x1
> +
> +typedef enum {
> +=C2=A0 Clean,
> +=C2=A0 Flush,
> +=C2=A0 Invld,
> +} 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 // TODO: Add check for CMO from CPU HOB.

Too many TODOs? One TODO at the top of the file (mentioning feature
detection, cache line size detection) should be enough. There's no
point in peppering these out throughout the file :)

Done.=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 Invld) && (Op !=3D Flush) && (Op = !=3D Clean)) {
> +=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"%a Performing Cache Management Operation %d= \n", __func__, Op)
> +=C2=A0 =C2=A0 );

nit: Can we pick a log style here? Like <something>: <log message&= gt;
In this case, "CacheOpCacheRange: Performing ...". It's just = prettier
and more greppable.

Done.=C2=A0
> +
> +=C2=A0 do {
> +=C2=A0 =C2=A0 switch (Op) {
> +=C2=A0 =C2=A0 =C2=A0 case Invld:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 RiscVCpuCacheInvalAsmCbo (Start);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +=C2=A0 =C2=A0 =C2=A0 case Flush:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 RiscVCpuCacheFlushAsmCbo (Start);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> +=C2=A0 =C2=A0 =C2=A0 case Clean:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 RiscVCpuCacheCleanAsmCbo (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 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 entire I-cache. Hence using Fence instruction for n= ow. P.S. Fence
nit: Invalidate *the* entire I-cache

Done.=C2=A0
Also, what do you mean? 1) you're calling CacheOpCacheRange for the ran= ge
2) please don't mix CBO and CMO. Too much terminology :)

Modified naming to be consistent. All CMO now.
<= div>=C2=A0
Does Zicbom only operate on the dcache? Or does it also touch the
icache? As far as I'm aware, riscv does not require a dcache/icache
coherency (like ARM), but a quick stroll through the extension's spec didn't lead me to much.
My understanding is that Z= icbom operates on both. It just needs an address to act on.
=C2= =A0
> +=C2=A0 instruction may or may not implement full I-cache invd functio= nality on all
> +=C2=A0 implementations.
>
>=C2=A0 **/
>=C2=A0 VOID
> @@ -56,12 +158,17 @@ 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 InvalidateInstructionCache ();
> +=C2=A0 if (RiscVIsCMOEnabled ()) {
> +=C2=A0 =C2=A0 CacheOpCacheRange (Address, Length, Invld);
> +=C2=A0 } else {
> +=C2=A0 =C2=A0 DEBUG (
> +=C2=A0 =C2=A0 =C2=A0 (DEBUG_WARN,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"%a:RISC-V unsupported function.\n&qu= ot;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"Invalidating the whole instruction c= ache instead.\n", __func__)
> +=C2=A0 =C2=A0 =C2=A0 );
nit: "%a: ", with a space, but i'd probably write this bit as=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"%a: Zicbom not enabled, invalidating= the whole instruction cache instead.\n", __func__)

Done with minor modification.

Given that this may be repeatedly called, can we just warn once (e.g
on a constructor) that CMO is disabled and leave this be? Since the
cache management is in fact being done correctly, but just in a slower
way.

> +=C2=A0 =C2=A0 InvalidateInstructionCache ();
> +=C2=A0 }
> +
>=C2=A0 =C2=A0 return Address;
>=C2=A0 }
>
> @@ -117,7 +224,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, Flush);
> +=C2=A0 } else {
> +=C2=A0 =C2=A0 DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported functi= on.\n", __func__));

Ok, so here it may make sense to ASSERT, as you're no longer complying<= br> to the function's behavior (and this is unsafe).

=C2=A0Done.
> +=C2=A0 }
> +
>=C2=A0 =C2=A0 return Address;
>=C2=A0 }
>
> @@ -156,10 +268,7 @@ WriteBackDataCache (
>
>=C2=A0 =C2=A0 If Length is greater than (MAX_ADDRESS - Address + 1), th= en ASSERT().
>
> -=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 @return Address of cache written in main memory.
> @@ -172,7 +281,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, Clean);
> +=C2=A0 } else {
> +=C2=A0 =C2=A0 DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported functi= on.\n", __func__));

Same comment here WRT assert.
Done.=C2=A0
> +=C2=A0 }
> +
>=C2=A0 =C2=A0 return Address;
>=C2=A0 }
>
> @@ -214,10 +328,7 @@ InvalidateDataCache (
>
>=C2=A0 =C2=A0 If Length is greater than (MAX_ADDRESS - Address + 1), th= en ASSERT().
>
> -=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 @return Address.
> @@ -230,6 +341,16 @@ 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, Invld);
> +=C2=A0 } else {
> +=C2=A0 =C2=A0 DEBUG (
> +=C2=A0 =C2=A0 =C2=A0 (DEBUG_WARN,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"%a:RISC-V unsupported function.\n&qu= ot;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"Invalidating the whole Data cache in= stead.\n", __func__)
> +=C2=A0 =C2=A0 =C2=A0 );

Aaaand the same comment here WRT warning.

Done.=C2=A0
Pedro


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

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

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

_._,_._,_
--000000000000c2cca80608dc0296--