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 27E8ED80186 for ; Fri, 1 Mar 2024 12:14:55 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=ACQfR5yV0meSLPgR787TDtUy2Bic93tK8dw2JSB8vx4=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1709295294; v=1; b=BNue/x9DUcyGG8JqPmnT9VuWt+GzUXByEs4ypZaNfm02cXthu7lGL9GGhq3F+rslblnCDix6 Ngn9KpDv315ihrIu79urkUluihTiawnxIlAbaZKSJq3kle4DRJNysY8ophb0f7WmHbeRXSBhNpz A+a5XjzEkgzpg58Dkouiid9E= X-Received: by 127.0.0.2 with SMTP id y4AZYY7687511xMrnzejCmDv; Fri, 01 Mar 2024 04:14:54 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.21249.1709295294042717007 for ; Fri, 01 Mar 2024 04:14:54 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-665-DBhTFI7iM_q2rO7JGCDXsQ-1; Fri, 01 Mar 2024 07:14:50 -0500 X-MC-Unique: DBhTFI7iM_q2rO7JGCDXsQ-1 X-Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 15E5128AC1F5; Fri, 1 Mar 2024 12:14:45 +0000 (UTC) X-Received: from [10.39.194.215] (unknown [10.39.194.215]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7500C4D917; Fri, 1 Mar 2024 12:14:42 +0000 (UTC) Message-ID: <28f73ccb-f5e6-ddb0-9f72-8e358a1203ed@redhat.com> Date: Fri, 1 Mar 2024 13:14:41 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension To: devel@edk2.groups.io, tphan@ventanamicro.com Cc: michael.d.kinney@intel.com, gaoliming@byosoft.com.cn, zhiguang.liu@intel.com, kraxel@redhat.com, rahul1.kumar@intel.com, ray.ni@intel.com, sunilvl@ventanamicro.com, jiewen.yao@intel.com, andrei.warkentin@intel.com, ardb+tianocore@kernel.org References: <20240301012924.16232-1-tphan@ventanamicro.com> <20240301012924.16232-3-tphan@ventanamicro.com> From: "Laszlo Ersek" In-Reply-To: <20240301012924.16232-3-tphan@ventanamicro.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: ige8Z9wM6rcb4zhfD2Kih007x7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Spam-Flag: yes X-Spam-Level: ************ X-GND-Spam-Score: 190 X-GND-Status: SPAM Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="BNue/x9D"; 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=redhat.com (policy=none) On 3/1/24 02:29, Tuan Phan wrote: > The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory attributes will be > supported when Svpbmt extension available. >=20 > Cc: Gerd Hoffmann > Cc: Laszlo Ersek > Cc: Rahul Kumar > Cc: Ray Ni > Signed-off-by: Tuan Phan > --- > .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101 +++++++++++++++--- > .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf | 1 + > 2 files changed, 88 insertions(+), 14 deletions(-) >=20 > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c b/UefiC= puPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > index 826a1d32a1d4..f4419bb8f380 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 > =20 > +#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[] =3D { SATP_MODE_SV57, SATP_MODE_SV48, SATP_= MODE_SV39, SATP_MODE_OFF }; > STATIC UINTN mMaxRootTableLevel; > STATIC UINTN mBitPerLevel; > @@ -489,32 +494,89 @@ UpdateRegionMapping ( > /** > Convert GCD attribute to RISC-V page attribute. > =20 > - @param GcdAttributes The GCD attribute. > + @param GcdAttributes The GCD attribute. > + @param RiscVAttribtues The pointer of RISC-V page attribute. > =20 > - @return The RISC-V page attribute. > + @retval EFI_INVALID_PARAMETER The RiscVAttribtues is NULL or cache t= ype mask not valid. > + @retval EFI_SUCCESS The operation succesfully. > =20 > **/ > STATIC > -UINTN > +EFI_STATUS > GcdAttributeToPageAttribute ( > - IN UINTN GcdAttributes > + IN UINTN GcdAttributes, Just noticing: why is GcdAttributes *not* UINT64 in the first place? All the bit macros we test against it, such as EFI_MEMORY_RO (0x0000000000020000ULL) are of type unsigned long long (UINT64). > + OUT UINTN *RiscVAttributes > ) > { > - UINTN RiscVAttributes; > + UINT64 CacheTypeMask; > + BOOLEAN PmbtExtEnabled =3D (PcdGet64 (PcdRiscVFeatureOverride) & RISC= V_CPU_FEATURE_PBMT_BITMASK) ? TRUE : FALSE; - Per the edk2 coding style, locals should not be initialized (separate assignment is needed). - Bitmask checks always need an explicit comparison, such as ((a & b) !=3D 0) or similar. Implicitly interpreting (a & b) as a truth value is not appropriate. - "(whatever) ? TRUE : FALSE" is both bad style and unnecessary. BOOLEAN PmbtExtEnabled; PmbtExtEnabled =3D (PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_PBMT_BITMASK) !=3D 0; > =20 > - RiscVAttributes =3D RISCV_PG_R | RISCV_PG_W | RISCV_PG_X; > + if (!RiscVAttributes) { - The coding style requires an explicit nullity check: if (RiscVAttributes =3D=3D NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + *RiscVAttributes =3D RISCV_PG_R | RISCV_PG_W | RISCV_PG_X; > =20 > // Determine protection attributes > if ((GcdAttributes & EFI_MEMORY_RO) !=3D 0) { > - RiscVAttributes &=3D ~(RISCV_PG_W); > + *RiscVAttributes &=3D ~(RISCV_PG_W); > } > =20 > // Process eXecute Never attribute > if ((GcdAttributes & EFI_MEMORY_XP) !=3D 0) { > - RiscVAttributes &=3D ~RISCV_PG_X; > + *RiscVAttributes &=3D ~RISCV_PG_X; > + } > + My next comment is unrelated to the patch, it's just something that catches my eye, and I think is worth fixing: RISCV_PG_W is BIT2 (0x00000004), and RISCV_PG_X is BIT3 (0x00000008). Meaning, they are of type *signed int* (INT32). Applying the bit-neg operator on them produces a negative value (because it flips the sign bit), which is very ugly. I suggest a separate patch for changing these into ~(UINTN)RISCV_PG_W ~(UINTN)RISCV_PG_X Alternatively, you could do *RiscVAttributes =3D RISCV_PG_R; if ((GcdAttributes & EFI_MEMORY_RO) =3D=3D 0) { *RiscVAttributes |=3D RISCV_PG_W; } if ((GcdAttributes & EFI_MEMORY_XP) =3D=3D 0) { *RiscVAttributes |=3D RISCV_PG_X; } Either way: separate patch. > + CacheTypeMask =3D GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK; > + if ((CacheTypeMask !=3D 0) && > + (((CacheTypeMask - 1) & CacheTypeMask) !=3D 0)) This is not what I recommended in my previous review . Compare: (CacheTypeMask !=3D 0) && ... versus (CacheTypeMask =3D=3D 0) || ... Both of these ensure that the power-of-two check in the second subcondition (i.e., the subtraction of 1) is avoided when CacheTypeMask is zero. In the first variant, you get (FALSE && ...), in the second variant, you get (TRUE || ...); therefore, the power-of-two check is short-circuited for a zero input in both variants. However, considering the larger CacheTypeMask validation, your variant is incorrect, because a zero CacheTypeMask will ultimately evaluate the condition to FALSE -- (FALSE && ...) is FALSE --, and so the "return EFI_INVALID_PARAMETER" statement will not be reached. Whereas (TRUE || ...) is TRUE, and so we return EFI_INVALID_PARAMETER for CacheTypeMask=3D= =3D0. > + { > + DEBUG ( > + ( > + DEBUG_ERROR, > + "%a: The cache type mask (0x%llX) should contain exactly one bit = set\n", - Edk2's PrintLib does not use "ll" length modifiers. %u, %x and %X are for UINT32, and %lu, %lx and %lX are for UINT64. Furthermore, you may replace "l" with "L" freely. - We generally group together the double parens for DEBUG invocations: DEBUG (( DEBUG_ERROR, "%a: The cache type mask (0x%lX) ...\n", __func__, CacheTypeMask )); > + __func__, > + CacheTypeMask > + ) > + ); The indentation of the closing parens is not correct either; please put your patches through uncrustify first. (CI will reject these issues anyway, in github pull requests.) For running uncrustify locally: - clone - check it out at tag 73.0.8 (the tag that edk2 CI uses on github is in ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml") - build it (IIRC it uses cmake) - with nothing dirty in the working tree (i.e., everything committed, or at least stashed to the index), run uncrustify \ -c .pytool/Plugin/UncrustifyCheck/uncrustify.cfg \ --replace \ --no-backup \ --if-changed \ -F file-list.txt > + return EFI_INVALID_PARAMETER; > } > =20 > - return RiscVAttributes; > + switch (CacheTypeMask) { > + case EFI_MEMORY_UC: > + if (PmbtExtEnabled) { > + *RiscVAttributes |=3D PTE_PBMT_IO; > + } else { > + DEBUG ( > + ( > + DEBUG_VERBOSE, > + "%a: EFI_MEMORY_UC set but Pmbt extension not available\n", > + __func__ > + ) > + ); > + } > + > + break; > + case EFI_MEMORY_WC: > + if (PmbtExtEnabled) { > + *RiscVAttributes |=3D 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 EFI_SUCCESS; > } > =20 > /** > @@ -537,21 +599,32 @@ RiscVSetMemoryAttributes ( > IN UINTN Attributes > ) > { > - UINTN PageAttributesSet; > + UINTN PageAttributesSet; > + UINTN PageAttributesClear; > + EFI_STATUS Status; > =20 > - PageAttributesSet =3D GcdAttributeToPageAttribute (Attributes); > + Status =3D GcdAttributeToPageAttribute (Attributes, &PageAttributesSet= ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > =20 > if (!RiscVMmuEnabled ()) { > return EFI_SUCCESS; > } > =20 > + PageAttributesClear =3D PTE_ATTRIBUTES_MASK; > + if ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_PBMT_BITMA= SK) !=3D 0) { > + PageAttributesClear |=3D PTE_PBMT_MASK; > + } > + > DEBUG ( > ( > DEBUG_VERBOSE, > - "%a: Set %llX page attribute 0x%X\n", > + "%a: %llX: set attributes 0x%X, clear attributes 0x%X\n", > __func__, > BaseAddress, > - PageAttributesSet > + PageAttributesSet, > + PageAttributesClear > ) > ); - UINT64 should be formatted with %[Ll][uxX]. - UINT32 should be formatted with %[uxX]. - UINTN is trickier, there is no dedicated conversion specifier. The portable solution (between 32-bit and 64-bit platforms in edk2) is to (a) cast the UINTN value to UINT64, (b) format the latter with %[Ll][uxX]. So you need something like DEBUG (( DEBUG_VERBOSE, "%a: %LX: set attributes 0x%LX, clear attributes 0x%LX\n", __func__, BaseAddress, // this is UINT64 (UINT64)PageAttributesSet, // originally UINTN (UINT64)PageAttributesClear // originally UINTN )); > =20 > @@ -559,7 +632,7 @@ RiscVSetMemoryAttributes ( > BaseAddress, > Length, > PageAttributesSet, > - PTE_ATTRIBUTES_MASK, > + PageAttributesClear, > (UINTN *)RiscVGetRootTranslateTable (), > TRUE > ); > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf b/Uef= iCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > index 51ebe1750e97..1dbaa81f3608 100644 > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > @@ -28,3 +28,4 @@ > =20 > [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVMmuMaxSatpMode ## CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES Laszlo -=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 (#116239): https://edk2.groups.io/g/devel/message/116239 Mute This Topic: https://groups.io/mt/104656466/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-