Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension

["Laszlo Ersek" <lersek@redhat.com>]

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

> +  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://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
    ));

>  
> @@ -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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-