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 76176D8027F for ; Tue, 24 Oct 2023 20:09:20 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=J7yzvawVznjYx1vqsUbO3OzB7J/vX2v/ZV+WEAokja4=; 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:Content-Transfer-Encoding; s=20140610; t=1698178159; v=1; b=jnrA9PiueShrnbfAmqv4rDJae5Ryh4YtU0CDzVwod/u2OAw/nZmpyr5JU0rJZF5wFmwG58TI mO87O6RoiHVTHzI3LS54+Jiqdj7zLxFsCnyBqrwVBauHKQNmL1ruUcT7rIIeclSmRgaZgckx0wM +ieuwSwETwVtSWlkhkrurlyo= X-Received: by 127.0.0.2 with SMTP id dYXlYY7687511xwz1c74ONFP; Tue, 24 Oct 2023 13:09:19 -0700 X-Received: from mail-ua1-f53.google.com (mail-ua1-f53.google.com [209.85.222.53]) by mx.groups.io with SMTP id smtpd.web11.28230.1698178158408288385 for ; Tue, 24 Oct 2023 13:09:18 -0700 X-Received: by mail-ua1-f53.google.com with SMTP id a1e0cc1a2514c-7a52a27fe03so1895145241.0 for ; Tue, 24 Oct 2023 13:09:18 -0700 (PDT) X-Gm-Message-State: L4FtLQns4AqQG6UbrZDXpdmix7686176AA= X-Google-Smtp-Source: AGHT+IGiqhpmDiE3TG5Aj/geANa+a0WI400j4j2MhP26XFJlEEs/YYIhIy3YjcB5KUP3lUTMgYaNXqp9seKCUT9vDfw= X-Received: by 2002:a67:cc08:0:b0:457:cb2a:e917 with SMTP id q8-20020a67cc08000000b00457cb2ae917mr11708548vsl.15.1698178157142; Tue, 24 Oct 2023 13:09:17 -0700 (PDT) MIME-Version: 1.0 References: <20231021173314.19363-1-dhaval@rivosinc.com> <20231021173314.19363-5-dhaval@rivosinc.com> In-Reply-To: <20231021173314.19363-5-dhaval@rivosinc.com> From: "Pedro Falcato" Date: Tue, 24 Oct 2023 21:09:05 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH v6 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V To: devel@edk2.groups.io, dhaval@rivosinc.com Cc: 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,pedro.falcato@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=jnrA9Piu; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=none) 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.A= ARCH64] > # @Prompt CPU Rng algorithm's GUID. > gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0= x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x00= 000037 > > +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64] > + # > + # Configurability to override RISC-V CPU Features > + # BIT 0 =3D Cache Management Operations. This bit is relevant only if > + # previous stage has feature enabled and user wants to disable it. > + # > + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UI= NT64|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/BaseCacheMaintenanceL= ib.inf b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf > index 6fd9cbe5f6c9..39a7fb963b49 100644 > --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf > @@ -56,3 +56,5 @@ [LibraryClasses] > BaseLib > DebugLib > > +[Pcd.RISCV64] > + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES > diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg= /Library/BaseCacheMaintenanceLib/RiscVCache.c > index 4eb18edb9aa7..6851970c9e16 100644 > --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c > @@ -1,7 +1,8 @@ > /** @file > - RISC-V specific functionality for cache. > + Implement Risc-V Cache Management Operations Why the change? You're effectively implementing cache management for riscv, you're not exclusively using any sort of extension (such as CMO). > > Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rig= hts 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 :) > + // If CMO is disabled in HW, skip Override check > + // Otherwise this PCD can override settings > + return ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_CMO_BI= TMASK) !=3D 0); > +} > + > +/** > + Performs required opeartion on cache lines in the cache coherency doma= in > + of the calling CPU. If Address is not aligned on a cache line boundary= , > + then entire cache line containing Address is operated. If Address + Le= ngth > + 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 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. > + > + 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 of = the > - calling CPU. > + calling CPU. Risc-V does not have currently an CBO implementation whic= h can > + invalidate entire I-cache. Hence using Fence instruction for now. P.S.= Fence nit: Invalidate *the* entire I-cache Also, what do you mean? 1) you're calling CacheOpCacheRange for the range 2) please don't mix CBO and CMO. Too much terminology :) 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. > + instruction may or may not implement full I-cache invd functionality o= n 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__) 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). > + } > + > 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 addressin= g > - mode, then Address is a virtual address. > + @param Address The base address of the data cache lines to write back= . > @param Length The number of bytes to write back from the data cache. > > @return Address of cache written in main memory. > @@ -172,7 +281,12 @@ WriteBackDataCacheRange ( > IN UINTN Length > ) > { > - DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__)); > + if (RiscVIsCMOEnabled ()) { > + CacheOpCacheRange (Address, Length, Clean); > + } else { > + DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__))= ; Same comment here WRT assert. > + } > + > 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 addressin= g mode, > - then Address is a virtual address. > + @param Address The base address of the data cache lines to invalidate= . > @param Length The number of bytes to invalidate from the data cache. > > @return Address. > @@ -230,6 +341,16 @@ InvalidateDataCacheRange ( > IN UINTN Length > ) > { > - DEBUG ((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __func__)); > + if (RiscVIsCMOEnabled ()) { > + CacheOpCacheRange (Address, Length, Invld); > + } else { > + DEBUG ( > + (DEBUG_WARN, > + "%a:RISC-V unsupported function.\n" > + "Invalidating the whole Data cache instead.\n", __func__) > + ); Aaaand the same comment here WRT warning. Pedro -=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 (#110010): https://edk2.groups.io/g/devel/message/110010 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-