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 49023AC1393 for ; Mon, 11 Dec 2023 13:12:05 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=P8xVHODrOw2amUHzCQfzEsmIhO1Fa4vuWbgL9UDoy5A=; c=relaxed/simple; d=groups.io; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Disposition:Content-Transfer-Encoding; s=20140610; t=1702300323; v=1; b=qFUD/dSLuQHzhc/xDCnTSg649E+J9ofskShycBH/MONbbC7xBAo6p+FbFG3xcAVg0ZNaAen2 w8Q0XEX+m8uJ8YnP80ilADjCNHbO+f/gIDbkA/H97m2RPE3zuroXDZ5uwLgKenE9/hLhUheOgwE KkHQeMiBT7NxuEYIlJ9dZv14= X-Received: by 127.0.0.2 with SMTP id QcvaYY7687511xiLO3557zcu; Mon, 11 Dec 2023 05:12:03 -0800 X-Received: from mail-oo1-f53.google.com (mail-oo1-f53.google.com [209.85.161.53]) by mx.groups.io with SMTP id smtpd.web11.7868.1702300323015608955 for ; Mon, 11 Dec 2023 05:12:03 -0800 X-Received: by mail-oo1-f53.google.com with SMTP id 006d021491bc7-5906e03a7a4so2352044eaf.1 for ; Mon, 11 Dec 2023 05:12:02 -0800 (PST) X-Gm-Message-State: mIJOZTRtyzTkGYlAS5XHccvGx7686176AA= X-Google-Smtp-Source: AGHT+IGYh1AARQIdBaSq8segeFdH5HHRVtQcKz3ooKmxBtaFUSAczPLwGuKqj6DAm7ebzDz5n0haVg== X-Received: by 2002:a05:6359:4c1f:b0:170:c56d:3504 with SMTP id kj31-20020a0563594c1f00b00170c56d3504mr1612111rwc.9.1702300322060; Mon, 11 Dec 2023 05:12:02 -0800 (PST) X-Received: from sunil-laptop ([106.51.188.200]) by smtp.gmail.com with ESMTPSA id a7-20020a056a000c8700b006ce9da2e389sm6460775pfv.13.2023.12.11.05.11.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 11 Dec 2023 05:12:01 -0800 (PST) Date: Mon, 11 Dec 2023 18:41:56 +0530 From: "Sunil V L" To: Dhaval Sharma Cc: devel@edk2.groups.io, Michael D Kinney , Liming Gao , Zhiguang Liu , Laszlo Ersek Subject: Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V Message-ID: References: <20231204082950.96914-1-dhaval@rivosinc.com> <20231204082950.96914-5-dhaval@rivosinc.com> MIME-Version: 1.0 In-Reply-To: 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,sunilvl@ventanamicro.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="qFUD/dSL"; 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 On Sun, Dec 10, 2023 at 07:51:12PM +0530, Dhaval Sharma wrote: > Thanks for the review. My comments inline: > > On Fri, Dec 8, 2023 at 9:58 AM 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 PM 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*|0x00000037 > > > > > > > > > > +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64] > > > > > + # > > > > > + # Configurability to override RISC-V CPU Features > > > > > + # BIT 0 = Cache Management Operations. This bit is relevant only > > if > > > > > + # previous stage has feature enabled and user wants to disable it. > > > > > + # > > > > > + > > > > > > gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UINT64|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. 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 > > 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. > Platforms may not be aligned with a profile. But I am fine if you want to add the flexibility as part of CPU HOB. > > > > > +#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) != 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 > > 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 == 0) { > > > > > + return; > > > > > + } > > > > > + > > > > > + if ((Op != CacheOpInvld) && (Op != CacheOpFlush) && (Op != > > > > CacheOpClean)) { > > > > > + return; > > > > > + } > > > > > + > > > > > + ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)Address)); > > > > > + > > > > > + CacheLineSize = RISCV_CACHE_BLOCK_SIZE; > > > > > + > > > > > + Start = (UINTN)Address; > > > > > + // > > > > > + // Calculate the cache line alignment > > > > > + // > > > > > + End = (Start + Length + (CacheLineSize - 1)) & ~(CacheLineSize > > - > > > > 1); > > > > > + Start &= ~((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 = (Start + Length + (CacheLineSize - 1)) & ~(CacheLineSize - 1); > > + Start &= ~((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. > I think the comment meant to follow a standard logging format since there was no ":" and a space in original change. I prefer __func__ over this so that we don't need to update multiple lines in case function name gets changed. > > > > > > > > [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 = Start + CacheLineSize; > > > > > + } while (Start != 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 > > > > 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 + 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 > > 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 > > > > 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? > > > > > 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. > I prefer to keep it unchanged. Thanks, Sunil -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112297): https://edk2.groups.io/g/devel/message/112297 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] -=-=-=-=-=-=-=-=-=-=-=-