On 2/7/24 02:29, Tuan Phan wrote:
> The GCD EFI_MEMORY_UC and EFI_MEMORY_WC attributes will be
> supported when Svpbmt extension available.
>
> Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> ---
>  .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 25 ++++++++++++++++++-
>  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf       |  1 +
>  2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> index 826a1d32a1d4..c50a28e97e4b 100644
> --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> @@ -36,6 +36,15 @@
>  #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)
> +
> +#define EFI_MEMORY_CACHETYPE_MASK  (EFI_MEMORY_UC | EFI_MEMORY_WC |  \
> +                                     EFI_MEMORY_WT | EFI_MEMORY_WB | \
> +                                     EFI_MEMORY_UCE)
> +

(1) I've stated this elsewhere -- introducing such a macro is justified,
but calling it EFI_* is not. The EFI_ prefix is reserved for the spec.

>  STATIC UINTN  mModeSupport[] = { SATP_MODE_SV57, SATP_MODE_SV48, SATP_MODE_SV39, SATP_MODE_OFF };
>  STATIC UINTN  mMaxRootTableLevel;
>  STATIC UINTN  mBitPerLevel;
> @@ -514,6 +523,20 @@ GcdAttributeToPageAttribute (
>      RiscVAttributes &= ~RISCV_PG_X;
>    }
> 
> +  if ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
> +    switch (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) {
> +      case EFI_MEMORY_UC:
> +        RiscVAttributes |= PTE_PBMT_IO;
> +        break;
> +      case EFI_MEMORY_WC:
> +        RiscVAttributes |= PTE_PBMT_NC;
> +        break;
> +      default:
> +        // Default PMA mode
> +        break;
> +    }
> +  }
> +
>    return RiscVAttributes;
>  }
> 

Several questions / observations:

(2) If the feature is cleared in the PCD, does it deserve a warning that
the attribute setting request cannot be honored?

(3) The memory cacheability attributes are expressed as distinct bits of
a bitmask because, for expressing *capabilities*, they must be possible
to OR together. However, when setting actual attributes, I think the
bitmask should contain *exactly* one bit set -- in other words, the
value of the bitmask should be an integral power of two (that's not hard
to check).

Do you agree about this? If so, I'd suggest rejecting the request (with
an appropriate status code) if zero bits, or multiple bits, are set.

  UINT64  CacheTypeMask;

  CacheType = GcdAttributes & MEMORY_CACHETYPE_MASK;
  if ((CacheType == 0) ||
      (((CacheType - 1) & CacheType) != 0)) {
    return EFI_INVALID_PARAMETER;
  }
  switch (CacheType) {
    ...
  }

This would of course require changing the GcdAttributeToPageAttribute()
prototype, because right now the function cannot return an error.

> @@ -559,7 +582,7 @@ RiscVSetMemoryAttributes (
>             BaseAddress,
>             Length,
>             PageAttributesSet,
> -           PTE_ATTRIBUTES_MASK,
> +           PTE_ATTRIBUTES_MASK | PTE_PBMT_MASK,
>             (UINTN *)RiscVGetRootTranslateTable (),
>             TRUE
>             );

(4) I feel we shouldn't try to clear PTE_PBMT_MASK if
PcdRiscVFeatureOverride tells us that Svpbmt is not available. Just a
thought.

> 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

Thanks
Laszlo