Thanks for the detailed review. Please see my comments below.

On Fri, Mar 1, 2024 at 4:14 AM 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 <ray.ni@intel.com>
> Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> ---
> .../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[] = { 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 = (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) != 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 = (PcdGet64 (PcdRiscVFeatureOverride) &
RISCV_CPU_FEATURE_PBMT_BITMASK) != 0;

Will fix it.

>
> - RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> + if (!RiscVAttributes) {

- The coding style requires an explicit nullity check:

if (RiscVAttributes == NULL) {

Will fix it.

> + return EFI_INVALID_PARAMETER;
> + }
> +
> + *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
>
> // Determine protection attributes
> if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> - RiscVAttributes &= ~(RISCV_PG_W);
> + *RiscVAttributes &= ~(RISCV_PG_W);
> }
>
> // Process eXecute Never attribute
> if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
> - RiscVAttributes &= ~RISCV_PG_X;
> + *RiscVAttributes &= ~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 = RISCV_PG_R;
if ((GcdAttributes & EFI_MEMORY_RO) == 0) {
*RiscVAttributes |= RISCV_PG_W;
}
if ((GcdAttributes & EFI_MEMORY_XP) == 0) {
*RiscVAttributes |= RISCV_PG_X;
}

Either way: separate patch.

> + CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
> + if ((CacheTypeMask != 0) &&
> + (((CacheTypeMask - 1) & CacheTypeMask) != 0))

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

Compare:

(CacheTypeMask != 0) && ...

versus

(CacheTypeMask == 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==0.

Actually the EDK2 passes (CacheTypeMask == 0) to this API during my debug session.

Given that situation, this function doesn't do anything when CacheTypeMask == 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 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.

- 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
<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

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 |= 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 |= 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 = GcdAttributeToPageAttribute (Attributes);
> + Status = GcdAttributeToPageAttribute (Attributes, &PageAttributesSet);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
>
> if (!RiscVMmuEnabled ()) {
> return EFI_SUCCESS;
> }
>
> + PageAttributesClear = PTE_ATTRIBUTES_MASK;
> + if ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
> + PageAttributesClear |= 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