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 570F574003C for ; Mon, 11 Dec 2023 15:54:31 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=TD7fF2ur8ReaxuYK4nY+Zv2KM3drZGidjRmAr15wHRE=; 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=1702310069; v=1; b=OuOqHLMC703MDoyUtaR5n+5GxzpcLDYWDdQwNhyJ+OjkyUkZH9eHPwQilmGvGo9M4VKYaN+P bWtq0OlGAUa2+5z77FO7PG8q43/rEraVMG/DQfiZoPzSllJiRsJYbxNRgBi84AwZNBTuCy1gzX4 8PhHINkGfKZWOcL9QM3BsHWk= X-Received: by 127.0.0.2 with SMTP id yRToYY7687511x9g8TpHGUHn; Mon, 11 Dec 2023 07:54:29 -0800 X-Received: from mail-vs1-f41.google.com (mail-vs1-f41.google.com [209.85.217.41]) by mx.groups.io with SMTP id smtpd.web10.11889.1702310069096709671 for ; Mon, 11 Dec 2023 07:54:29 -0800 X-Received: by mail-vs1-f41.google.com with SMTP id ada2fe7eead31-4649d22b78aso2617730137.0 for ; Mon, 11 Dec 2023 07:54:28 -0800 (PST) X-Gm-Message-State: GfevgD30TXAC91FfQydZ3WmYx7686176AA= X-Google-Smtp-Source: AGHT+IFvTgNNIXSB0jsYjS7EClAsK0594jVkd1+f0iRQbXuryKb6svyeRVXU9anHSmJ0EPDrVCLyN3YN64nYH5U2Yqc= X-Received: by 2002:a05:6102:509f:b0:464:9e34:e459 with SMTP id bl31-20020a056102509f00b004649e34e459mr4095153vsb.13.1702310067689; Mon, 11 Dec 2023 07:54:27 -0800 (PST) MIME-Version: 1.0 References: <20231204082950.96914-1-dhaval@rivosinc.com> <20231204082950.96914-5-dhaval@rivosinc.com> In-Reply-To: <20231204082950.96914-5-dhaval@rivosinc.com> From: "Pedro Falcato" Date: Mon, 11 Dec 2023 15:54:16 +0000 Message-ID: Subject: Re: [edk2-devel] [PATCH v9 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=OuOqHLMC; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=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 On Mon, Dec 4, 2023 at 8:30=E2=80=AFAM 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 messa= ge > - Added RB tag > V6: > - 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 | 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.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..601a38d6c109 100644 > --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf > @@ -56,3 +56,8 @@ [LibraryClasses] > BaseLib > DebugLib > > +[LibraryClasses.RISCV64] > + PcdLib > + > +[Pcd.RISCV64] > + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES > diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg= /Library/BaseCacheMaintenanceLib/RiscVCache.c > index ac2a3c23a249..cacc38eff4f4 100644 > --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c > @@ -2,6 +2,7 @@ > RISC-V specific functionality for cache. > > Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rig= hts 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 > +#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_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 CacheOpInvld) && (Op !=3D CacheOpFlush) && (Op !=3D Cache= OpClean)) { > + 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:\ > + Performing Cache Management Operation %d \n", Op) > + ); Two comments: 1) You probably want DEBUG_VERBOSE here, if anything. Cache operations may be too common/boring for an INFO log. 2) Your string splitting looks waaaaaaacky. That's not how you split strings. See the differences: https://godbolt.org/z/64WjT9oKo (If what you're doing actually results in normal-ish looking strings, it may be due to a weird CPP corner case I'm not aware of. Not my fault :P) > + > + 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 implementation whic= h 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 functiona= lity > + 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 Lengt= h. If > - Address is not aligned on a cache line boundary, then entire instructi= on > - cache line containing Address is invalidated. If Address + Length is n= ot > - aligned on a cache line boundary, then the entire instruction cache li= ne > - containing Address + Length -1 is invalidated. This function may choos= e to > - invalidate the entire instruction cache if that is more efficient than > - invalidating the specified range. If Length is 0, then no instruction = cache > - lines are invalidated. Address is returned. > - > - If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT(). > - > + An operation from a CMO instruction is defined to operate only on the = copies of a cache block that are > + cached in the caches accessible by the explicit memory accesses perfor= med 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. > @param Address The base address of the instruction cache lines to > invalidate. If the CPU is in a physical addressing mod= e, then > Address is a physical address. If the CPU is in a virt= ual > @@ -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")); > 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")); ASSERT(FALSE) here as well? > @@ -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") > + ); I don't understand why the strings are getting so weirdly split (using the \ and in such short chunks). Is this uncrustify? This also applies to the above DEBUG()'s. --=20 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 (#112313): https://edk2.groups.io/g/devel/message/112313 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-