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 8A32A740039 for ; Thu, 7 Mar 2024 22:00:18 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=n2fxw3rsyMASTH3psPFQZxBpJsnDaja6utUfE+gsxJA=; 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=20240206; t=1709848817; v=1; b=ljSVAuzTRkOc6SJweUu/pwYREdfV01fzl39NDkZFs4yscIcEA9J82UWvIASWelNyoUHYjSw9 dTheWqn4S9V8WZ7mw1i93MYPmpZQM/cZeYEoFPAWlKravNr2gIgHo4bF7ad3lBsAOax4bCcvBjb ZXUFQPqxXfmOkpf5KS+wfgRaOLAZ31qWq4DAt0jacJLfc7BSyIC0teAYKiRF7KUPWXOa5Ofj6G9 iOQusnrU9s4Rb3TcGAigwW98BoOKtUA0JGgytVznYCEOAa3vAIx6BEa8xgjc16jlTRdcO7OC5nA bGnP3HAh0Qxr994LQz2giYZMuhA82C2YXLZCV6Buzroag== X-Received: by 127.0.0.2 with SMTP id IKhEYY7687511xWgLGojtMyo; Thu, 07 Mar 2024 14:00:17 -0800 X-Received: from mail-ua1-f45.google.com (mail-ua1-f45.google.com [209.85.222.45]) by mx.groups.io with SMTP id smtpd.web10.6187.1709848816302584613 for ; Thu, 07 Mar 2024 14:00:16 -0800 X-Received: by mail-ua1-f45.google.com with SMTP id a1e0cc1a2514c-7daf957595bso478777241.1 for ; Thu, 07 Mar 2024 14:00:16 -0800 (PST) X-Gm-Message-State: 0ENSjn9tQ34u0xFN0YW1GTLIx7686176AA= X-Google-Smtp-Source: AGHT+IFCXceMf2kFGMHL1GvolRvPNGIgIH+6RLY+hxYNwWLCWquTG26uUdyh0GgvPCKVAjThxbjG5DtdepDpo+9jYAc= X-Received: by 2002:a05:6122:335:b0:4d3:c4f7:683f with SMTP id d21-20020a056122033500b004d3c4f7683fmr1860534vko.10.1709848815188; Thu, 07 Mar 2024 14:00:15 -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> <54fbffbf-59d2-ea4a-c202-986485b01e83@redhat.com> In-Reply-To: <54fbffbf-59d2-ea4a-c202-986485b01e83@redhat.com> From: "Tuan Phan" Date: Thu, 07 Mar 2024 14:00:16 -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="0000000000003fd91006131933a3" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=ljSVAuzT; 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 --0000000000003fd91006131933a3 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Mar 4, 2024 at 10:01=E2=80=AFAM Laszlo Ersek wr= ote: > On 3/2/24 00:20, Tuan Phan wrote: > > Thanks for the detailed review. Please see my comments below. > > > > On Fri, Mar 1, 2024 at 4:14=E2=80=AFAM Laszlo Ersek > > 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 > > > > 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_SV4= 8, > > 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-ne= g > > operator on them produces a negative value (because it flips the si= gn > > 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 secon= d > > variant, you get (TRUE || ...); therefore, the power-of-two check i= s > > 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 "retur= n > > 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 =3D=3D 0 so I think > > it should not give the warning message. > > I would be curious how that can happen; to me a CacheTypeMask=3D=3D0 inpu= t > looks somewhat invalid. > > Either way, if such an input *is* valid, then there is a different > problem with the patch: in the debug message we say that the cache type > mask should contain *exactly one* bit set. That's not correct then: it > should say *at most one* bit set. (Because the value 0 has 0 bits set, > and apparently that is valid input.) > How about: "More than one bit set in cache type mask" ? It is a clear message that we don't expect more than 1 bit set if not zero. > > > > > > > > > + { > > > + 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 m= ay > > 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. > > It's very strange. Do you know what your original code (the input to > uncrustify) looked like? I wonder if uncrustify produces strange output > if it sees unexpected input. Normally I wouldn't expect uncrustify to > change the "((" format that I'm proposing. If it still does, then my > request is invalid of course (uncrustify has priority, whatever it does). > I checked and it comes from an un-correct uncrustify version I used before. It should be good now. > > Thanks! > Laszlo > > > > > > > For running uncrustify locally: > > > > - clone > > < > https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify < > 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 i= s > 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, > > &PageAttributesSet); > > > + 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. Th= e > > 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 ## CONSUM= ES > > > + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUM= ES > > > > Laszlo > > > >=20 > > -=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 (#116486): https://edk2.groups.io/g/devel/message/116486 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- --0000000000003fd91006131933a3 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Mon, Mar 4, 2024 = at 10:01=E2=80=AFAM Laszlo Ersek <l= ersek@redhat.com> wrote:
On 3/2/24 00:20, Tuan Phan wrote:
> Thanks for the detailed review. Please see my comments below.
>
> On Fri, Mar 1, 2024 at 4:14=E2=80=AFAM Laszlo Ersek <lersek@redhat.com
> <mailto:lers= ek@redhat.com>> wrote:
>
>=C2=A0 =C2=A0 =C2=A0On 3/1/24 02:29, Tuan Phan wrote:
>=C2=A0 =C2=A0 =C2=A0> The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory= attributes will be
>=C2=A0 =C2=A0 =C2=A0> supported when Svpbmt extension available.
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> Cc: Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>> >=C2=A0 =C2=A0 =C2=A0> Cc: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>> >=C2=A0 =C2=A0 =C2=A0> Cc: Rahul Kumar <rahul1.kumar@intel.com
>=C2=A0 =C2=A0 =C2=A0<mailto:rahul1.kumar@intel.com>>
>=C2=A0 =C2=A0 =C2=A0> Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
>=C2=A0 =C2=A0 =C2=A0> Signed-off-by: Tuan Phan <tphan@ventanamicro.com
>=C2=A0 =C2=A0 =C2=A0<mailto:tphan@ventanamicro.com>>
>=C2=A0 =C2=A0 =C2=A0> ---
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 .../Library/BaseRiscVMmuLib/BaseRiscVMmu= Lib.c | 101
>=C2=A0 =C2=A0 =C2=A0+++++++++++++++---
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf= =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 =C2=A01 +
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 2 files changed, 88 insertions(+), 14 de= letions(-)
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLi= b/BaseRiscVMmuLib.c
>=C2=A0 =C2=A0 =C2=A0b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLi= b.c
>=C2=A0 =C2=A0 =C2=A0> index 826a1d32a1d4..f4419bb8f380 100644
>=C2=A0 =C2=A0 =C2=A0> --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseR= iscVMmuLib.c
>=C2=A0 =C2=A0 =C2=A0> +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseR= iscVMmuLib.c
>=C2=A0 =C2=A0 =C2=A0> @@ -36,6 +36,11 @@
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 #define PTE_PPN_SHIFT=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A010
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 #define RISCV_MMU_PAGE_SHIFT=C2=A0 12 >=C2=A0 =C2=A0 =C2=A0>=C2=A0
>=C2=A0 =C2=A0 =C2=A0> +#define RISCV_CPU_FEATURE_PBMT_BITMASK=C2=A0 = BIT2
>=C2=A0 =C2=A0 =C2=A0> +#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
>=C2=A0 =C2=A0 =C2=A0> +#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
>=C2=A0 =C2=A0 =C2=A0> +#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 =C2=A0 =C2=A0> +
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 STATIC UINTN=C2=A0 mModeSupport[] =3D { = SATP_MODE_SV57, SATP_MODE_SV48,
>=C2=A0 =C2=A0 =C2=A0SATP_MODE_SV39, SATP_MODE_OFF };
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 STATIC UINTN=C2=A0 mMaxRootTableLevel; >=C2=A0 =C2=A0 =C2=A0>=C2=A0 STATIC UINTN=C2=A0 mBitPerLevel;
>=C2=A0 =C2=A0 =C2=A0> @@ -489,32 +494,89 @@ UpdateRegionMapping ( >=C2=A0 =C2=A0 =C2=A0>=C2=A0 /**
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 Convert GCD attribute to RISC-V p= age attribute.
>=C2=A0 =C2=A0 =C2=A0>=C2=A0
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 @param=C2=A0 GcdAttributes The GCD att= ribute.
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 @param=C2=A0 GcdAttributes=C2=A0 =C2= =A0The GCD attribute.
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 @param=C2=A0 RiscVAttribtues The point= er of RISC-V page attribute.
>=C2=A0 =C2=A0 =C2=A0>=C2=A0
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 @return=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0The RISC-V page attribute.
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 @retval EFI_INVALID_PARAMETER=C2=A0 = =C2=A0The RiscVAttribtues is NULL or
>=C2=A0 =C2=A0 =C2=A0cache type mask not valid.
>=C2=A0 =C2=A0 =C2=A0> +=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>=C2=A0
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 **/
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 STATIC
>=C2=A0 =C2=A0 =C2=A0> -UINTN
>=C2=A0 =C2=A0 =C2=A0> +EFI_STATUS
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 GcdAttributeToPageAttribute (
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 IN UINTN=C2=A0 GcdAttributes
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 IN UINTN=C2=A0 =C2=A0GcdAttributes, >
>=C2=A0 =C2=A0 =C2=A0Just noticing: why is GcdAttributes *not* UINT64 in= the first place?
>
>=C2=A0 =C2=A0 =C2=A0All the bit macros we test against it, such as EFI_= MEMORY_RO
>=C2=A0 =C2=A0 =C2=A0(0x0000000000020000ULL) are of type unsigned long l= ong (UINT64).
>
> Good catch. Will fix it.=C2=A0
>
>
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 OUT UINTN=C2=A0 *RiscVAttributes
>=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 UINTN=C2=A0 RiscVAttributes;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 UINT64=C2=A0 =C2=A0CacheTypeMask;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 BOOLEAN=C2=A0 PmbtExtEnabled =3D (PcdG= et64 (PcdRiscVFeatureOverride) &
>=C2=A0 =C2=A0 =C2=A0RISCV_CPU_FEATURE_PBMT_BITMASK) ? TRUE : FALSE;
>
>=C2=A0 =C2=A0 =C2=A0- Per the edk2 coding style, locals should not be i= nitialized (separate
>=C2=A0 =C2=A0 =C2=A0assignment is needed).
>
>=C2=A0 =C2=A0 =C2=A0- Bitmask checks always need an explicit comparison= , such as
>
>=C2=A0 =C2=A0 =C2=A0=C2=A0 ((a & b) !=3D 0)
>
>=C2=A0 =C2=A0 =C2=A0or similar. Implicitly interpreting (a & b) as = a truth value is not
>=C2=A0 =C2=A0 =C2=A0appropriate.
>
>=C2=A0 =C2=A0 =C2=A0- "(whatever) ? TRUE : FALSE" is both bad= style and unnecessary.
>
>=C2=A0 =C2=A0 =C2=A0=C2=A0 BOOLEAN=C2=A0 PmbtExtEnabled;
>
>=C2=A0 =C2=A0 =C2=A0=C2=A0 PmbtExtEnabled =3D (PcdGet64 (PcdRiscVFeatur= eOverride) &
>=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 RISCV_CPU_FEATURE_PBMT_BITMASK) !=3D 0;
>
> Will fix it.=C2=A0
>
>=C2=A0 =C2=A0 =C2=A0>=C2=A0
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 RiscVAttributes =3D RISCV_PG_R | RISCV= _PG_W | RISCV_PG_X;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 if (!RiscVAttributes) {
>
>=C2=A0 =C2=A0 =C2=A0- The coding style requires an explicit nullity che= ck:
>
>=C2=A0 =C2=A0 =C2=A0=C2=A0 if (RiscVAttributes =3D=3D NULL) {
>
> Will fix it. =C2=A0
>
>
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 return EFI_INVALID_PARAMETER; >=C2=A0 =C2=A0 =C2=A0> +=C2=A0 }
>=C2=A0 =C2=A0 =C2=A0> +
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 *RiscVAttributes =3D RISCV_PG_R | RISC= V_PG_W | RISCV_PG_X;
>=C2=A0 =C2=A0 =C2=A0>=C2=A0
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 // Determine protection attribute= s
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 if ((GcdAttributes & EFI_MEMO= RY_RO) !=3D 0) {
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 RiscVAttributes &=3D ~(RISC= V_PG_W);
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 *RiscVAttributes &=3D ~(RIS= CV_PG_W);
>=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 // Process eXecute Never attribut= e
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 if ((GcdAttributes & EFI_MEMO= RY_XP) !=3D 0) {
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 RiscVAttributes &=3D ~RISCV= _PG_X;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 *RiscVAttributes &=3D ~RISC= V_PG_X;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 }
>=C2=A0 =C2=A0 =C2=A0> +
>
>=C2=A0 =C2=A0 =C2=A0My next comment is unrelated to the patch, it's= just something that
>=C2=A0 =C2=A0 =C2=A0catches my eye, and I think is worth fixing:
>
>=C2=A0 =C2=A0 =C2=A0RISCV_PG_W is BIT2 (0x00000004), and RISCV_PG_X is = BIT3 (0x00000008).
>=C2=A0 =C2=A0 =C2=A0Meaning, they are of type *signed int* (INT32). App= lying the bit-neg
>=C2=A0 =C2=A0 =C2=A0operator on them produces a negative value (because= it flips the sign
>=C2=A0 =C2=A0 =C2=A0bit), which is very ugly.
>
>=C2=A0 =C2=A0 =C2=A0I suggest a separate patch for changing these into<= br> >
>=C2=A0 =C2=A0 =C2=A0=C2=A0 ~(UINTN)RISCV_PG_W
>=C2=A0 =C2=A0 =C2=A0=C2=A0 ~(UINTN)RISCV_PG_X
>
>=C2=A0 =C2=A0 =C2=A0Alternatively, you could do
>
> Will fix it in a separate patch along with the above change.
>
>
>=C2=A0 =C2=A0 =C2=A0=C2=A0 *RiscVAttributes =3D RISCV_PG_R;
>=C2=A0 =C2=A0 =C2=A0=C2=A0 if ((GcdAttributes & EFI_MEMORY_RO) =3D= =3D 0) {
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 *RiscVAttributes |=3D RISCV_PG_W;
>=C2=A0 =C2=A0 =C2=A0=C2=A0 }
>=C2=A0 =C2=A0 =C2=A0=C2=A0 if ((GcdAttributes & EFI_MEMORY_XP) =3D= =3D 0) {
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 *RiscVAttributes |=3D RISCV_PG_X;
>=C2=A0 =C2=A0 =C2=A0=C2=A0 }
>
>=C2=A0 =C2=A0 =C2=A0Either way: separate patch.
>
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 CacheTypeMask =3D GcdAttributes & = EFI_CACHE_ATTRIBUTE_MASK;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 if ((CacheTypeMask !=3D 0) &&<= br> >=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 (((CacheTypeMask - 1) &a= mp; CacheTypeMask) !=3D 0))
>
>=C2=A0 =C2=A0 =C2=A0This is not what I recommended in my previous revie= w
>=C2=A0 =C2=A0 =C2=A0<https://edk2.groups.io/g/de= vel/message/115243
>=C2=A0 =C2=A0 =C2=A0<https://edk2.groups.io/g/de= vel/message/115243>>.
>
>=C2=A0 =C2=A0 =C2=A0Compare:
>
>=C2=A0 =C2=A0 =C2=A0=C2=A0 (CacheTypeMask !=3D 0) && ...
>
>=C2=A0 =C2=A0 =C2=A0versus
>
>=C2=A0 =C2=A0 =C2=A0=C2=A0 (CacheTypeMask =3D=3D 0) || ...
>
>=C2=A0 =C2=A0 =C2=A0Both of these ensure that the power-of-two check in= the second
>=C2=A0 =C2=A0 =C2=A0subcondition (i.e., the subtraction of 1) is avoide= d when CacheTypeMask
>=C2=A0 =C2=A0 =C2=A0is zero. In the first variant, you get (FALSE &= & ...), in the second
>=C2=A0 =C2=A0 =C2=A0variant, you get (TRUE || ...); therefore, the powe= r-of-two check is
>=C2=A0 =C2=A0 =C2=A0short-circuited for a zero input in both variants.<= br> >
>=C2=A0 =C2=A0 =C2=A0However, considering the larger CacheTypeMask valid= ation, your variant
>=C2=A0 =C2=A0 =C2=A0is incorrect, because a zero CacheTypeMask will ult= imately evaluate the
>=C2=A0 =C2=A0 =C2=A0condition to FALSE -- (FALSE && ...) is FAL= SE --, and so the "return
>=C2=A0 =C2=A0 =C2=A0EFI_INVALID_PARAMETER" statement will not be r= eached. Whereas (TRUE ||
>=C2=A0 =C2=A0 =C2=A0...) is TRUE, and so we return EFI_INVALID_PARAMETE= R for
>=C2=A0 =C2=A0 =C2=A0CacheTypeMask=3D=3D0.
>
> Actually the EDK2 passes (CacheTypeMask =3D=3D 0) to this API during m= y
> debug session.
> Given that situation, this function doesn't do anything when
> CacheTypeMask =C2=A0=3D=3D 0 so I think
> it should not give the warning message.

I would be curious how that can happen; to me a CacheTypeMask=3D=3D0 input<= br> looks somewhat invalid.

Either way, if such an input *is* valid, then there is a different
problem with the patch: in the debug message we say that the cache type
mask should contain *exactly one* bit set. That's not correct then: it<= br> should say *at most one* bit set. (Because the value 0 has 0 bits set,
and apparently that is valid input.)
How about:=C2=A0 = "More than one bit set in cache type mask" ? It is a clear messag= e that we don't expect
more than 1 bit set if not zero.
=


>
>
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 {
>=C2=A0 =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=A0 =C2=A0 =C2=A0DEBUG_ERROR,
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"%a: The cach= e type mask (0x%llX) should contain exactly
>=C2=A0 =C2=A0 =C2=A0one bit set\n",
>
>=C2=A0 =C2=A0 =C2=A0- Edk2's PrintLib does not use "ll" l= ength modifiers. %u, %x and %X are
>=C2=A0 =C2=A0 =C2=A0for UINT32, and %lu, %lx and %lX are for UINT64. Fu= rthermore, you may
>=C2=A0 =C2=A0 =C2=A0replace "l" with "L" freely. >
> Will fix it.=C2=A0
>
>
>=C2=A0 =C2=A0 =C2=A0- We generally group together the double parens for= DEBUG invocations:
>
>=C2=A0 =C2=A0 =C2=A0=C2=A0 DEBUG ((
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 DEBUG_ERROR,
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 "%a: The cache type mask (0x%lX)= ...\n",
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 __func__,
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 CacheTypeMask
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 ));
>
>=C2=A0 =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=A0CacheTypeMask
>=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=A0The indentation of the closing parens is not correc= t either; please put
>=C2=A0 =C2=A0 =C2=A0your patches through uncrustify first. (CI will rej= ect these issues
>=C2=A0 =C2=A0 =C2=A0anyway, in github pull requests.)
>
> Actually this code is the result of uncrustify modification. Let me > check if anything
> wrong with uncrustify.=C2=A0

It's very strange. Do you know what your original code (the input to uncrustify) looked like? I wonder if uncrustify produces strange output
if it sees unexpected input. Normally I wouldn't expect uncrustify to change the "((" format that I'm proposing. If it still does, = then my
request is invalid of course (uncrustify has priority, whatever it does).
I checked and it comes from an un-correct uncrustify ve= rsion I used before. It should be good now.

Thanks!
Laszlo

>
>
>=C2=A0 =C2=A0 =C2=A0For running uncrustify locally:
>
>=C2=A0 =C2=A0 =C2=A0- clone
>=C2=A0 =C2=A0 =C2=A0<http= s://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify <https://projectmu@dev.azure.com/pr= ojectmu/Uncrustify/_git/Uncrustify>>
>
>=C2=A0 =C2=A0 =C2=A0- check it out at tag 73.0.8 (the tag that edk2 CI = uses on github is in
>=C2=A0 =C2=A0 =C2=A0".pytool/Plugin/UncrustifyCheck/uncrustify_ext= _dep.yaml")
>
>=C2=A0 =C2=A0 =C2=A0- build it (IIRC it uses cmake)
>
>=C2=A0 =C2=A0 =C2=A0- with nothing dirty in the working tree (i.e., eve= rything committed, or
>=C2=A0 =C2=A0 =C2=A0at least stashed to the index), run
>
>=C2=A0 =C2=A0 =C2=A0=C2=A0 uncrustify \
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 -c .pytool/Plugin/UncrustifyCheck/unc= rustify.cfg \
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 --replace \
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 --no-backup \
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 --if-changed \
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 -F file-list.txt
>
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 return EFI_INVALID_PARAMETER; >=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 return RiscVAttributes;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 switch (CacheTypeMask) {
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 case EFI_MEMORY_UC:
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 if (PmbtExtEnabled) { >=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 *RiscVAttributes = |=3D PTE_PBMT_IO;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 } else {
>=C2=A0 =C2=A0 =C2=A0> +=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=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DEBU= G_VERBOSE,
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&quo= t;%a: EFI_MEMORY_UC set but Pmbt extension not
>=C2=A0 =C2=A0 =C2=A0available\n",
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0__fu= nc__
>=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 );
>=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 =C2=A0> +=C2=A0 =C2=A0 case EFI_MEMORY_WC:
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 if (PmbtExtEnabled) { >=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 *RiscVAttributes = |=3D PTE_PBMT_NC;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 } else {
>=C2=A0 =C2=A0 =C2=A0> +=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=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DEBU= G_VERBOSE,
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&quo= t;%a: EFI_MEMORY_WC set but Pmbt extension not
>=C2=A0 =C2=A0 =C2=A0available\n",
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0__fu= nc__
>=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 );
>=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 =C2=A0> +=C2=A0 =C2=A0 default:
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 // Default PMA mode
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 break;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 }
>=C2=A0 =C2=A0 =C2=A0> +
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 return EFI_SUCCESS;
>=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> @@ -537,21 +599,32 @@ RiscVSetMemoryAttributes= (
>=C2=A0 =C2=A0 =C2=A0>=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 =C2=A0 )
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 UINTN=C2=A0 PageAttributesSet;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 UINTN=C2=A0 =C2=A0 =C2=A0 =C2=A0PageAt= tributesSet;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 UINTN=C2=A0 =C2=A0 =C2=A0 =C2=A0PageAt= tributesClear;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 EFI_STATUS=C2=A0 Status;
>=C2=A0 =C2=A0 =C2=A0>=C2=A0
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 PageAttributesSet =3D GcdAttributeToPa= geAttribute (Attributes);
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 Status =3D GcdAttributeToPageAttribute= (Attributes,
>=C2=A0 =C2=A0 =C2=A0&PageAttributesSet);
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 if (EFI_ERROR (Status)) {
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 return Status;
>=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 if (!RiscVMmuEnabled ()) {
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 return EFI_SUCCESS;
>=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 PageAttributesClear =3D PTE_ATTRIBUTES= _MASK;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 if ((PcdGet64 (PcdRiscVFeatureOverride= ) &
>=C2=A0 =C2=A0 =C2=A0RISCV_CPU_FEATURE_PBMT_BITMASK) !=3D 0) {
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 PageAttributesClear |=3D PTE_PB= MT_MASK;
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 }
>=C2=A0 =C2=A0 =C2=A0> +
>=C2=A0 =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=A0 =C2=A0 =C2=A0DEBUG_VERBOSE,
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0"%a: Set %llX page a= ttribute 0x%X\n",
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0"%a: %llX: set attri= butes 0x%X, clear attributes 0x%X\n",
>=C2=A0 =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=A0BaseAddress,
>=C2=A0 =C2=A0 =C2=A0> -=C2=A0 =C2=A0 =C2=A0PageAttributesSet
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0PageAttributesSet,
>=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 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 );
>
>=C2=A0 =C2=A0 =C2=A0- UINT64 should be formatted with %[Ll][uxX].
>
>=C2=A0 =C2=A0 =C2=A0- UINT32 should be formatted with %[uxX].
>
>=C2=A0 =C2=A0 =C2=A0- UINTN is trickier, there is no dedicated conversi= on specifier. The
>=C2=A0 =C2=A0 =C2=A0portable solution (between 32-bit and 64-bit platfo= rms in edk2) is to
>=C2=A0 =C2=A0 =C2=A0(a) cast the UINTN value to UINT64, (b) format the = latter with
>=C2=A0 =C2=A0 =C2=A0%[Ll][uxX].
>
>=C2=A0 =C2=A0 =C2=A0So you need something like
>
>=C2=A0 =C2=A0 =C2=A0=C2=A0 DEBUG ((
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 DEBUG_VERBOSE,
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 "%a: %LX: set attributes 0x%LX, = clear attributes 0x%LX\n",
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 __func__,
>=C2=A0 =C2=A0 =C2=A0=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 =C2=A0=C2=A0 =C2=A0 (UINT64)PageAttributesSet,=C2=A0 // o= riginally UINTN
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 (UINT64)PageAttributesClear // origin= ally UINTN
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 ));
>
> Thanks for the suggestion. Will fix it.=C2=A0
>
>
>=C2=A0 =C2=A0 =C2=A0>=C2=A0
>=C2=A0 =C2=A0 =C2=A0> @@ -559,7 +632,7 @@ RiscVSetMemoryAttributes (=
>=C2=A0 =C2=A0 =C2=A0>=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=A0 =C2=A0 =C2=A0 =C2= =A0Length,
>=C2=A0 =C2=A0 =C2=A0>=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=A0 =C2=A0 =C2=A0 =C2=A0PTE_= ATTRIBUTES_MASK,
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Page= AttributesClear,
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0(UINTN *)RiscVGetRootTranslateTable (),
>=C2=A0 =C2=A0 =C2=A0>=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 =C2=A0 =C2=A0 =C2= =A0);
>=C2=A0 =C2=A0 =C2=A0> diff --git
>=C2=A0 =C2=A0 =C2=A0a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLi= b.inf
>=C2=A0 =C2=A0 =C2=A0b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLi= b.inf
>=C2=A0 =C2=A0 =C2=A0> index 51ebe1750e97..1dbaa81f3608 100644
>=C2=A0 =C2=A0 =C2=A0> --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseR= iscVMmuLib.inf
>=C2=A0 =C2=A0 =C2=A0> +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseR= iscVMmuLib.inf
>=C2=A0 =C2=A0 =C2=A0> @@ -28,3 +28,4 @@
>=C2=A0 =C2=A0 =C2=A0>=C2=A0
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 [Pcd]
>=C2=A0 =C2=A0 =C2=A0>=C2=A0 =C2=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuR= iscVMmuMaxSatpMode=C2=A0 ## CONSUMES
>=C2=A0 =C2=A0 =C2=A0> +=C2=A0 gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatu= reOverride=C2=A0 =C2=A0 =C2=A0## CONSUMES
>
>=C2=A0 =C2=A0 =C2=A0Laszlo
>=20
_._,_._,_
Groups.io Links:

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

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

_._,_._,_
--0000000000003fd91006131933a3--