[edk2-devel] [PATCH v4 0/4] RISC-V: Support Svpbmt extension

[edk2-devel] [PATCH v4 0/4] RISC-V: Support Svpbmt extension

[Tuan Phan]

This series adds support for RISC-V Svpbmt extension.

The GCD EFI_MEMORY_UC and EFI_MEMORY_WC attributes will
be mapped to IO and NC mode defined in PBMT field.

v4:
  - Changed UINTN to UINT64.
  - Fixed format error.
  - Addressed Andrei's comment.
v3:
  - Addressed Laszlo's comments.
v2:
  - Generated patch for each package.

Tuan Phan (4):
  MdePkg.dec: RISC-V: Define override bit for Svpbmt extension
  UefiCpuPkg: RISC-V: MMU: Explictly use UINT64 instead of UINTN
  UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension
  OvmfPkg/RiscVVirt: Disable Svpbmt extension

 MdePkg/MdePkg.dec                             |   2 +
 OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc           |   2 +-
 .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 252 +++++++++++-------
 .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf       |   1 +
 4 files changed, 159 insertions(+), 98 deletions(-)

-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116775): https://edk2.groups.io/g/devel/message/116775
Mute This Topic: https://groups.io/mt/104934682/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v4 1/4] MdePkg.dec: RISC-V: Define override bit for Svpbmt extension

[Tuan Phan]

Define the BIT 2 as the override bit for Svpbmt extension. This will
be used by RISC-V MMU library to support EFI_MEMORY_UC and
EFI_MEMORY_WC.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
 MdePkg/MdePkg.dec | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 0459418906f8..6850acb96b92 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2407,6 +2407,8 @@
   # previous stage has feature enabled and user wants to disable it.
   # BIT 1 = Supervisor Time Compare (Sstc). This bit is relevant only if
   # previous stage has feature enabled and user wants to disable it.
+  # BIT 2 = Page-Based Memory Types (Pbmt). This bit is relevant only if
+  # previous stage has feature enabled and user wants to disable it.
   #
   gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|UINT64|0x69
 
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116776): https://edk2.groups.io/g/devel/message/116776
Mute This Topic: https://groups.io/mt/104934687/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4 1/4] MdePkg.dec: RISC-V: Define override bit for Svpbmt extension

[Sunil V L]

On Thu, Mar 14, 2024 at 01:19:14PM -0700, Tuan Phan wrote:
> Define the BIT 2 as the override bit for Svpbmt extension. This will
> be used by RISC-V MMU library to support EFI_MEMORY_UC and
> EFI_MEMORY_WC.
> 
Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116841): https://edk2.groups.io/g/devel/message/116841
Mute This Topic: https://groups.io/mt/104934687/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v4 2/4] UefiCpuPkg: RISC-V: MMU: Explictly use UINT64 instead of UINTN

[Tuan Phan]

While UINTN defined for RISC-V 64 bits is UINT64, explictly using UINT64
for those variables that clearly are UINT64.

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 | 158 +++++++++---------
 1 file changed, 76 insertions(+), 82 deletions(-)

diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
index 826a1d32a1d4..46ba4b4709b1 100644
--- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
+++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c
@@ -65,7 +65,7 @@ RiscVMmuEnabled (
 
 **/
 STATIC
-UINTN
+UINT64
 RiscVGetRootTranslateTable (
   VOID
   )
@@ -86,7 +86,7 @@ RiscVGetRootTranslateTable (
 STATIC
 BOOLEAN
 IsValidPte (
-  IN  UINTN  Entry
+  IN  UINT64  Entry
   )
 {
   if (((Entry & RISCV_PG_V) == 0) ||
@@ -107,9 +107,9 @@ IsValidPte (
 
 **/
 STATIC
-UINTN
+UINT64
 SetValidPte (
-  IN  UINTN  Entry
+  IN  UINT64  Entry
   )
 {
   /* Set Valid and Global mapping bits */
@@ -128,7 +128,7 @@ SetValidPte (
 STATIC
 BOOLEAN
 IsBlockEntry (
-  IN  UINTN  Entry
+  IN  UINT64  Entry
   )
 {
   return IsValidPte (Entry) &&
@@ -147,7 +147,7 @@ IsBlockEntry (
 STATIC
 BOOLEAN
 IsTableEntry (
-  IN  UINTN  Entry
+  IN  UINT64  Entry
   )
 {
   return IsValidPte (Entry) &&
@@ -163,13 +163,13 @@ IsTableEntry (
 
 **/
 STATIC
-UINTN
+UINT64
 SetTableEntry (
-  IN  UINTN  Entry
+  IN  UINT64  Entry
   )
 {
   Entry  = SetValidPte (Entry);
-  Entry &= ~(RISCV_PG_X | RISCV_PG_W | RISCV_PG_R);
+  Entry &= ~(UINT64)(RISCV_PG_X | RISCV_PG_W | RISCV_PG_R);
 
   return Entry;
 }
@@ -186,9 +186,9 @@ SetTableEntry (
 STATIC
 VOID
 ReplaceTableEntry (
-  IN  UINTN    *Entry,
-  IN  UINTN    Value,
-  IN  UINTN    RegionStart,
+  IN  UINT64   *Entry,
+  IN  UINT64   Value,
+  IN  UINT64   RegionStart,
   IN  BOOLEAN  IsLiveBlockMapping
   )
 {
@@ -208,9 +208,9 @@ ReplaceTableEntry (
 
 **/
 STATIC
-UINTN
+UINT64
 GetPpnfromPte (
-  IN UINTN  Entry
+  IN UINT64  Entry
   )
 {
   return ((Entry & PTE_PPN_MASK) >> PTE_PPN_SHIFT);
@@ -226,13 +226,13 @@ GetPpnfromPte (
 
 **/
 STATIC
-UINTN
+UINT64
 SetPpnToPte (
-  UINTN  Entry,
-  UINTN  Address
+  UINT64  Entry,
+  UINT64  Address
   )
 {
-  UINTN  Ppn;
+  UINT64  Ppn;
 
   Ppn = ((Address >> RISCV_MMU_PAGE_SHIFT) << PTE_PPN_SHIFT);
   ASSERT (~(Ppn & ~PTE_PPN_MASK));
@@ -250,8 +250,8 @@ SetPpnToPte (
 STATIC
 VOID
 FreePageTablesRecursive (
-  IN  UINTN  *TranslationTable,
-  IN  UINTN  Level
+  IN  UINT64  *TranslationTable,
+  IN  UINTN   Level
   )
 {
   UINTN  Index;
@@ -260,8 +260,8 @@ FreePageTablesRecursive (
     for (Index = 0; Index < mTableEntryCount; Index++) {
       if (IsTableEntry (TranslationTable[Index])) {
         FreePageTablesRecursive (
-          (UINTN *)(GetPpnfromPte ((TranslationTable[Index])) <<
-                    RISCV_MMU_PAGE_SHIFT),
+          (UINT64 *)(GetPpnfromPte ((TranslationTable[Index])) <<
+                     RISCV_MMU_PAGE_SHIFT),
           Level + 1
           );
       }
@@ -289,22 +289,22 @@ FreePageTablesRecursive (
 STATIC
 EFI_STATUS
 UpdateRegionMappingRecursive (
-  IN  UINTN    RegionStart,
-  IN  UINTN    RegionEnd,
-  IN  UINTN    AttributeSetMask,
-  IN  UINTN    AttributeClearMask,
-  IN  UINTN    *PageTable,
+  IN  UINT64   RegionStart,
+  IN  UINT64   RegionEnd,
+  IN  UINT64   AttributeSetMask,
+  IN  UINT64   AttributeClearMask,
+  IN  UINT64   *PageTable,
   IN  UINTN    Level,
   IN  BOOLEAN  TableIsLive
   )
 {
   EFI_STATUS  Status;
-  UINTN       BlockShift;
-  UINTN       BlockMask;
-  UINTN       BlockEnd;
-  UINTN       *Entry;
-  UINTN       EntryValue;
-  UINTN       *TranslationTable;
+  UINT64      BlockShift;
+  UINT64      BlockMask;
+  UINT64      BlockEnd;
+  UINT64      *Entry;
+  UINT64      EntryValue;
+  UINT64      *TranslationTable;
   BOOLEAN     NextTableIsLive;
 
   ASSERT (Level < mMaxRootTableLevel);
@@ -313,18 +313,16 @@ UpdateRegionMappingRecursive (
   BlockShift = (mMaxRootTableLevel - Level - 1) * mBitPerLevel + RISCV_MMU_PAGE_SHIFT;
   BlockMask  = MAX_ADDRESS >> (64 - BlockShift);
 
-  DEBUG (
-    (
-     DEBUG_VERBOSE,
-     "%a(%d): %llx - %llx set %lx clr %lx\n",
-     __func__,
-     Level,
-     RegionStart,
-     RegionEnd,
-     AttributeSetMask,
-     AttributeClearMask
-    )
-    );
+  DEBUG ((
+    DEBUG_VERBOSE,
+    "%a(%d): %LX - %LX set %LX clr %LX\n",
+    __func__,
+    Level,
+    RegionStart,
+    RegionEnd,
+    AttributeSetMask,
+    AttributeClearMask
+    ));
 
   for ( ; RegionStart < RegionEnd; RegionStart = BlockEnd) {
     BlockEnd = MIN (RegionEnd, (RegionStart | BlockMask) + 1);
@@ -380,7 +378,7 @@ UpdateRegionMappingRecursive (
 
         NextTableIsLive = FALSE;
       } else {
-        TranslationTable = (UINTN *)(GetPpnfromPte (*Entry) << RISCV_MMU_PAGE_SHIFT);
+        TranslationTable = (UINT64 *)(GetPpnfromPte (*Entry) << RISCV_MMU_PAGE_SHIFT);
         NextTableIsLive  = TableIsLive;
       }
 
@@ -412,7 +410,7 @@ UpdateRegionMappingRecursive (
       }
 
       if (!IsTableEntry (*Entry)) {
-        EntryValue = SetPpnToPte (0, (UINTN)TranslationTable);
+        EntryValue = SetPpnToPte (0, (UINT64)TranslationTable);
         EntryValue = SetTableEntry (EntryValue);
         ReplaceTableEntry (
           Entry,
@@ -463,11 +461,11 @@ UpdateRegionMappingRecursive (
 STATIC
 EFI_STATUS
 UpdateRegionMapping (
-  IN  UINTN    RegionStart,
-  IN  UINTN    RegionLength,
-  IN  UINTN    AttributeSetMask,
-  IN  UINTN    AttributeClearMask,
-  IN  UINTN    *RootTable,
+  IN  UINT64   RegionStart,
+  IN  UINT64   RegionLength,
+  IN  UINT64   AttributeSetMask,
+  IN  UINT64   AttributeClearMask,
+  IN  UINT64   *RootTable,
   IN  BOOLEAN  TableIsLive
   )
 {
@@ -495,23 +493,23 @@ UpdateRegionMapping (
 
 **/
 STATIC
-UINTN
+UINT64
 GcdAttributeToPageAttribute (
-  IN UINTN  GcdAttributes
+  IN UINT64  GcdAttributes
   )
 {
-  UINTN  RiscVAttributes;
+  UINT64  RiscVAttributes;
 
   RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
 
   // Determine protection attributes
   if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
-    RiscVAttributes &= ~(RISCV_PG_W);
+    RiscVAttributes &= ~(UINT64)(RISCV_PG_W);
   }
 
   // Process eXecute Never attribute
   if ((GcdAttributes & EFI_MEMORY_XP) != 0) {
-    RiscVAttributes &= ~RISCV_PG_X;
+    RiscVAttributes &= ~(UINT64)RISCV_PG_X;
   }
 
   return RiscVAttributes;
@@ -533,11 +531,11 @@ EFI_STATUS
 EFIAPI
 RiscVSetMemoryAttributes (
   IN EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN UINTN                 Length,
-  IN UINTN                 Attributes
+  IN UINT64                Length,
+  IN UINT64                Attributes
   )
 {
-  UINTN  PageAttributesSet;
+  UINT64  PageAttributesSet;
 
   PageAttributesSet = GcdAttributeToPageAttribute (Attributes);
 
@@ -560,7 +558,7 @@ RiscVSetMemoryAttributes (
            Length,
            PageAttributesSet,
            PTE_ATTRIBUTES_MASK,
-           (UINTN *)RiscVGetRootTranslateTable (),
+           (UINT64 *)RiscVGetRootTranslateTable (),
            TRUE
            );
 }
@@ -583,8 +581,8 @@ RiscVMmuSetSatpMode  (
   )
 {
   VOID                             *TranslationTable;
-  UINTN                            SatpReg;
-  UINTN                            Ppn;
+  UINT64                           SatpReg;
+  UINT64                           Ppn;
   EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *MemoryMap;
   UINTN                            NumberOfDescriptors;
   UINTN                            Index;
@@ -622,7 +620,7 @@ RiscVMmuSetSatpMode  (
     return EFI_OUT_OF_RESOURCES;
   }
 
-  ZeroMem (TranslationTable, mTableEntryCount * sizeof (UINTN));
+  ZeroMem (TranslationTable, mTableEntryCount * sizeof (UINT64));
 
   NumberOfDescriptors = 0;
   MemoryMap           = NULL;
@@ -662,7 +660,7 @@ RiscVMmuSetSatpMode  (
     DisableInterrupts ();
   }
 
-  Ppn = (UINTN)TranslationTable >> RISCV_MMU_PAGE_SHIFT;
+  Ppn = (UINT64)TranslationTable >> RISCV_MMU_PAGE_SHIFT;
   ASSERT (!(Ppn & ~(SATP64_PPN)));
 
   SatpReg  = Ppn;
@@ -671,14 +669,12 @@ RiscVMmuSetSatpMode  (
   RiscVSetSupervisorAddressTranslationRegister (SatpReg);
   /* Check if HW support the setup satp mode */
   if (SatpReg != RiscVGetSupervisorAddressTranslationRegister ()) {
-    DEBUG (
-      (
-       DEBUG_VERBOSE,
-       "%a: HW does not support SATP mode:%d\n",
-       __func__,
-       SatpMode
-      )
-      );
+    DEBUG ((
+      DEBUG_VERBOSE,
+      "%a: HW does not support SATP mode:%d\n",
+      __func__,
+      SatpMode
+      ));
     FreePageTablesRecursive (TranslationTable, 0);
     return EFI_DEVICE_ERROR;
   }
@@ -706,7 +702,7 @@ RiscVConfigureMmu (
   )
 {
   EFI_STATUS  Status;
-  INTN        Idx;
+  UINTN       Idx;
 
   Status = EFI_SUCCESS;
 
@@ -719,14 +715,12 @@ RiscVConfigureMmu (
       return Status;
     }
 
-    DEBUG (
-      (
-       DEBUG_INFO,
-       "%a: SATP mode %d successfully configured\n",
-       __func__,
-       mModeSupport[Idx]
-      )
-      );
+    DEBUG ((
+      DEBUG_INFO,
+      "%a: SATP mode %d successfully configured\n",
+      __func__,
+      mModeSupport[Idx]
+      ));
     break;
   }
 
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116777): https://edk2.groups.io/g/devel/message/116777
Mute This Topic: https://groups.io/mt/104934688/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Tuan Phan]

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 | 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 UINT64                Attributes
   )
 {
-  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/BaseRiscVMmuLib.inf
@@ -28,3 +28,4 @@
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVMmuMaxSatpMode  ## CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride     ## CONSUMES
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116779): https://edk2.groups.io/g/devel/message/116779
Mute This Topic: https://groups.io/mt/104934719/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[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 <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 | 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 UINT64                Attributes
>    )
>  {
> -  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,
> +    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/BaseRiscVMmuLib.inf
> @@ -28,3 +28,4 @@
>  
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVMmuMaxSatpMode  ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride     ## CONSUMES
> -- 
> 2.25.1
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116839): https://edk2.groups.io/g/devel/message/116839
Mute This Topic: https://groups.io/mt/104934719/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Tuan Phan]

[-- Attachment #1: Type: text/plain, Size: 7092 bytes --]

Hi Sunil,

On Mon, Mar 18, 2024 at 6:00 AM Sunil V L <sunilvl@ventanamicro.com> 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 <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 | 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 UINT64                Attributes
> >    )
> >  {
> > -  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 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,
> > +    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/BaseRiscVMmuLib.inf
> > @@ -28,3 +28,4 @@
> >
> >  [Pcd]
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVMmuMaxSatpMode  ## CONSUMES
> > +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride     ## CONSUMES
> > --
> > 2.25.1
> >
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116901): https://edk2.groups.io/g/devel/message/116901
Mute This Topic: https://groups.io/mt/104934719/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 9826 bytes --]

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

[Tuan Phan]

[-- Attachment #1: Type: text/plain, Size: 7540 bytes --]

On Tue, Mar 19, 2024 at 9:45 AM Tuan Phan via groups.io <tphan=
ventanamicro.com@groups.io> wrote:

> Hi Sunil,
>
> On Mon, Mar 18, 2024 at 6:00 AM Sunil V L <sunilvl@ventanamicro.com>
> 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 <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 | 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 UINT64                Attributes
>> >    )
>> >  {
>> > -  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 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,
>> > +    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/BaseRiscVMmuLib.inf
>> > @@ -28,3 +28,4 @@
>> >
>> >  [Pcd]
>> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVMmuMaxSatpMode  ## CONSUMES
>> > +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride     ## CONSUMES
>> > --
>> > 2.25.1
>> >
>>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117338): https://edk2.groups.io/g/devel/message/117338
Mute This Topic: https://groups.io/mt/104934719/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 10636 bytes --]

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

[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 <tphan=
> ventanamicro.com@groups.io> wrote:
> 
> > Hi Sunil,
> >
> > On Mon, Mar 18, 2024 at 6:00 AM Sunil V L <sunilvl@ventanamicro.com>
> > 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 <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 | 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;
> >> >  }
> >> >
> >> >  /**
> >> > @@ -535,29 +590,38 @@ RiscVSetMemoryAttributes (
> >> >    IN UINT64                Attributes
> >> >    )
> >> >  {
> >> > -  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.
> >
Okay. Thanks!

> >>
> >> >    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,
> >> > +    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/BaseRiscVMmuLib.inf
> >> > @@ -28,3 +28,4 @@
> >> >
> >> >  [Pcd]
> >> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVMmuMaxSatpMode  ## CONSUMES
> >> > +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride     ## CONSUMES
> >> > --
Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117490): https://edk2.groups.io/g/devel/message/117490
Mute This Topic: https://groups.io/mt/104934719/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v4 4/4] OvmfPkg/RiscVVirt: Disable Svpbmt extension

[Tuan Phan]

Disable Svpbmt extension as QEMU not enables it in default config.

Cc: Andrei Warkentin <andrei.warkentin@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Sunil V L <sunilvl@ventanamicro.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
 OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
index 6bc7c90f31dc..b8338d2eb5f5 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
+++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
@@ -203,7 +203,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
 
 [PcdsFixedAtBuild.common]
-  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFC
+  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFF8
   gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|1000000
   gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|1000000
   gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116778): https://edk2.groups.io/g/devel/message/116778
Mute This Topic: https://groups.io/mt/104934689/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4 4/4] OvmfPkg/RiscVVirt: Disable Svpbmt extension

[Sunil V L]

On Thu, Mar 14, 2024 at 01:19:17PM -0700, Tuan Phan wrote:
> Disable Svpbmt extension as QEMU not enables it in default config.
> 
Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116840): https://edk2.groups.io/g/devel/message/116840
Mute This Topic: https://groups.io/mt/104934689/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sunil V L]

On Thu, Mar 14, 2024 at 01:19:13PM -0700, Tuan Phan wrote:
> This series adds support for RISC-V Svpbmt extension.
> 
> The GCD EFI_MEMORY_UC and EFI_MEMORY_WC attributes will
> be mapped to IO and NC mode defined in PBMT field.
> 
> v4:
>   - Changed UINTN to UINT64.
>   - Fixed format error.
>   - Addressed Andrei's comment.
> v3:
>   - Addressed Laszlo's comments.
> v2:
>   - Generated patch for each package.
> 
> Tuan Phan (4):
>   MdePkg.dec: RISC-V: Define override bit for Svpbmt extension
>   UefiCpuPkg: RISC-V: MMU: Explictly use UINT64 instead of UINTN
>   UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension
>   OvmfPkg/RiscVVirt: Disable Svpbmt extension
> 
>  MdePkg/MdePkg.dec                             |   2 +
>  OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc           |   2 +-
>  .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 252 +++++++++++-------
>  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf       |   1 +
>  4 files changed, 159 insertions(+), 98 deletions(-)
> 
> -- 
Merged as #5535

Thanks,
Sunil


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117491): https://edk2.groups.io/g/devel/message/117491
Mute This Topic: https://groups.io/mt/104934682/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-