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 98E0A941082 for ; Fri, 8 Dec 2023 04:28:15 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=xSIpe8owlf8UWUtzXngGwClUpd+qYYJheJ1P1+ZAdCA=; 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=1702009694; v=1; b=of5bTCKEo47CVFDqol6LKPI16ryMjasQcERm9TjRmWrfiZKh9W+kgpLItMUtB5m8BcPfUfqa 8GiFgeLFGqvXVSGC1lXyGTC5io3NTJUZ9qFygUPd9WmEkylQ01ynAxsmL/wFA1EIVIOw0PLDXCL 1YmCf1GK6073tt/etRHV491w= X-Received: by 127.0.0.2 with SMTP id HTBBYY7687511xkCLoMla33c; Thu, 07 Dec 2023 20:28:14 -0800 X-Received: from mail-oa1-f46.google.com (mail-oa1-f46.google.com [209.85.160.46]) by mx.groups.io with SMTP id smtpd.web10.13213.1702009693399817539 for ; Thu, 07 Dec 2023 20:28:13 -0800 X-Received: by mail-oa1-f46.google.com with SMTP id 586e51a60fabf-1fb37f25399so1098632fac.1 for ; Thu, 07 Dec 2023 20:28:13 -0800 (PST) X-Gm-Message-State: Js9G4wb465jKOYPURo8paqAGx7686176AA= X-Google-Smtp-Source: AGHT+IEH3tyin4h5g/Vx+Lx3oYO+Wkr5NJupRVKyYpgVKvoLapCebUh8AE3c4EpjspLB8BKptT9UAQ== X-Received: by 2002:a05:6871:430c:b0:1fa:dae2:8406 with SMTP id lu12-20020a056871430c00b001fadae28406mr4281050oab.0.1702009692693; Thu, 07 Dec 2023 20:28:12 -0800 (PST) X-Received: from sunil-laptop ([106.51.188.200]) by smtp.gmail.com with ESMTPSA id v34-20020a634662000000b005c259cef481sm589515pgk.59.2023.12.07.20.28.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Dec 2023 20:28:12 -0800 (PST) Date: Fri, 8 Dec 2023 09:58:06 +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=of5bTCKE; 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 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. > > +#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. > > > > [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. -Sunil -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112207): https://edk2.groups.io/g/devel/message/112207 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] -=-=-=-=-=-=-=-=-=-=-=-