[edk2-devel] [PATCH v4 3/4] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension

2024-03-14 Thread Tuan Phan 
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 | 106 ++
 .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf   |   1 +
 2 files changed, 86 insertions(+), 21 deletions(-)

diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c 
b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
index 46ba4b4709b1..34300dca5c34 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;
@@ -487,32 +492,82 @@ UpdateRegionMapping (
 /**
   Convert GCD attribute to RISC-V page attribute.
 
-  @param  GcdAttributes The GCD attribute.
+  @param  GcdAttributes   The GCD attribute.
+  @param  RiscVAttributes The pointer of RISC-V page attribute.
 
-  @return   The RISC-V page attribute.
+  @retval EFI_INVALID_PARAMETER   The RiscVAttributes is NULL or cache type 
mask not valid.
+  @retval EFI_SUCCESS The operation succesfully.
 
 **/
 STATIC
-UINT64
+EFI_STATUS
 GcdAttributeToPageAttribute (
-  IN UINT64  GcdAttributes
+  IN UINT64   GcdAttributes,
+  OUT UINT64  *RiscVAttributes
   )
 {
-  UINT64  RiscVAttributes;
+  UINT64   CacheTypeMask;
+  BOOLEAN  PmbtExtEnabled;
 
-  RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
+  if (RiscVAttributes == NULL) {
+return EFI_INVALID_PARAMETER;
+  }
+
+  *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
+
+  PmbtExtEnabled = FALSE;
+  if ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_PBMT_BITMASK) != 
0) {
+PmbtExtEnabled = TRUE;
+  }
 
   // Determine protection attributes
   if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
-RiscVAttributes &= ~(UINT64)(RISCV_PG_W);
+*RiscVAttributes &= ~(UINT64)(RISCV_PG_W);
   }
 
   // Process eXecute Never attribute
   if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
-RiscVAttributes &= ~(UINT64)RISCV_PG_X;
+*RiscVAttributes &= ~(UINT64)RISCV_PG_X;
+  }
+
+  CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
+  if ((CacheTypeMask != 0) &&
+  (((CacheTypeMask - 1) & CacheTypeMask) != 0))
+  {
+DEBUG ((
+  DEBUG_ERROR,
+  "%a: More than one bit set in cache type mask (0x%LX)\n",
+  __func__,
+  CacheTypeMask
+  ));
+return EFI_INVALID_PARAMETER;
+  }
+
+  switch (CacheTypeMask) {
+case EFI_MEMORY_UC:
+  if (PmbtExtEnabled) {
+*RiscVAttributes |= PTE_PBMT_IO;
+  }
+
+  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 RiscVAttributes;
+  return EFI_SUCCESS;
 }
 
 /**
@@ -535,29 +590,38 @@ RiscVSetMemoryAttributes (
   IN UINT64Attributes
   )
 {
-  UINT64  PageAttributesSet;
+  UINT64  PageAttributesSet;
+  UINT64  PageAttributesClear;
+  EFI_STATUS  Status;
 
-  PageAttributesSet = GcdAttributeToPageAttribute (Attributes);
+  Status = GcdAttributeToPageAttribute (Attributes, &PageAttributesSet);
+  if (EFI_ERROR (Status)) {
+return Status;
+  }
 
   if (!RiscVMmuEnabled ()) {
 return EFI_SUCCESS;
   }
 
-  DEBUG (
-(
- DEBUG_VERBOSE,
- "%a: Set %llX page attribute 0x%X\n",
- __func__,
- BaseAddress,
- PageAttributesSet
-)
-);
+  PageAttributesClear = PTE_ATTRIBUTES_MASK;
+  if ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_PBMT_BITMASK) != 
0) {
+PageAttributesClear |= PTE_PBMT_MASK;
+  }
+
+  DEBUG ((
+DEBUG_VERBOSE,
+"%a: %LX: set attributes 0x%LX, clear attributes 0x%LX\n",
+__func__,
+BaseAddress,
+PageAttributesSet,
+PageAttributesClear
+));
 
   return UpdateRegionMapping (
BaseAddress,
Length,
PageAttributesSet,
-   PTE_ATTRIBUTES_MASK,
+   PageAttributesClear,
(UINT64 *)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/BaseRis

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

2024-03-18 Thread Sunil V L 
Hi Tuan,

On Thu, Mar 14, 2024 at 01:19:16PM -0700, 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 | 106 ++
>  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf   |   1 +
>  2 files changed, 86 insertions(+), 21 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c 
> b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> index 46ba4b4709b1..34300dca5c34 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;
> @@ -487,32 +492,82 @@ UpdateRegionMapping (
>  /**
>Convert GCD attribute to RISC-V page attribute.
>  
> -  @param  GcdAttributes The GCD attribute.
> +  @param  GcdAttributes   The GCD attribute.
> +  @param  RiscVAttributes The pointer of RISC-V page attribute.
>  
> -  @return   The RISC-V page attribute.
> +  @retval EFI_INVALID_PARAMETER   The RiscVAttributes is NULL or cache type 
> mask not valid.
> +  @retval EFI_SUCCESS The operation succesfully.
>  
>  **/
>  STATIC
> -UINT64
> +EFI_STATUS
>  GcdAttributeToPageAttribute (
> -  IN UINT64  GcdAttributes
> +  IN UINT64   GcdAttributes,
> +  OUT UINT64  *RiscVAttributes
>)
>  {
> -  UINT64  RiscVAttributes;
> +  UINT64   CacheTypeMask;
> +  BOOLEAN  PmbtExtEnabled;
>  
Why not read the PCD once and save in a static variable?

> -  RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> +  if (RiscVAttributes == NULL) {
> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> +
> +  PmbtExtEnabled = FALSE;
> +  if ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_PBMT_BITMASK) 
> != 0) {
> +PmbtExtEnabled = TRUE;
> +  }
>  
>// Determine protection attributes
>if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> -RiscVAttributes &= ~(UINT64)(RISCV_PG_W);
> +*RiscVAttributes &= ~(UINT64)(RISCV_PG_W);
>}
>  
>// Process eXecute Never attribute
>if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
> -RiscVAttributes &= ~(UINT64)RISCV_PG_X;
> +*RiscVAttributes &= ~(UINT64)RISCV_PG_X;
> +  }
> +
> +  CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
> +  if ((CacheTypeMask != 0) &&
> +  (((CacheTypeMask - 1) & CacheTypeMask) != 0))
> +  {
> +DEBUG ((
> +  DEBUG_ERROR,
> +  "%a: More than one bit set in cache type mask (0x%LX)\n",
> +  __func__,
> +  CacheTypeMask
> +  ));
> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  switch (CacheTypeMask) {
> +case EFI_MEMORY_UC:
> +  if (PmbtExtEnabled) {
> +*RiscVAttributes |= PTE_PBMT_IO;
> +  }
> +
> +  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 RiscVAttributes;
> +  return EFI_SUCCESS;
>  }
>  
>  /**
> @@ -535,29 +590,38 @@ RiscVSetMemoryAttributes (
>IN UINT64Attributes
>)
>  {
> -  UINT64  PageAttributesSet;
> +  UINT64  PageAttributesSet;
> +  UINT64  PageAttributesClear;
> +  EFI_STATUS  Status;
>  
> -  PageAttributesSet = GcdAttributeToPageAttribute (Attributes);
> +  Status = GcdAttributeToPageAttribute (Attributes, &PageAttributesSet);
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
>
Is there a reason to do this prior to checking RiscVMmuEnabled()?

>if (!RiscVMmuEnabled ()) {
>  return EFI_SUCCESS;
>}
>  
> -  DEBUG (
> -(
> - DEBUG_VERBOSE,
> - "%a: Set %llX page attribute 0x%X\n",
> - __func__,
> - BaseAddress,
> - PageAttributesSet
> -)
> -);
> +  PageAttributesClear = PTE_ATTRIBUTES_MASK;
> +  if ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_PBMT_BITMASK) 
> != 0) {
> +PageAttributesClear |= PTE_PBMT_MASK;
> +  }
> +
I think static variable would be better.

> +  DEBUG ((
> +DEBUG_VERBOSE,
> +"%a: %LX: set attributes 0x%LX, clear attributes 0x%LX\n",
> +__func__,
> +BaseAddress,
> +PageAttributesSet,
> +

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

2024-03-19 Thread Tuan Phan 
Hi Sunil,

On Mon, Mar 18, 2024 at 6:00 AM Sunil V L  wrote:

> Hi Tuan,
>
> On Thu, Mar 14, 2024 at 01:19:16PM -0700, 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 | 106 ++
> >  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf   |   1 +
> >  2 files changed, 86 insertions(+), 21 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> > index 46ba4b4709b1..34300dca5c34 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;
> > @@ -487,32 +492,82 @@ UpdateRegionMapping (
> >  /**
> >Convert GCD attribute to RISC-V page attribute.
> >
> > -  @param  GcdAttributes The GCD attribute.
> > +  @param  GcdAttributes   The GCD attribute.
> > +  @param  RiscVAttributes The pointer of RISC-V page attribute.
> >
> > -  @return   The RISC-V page attribute.
> > +  @retval EFI_INVALID_PARAMETER   The RiscVAttributes is NULL or cache
> type mask not valid.
> > +  @retval EFI_SUCCESS The operation succesfully.
> >
> >  **/
> >  STATIC
> > -UINT64
> > +EFI_STATUS
> >  GcdAttributeToPageAttribute (
> > -  IN UINT64  GcdAttributes
> > +  IN UINT64   GcdAttributes,
> > +  OUT UINT64  *RiscVAttributes
> >)
> >  {
> > -  UINT64  RiscVAttributes;
> > +  UINT64   CacheTypeMask;
> > +  BOOLEAN  PmbtExtEnabled;
> >
> Why not read the PCD once and save in a static variable?
>
I can put it into a static variable if you think it is more clean.

>
> > -  RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> > +  if (RiscVAttributes == NULL) {
> > +return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> > +
> > +  PmbtExtEnabled = FALSE;
> > +  if ((PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
> > +PmbtExtEnabled = TRUE;
> > +  }
> >
> >// Determine protection attributes
> >if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> > -RiscVAttributes &= ~(UINT64)(RISCV_PG_W);
> > +*RiscVAttributes &= ~(UINT64)(RISCV_PG_W);
> >}
> >
> >// Process eXecute Never attribute
> >if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
> > -RiscVAttributes &= ~(UINT64)RISCV_PG_X;
> > +*RiscVAttributes &= ~(UINT64)RISCV_PG_X;
> > +  }
> > +
> > +  CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
> > +  if ((CacheTypeMask != 0) &&
> > +  (((CacheTypeMask - 1) & CacheTypeMask) != 0))
> > +  {
> > +DEBUG ((
> > +  DEBUG_ERROR,
> > +  "%a: More than one bit set in cache type mask (0x%LX)\n",
> > +  __func__,
> > +  CacheTypeMask
> > +  ));
> > +return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  switch (CacheTypeMask) {
> > +case EFI_MEMORY_UC:
> > +  if (PmbtExtEnabled) {
> > +*RiscVAttributes |= PTE_PBMT_IO;
> > +  }
> > +
> > +  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 RiscVAttributes;
> > +  return EFI_SUCCESS;
> >  }
> >
> >  /**
> > @@ -535,29 +590,38 @@ RiscVSetMemoryAttributes (
> >IN UINT64Attributes
> >)
> >  {
> > -  UINT64  PageAttributesSet;
> > +  UINT64  PageAttributesSet;
> > +  UINT64  PageAttributesClear;
> > +  EFI_STATUS  Status;
> >
> > -  PageAttributesSet = GcdAttributeToPageAttribute (Attributes);
> > +  Status = GcdAttributeToPageAttribute (Attributes, &PageAttributesSet);
> > +  if (EFI_ERROR (Status)) {
> > +return Status;
> > +  }
> >
> Is there a reason to do this prior to checking RiscVMmuEnabled()?
>
Only reason is return error due to invalid parameter before return
EFI_SUCCESS if Mmu not enabled.

>
> >if (!RiscVMmuEnabled ()) {
> >  return EFI_SUCCESS;
> >}
> >
> > -  DEBUG (
> > -(
> > - DEBUG_VERBOSE,
> > - "%a: Set %llX page attri

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

2024-04-02 Thread Tuan Phan 
On Tue, Mar 19, 2024 at 9:45 AM Tuan Phan via groups.io  wrote:

> Hi Sunil,
>
> On Mon, Mar 18, 2024 at 6:00 AM Sunil V L 
> wrote:
>
>> Hi Tuan,
>>
>> On Thu, Mar 14, 2024 at 01:19:16PM -0700, 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 | 106 ++
>> >  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf   |   1 +
>> >  2 files changed, 86 insertions(+), 21 deletions(-)
>> >
>> > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
>> b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
>> > index 46ba4b4709b1..34300dca5c34 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;
>> > @@ -487,32 +492,82 @@ UpdateRegionMapping (
>> >  /**
>> >Convert GCD attribute to RISC-V page attribute.
>> >
>> > -  @param  GcdAttributes The GCD attribute.
>> > +  @param  GcdAttributes   The GCD attribute.
>> > +  @param  RiscVAttributes The pointer of RISC-V page attribute.
>> >
>> > -  @return   The RISC-V page attribute.
>> > +  @retval EFI_INVALID_PARAMETER   The RiscVAttributes is NULL or cache
>> type mask not valid.
>> > +  @retval EFI_SUCCESS The operation succesfully.
>> >
>> >  **/
>> >  STATIC
>> > -UINT64
>> > +EFI_STATUS
>> >  GcdAttributeToPageAttribute (
>> > -  IN UINT64  GcdAttributes
>> > +  IN UINT64   GcdAttributes,
>> > +  OUT UINT64  *RiscVAttributes
>> >)
>> >  {
>> > -  UINT64  RiscVAttributes;
>> > +  UINT64   CacheTypeMask;
>> > +  BOOLEAN  PmbtExtEnabled;
>> >
>> Why not read the PCD once and save in a static variable?
>>
> I can put it into a static variable if you think it is more clean.
>
Looks like PcdRiscVFeatureOverride can be a patchable PCD so putting it to
a static variable may not work.

>
>> > -  RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
>> > +  if (RiscVAttributes == NULL) {
>> > +return EFI_INVALID_PARAMETER;
>> > +  }
>> > +
>> > +  *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
>> > +
>> > +  PmbtExtEnabled = FALSE;
>> > +  if ((PcdGet64 (PcdRiscVFeatureOverride) &
>> RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
>> > +PmbtExtEnabled = TRUE;
>> > +  }
>> >
>> >// Determine protection attributes
>> >if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
>> > -RiscVAttributes &= ~(UINT64)(RISCV_PG_W);
>> > +*RiscVAttributes &= ~(UINT64)(RISCV_PG_W);
>> >}
>> >
>> >// Process eXecute Never attribute
>> >if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
>> > -RiscVAttributes &= ~(UINT64)RISCV_PG_X;
>> > +*RiscVAttributes &= ~(UINT64)RISCV_PG_X;
>> > +  }
>> > +
>> > +  CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
>> > +  if ((CacheTypeMask != 0) &&
>> > +  (((CacheTypeMask - 1) & CacheTypeMask) != 0))
>> > +  {
>> > +DEBUG ((
>> > +  DEBUG_ERROR,
>> > +  "%a: More than one bit set in cache type mask (0x%LX)\n",
>> > +  __func__,
>> > +  CacheTypeMask
>> > +  ));
>> > +return EFI_INVALID_PARAMETER;
>> > +  }
>> > +
>> > +  switch (CacheTypeMask) {
>> > +case EFI_MEMORY_UC:
>> > +  if (PmbtExtEnabled) {
>> > +*RiscVAttributes |= PTE_PBMT_IO;
>> > +  }
>> > +
>> > +  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 RiscVAttributes;
>> > +  return EFI_SUCCESS;
>> >  }
>> >
>> >  /**
>> > @@ -535,29 +590,38 @@ RiscVSetMemoryAttributes (
>> >IN UINT64Attributes
>> >)
>> >  {
>> > -  UINT64  PageAttributesSet;
>> > +  UINT64  PageAttributesSet;
>> > +  UINT64  PageAttributesClear;
>> > +  EFI_STATUS  Status;
>> >
>> > -  PageAttributesSet = GcdAttributeToPageAttribute (Attributes);
>> > +  Status = GcdAttributeToPageAttribute (Attributes,
>> &PageAttributesSet);
>> > +  if (EFI_ERROR (Status)) {
>> > +return Status;
>> > +  }
>> >
>

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

2024-04-07 Thread Sunil V L 
On Tue, Apr 02, 2024 at 03:42:19PM -0700, Tuan Phan wrote:
> On Tue, Mar 19, 2024 at 9:45 AM Tuan Phan via groups.io  ventanamicro@groups.io> wrote:
> 
> > Hi Sunil,
> >
> > On Mon, Mar 18, 2024 at 6:00 AM Sunil V L 
> > wrote:
> >
> >> Hi Tuan,
> >>
> >> On Thu, Mar 14, 2024 at 01:19:16PM -0700, 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 | 106 ++
> >> >  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf   |   1 +
> >> >  2 files changed, 86 insertions(+), 21 deletions(-)
> >> >
> >> > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> >> b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
> >> > index 46ba4b4709b1..34300dca5c34 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;
> >> > @@ -487,32 +492,82 @@ UpdateRegionMapping (
> >> >  /**
> >> >Convert GCD attribute to RISC-V page attribute.
> >> >
> >> > -  @param  GcdAttributes The GCD attribute.
> >> > +  @param  GcdAttributes   The GCD attribute.
> >> > +  @param  RiscVAttributes The pointer of RISC-V page attribute.
> >> >
> >> > -  @return   The RISC-V page attribute.
> >> > +  @retval EFI_INVALID_PARAMETER   The RiscVAttributes is NULL or cache
> >> type mask not valid.
> >> > +  @retval EFI_SUCCESS The operation succesfully.
> >> >
> >> >  **/
> >> >  STATIC
> >> > -UINT64
> >> > +EFI_STATUS
> >> >  GcdAttributeToPageAttribute (
> >> > -  IN UINT64  GcdAttributes
> >> > +  IN UINT64   GcdAttributes,
> >> > +  OUT UINT64  *RiscVAttributes
> >> >)
> >> >  {
> >> > -  UINT64  RiscVAttributes;
> >> > +  UINT64   CacheTypeMask;
> >> > +  BOOLEAN  PmbtExtEnabled;
> >> >
> >> Why not read the PCD once and save in a static variable?
> >>
> > I can put it into a static variable if you think it is more clean.
> >
> Looks like PcdRiscVFeatureOverride can be a patchable PCD so putting it to
> a static variable may not work.
> 
I don't think that will be an issue for this use case. But I don't have
major issue keeping like this itself.

> >
> >> > -  RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> >> > +  if (RiscVAttributes == NULL) {
> >> > +return EFI_INVALID_PARAMETER;
> >> > +  }
> >> > +
> >> > +  *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
> >> > +
> >> > +  PmbtExtEnabled = FALSE;
> >> > +  if ((PcdGet64 (PcdRiscVFeatureOverride) &
> >> RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) {
> >> > +PmbtExtEnabled = TRUE;
> >> > +  }
> >> >
> >> >// Determine protection attributes
> >> >if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> >> > -RiscVAttributes &= ~(UINT64)(RISCV_PG_W);
> >> > +*RiscVAttributes &= ~(UINT64)(RISCV_PG_W);
> >> >}
> >> >
> >> >// Process eXecute Never attribute
> >> >if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
> >> > -RiscVAttributes &= ~(UINT64)RISCV_PG_X;
> >> > +*RiscVAttributes &= ~(UINT64)RISCV_PG_X;
> >> > +  }
> >> > +
> >> > +  CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK;
> >> > +  if ((CacheTypeMask != 0) &&
> >> > +  (((CacheTypeMask - 1) & CacheTypeMask) != 0))
> >> > +  {
> >> > +DEBUG ((
> >> > +  DEBUG_ERROR,
> >> > +  "%a: More than one bit set in cache type mask (0x%LX)\n",
> >> > +  __func__,
> >> > +  CacheTypeMask
> >> > +  ));
> >> > +return EFI_INVALID_PARAMETER;
> >> > +  }
> >> > +
> >> > +  switch (CacheTypeMask) {
> >> > +case EFI_MEMORY_UC:
> >> > +  if (PmbtExtEnabled) {
> >> > +*RiscVAttributes |= PTE_PBMT_IO;
> >> > +  }
> >> > +
> >> > +  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 RiscVAttributes;
> >> > +  return EFI_SUCCESS;
> >> >  }
> >> >
> >> >  /**
> >> >