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 <kra...@redhat.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Rahul Kumar <rahul1.ku...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Signed-off-by: Tuan Phan <tp...@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).
> + 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;
>
> - RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> + if (!RiscVAttributes) {
- The coding style requires an explicit nullity check:
if (RiscVAttributes == NULL) {
> + 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
*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.
> + {
> + 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
<https://projec...@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
));
>
> @@ -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
-=-=-=-=-=-=-=-=-=-=-=-
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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
