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 10F46D801A6 for ; Mon, 8 Apr 2024 04:47:27 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=rnrumDtRAwT8MaTq0p6eRUEqNkyCpp0iHsn1UkVGa/0=; 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:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Disposition:Content-Transfer-Encoding; s=20240206; t=1712551646; v=1; b=nBgAfMu5+StHx2TNQ58eR0/yerXh3GvvOtQ0UDG/aMBApoWqF1xwTHWBLxriYnJNCWBzgOL+ eIvpcFvjk+O8AogRYRjfy/nhRfoVnfPfACtQAmpFq9yl8OLHX8I0/+LfWsunZwXDbBRtOCo+bsf TeoQoJzMzCh0FHBKJFq1/50VlQqaSwolF+SfB1AMc5mvOEHLKWxyRFiM4bL1LfHRjK/6xGi8iXt amM5aL9kxGo0vDojEcVkeY91nvZwTo/ElBQOEkyVyFwy+Mts9N6ORtwFAkVMysV+FZLKwEOaYED 3dFhNE4ia5stWG9JZVE5PBKHcS1C/M1lXXfsY3A0b1VAQ== X-Received: by 127.0.0.2 with SMTP id c9haYY7687511xrgGlk1eImO; Sun, 07 Apr 2024 21:47:26 -0700 X-Received: from mail-pl1-f176.google.com (mail-pl1-f176.google.com [209.85.214.176]) by mx.groups.io with SMTP id smtpd.web10.98968.1712551645988545803 for ; Sun, 07 Apr 2024 21:47:26 -0700 X-Received: by mail-pl1-f176.google.com with SMTP id d9443c01a7336-1e419d203bdso3795525ad.1 for ; Sun, 07 Apr 2024 21:47:25 -0700 (PDT) X-Gm-Message-State: aGHcQlYnwWPwym3MklktRlbyx7686176AA= X-Google-Smtp-Source: AGHT+IEtbdf331n8hW32zlgSjGNUCq/0bAcri6PaT4wktCvBbvxpnts3CciUZvnyDapy0RLyO1ayPA== X-Received: by 2002:a17:902:f60f:b0:1e4:3535:a85 with SMTP id n15-20020a170902f60f00b001e435350a85mr1008471plg.7.1712551645366; Sun, 07 Apr 2024 21:47:25 -0700 (PDT) X-Received: from sunil-laptop ([106.51.187.230]) by smtp.gmail.com with ESMTPSA id o5-20020a170902d4c500b001e3f148ffd3sm2503688plg.201.2024.04.07.21.47.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 07 Apr 2024 21:47:24 -0700 (PDT) Date: Mon, 8 Apr 2024 10:17:18 +0530 From: "Sunil V L" To: Tuan Phan Cc: devel@edk2.groups.io, michael.d.kinney@intel.com, gaoliming@byosoft.com.cn, zhiguang.liu@intel.com, kraxel@redhat.com, lersek@redhat.com, rahul1.kumar@intel.com, ray.ni@intel.com, jiewen.yao@intel.com, andrei.warkentin@intel.com, ardb+tianocore@kernel.org Subject: Re: [edk2-devel] [PATCH v4 3/4] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension Message-ID: References: <20240314201917.1756-1-tphan@ventanamicro.com> <20240314201917.1756-4-tphan@ventanamicro.com> <17BE38330B281CA5.29196@groups.io> 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 Resent-Date: Sun, 07 Apr 2024 21:47:26 -0700 Resent-From: sunilvl@ventanamicro.com 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=20240206 header.b=nBgAfMu5; 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=none On Tue, Apr 02, 2024 at 03:42:19PM -0700, Tuan Phan wrote: > On Tue, Mar 19, 2024 at 9:45 AM Tuan Phan via groups.io ventanamicro.com@groups.io> wrote: > > > Hi Sunil, > > > > On Mon, Mar 18, 2024 at 6:00 AM Sunil V L > > wrote: > > > >> Hi Tuan, > >> > >> On Thu, Mar 14, 2024 at 01:19:16PM -0700, Tuan Phan wrote: > >> > The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory attributes will be > >> > supported when Svpbmt extension available. > >> > > >> > Cc: Gerd Hoffmann > >> > Cc: Laszlo Ersek > >> > Cc: Rahul Kumar > >> > Cc: Ray Ni > >> > Signed-off-by: Tuan Phan > >> > --- > >> > .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 106 ++++++++++++++---- > >> > .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf | 1 + > >> > 2 files changed, 86 insertions(+), 21 deletions(-) > >> > > >> > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > >> b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > >> > index 46ba4b4709b1..34300dca5c34 100644 > >> > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > >> > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > >> > @@ -36,6 +36,11 @@ > >> > #define PTE_PPN_SHIFT 10 > >> > #define RISCV_MMU_PAGE_SHIFT 12 > >> > > >> > +#define RISCV_CPU_FEATURE_PBMT_BITMASK BIT2 > >> > +#define PTE_PBMT_NC BIT61 > >> > +#define PTE_PBMT_IO BIT62 > >> > +#define PTE_PBMT_MASK (PTE_PBMT_NC | PTE_PBMT_IO) > >> > + > >> > STATIC UINTN mModeSupport[] = { SATP_MODE_SV57, SATP_MODE_SV48, > >> SATP_MODE_SV39, SATP_MODE_OFF }; > >> > STATIC UINTN mMaxRootTableLevel; > >> > STATIC UINTN mBitPerLevel; > >> > @@ -487,32 +492,82 @@ UpdateRegionMapping ( > >> > /** > >> > Convert GCD attribute to RISC-V page attribute. > >> > > >> > - @param GcdAttributes The GCD attribute. > >> > + @param GcdAttributes The GCD attribute. > >> > + @param RiscVAttributes The pointer of RISC-V page attribute. > >> > > >> > - @return The RISC-V page attribute. > >> > + @retval EFI_INVALID_PARAMETER The RiscVAttributes is NULL or cache > >> type mask not valid. > >> > + @retval EFI_SUCCESS The operation succesfully. > >> > > >> > **/ > >> > STATIC > >> > -UINT64 > >> > +EFI_STATUS > >> > GcdAttributeToPageAttribute ( > >> > - IN UINT64 GcdAttributes > >> > + IN UINT64 GcdAttributes, > >> > + OUT UINT64 *RiscVAttributes > >> > ) > >> > { > >> > - UINT64 RiscVAttributes; > >> > + UINT64 CacheTypeMask; > >> > + BOOLEAN PmbtExtEnabled; > >> > > >> Why not read the PCD once and save in a static variable? > >> > > I can put it into a static variable if you think it is more clean. > > > Looks like PcdRiscVFeatureOverride can be a patchable PCD so putting it to > a static variable may not work. > I don't think that will be an issue for this use case. But I don't have major issue keeping like this itself. > > > >> > - RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X; > >> > + if (RiscVAttributes == NULL) { > >> > + return EFI_INVALID_PARAMETER; > >> > + } > >> > + > >> > + *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X; > >> > + > >> > + PmbtExtEnabled = FALSE; > >> > + if ((PcdGet64 (PcdRiscVFeatureOverride) & > >> RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) { > >> > + PmbtExtEnabled = TRUE; > >> > + } > >> > > >> > // Determine protection attributes > >> > if ((GcdAttributes & EFI_MEMORY_RO) != 0) { > >> > - RiscVAttributes &= ~(UINT64)(RISCV_PG_W); > >> > + *RiscVAttributes &= ~(UINT64)(RISCV_PG_W); > >> > } > >> > > >> > // Process eXecute Never attribute > >> > if ((GcdAttributes & EFI_MEMORY_XP) != 0) { > >> > - RiscVAttributes &= ~(UINT64)RISCV_PG_X; > >> > + *RiscVAttributes &= ~(UINT64)RISCV_PG_X; > >> > + } > >> > + > >> > + CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK; > >> > + if ((CacheTypeMask != 0) && > >> > + (((CacheTypeMask - 1) & CacheTypeMask) != 0)) > >> > + { > >> > + DEBUG (( > >> > + DEBUG_ERROR, > >> > + "%a: More than one bit set in cache type mask (0x%LX)\n", > >> > + __func__, > >> > + CacheTypeMask > >> > + )); > >> > + return EFI_INVALID_PARAMETER; > >> > + } > >> > + > >> > + switch (CacheTypeMask) { > >> > + case EFI_MEMORY_UC: > >> > + if (PmbtExtEnabled) { > >> > + *RiscVAttributes |= PTE_PBMT_IO; > >> > + } > >> > + > >> > + break; > >> > + case EFI_MEMORY_WC: > >> > + if (PmbtExtEnabled) { > >> > + *RiscVAttributes |= PTE_PBMT_NC; > >> > + } else { > >> > + DEBUG (( > >> > + DEBUG_VERBOSE, > >> > + "%a: EFI_MEMORY_WC set but Pmbt extension not available\n", > >> > + __func__ > >> > + )); > >> > + } > >> > + > >> > + break; > >> > + default: > >> > + // Default PMA mode > >> > + break; > >> > } > >> > > >> > - return RiscVAttributes; > >> > + return EFI_SUCCESS; > >> > } > >> > > >> > /** > >> > @@ -535,29 +590,38 @@ RiscVSetMemoryAttributes ( > >> > IN UINT64 Attributes > >> > ) > >> > { > >> > - UINT64 PageAttributesSet; > >> > + UINT64 PageAttributesSet; > >> > + UINT64 PageAttributesClear; > >> > + EFI_STATUS Status; > >> > > >> > - PageAttributesSet = GcdAttributeToPageAttribute (Attributes); > >> > + Status = GcdAttributeToPageAttribute (Attributes, > >> &PageAttributesSet); > >> > + if (EFI_ERROR (Status)) { > >> > + return Status; > >> > + } > >> > > >> Is there a reason to do this prior to checking RiscVMmuEnabled()? > >> > > Only reason is return error due to invalid parameter before return > > EFI_SUCCESS if Mmu not enabled. > > Okay. Thanks! > >> > >> > if (!RiscVMmuEnabled ()) { > >> > return EFI_SUCCESS; > >> > } > >> > > >> > - DEBUG ( > >> > - ( > >> > - DEBUG_VERBOSE, > >> > - "%a: Set %llX page attribute 0x%X\n", > >> > - __func__, > >> > - BaseAddress, > >> > - PageAttributesSet > >> > - ) > >> > - ); > >> > + PageAttributesClear = PTE_ATTRIBUTES_MASK; > >> > + if ((PcdGet64 (PcdRiscVFeatureOverride) & > >> RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) { > >> > + PageAttributesClear |= PTE_PBMT_MASK; > >> > + } > >> > + > >> I think static variable would be better. > >> > >> > + DEBUG (( > >> > + DEBUG_VERBOSE, > >> > + "%a: %LX: set attributes 0x%LX, clear attributes 0x%LX\n", > >> > + __func__, > >> > + BaseAddress, > >> > + PageAttributesSet, > >> > + PageAttributesClear > >> > + )); > >> > > >> > return UpdateRegionMapping ( > >> > BaseAddress, > >> > Length, > >> > PageAttributesSet, > >> > - PTE_ATTRIBUTES_MASK, > >> > + PageAttributesClear, > >> > (UINT64 *)RiscVGetRootTranslateTable (), > >> > TRUE > >> > ); > >> > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > >> b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > >> > index 51ebe1750e97..1dbaa81f3608 100644 > >> > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > >> > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > >> > @@ -28,3 +28,4 @@ > >> > > >> > [Pcd] > >> > gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVMmuMaxSatpMode ## CONSUMES > >> > + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES > >> > -- Reviewed-by: Sunil V L -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117490): https://edk2.groups.io/g/devel/message/117490 Mute This Topic: https://groups.io/mt/104934719/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-