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 4BA15AC1B4F for ; Fri, 1 Mar 2024 23:20:42 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=g4+nYUanx41SWtqd/dkbF+9DGHHnUG08YV57kBaJtoI=; 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; s=20140610; t=1709335241; v=1; b=mewyBdp2r083ETucnmLlnw40pHlrgWpXsWsMxQLfl1c4uVDRGODdl2ml6dG8ZMxokXdA7MSa EQGSDX/Zn7Sso5V66ok+ksoa2ILR/D98UACSaRyOdR+ycHWxnIPmomvEJaeLTxJYoChzdSaHOLq DJFp0Qh5O43Z+Oq2Q36qTwP4= X-Received: by 127.0.0.2 with SMTP id TUwPYY7687511xVWkgV51wws; Fri, 01 Mar 2024 15:20:41 -0800 X-Received: from mail-ua1-f44.google.com (mail-ua1-f44.google.com [209.85.222.44]) by mx.groups.io with SMTP id smtpd.web10.38409.1709335240012918756 for ; Fri, 01 Mar 2024 15:20:40 -0800 X-Received: by mail-ua1-f44.google.com with SMTP id a1e0cc1a2514c-7da763255b7so1083832241.2 for ; Fri, 01 Mar 2024 15:20:39 -0800 (PST) X-Gm-Message-State: 2mgTfflkYP0fg1MMDX1cybIdx7686176AA= X-Google-Smtp-Source: AGHT+IGkw6BNj/3E/wTNQfDGyzfb9/bYGx08prpQRoDwXmeGmiVd0O9wXdccoyi+8xiyxDNkkt8yS6mFMlQmk+Ow2wI= X-Received: by 2002:a67:f64c:0:b0:472:53e0:6549 with SMTP id u12-20020a67f64c000000b0047253e06549mr2739722vso.14.1709335238939; Fri, 01 Mar 2024 15:20:38 -0800 (PST) MIME-Version: 1.0 References: <20240301012924.16232-1-tphan@ventanamicro.com> <20240301012924.16232-3-tphan@ventanamicro.com> <28f73ccb-f5e6-ddb0-9f72-8e358a1203ed@redhat.com> In-Reply-To: <28f73ccb-f5e6-ddb0-9f72-8e358a1203ed@redhat.com> From: "Tuan Phan" Date: Fri, 1 Mar 2024 15:20:28 -0800 Message-ID: Subject: Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension To: Laszlo Ersek Cc: devel@edk2.groups.io, 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 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,tphan@ventanamicro.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="000000000000b82d240612a19f33" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=mewyBdp2; 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 --000000000000b82d240612a19f33 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Thanks for the detailed review. Please see my comments below. On Fri, Mar 1, 2024 at 4:14=E2=80=AFAM Laszlo Ersek wro= te: > 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. > > > > 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(-) > > > > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > b/UefiCpuPkg/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 > > > > +#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. > > > > - @param GcdAttributes The GCD attribute. > > + @param GcdAttributes The GCD attribute. > > + @param RiscVAttribtues The pointer of RISC-V page attribute. > > > > - @return The RISC-V page attribute. > > + @retval EFI_INVALID_PARAMETER The RiscVAttribtues is NULL or cache > type mask not valid. > > + @retval EFI_SUCCESS The operation succesfully. > > > > **/ > > 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). > Good catch. Will fix it. > > > + OUT UINTN *RiscVAttributes > > ) > > { > > - UINTN RiscVAttributes; > > + UINT64 CacheTypeMask; > > + BOOLEAN PmbtExtEnabled =3D (PcdGet64 (PcdRiscVFeatureOverride) & > RISCV_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; > > Will fix it. > > > > - 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) { > Will fix it. > > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + *RiscVAttributes =3D RISCV_PG_R | RISCV_PG_W | RISCV_PG_X; > > > > // Determine protection attributes > > if ((GcdAttributes & EFI_MEMORY_RO) !=3D 0) { > > - RiscVAttributes &=3D ~(RISCV_PG_W); > > + *RiscVAttributes &=3D ~(RISCV_PG_W); > > } > > > > // 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 > Will fix it in a separate patch along with the above change. > > *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. > Actually the EDK2 passes (CacheTypeMask =3D=3D 0) to this API during my deb= ug session. Given that situation, this function doesn't do anything when CacheTypeMask =3D=3D 0 so I think it should not give the warning message. > > > + { > > + DEBUG ( > > + ( > > + DEBUG_ERROR, > > + "%a: The cache type mask (0x%llX) should contain exactly one bi= t > 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. > Will fix it. > > - 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.) > Actually this code is the result of uncrustify modification. Let me check if anything wrong with uncrustify. > > 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; > > } > > > > - 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; > > } > > > > /** > > @@ -537,21 +599,32 @@ RiscVSetMemoryAttributes ( > > IN UINTN Attributes > > ) > > { > > - UINTN PageAttributesSet; > > + UINTN PageAttributesSet; > > + UINTN PageAttributesClear; > > + EFI_STATUS Status; > > > > - PageAttributesSet =3D GcdAttributeToPageAttribute (Attributes); > > + Status =3D GcdAttributeToPageAttribute (Attributes, &PageAttributesS= et); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > > > if (!RiscVMmuEnabled ()) { > > return EFI_SUCCESS; > > } > > > > + PageAttributesClear =3D PTE_ATTRIBUTES_MASK; > > + if ((PcdGet64 (PcdRiscVFeatureOverride) & > RISCV_CPU_FEATURE_PBMT_BITMASK) !=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 > )); > Thanks for the suggestion. Will fix it. > > > > > @@ -559,7 +632,7 @@ RiscVSetMemoryAttributes ( > > BaseAddress, > > Length, > > PageAttributesSet, > > - PTE_ATTRIBUTES_MASK, > > + PageAttributesClear, > > (UINTN *)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 > > 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 (#116286): https://edk2.groups.io/g/devel/message/116286 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- --000000000000b82d240612a19f33 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks for the detailed review. Please see my comment= s below.

On Fri, Mar 1, 2024 at 4:14=E2=80=AFAM Laszlo Ersek <lersek@redhat.com> wrote:
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.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Ray Ni <r= ay.ni@intel.com>
> Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> ---
>=C2=A0 .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101 ++++++++++++= +++---
>=C2=A0 .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf=C2=A0 =C2=A0 =C2=A0 =C2= =A0|=C2=A0 =C2=A01 +
>=C2=A0 2 files changed, 88 insertions(+), 14 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c b/Ue= fiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> index 826a1d32a1d4..f4419bb8f380 100644
> --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> @@ -36,6 +36,11 @@
>=C2=A0 #define PTE_PPN_SHIFT=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A010
>=C2=A0 #define RISCV_MMU_PAGE_SHIFT=C2=A0 12
>=C2=A0
> +#define RISCV_CPU_FEATURE_PBMT_BITMASK=C2=A0 BIT2
> +#define PTE_PBMT_NC=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0BIT61
> +#define PTE_PBMT_IO=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0BIT62
> +#define PTE_PBMT_MASK=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0(PTE_PBMT_NC | PTE_PBMT_IO)
> +
>=C2=A0 STATIC UINTN=C2=A0 mModeSupport[] =3D { SATP_MODE_SV57, SATP_MOD= E_SV48, SATP_MODE_SV39, SATP_MODE_OFF };
>=C2=A0 STATIC UINTN=C2=A0 mMaxRootTableLevel;
>=C2=A0 STATIC UINTN=C2=A0 mBitPerLevel;
> @@ -489,32 +494,89 @@ UpdateRegionMapping (
>=C2=A0 /**
>=C2=A0 =C2=A0 Convert GCD attribute to RISC-V page attribute.
>=C2=A0
> -=C2=A0 @param=C2=A0 GcdAttributes The GCD attribute.
> +=C2=A0 @param=C2=A0 GcdAttributes=C2=A0 =C2=A0The GCD attribute.
> +=C2=A0 @param=C2=A0 RiscVAttribtues The pointer of RISC-V page attrib= ute.
>=C2=A0
> -=C2=A0 @return=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= The RISC-V page attribute.
> +=C2=A0 @retval EFI_INVALID_PARAMETER=C2=A0 =C2=A0The RiscVAttribtues = is NULL or cache type mask not valid.
> +=C2=A0 @retval EFI_SUCCESS=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0The operation succesfully.
>=C2=A0
>=C2=A0 **/
>=C2=A0 STATIC
> -UINTN
> +EFI_STATUS
>=C2=A0 GcdAttributeToPageAttribute (
> -=C2=A0 IN UINTN=C2=A0 GcdAttributes
> +=C2=A0 IN UINTN=C2=A0 =C2=A0GcdAttributes,

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).
Good catch. Will fix it.=C2=A0

> +=C2=A0 OUT UINTN=C2=A0 *RiscVAttributes
>=C2=A0 =C2=A0 )
>=C2=A0 {
> -=C2=A0 UINTN=C2=A0 RiscVAttributes;
> +=C2=A0 UINT64=C2=A0 =C2=A0CacheTypeMask;
> +=C2=A0 BOOLEAN=C2=A0 PmbtExtEnabled =3D (PcdGet64 (PcdRiscVFeatureOve= rride) & RISCV_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

=C2=A0 ((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.<= br>
=C2=A0 BOOLEAN=C2=A0 PmbtExtEnabled;

=C2=A0 PmbtExtEnabled =3D (PcdGet64 (PcdRiscVFeatureOverride) &
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 RISCV= _CPU_FEATURE_PBMT_BITMASK) !=3D 0;

Will fix it.=C2=A0
>=C2=A0
> -=C2=A0 RiscVAttributes =3D RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> +=C2=A0 if (!RiscVAttributes) {

- The coding style requires an explicit nullity check:

=C2=A0 if (RiscVAttributes =3D=3D NULL) {
Will fix it.= =C2=A0

> +=C2=A0 =C2=A0 return EFI_INVALID_PARAMETER;
> +=C2=A0 }
> +
> +=C2=A0 *RiscVAttributes =3D RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
>=C2=A0
>=C2=A0 =C2=A0 // Determine protection attributes
>=C2=A0 =C2=A0 if ((GcdAttributes & EFI_MEMORY_RO) !=3D 0) {
> -=C2=A0 =C2=A0 RiscVAttributes &=3D ~(RISCV_PG_W);
> +=C2=A0 =C2=A0 *RiscVAttributes &=3D ~(RISCV_PG_W);
>=C2=A0 =C2=A0 }
>=C2=A0
>=C2=A0 =C2=A0 // Process eXecute Never attribute
>=C2=A0 =C2=A0 if ((GcdAttributes & EFI_MEMORY_XP) !=3D 0) {
> -=C2=A0 =C2=A0 RiscVAttributes &=3D ~RISCV_PG_X;
> +=C2=A0 =C2=A0 *RiscVAttributes &=3D ~RISCV_PG_X;
> +=C2=A0 }
> +

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

=C2=A0 ~(UINTN)RISCV_PG_W
=C2=A0 ~(UINTN)RISCV_PG_X

Alternatively, you could do
Will fix it in a separate = patch along with the above change.

=C2=A0 *RiscVAttributes =3D RISCV_PG_R;
=C2=A0 if ((GcdAttributes & EFI_MEMORY_RO) =3D=3D 0) {
=C2=A0 =C2=A0 *RiscVAttributes |=3D RISCV_PG_W;
=C2=A0 }
=C2=A0 if ((GcdAttributes & EFI_MEMORY_XP) =3D=3D 0) {
=C2=A0 =C2=A0 *RiscVAttributes |=3D RISCV_PG_X;
=C2=A0 }

Either way: separate patch.

> +=C2=A0 CacheTypeMask =3D GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK= ;
> +=C2=A0 if ((CacheTypeMask !=3D 0) &&
> +=C2=A0 =C2=A0 =C2=A0 (((CacheTypeMask - 1) & CacheTypeMask) !=3D = 0))

This is not what I recommended in my previous review
<https://edk2.groups.io/g/devel/message/115243&g= t;.

Compare:

=C2=A0 (CacheTypeMask !=3D 0) && ...

versus

=C2=A0 (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 secon= d
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.
Actually the EDK2 passes (CacheTypeMask =3D=3D 0= ) to this API during my debug session.
Given that situation, this= function doesn't do anything when CacheTypeMask =C2=A0=3D=3D 0 so I th= ink
it should not give the warning message.

> +=C2=A0 {
> +=C2=A0 =C2=A0 DEBUG (
> +=C2=A0 =C2=A0 =C2=A0 (
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0DEBUG_ERROR,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"%a: The cache type mask (0x%llX) sho= uld 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.
Will = fix it.=C2=A0

- We generally group together the double parens for DEBUG invocations:

=C2=A0 DEBUG ((
=C2=A0 =C2=A0 DEBUG_ERROR,
=C2=A0 =C2=A0 "%a: The cache type mask (0x%lX) ...\n",
=C2=A0 =C2=A0 __func__,
=C2=A0 =C2=A0 CacheTypeMask
=C2=A0 =C2=A0 ));

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0__func__,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0CacheTypeMask
> +=C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 );

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.)
Actually this code i= s the result of uncrustify modification. Let me check if anything
wrong with uncrustify.=C2=A0

For running uncrustify locally:

- clone
<https://projectmu@dev.azure.= com/projectmu/Uncrustify/_git/Uncrustify>

- 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

=C2=A0 uncrustify \
=C2=A0 =C2=A0 -c .pytool/Plugin/UncrustifyCheck/uncrustify.cfg \
=C2=A0 =C2=A0 --replace \
=C2=A0 =C2=A0 --no-backup \
=C2=A0 =C2=A0 --if-changed \
=C2=A0 =C2=A0 -F file-list.txt

> +=C2=A0 =C2=A0 return EFI_INVALID_PARAMETER;
>=C2=A0 =C2=A0 }
>=C2=A0
> -=C2=A0 return RiscVAttributes;
> +=C2=A0 switch (CacheTypeMask) {
> +=C2=A0 =C2=A0 case EFI_MEMORY_UC:
> +=C2=A0 =C2=A0 =C2=A0 if (PmbtExtEnabled) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 *RiscVAttributes |=3D PTE_PBMT_IO;
> +=C2=A0 =C2=A0 =C2=A0 } else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 DEBUG (
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DEBUG_VERBOSE,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"%a: EFI_MEMORY_UC set = but Pmbt extension not available\n",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0__func__
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 );
> +=C2=A0 =C2=A0 =C2=A0 }
> +
> +=C2=A0 =C2=A0 =C2=A0 break;
> +=C2=A0 =C2=A0 case EFI_MEMORY_WC:
> +=C2=A0 =C2=A0 =C2=A0 if (PmbtExtEnabled) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 *RiscVAttributes |=3D PTE_PBMT_NC;
> +=C2=A0 =C2=A0 =C2=A0 } else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 DEBUG (
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DEBUG_VERBOSE,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"%a: EFI_MEMORY_WC set = but Pmbt extension not available\n",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0__func__
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 );
> +=C2=A0 =C2=A0 =C2=A0 }
> +
> +=C2=A0 =C2=A0 =C2=A0 break;
> +=C2=A0 =C2=A0 default:
> +=C2=A0 =C2=A0 =C2=A0 // Default PMA mode
> +=C2=A0 =C2=A0 =C2=A0 break;
> +=C2=A0 }
> +
> +=C2=A0 return EFI_SUCCESS;
>=C2=A0 }
>=C2=A0
>=C2=A0 /**
> @@ -537,21 +599,32 @@ RiscVSetMemoryAttributes (
>=C2=A0 =C2=A0 IN UINTN=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0Attributes
>=C2=A0 =C2=A0 )
>=C2=A0 {
> -=C2=A0 UINTN=C2=A0 PageAttributesSet;
> +=C2=A0 UINTN=C2=A0 =C2=A0 =C2=A0 =C2=A0PageAttributesSet;
> +=C2=A0 UINTN=C2=A0 =C2=A0 =C2=A0 =C2=A0PageAttributesClear;
> +=C2=A0 EFI_STATUS=C2=A0 Status;
>=C2=A0
> -=C2=A0 PageAttributesSet =3D GcdAttributeToPageAttribute (Attributes)= ;
> +=C2=A0 Status =3D GcdAttributeToPageAttribute (Attributes, &PageA= ttributesSet);
> +=C2=A0 if (EFI_ERROR (Status)) {
> +=C2=A0 =C2=A0 return Status;
> +=C2=A0 }
>=C2=A0
>=C2=A0 =C2=A0 if (!RiscVMmuEnabled ()) {
>=C2=A0 =C2=A0 =C2=A0 return EFI_SUCCESS;
>=C2=A0 =C2=A0 }
>=C2=A0
> +=C2=A0 PageAttributesClear =3D PTE_ATTRIBUTES_MASK;
> +=C2=A0 if ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATUR= E_PBMT_BITMASK) !=3D 0) {
> +=C2=A0 =C2=A0 PageAttributesClear |=3D PTE_PBMT_MASK;
> +=C2=A0 }
> +
>=C2=A0 =C2=A0 DEBUG (
>=C2=A0 =C2=A0 =C2=A0 (
>=C2=A0 =C2=A0 =C2=A0 =C2=A0DEBUG_VERBOSE,
> -=C2=A0 =C2=A0 =C2=A0"%a: Set %llX page attribute 0x%X\n", > +=C2=A0 =C2=A0 =C2=A0"%a: %llX: set attributes 0x%X, clear attrib= utes 0x%X\n",
>=C2=A0 =C2=A0 =C2=A0 =C2=A0__func__,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0BaseAddress,
> -=C2=A0 =C2=A0 =C2=A0PageAttributesSet
> +=C2=A0 =C2=A0 =C2=A0PageAttributesSet,
> +=C2=A0 =C2=A0 =C2=A0PageAttributesClear
>=C2=A0 =C2=A0 =C2=A0 )
>=C2=A0 =C2=A0 =C2=A0 );

- 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].<= br>
So you need something like

=C2=A0 DEBUG ((
=C2=A0 =C2=A0 DEBUG_VERBOSE,
=C2=A0 =C2=A0 "%a: %LX: set attributes 0x%LX, clear attributes 0x%LX\n= ",
=C2=A0 =C2=A0 __func__,
=C2=A0 =C2=A0 BaseAddress,=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 // this is UINT64
=C2=A0 =C2=A0 (UINT64)PageAttributesSet,=C2=A0 // originally UINTN
=C2=A0 =C2=A0 (UINT64)PageAttributesClear // originally UINTN
=C2=A0 =C2=A0 ));
Thanks for the suggestion. Will fix = it.=C2=A0

>=C2=A0
> @@ -559,7 +632,7 @@ RiscVSetMemoryAttributes (
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0BaseAddress,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Length,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0PageAttributesSet,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0PTE_ATTRIBUTES_MASK,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0PageAttributesClear,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(UINTN *)RiscVGetRootTr= anslateTable (),
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0TRUE
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0);
> 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 @@
>=C2=A0
>=C2=A0 [Pcd]
>=C2=A0 =C2=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVMmuMaxSatpMode=C2=A0= ## CONSUMES
> +=C2=A0 gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride=C2=A0 =C2=A0 = =C2=A0## CONSUMES

Laszlo

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#116286) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--000000000000b82d240612a19f33--