public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 0/7] RISC-V: Add MMU support
@ 2023-03-06 17:33 Tuan Phan
  2023-03-06 17:33 ` [PATCH 1/7] MdePkg/BaseLib: RISC-V: Support getting satp register value Tuan Phan
                   ` (7 more replies)
  0 siblings, 8 replies; 16+ messages in thread
From: Tuan Phan @ 2023-03-06 17:33 UTC (permalink / raw)
  To: devel
  Cc: michael.d.kinney, gaoliming, zhiguang.liu, sunilvl, git,
	andrei.warkentin, Tuan Phan

This series adds MMU support for RISC-V. Only SV39/48/57 modes
are supported and tested. The MMU is required to support setting
page attribute which is the first basic step to support security
booting on RISC-V.

There are three parts:
1. Add MMU core to UefiCpuPkg. MMU will be enabled during
CpuDxe initialization.
2. Fix OvmfPkg/VirtNorFlashDxe that failed to add flash base
address to GCD if already done.
3. Enable MMU for RiscVVirt platform and populating its device
resources in SEC phase. All resources should be populated in HOB
or added to GCD by driver before accessing them when MMU enabled.

Tuan Phan (7):
  MdePkg/BaseLib: RISC-V: Support getting satp register value
  MdePkg/Register: RISC-V: Add satp mode bits shift definition
  UefiCpuPkg: RISC-V: Support MMU with SV39/48/57 mode
  OvmfPkg/RiscVVirt: VirtNorFlashPlatformLib: Fix wrong flash size
  OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists
  OvmfPkg/RiscVVirt: SEC: Add IO memory resource hob for platform
    devices
  OvmfPkg/RiscVVirt: Enable MMU with SV39 mode

 MdePkg/Include/Library/BaseLib.h              |   5 +
 .../Include/Register/RiscV64/RiscVEncoding.h  |   7 +-
 MdePkg/Library/BaseLib/RiscV64/RiscVMmu.S     |   8 +
 .../VirtNorFlashStaticLib.c                   |   3 +-
 OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc           |   1 +
 OvmfPkg/RiscVVirt/Sec/Memory.c                |  17 -
 OvmfPkg/RiscVVirt/Sec/Platform.c              |  62 +++
 OvmfPkg/RiscVVirt/Sec/SecMain.inf             |   1 +
 OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c     |  25 +-
 UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c             |  10 +-
 UefiCpuPkg/CpuDxeRiscV64/CpuDxe.h             |   1 +
 UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf    |   5 +
 UefiCpuPkg/CpuDxeRiscV64/Mmu.c                | 493 ++++++++++++++++++
 UefiCpuPkg/CpuDxeRiscV64/Mmu.h                |  33 ++
 UefiCpuPkg/CpuDxeRiscV64/MmuCore.S            |  29 ++
 UefiCpuPkg/UefiCpuPkg.dec                     |   8 +
 16 files changed, 676 insertions(+), 32 deletions(-)
 create mode 100644 UefiCpuPkg/CpuDxeRiscV64/Mmu.c
 create mode 100644 UefiCpuPkg/CpuDxeRiscV64/Mmu.h
 create mode 100644 UefiCpuPkg/CpuDxeRiscV64/MmuCore.S

-- 
2.25.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/7] MdePkg/BaseLib: RISC-V: Support getting satp register value
  2023-03-06 17:33 [PATCH 0/7] RISC-V: Add MMU support Tuan Phan
@ 2023-03-06 17:33 ` Tuan Phan
  2023-03-06 17:33 ` [PATCH 2/7] MdePkg/Register: RISC-V: Add satp mode bits shift definition Tuan Phan
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Tuan Phan @ 2023-03-06 17:33 UTC (permalink / raw)
  To: devel
  Cc: michael.d.kinney, gaoliming, zhiguang.liu, sunilvl, git,
	andrei.warkentin, Tuan Phan

Add an API to retrieve satp register value.

Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
 MdePkg/Include/Library/BaseLib.h          | 5 +++++
 MdePkg/Library/BaseLib/RiscV64/RiscVMmu.S | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 8f2df76c29a3..5d7067ee854e 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -181,6 +181,11 @@ RiscVSetSupervisorAddressTranslationRegister (
   IN UINT64
   );
 
+UINT64
+RiscVGetSupervisorAddressTranslationRegister (
+  VOID
+  );
+
 UINT64
 RiscVReadTimer (
   VOID
diff --git a/MdePkg/Library/BaseLib/RiscV64/RiscVMmu.S b/MdePkg/Library/BaseLib/RiscV64/RiscVMmu.S
index ac8f92f38aed..c9cf60c1664b 100644
--- a/MdePkg/Library/BaseLib/RiscV64/RiscVMmu.S
+++ b/MdePkg/Library/BaseLib/RiscV64/RiscVMmu.S
@@ -21,3 +21,11 @@
 ASM_FUNC (RiscVSetSupervisorAddressTranslationRegister)
     csrw  CSR_SATP, a0
     ret
+
+//
+// Get the value of Supervisor Address Translation and
+// Protection Register.
+//
+ASM_FUNC (RiscVGetSupervisorAddressTranslationRegister)
+    csrr  a0, CSR_SATP
+    ret
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/7] MdePkg/Register: RISC-V: Add satp mode bits shift definition
  2023-03-06 17:33 [PATCH 0/7] RISC-V: Add MMU support Tuan Phan
  2023-03-06 17:33 ` [PATCH 1/7] MdePkg/BaseLib: RISC-V: Support getting satp register value Tuan Phan
@ 2023-03-06 17:33 ` Tuan Phan
  2023-03-06 17:33 ` [PATCH 3/7] UefiCpuPkg: RISC-V: Support MMU with SV39/48/57 mode Tuan Phan
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Tuan Phan @ 2023-03-06 17:33 UTC (permalink / raw)
  To: devel
  Cc: michael.d.kinney, gaoliming, zhiguang.liu, sunilvl, git,
	andrei.warkentin, Tuan Phan

The satp mode bits shift is used cross modules. It should be defined
in one place.

Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
 MdePkg/Include/Register/RiscV64/RiscVEncoding.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/MdePkg/Include/Register/RiscV64/RiscVEncoding.h b/MdePkg/Include/Register/RiscV64/RiscVEncoding.h
index 5c2989b797bf..c60972892825 100644
--- a/MdePkg/Include/Register/RiscV64/RiscVEncoding.h
+++ b/MdePkg/Include/Register/RiscV64/RiscVEncoding.h
@@ -58,9 +58,10 @@
 #define PRV_S  1UL
 #define PRV_M  3UL
 
-#define SATP64_MODE  0xF000000000000000ULL
-#define SATP64_ASID  0x0FFFF00000000000ULL
-#define SATP64_PPN   0x00000FFFFFFFFFFFULL
+#define SATP64_MODE       0xF000000000000000ULL
+#define SATP64_MODE_SHIFT 60
+#define SATP64_ASID       0x0FFFF00000000000ULL
+#define SATP64_PPN        0x00000FFFFFFFFFFFULL
 
 #define SATP_MODE_OFF   0UL
 #define SATP_MODE_SV32  1UL
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/7] UefiCpuPkg: RISC-V: Support MMU with SV39/48/57 mode
  2023-03-06 17:33 [PATCH 0/7] RISC-V: Add MMU support Tuan Phan
  2023-03-06 17:33 ` [PATCH 1/7] MdePkg/BaseLib: RISC-V: Support getting satp register value Tuan Phan
  2023-03-06 17:33 ` [PATCH 2/7] MdePkg/Register: RISC-V: Add satp mode bits shift definition Tuan Phan
@ 2023-03-06 17:33 ` Tuan Phan
  2023-03-06 17:33 ` [PATCH 4/7] OvmfPkg/RiscVVirt: VirtNorFlashPlatformLib: Fix wrong flash size Tuan Phan
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Tuan Phan @ 2023-03-06 17:33 UTC (permalink / raw)
  To: devel
  Cc: michael.d.kinney, gaoliming, zhiguang.liu, sunilvl, git,
	andrei.warkentin, Tuan Phan

During CpuDxe initialization, MMU will be setup based on the value
get from the PCD satp mode. Default is bare mode.

Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
 UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c          |  10 +-
 UefiCpuPkg/CpuDxeRiscV64/CpuDxe.h          |   1 +
 UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf |   5 +
 UefiCpuPkg/CpuDxeRiscV64/Mmu.c             | 493 +++++++++++++++++++++
 UefiCpuPkg/CpuDxeRiscV64/Mmu.h             |  33 ++
 UefiCpuPkg/CpuDxeRiscV64/MmuCore.S         |  29 ++
 UefiCpuPkg/UefiCpuPkg.dec                  |   8 +
 7 files changed, 577 insertions(+), 2 deletions(-)
 create mode 100644 UefiCpuPkg/CpuDxeRiscV64/Mmu.c
 create mode 100644 UefiCpuPkg/CpuDxeRiscV64/Mmu.h
 create mode 100644 UefiCpuPkg/CpuDxeRiscV64/MmuCore.S

diff --git a/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c b/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c
index 7551e0653603..144e4b49ea5a 100644
--- a/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c
@@ -9,6 +9,7 @@
 **/
 
 #include "CpuDxe.h"
+#include "Mmu.h"
 
 //
 // Global Variables
@@ -296,8 +297,7 @@ CpuSetMemoryAttributes (
   IN UINT64                 Attributes
   )
 {
-  DEBUG ((DEBUG_INFO, "%a: Set memory attributes not supported yet\n", __FUNCTION__));
-  return EFI_SUCCESS;
+  return RiscVSetMemoryAttributes (BaseAddress, Length, Attributes);
 }
 
 /**
@@ -340,6 +340,12 @@ InitializeCpu (
   //
   DisableInterrupts ();
 
+  //
+  // Enable MMU
+  //
+  Status = RiscVConfigureMmu (PcdGet64 (PcdCpuRiscVSatpMode));
+  ASSERT_EFI_ERROR (Status);
+
   //
   // Install Boot protocol
   //
diff --git a/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.h b/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.h
index 49f4e119665a..2f2f970a7887 100644
--- a/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.h
+++ b/UefiCpuPkg/CpuDxeRiscV64/CpuDxe.h
@@ -20,6 +20,7 @@
 #include <Library/DebugLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiDriverEntryPoint.h>
+#include <Register/RiscV64/RiscVEncoding.h>
 
 /**
   Flush CPU data cache. If the instruction cache is fully coherent
diff --git a/UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf b/UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf
index e8fa25446aef..6c2d65be789d 100644
--- a/UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf
+++ b/UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf
@@ -37,10 +37,14 @@
   TimerLib
   PeCoffGetEntryPointLib
   RiscVSbiLib
+  CacheMaintenanceLib
 
 [Sources]
   CpuDxe.c
   CpuDxe.h
+  Mmu.c
+  Mmu.h
+  MmuCore.S
 
 [Protocols]
   gEfiCpuArchProtocolGuid                       ## PRODUCES
@@ -60,6 +64,7 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList              ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize                    ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency             ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVSatpMode                         ## CONSUMES
 
 [Depex]
   TRUE
diff --git a/UefiCpuPkg/CpuDxeRiscV64/Mmu.c b/UefiCpuPkg/CpuDxeRiscV64/Mmu.c
new file mode 100644
index 000000000000..bec78b0ea514
--- /dev/null
+++ b/UefiCpuPkg/CpuDxeRiscV64/Mmu.c
@@ -0,0 +1,493 @@
+/** @file
+*  MMU implementation for RISC-V
+*
+*  Copyright (c) 2011-2020, ARM Limited. All rights reserved.
+*  Copyright (c) 2016, Linaro Limited. All rights reserved.
+*  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+*  Copyright (c) 2023, Ventana Micro Systems Inc. All Rights Reserved.<BR>
+*
+*  SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include <PiDxe.h>
+#include <Uefi.h>
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/CacheMaintenanceLib.h>
+#include <Library/DebugLib.h>
+#include <Library/DxeServicesTableLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/PcdLib.h>
+#include <Register/RiscV64/RiscVEncoding.h>
+#include "Mmu.h"
+
+#define RISCV_PG_V                      BIT0
+#define RISCV_PG_R                      BIT1
+#define RISCV_PG_W                      BIT2
+#define RISCV_PG_X                      BIT3
+#define RISCV_PG_G                      BIT5
+#define RISCV_PG_A                      BIT6
+#define RISCV_PG_D                      BIT7
+#define PTE_ATTRIBUTES_MASK             0xE
+
+#define PTE_PPN_MASK                    0x3FFFFFFFFFFC00ULL
+#define PTE_PPN_SHIFT                   10
+#define RISCV_MMU_PAGE_SHIFT            12
+
+STATIC UINTN                            mMaxRootTableLevel;
+STATIC UINTN                            mBitPerLevel;
+STATIC UINTN                            mTableEntryCount;
+
+STATIC
+BOOLEAN
+RiscVMmuEnabled (VOID)
+{
+  return ((RiscVGetSupervisorAddressTranslationRegister () &
+              SATP64_MODE) != (SATP_MODE_OFF << SATP64_MODE_SHIFT));
+}
+
+STATIC
+UINTN
+RiscVGetRootTranslateTable (VOID)
+{
+  return (RiscVGetSupervisorAddressTranslationRegister () & SATP64_PPN) <<
+            RISCV_MMU_PAGE_SHIFT;
+}
+
+STATIC
+BOOLEAN
+IsValidPte (
+  IN  UINTN  Entry
+  )
+{
+  if (!(Entry & RISCV_PG_V) ||
+      (((Entry & (RISCV_PG_R | RISCV_PG_W)) == RISCV_PG_W))) {
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
+STATIC
+UINTN
+SetValidPte (
+  IN  UINTN  Entry
+  )
+{
+  /* Set Valid and Global mapping bits */
+  return Entry | RISCV_PG_G | RISCV_PG_V;
+}
+
+STATIC
+BOOLEAN
+IsBlockEntry (
+  IN  UINTN  Entry
+  )
+{
+  return IsValidPte (Entry) &&
+          (Entry & (RISCV_PG_X | RISCV_PG_R));
+}
+
+STATIC
+BOOLEAN
+IsTableEntry (
+  IN  UINTN  Entry
+  )
+{
+  return IsValidPte (Entry) &&
+          !IsBlockEntry (Entry);
+}
+
+STATIC
+UINTN
+SetTableEntry (
+  IN  UINTN  Entry
+  )
+{
+  Entry = SetValidPte (Entry);
+  Entry &= ~(RISCV_PG_X | RISCV_PG_W | RISCV_PG_R);
+
+  return Entry;
+}
+
+STATIC
+VOID
+ReplaceTableEntry (
+  IN  UINTN    *Entry,
+  IN  UINTN    Value,
+  IN  UINTN    RegionStart,
+  IN  BOOLEAN  IsLiveBlockMapping
+  )
+{
+  *Entry = Value;
+
+  if (IsLiveBlockMapping && RiscVMmuEnabled ()) {
+    RiscVLocalTlbFlush (RegionStart);
+  }
+}
+
+STATIC
+UINTN
+GetPpnfromPte (UINTN Entry, UINTN Level)
+{
+  return ((Entry & PTE_PPN_MASK) >> PTE_PPN_SHIFT);
+}
+
+STATIC
+UINTN
+SetPpnToPte (UINTN Entry, UINTN Address, UINTN Level)
+{
+  UINTN  Ppn;
+
+  Ppn = ((Address >> RISCV_MMU_PAGE_SHIFT) << PTE_PPN_SHIFT);
+  ASSERT (~(Ppn & ~PTE_PPN_MASK));
+  Entry &= ~PTE_PPN_MASK;
+  return Entry | Ppn;
+}
+
+STATIC
+VOID
+FreePageTablesRecursive (
+  IN  UINTN   *TranslationTable,
+  IN  UINTN   Level
+  )
+{
+  UINTN  Index;
+
+  if (Level < mMaxRootTableLevel - 1) {
+    for (Index = 0; Index < mTableEntryCount; Index++) {
+      if (IsTableEntry (TranslationTable[Index])) {
+        FreePageTablesRecursive (
+          (UINTN *)(GetPpnfromPte ((TranslationTable[Index]), Level) <<
+                      RISCV_MMU_PAGE_SHIFT),
+          Level + 1
+          );
+      }
+    }
+  }
+
+  FreePages (TranslationTable, 1);
+}
+
+STATIC
+EFI_STATUS
+UpdateRegionMappingRecursive (
+  IN  UINTN    RegionStart,
+  IN  UINTN    RegionEnd,
+  IN  UINTN    AttributeSetMask,
+  IN  UINTN    AttributeClearMask,
+  IN  UINTN    *PageTable,
+  IN  UINTN    Level,
+  IN  BOOLEAN  TableIsLive
+  )
+{
+  EFI_STATUS  Status;
+  UINTN       BlockShift;
+  UINTN       BlockMask;
+  UINTN       BlockEnd;
+  UINTN       *Entry;
+  UINTN       EntryValue;
+  UINTN       *TranslationTable;
+  BOOLEAN     NextTableIsLive;
+
+  ASSERT (Level < mMaxRootTableLevel);
+  ASSERT (((RegionStart | RegionEnd) & EFI_PAGE_MASK) == 0);
+
+  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
+    ));
+
+  for ( ; RegionStart < RegionEnd; RegionStart = BlockEnd) {
+    BlockEnd = MIN (RegionEnd, (RegionStart | BlockMask) + 1);
+    Entry    = &PageTable[(RegionStart >> BlockShift) & (mTableEntryCount - 1)];
+
+    //
+    // If RegionStart or BlockEnd is not aligned to the block size at this
+    // level, we will have to create a table mapping in order to map less
+    // than a block, and recurse to create the block or page entries at
+    // the next level. No block mappings are allowed at all at level 0,
+    // so in that case, we have to recurse unconditionally.
+    //
+    if (Level == 0 ||
+        (((RegionStart | BlockEnd) & BlockMask) != 0) || IsTableEntry (*Entry))
+    {
+      ASSERT (Level < mMaxRootTableLevel - 1);
+      if (!IsTableEntry (*Entry)) {
+        //
+        // No table entry exists yet, so we need to allocate a page table
+        // for the next level.
+        //
+        TranslationTable = AllocatePages (1);
+        if (TranslationTable == NULL) {
+          return EFI_OUT_OF_RESOURCES;
+        }
+        ZeroMem (TranslationTable, EFI_PAGE_SIZE);
+
+        if (IsBlockEntry (*Entry)) {
+          //
+          // We are splitting an existing block entry, so we have to populate
+          // the new table with the attributes of the block entry it replaces.
+          //
+          Status = UpdateRegionMappingRecursive (
+                     RegionStart & ~BlockMask,
+                     (RegionStart | BlockMask) + 1,
+                     *Entry & PTE_ATTRIBUTES_MASK,
+                     PTE_ATTRIBUTES_MASK,
+                     TranslationTable,
+                     Level + 1,
+                     FALSE
+                     );
+          if (EFI_ERROR (Status)) {
+            //
+            // The range we passed to UpdateRegionMappingRecursive () is block
+            // aligned, so it is guaranteed that no further pages were allocated
+            // by it, and so we only have to free the page we allocated here.
+            //
+            FreePages (TranslationTable, 1);
+            return Status;
+          }
+        }
+        NextTableIsLive = FALSE;
+      } else {
+        TranslationTable = (UINTN *)(GetPpnfromPte (*Entry, Level) << RISCV_MMU_PAGE_SHIFT);
+        NextTableIsLive  = TableIsLive;
+      }
+
+      //
+      // Recurse to the next level
+      //
+      Status = UpdateRegionMappingRecursive (
+                 RegionStart,
+                 BlockEnd,
+                 AttributeSetMask,
+                 AttributeClearMask,
+                 TranslationTable,
+                 Level + 1,
+                 NextTableIsLive
+                 );
+      if (EFI_ERROR (Status)) {
+        if (!IsTableEntry (*Entry)) {
+          //
+          // We are creating a new table entry, so on failure, we can free all
+          // allocations we made recursively, given that the whole subhierarchy
+          // has not been wired into the live page tables yet. (This is not
+          // possible for existing table entries, since we cannot revert the
+          // modifications we made to the subhierarchy it represents.)
+          //
+          FreePageTablesRecursive (TranslationTable, Level + 1);
+        }
+        return Status;
+      }
+
+      if (!IsTableEntry (*Entry)) {
+        EntryValue = SetPpnToPte (0, (UINTN)TranslationTable, Level);
+        EntryValue = SetTableEntry (EntryValue);
+        ReplaceTableEntry (
+          Entry,
+          EntryValue,
+          RegionStart,
+          TableIsLive
+          );
+      }
+    } else {
+      EntryValue = (*Entry & ~AttributeClearMask) | AttributeSetMask;
+      //
+      // We don't have page fault exception handler when a virtual page is accessed and
+      // the A bit is clear, or is written and the D bit is clear.
+      // So just set A for read and D for write permission.
+      //
+      if (AttributeSetMask & RISCV_PG_R) {
+        EntryValue |= RISCV_PG_A;
+      }
+      if (AttributeSetMask & RISCV_PG_W) {
+        EntryValue |= RISCV_PG_D;
+      }
+      EntryValue = SetPpnToPte (EntryValue, RegionStart, Level);
+      EntryValue = SetValidPte (EntryValue);
+      ReplaceTableEntry (Entry, EntryValue, RegionStart, TableIsLive);
+    }
+  }
+
+  return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+UpdateRegionMapping (
+  IN  UINTN    RegionStart,
+  IN  UINTN    RegionLength,
+  IN  UINTN    AttributeSetMask,
+  IN  UINTN    AttributeClearMask,
+  IN  UINTN    *RootTable,
+  IN  BOOLEAN  TableIsLive
+  )
+{
+  if (((RegionStart | RegionLength) & EFI_PAGE_MASK) != 0) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  return UpdateRegionMappingRecursive (
+           RegionStart,
+           RegionStart + RegionLength,
+           AttributeSetMask,
+           AttributeClearMask,
+           RootTable,
+           0,
+           TableIsLive
+           );
+}
+
+STATIC
+UINTN
+GcdAttributeToPageAttribute (
+  IN UINTN  GcdAttributes
+  )
+{
+  UINTN RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X;
+
+  // Determine protection attributes
+  if (GcdAttributes & EFI_MEMORY_RO) {
+    RiscVAttributes &= ~(RISCV_PG_W);
+  }
+
+  // Process eXecute Never attribute
+  if (GcdAttributes & EFI_MEMORY_XP) {
+    RiscVAttributes &= ~RISCV_PG_X;
+  }
+
+  return RiscVAttributes;
+}
+
+EFI_STATUS
+EFIAPI
+RiscVSetMemoryAttributes (
+  IN EFI_PHYSICAL_ADDRESS  BaseAddress,
+  IN UINTN                 Length,
+  IN UINTN                 Attributes
+  )
+{
+  UINTN  PageAttributesSet = GcdAttributeToPageAttribute (Attributes);
+
+  if (!RiscVMmuEnabled ()) {
+    return EFI_SUCCESS;
+  }
+
+  DEBUG ((
+    DEBUG_VERBOSE,
+    "%a: Set %llX page attribute 0x%X\n",
+    __func__,
+    BaseAddress,
+    PageAttributesSet
+    ));
+
+  return UpdateRegionMapping (
+           BaseAddress,
+           Length,
+           PageAttributesSet,
+           PTE_ATTRIBUTES_MASK,
+           (UINTN *)RiscVGetRootTranslateTable (),
+           TRUE
+           );
+}
+
+EFI_STATUS
+EFIAPI
+RiscVConfigureMmu (UINTN SatpMode)
+{
+  VOID                             *TranslationTable;
+  UINTN                            SatpReg;
+  UINTN                            Ppn;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *MemoryMap;
+  UINTN                            NumberOfDescriptors;
+  UINTN                            Index;
+  EFI_STATUS                       Status;
+
+  switch (SatpMode) {
+  case SATP_MODE_OFF:
+    return EFI_SUCCESS;
+  case SATP_MODE_SV39:
+    mMaxRootTableLevel = 3;
+    mBitPerLevel = 9;
+    mTableEntryCount = 512;
+    break;
+  case SATP_MODE_SV48:
+    mMaxRootTableLevel = 4;
+    mBitPerLevel = 9;
+    mTableEntryCount = 512;
+    break;
+  case SATP_MODE_SV57:
+    mMaxRootTableLevel = 5;
+    mBitPerLevel = 9;
+    mTableEntryCount = 512;
+    break;
+  default:
+    return EFI_INVALID_PARAMETER;
+  }
+
+  // Allocate pages for translation table
+  TranslationTable = AllocatePages (1);
+  if (TranslationTable == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+  ZeroMem (TranslationTable, mTableEntryCount * sizeof (UINTN));
+
+  NumberOfDescriptors = 0;
+  MemoryMap           = NULL;
+  Status = gDS->GetMemorySpaceMap (
+                  &NumberOfDescriptors,
+                  &MemoryMap
+                  );
+  ASSERT_EFI_ERROR (Status);
+
+  for (Index = 0; Index < NumberOfDescriptors; Index++) {
+    if (MemoryMap[Index].GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo) {
+      // Default Read/Write attribute for memory mapped IO
+      UpdateRegionMapping (MemoryMap[Index].BaseAddress,
+        MemoryMap[Index].Length,
+        RISCV_PG_R | RISCV_PG_W,
+        PTE_ATTRIBUTES_MASK,
+        TranslationTable,
+        FALSE);
+    } else if (MemoryMap[Index].GcdMemoryType == EfiGcdMemoryTypeSystemMemory) {
+      // Default Read/Write/Execute attribute for system memory
+      UpdateRegionMapping (MemoryMap[Index].BaseAddress,
+        MemoryMap[Index].Length,
+        RISCV_PG_R | RISCV_PG_W | RISCV_PG_X,
+        PTE_ATTRIBUTES_MASK,
+        TranslationTable,
+        FALSE);
+    }
+  }
+  FreePool ((VOID *)MemoryMap);
+
+  if (GetInterruptState ()) {
+    DisableInterrupts ();
+  }
+
+  Ppn = (UINTN)TranslationTable >> RISCV_MMU_PAGE_SHIFT;
+  ASSERT (!(Ppn & ~(SATP64_PPN)));
+
+  SatpReg = Ppn;
+  SatpReg |= (SatpMode <<
+              SATP64_MODE_SHIFT) & SATP64_MODE;
+  RiscVSetSupervisorAddressTranslationRegister (SatpReg);
+  RiscVLocalTlbFlushAll ();
+
+  if (GetInterruptState ()) {
+    EnableInterrupts ();
+  }
+
+  return Status;
+}
diff --git a/UefiCpuPkg/CpuDxeRiscV64/Mmu.h b/UefiCpuPkg/CpuDxeRiscV64/Mmu.h
new file mode 100644
index 000000000000..9ee0a4bfab61
--- /dev/null
+++ b/UefiCpuPkg/CpuDxeRiscV64/Mmu.h
@@ -0,0 +1,33 @@
+/** @file
+
+  Copyright (c) 2015 - 2016, Linaro Ltd. All rights reserved.<BR>
+  Copyright (c) 2023, Ventana Micro Systems Inc. All Rights Reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef MMU_H_
+#define MMU_H_
+
+VOID
+EFIAPI
+RiscVLocalTlbFlushAll (VOID);
+
+VOID
+EFIAPI
+RiscVLocalTlbFlush (UINTN VirtAddr);
+
+EFI_STATUS
+EFIAPI
+RiscVSetMemoryAttributes (
+  IN EFI_PHYSICAL_ADDRESS  BaseAddress,
+  IN UINT64                Length,
+  IN UINT64                Attributes
+  );
+
+EFI_STATUS
+EFIAPI
+RiscVConfigureMmu (UINTN SatpMode);
+
+#endif /* MMU_H_ */
diff --git a/UefiCpuPkg/CpuDxeRiscV64/MmuCore.S b/UefiCpuPkg/CpuDxeRiscV64/MmuCore.S
new file mode 100644
index 000000000000..d0cbd91ffcc0
--- /dev/null
+++ b/UefiCpuPkg/CpuDxeRiscV64/MmuCore.S
@@ -0,0 +1,29 @@
+/** @file
+*
+*  Copyright (c) 2023, Ventana Micro Systems Inc. All Rights Reserved.<BR>
+*
+*  SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include <Base.h>
+#include <Register/RiscV64/RiscVImpl.h>
+
+.text
+.align 3
+
+//
+// Local tlb flush all.
+//
+//
+ASM_FUNC (RiscVLocalTlbFlushAll)
+    sfence.vma
+    ret
+
+//
+// Local tlb flush at a virtual address
+// @retval a0 : virtual address.
+//
+ASM_FUNC (RiscVLocalTlbFlush)
+    sfence.vma a0
+    ret
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 2115aa4387a2..b4e2be2e3880 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -360,6 +360,14 @@
   # @Prompt Access to non-SMRAM memory is restricted to reserved, runtime and ACPI NVS type after SmmReadyToLock.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmRestrictedMemoryAccess|TRUE|BOOLEAN|0x3213210F
 
+[PcdsFixedAtBuild.RISCV64]
+  ## Configure SATP mode for RISCV
+  #  0 - Bare mode
+  #  8 - SV39 mode
+  #  9 - SV48 mode
+  # 10 - SV57 mode
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVSatpMode|0x0|UINT64|0x70000001
+
 [PcdsDynamic, PcdsDynamicEx]
   ## Contains the pointer to a CPU S3 data buffer of structure ACPI_CPU_DATA.
   # @Prompt The pointer to a CPU S3 data buffer.
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/7] OvmfPkg/RiscVVirt: VirtNorFlashPlatformLib: Fix wrong flash size
  2023-03-06 17:33 [PATCH 0/7] RISC-V: Add MMU support Tuan Phan
                   ` (2 preceding siblings ...)
  2023-03-06 17:33 ` [PATCH 3/7] UefiCpuPkg: RISC-V: Support MMU with SV39/48/57 mode Tuan Phan
@ 2023-03-06 17:33 ` Tuan Phan
  2023-03-06 17:33 ` [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists Tuan Phan
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Tuan Phan @ 2023-03-06 17:33 UTC (permalink / raw)
  To: devel
  Cc: michael.d.kinney, gaoliming, zhiguang.liu, sunilvl, git,
	andrei.warkentin, Tuan Phan

The size should be for single region, not the whole firmware FD.

Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
 .../Library/VirtNorFlashPlatformLib/VirtNorFlashStaticLib.c    | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/RiscVVirt/Library/VirtNorFlashPlatformLib/VirtNorFlashStaticLib.c b/OvmfPkg/RiscVVirt/Library/VirtNorFlashPlatformLib/VirtNorFlashStaticLib.c
index fdc2ccb6294e..067065bd6ef9 100644
--- a/OvmfPkg/RiscVVirt/Library/VirtNorFlashPlatformLib/VirtNorFlashStaticLib.c
+++ b/OvmfPkg/RiscVVirt/Library/VirtNorFlashPlatformLib/VirtNorFlashStaticLib.c
@@ -24,7 +24,8 @@ VIRT_NOR_FLASH_DESCRIPTION  mNorFlashDevice =
 {
   FixedPcdGet32 (PcdOvmfFdBaseAddress),
   FixedPcdGet64 (PcdFlashNvStorageVariableBase),
-  FixedPcdGet32 (PcdOvmfFirmwareFdSize),
+  FixedPcdGet32 (PcdOvmfFirmwareFdSize) - 
+          (FixedPcdGet64 (PcdFlashNvStorageVariableBase) - FixedPcdGet32 (PcdOvmfFdBaseAddress)),
   QEMU_NOR_BLOCK_SIZE
 };
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists
  2023-03-06 17:33 [PATCH 0/7] RISC-V: Add MMU support Tuan Phan
                   ` (3 preceding siblings ...)
  2023-03-06 17:33 ` [PATCH 4/7] OvmfPkg/RiscVVirt: VirtNorFlashPlatformLib: Fix wrong flash size Tuan Phan
@ 2023-03-06 17:33 ` Tuan Phan
  2023-03-06 17:53   ` [edk2-devel] " Ard Biesheuvel
  2023-03-06 17:33 ` [PATCH 6/7] OvmfPkg/RiscVVirt: SEC: Add IO memory resource hob for platform devices Tuan Phan
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Tuan Phan @ 2023-03-06 17:33 UTC (permalink / raw)
  To: devel
  Cc: michael.d.kinney, gaoliming, zhiguang.liu, sunilvl, git,
	andrei.warkentin, Tuan Phan

The flash base address can be added to GCD before this driver run.
So only add it if it has not been done.

Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c | 25 +++++++++++++++--------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
index f9a41f6aab0f..8875824f3333 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
@@ -372,10 +372,11 @@ NorFlashFvbInitialize (
   IN NOR_FLASH_INSTANCE  *Instance
   )
 {
-  EFI_STATUS     Status;
-  UINT32         FvbNumLba;
-  EFI_BOOT_MODE  BootMode;
-  UINTN          RuntimeMmioRegionSize;
+  EFI_STATUS                      Status;
+  UINT32                          FvbNumLba;
+  EFI_BOOT_MODE                   BootMode;
+  UINTN                           RuntimeMmioRegionSize;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
 
   DEBUG ((DEBUG_BLKIO, "NorFlashFvbInitialize\n"));
   ASSERT ((Instance != NULL));
@@ -390,13 +391,19 @@ NorFlashFvbInitialize (
   //       is written as the base of the flash region (ie: Instance->DeviceBaseAddress)
   RuntimeMmioRegionSize = (Instance->RegionBaseAddress - Instance->DeviceBaseAddress) + Instance->Size;
 
-  Status = gDS->AddMemorySpace (
-                  EfiGcdMemoryTypeMemoryMappedIo,
+  Status = gDS->GetMemorySpaceDescriptor (
                   Instance->DeviceBaseAddress,
-                  RuntimeMmioRegionSize,
-                  EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
+                  &Desc
                   );
-  ASSERT_EFI_ERROR (Status);
+  if (Status == EFI_NOT_FOUND) {
+    Status = gDS->AddMemorySpace (
+                    EfiGcdMemoryTypeMemoryMappedIo,
+                    Instance->DeviceBaseAddress,
+                    RuntimeMmioRegionSize,
+                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
+                    );
+    ASSERT_EFI_ERROR (Status);
+  }
 
   Status = gDS->SetMemorySpaceAttributes (
                   Instance->DeviceBaseAddress,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 6/7] OvmfPkg/RiscVVirt: SEC: Add IO memory resource hob for platform devices
  2023-03-06 17:33 [PATCH 0/7] RISC-V: Add MMU support Tuan Phan
                   ` (4 preceding siblings ...)
  2023-03-06 17:33 ` [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists Tuan Phan
@ 2023-03-06 17:33 ` Tuan Phan
  2023-03-06 17:33 ` [PATCH 7/7] OvmfPkg/RiscVVirt: Enable MMU with SV39 mode Tuan Phan
  2023-03-09 19:19 ` [PATCH 0/7] RISC-V: Add MMU support Tuan Phan
  7 siblings, 0 replies; 16+ messages in thread
From: Tuan Phan @ 2023-03-06 17:33 UTC (permalink / raw)
  To: devel
  Cc: michael.d.kinney, gaoliming, zhiguang.liu, sunilvl, git,
	andrei.warkentin, Tuan Phan

Normally, DXE driver would add device resource to GCD before start using.
But some key resources such as uart, flash base address are being accessing
directly in some core modules.

Those resources should be populated to HOB in SEC phase so they are
added to GCD before anyone can access them.

Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
 OvmfPkg/RiscVVirt/Sec/Platform.c  | 62 +++++++++++++++++++++++++++++++
 OvmfPkg/RiscVVirt/Sec/SecMain.inf |  1 +
 2 files changed, 63 insertions(+)

diff --git a/OvmfPkg/RiscVVirt/Sec/Platform.c b/OvmfPkg/RiscVVirt/Sec/Platform.c
index e8fd126cf800..63bc21eb3f60 100644
--- a/OvmfPkg/RiscVVirt/Sec/Platform.c
+++ b/OvmfPkg/RiscVVirt/Sec/Platform.c
@@ -21,6 +21,63 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <libfdt.h>
 #include <Guid/FdtHob.h>
 
+/**
+  Build memory map I/O range resource HOB using the
+  base address and size.
+
+  @param  MemoryBase     Memory map I/O base.
+  @param  MemorySize     Memory map I/O size.
+
+**/
+STATIC
+VOID
+AddIoMemoryBaseSizeHob (
+  EFI_PHYSICAL_ADDRESS  MemoryBase,
+  UINT64                MemorySize
+  )
+{
+  /* Align to EFI_PAGE_SIZE */
+  MemorySize = ALIGN_VALUE (MemorySize, EFI_PAGE_SIZE);
+  BuildResourceDescriptorHob (
+    EFI_RESOURCE_MEMORY_MAPPED_IO,
+    EFI_RESOURCE_ATTRIBUTE_PRESENT     |
+    EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
+    EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
+    EFI_RESOURCE_ATTRIBUTE_TESTED,
+    MemoryBase,
+    MemorySize
+    );
+}
+
+/**
+  Populate IO resources from FDT that not added to GCD by its
+  driver in the DXE phase. 
+
+  @param  FdtBase       Fdt base address
+  @param  Compatible    Compatible string
+
+**/
+STATIC
+VOID
+PopulateIoResources (
+  VOID          *FdtBase,
+  CONST CHAR8*  Compatible
+  )
+{
+  UINT64  *Reg;
+  INT32   Node, LenP;
+
+  Node = fdt_node_offset_by_compatible (FdtBase, -1, Compatible);
+  while (Node != -FDT_ERR_NOTFOUND) {
+    Reg = (UINT64 *)fdt_getprop (FdtBase, Node, "reg", &LenP);
+    if (Reg) {
+      ASSERT (LenP == (2 * sizeof (UINT64)));
+      AddIoMemoryBaseSizeHob (SwapBytes64 (Reg[0]), SwapBytes64 (Reg[1]));
+    }
+    Node = fdt_node_offset_by_compatible (FdtBase, Node, Compatible);
+  }
+}
+
 /**
   @retval EFI_SUCCESS            The address of FDT is passed in HOB.
           EFI_UNSUPPORTED        Can't locate FDT.
@@ -80,5 +137,10 @@ PlatformPeimInitialization (
 
   BuildFvHob (PcdGet32 (PcdOvmfDxeMemFvBase), PcdGet32 (PcdOvmfDxeMemFvSize));
 
+  PopulateIoResources (Base, "ns16550a");
+  PopulateIoResources (Base, "qemu,fw-cfg-mmio");
+  PopulateIoResources (Base, "virtio,mmio");
+  AddIoMemoryBaseSizeHob (PcdGet32 (PcdOvmfFdBaseAddress), PcdGet32 (PcdOvmfFirmwareFdSize));
+
   return EFI_SUCCESS;
 }
diff --git a/OvmfPkg/RiscVVirt/Sec/SecMain.inf b/OvmfPkg/RiscVVirt/Sec/SecMain.inf
index aed35d3af596..e1f562264eea 100644
--- a/OvmfPkg/RiscVVirt/Sec/SecMain.inf
+++ b/OvmfPkg/RiscVVirt/Sec/SecMain.inf
@@ -61,6 +61,7 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize
 
 [Guids]
   gFdtHobGuid
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 7/7] OvmfPkg/RiscVVirt: Enable MMU with SV39 mode
  2023-03-06 17:33 [PATCH 0/7] RISC-V: Add MMU support Tuan Phan
                   ` (5 preceding siblings ...)
  2023-03-06 17:33 ` [PATCH 6/7] OvmfPkg/RiscVVirt: SEC: Add IO memory resource hob for platform devices Tuan Phan
@ 2023-03-06 17:33 ` Tuan Phan
  2023-03-09  0:46   ` Andrei Warkentin
  2023-03-09 19:19 ` [PATCH 0/7] RISC-V: Add MMU support Tuan Phan
  7 siblings, 1 reply; 16+ messages in thread
From: Tuan Phan @ 2023-03-06 17:33 UTC (permalink / raw)
  To: devel
  Cc: michael.d.kinney, gaoliming, zhiguang.liu, sunilvl, git,
	andrei.warkentin, Tuan Phan

As MMU will be enabled in CpuDxe, remove the code that set up satp
mode in SEC phase.

Enable SV39 as default mode.

Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
 OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc |  1 +
 OvmfPkg/RiscVVirt/Sec/Memory.c      | 17 -----------------
 2 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
index 731f54f73f81..ef268481ca07 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
+++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
@@ -207,6 +207,7 @@
   gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0
   gEfiMdePkgTokenSpaceGuid.PcdSpinLockTimeout|10000000
   gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|320
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVSatpMode|8
 
   # DEBUG_ASSERT_ENABLED       0x01
   # DEBUG_PRINT_ENABLED        0x02
diff --git a/OvmfPkg/RiscVVirt/Sec/Memory.c b/OvmfPkg/RiscVVirt/Sec/Memory.c
index 70935b07b56b..0b589cd1d071 100644
--- a/OvmfPkg/RiscVVirt/Sec/Memory.c
+++ b/OvmfPkg/RiscVVirt/Sec/Memory.c
@@ -110,21 +110,6 @@ AddMemoryRangeHob (
   AddMemoryBaseSizeHob (MemoryBase, (UINT64)(MemoryLimit - MemoryBase));
 }
 
-/**
-  Configure MMU
-**/
-STATIC
-VOID
-InitMmu (
-  )
-{
-  //
-  // Set supervisor translation mode to Bare mode
-  //
-  RiscVSetSupervisorAddressTranslationRegister ((UINT64)SATP_MODE_OFF << 60);
-  DEBUG ((DEBUG_INFO, "%a: Set Supervisor address mode to bare-metal mode.\n", __FUNCTION__));
-}
-
 /**
   Publish system RAM and reserve memory regions.
 
@@ -255,8 +240,6 @@ MemoryPeimInitialization (
     }
   }
 
-  InitMmu ();
-
   BuildMemoryTypeInformationHob ();
 
   return EFI_SUCCESS;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [edk2-devel] [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists
  2023-03-06 17:33 ` [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists Tuan Phan
@ 2023-03-06 17:53   ` Ard Biesheuvel
  2023-05-24 18:13     ` Tuan Phan
  0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2023-03-06 17:53 UTC (permalink / raw)
  To: devel, tphan
  Cc: michael.d.kinney, gaoliming, zhiguang.liu, sunilvl, git,
	andrei.warkentin

On Mon, 6 Mar 2023 at 18:33, Tuan Phan <tphan@ventanamicro.com> wrote:
>
> The flash base address can be added to GCD before this driver run.
> So only add it if it has not been done.
>

How do you end up in this situation?

You cannot skip this registration, as it is required to get the region
marked as EFI_MEMORY_RUNTIME, and without that, EFI variables will be
broken when running under the OS.

> Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> ---
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c | 25 +++++++++++++++--------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> index f9a41f6aab0f..8875824f3333 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> @@ -372,10 +372,11 @@ NorFlashFvbInitialize (
>    IN NOR_FLASH_INSTANCE  *Instance
>    )
>  {
> -  EFI_STATUS     Status;
> -  UINT32         FvbNumLba;
> -  EFI_BOOT_MODE  BootMode;
> -  UINTN          RuntimeMmioRegionSize;
> +  EFI_STATUS                      Status;
> +  UINT32                          FvbNumLba;
> +  EFI_BOOT_MODE                   BootMode;
> +  UINTN                           RuntimeMmioRegionSize;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
>
>    DEBUG ((DEBUG_BLKIO, "NorFlashFvbInitialize\n"));
>    ASSERT ((Instance != NULL));
> @@ -390,13 +391,19 @@ NorFlashFvbInitialize (
>    //       is written as the base of the flash region (ie: Instance->DeviceBaseAddress)
>    RuntimeMmioRegionSize = (Instance->RegionBaseAddress - Instance->DeviceBaseAddress) + Instance->Size;
>
> -  Status = gDS->AddMemorySpace (
> -                  EfiGcdMemoryTypeMemoryMappedIo,
> +  Status = gDS->GetMemorySpaceDescriptor (
>                    Instance->DeviceBaseAddress,
> -                  RuntimeMmioRegionSize,
> -                  EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> +                  &Desc
>                    );
> -  ASSERT_EFI_ERROR (Status);
> +  if (Status == EFI_NOT_FOUND) {
> +    Status = gDS->AddMemorySpace (
> +                    EfiGcdMemoryTypeMemoryMappedIo,
> +                    Instance->DeviceBaseAddress,
> +                    RuntimeMmioRegionSize,
> +                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> +                    );
> +    ASSERT_EFI_ERROR (Status);
> +  }
>
>    Status = gDS->SetMemorySpaceAttributes (
>                    Instance->DeviceBaseAddress,
> --
> 2.25.1
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#100754): https://edk2.groups.io/g/devel/message/100754
> Mute This Topic: https://groups.io/mt/97430554/1131722
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb@kernel.org]
> ------------
>
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 7/7] OvmfPkg/RiscVVirt: Enable MMU with SV39 mode
  2023-03-06 17:33 ` [PATCH 7/7] OvmfPkg/RiscVVirt: Enable MMU with SV39 mode Tuan Phan
@ 2023-03-09  0:46   ` Andrei Warkentin
  0 siblings, 0 replies; 16+ messages in thread
From: Andrei Warkentin @ 2023-03-09  0:46 UTC (permalink / raw)
  To: Tuan Phan, devel@edk2.groups.io
  Cc: Kinney, Michael D, Gao, Liming, Liu, Zhiguang,
	sunilvl@ventanamicro.com, git@danielschaefer.me

First of all, it's great to see MMU support. I look forward to testing this patch set and reporting back as it ties in well with the OpRom emulation work we've been doing.

Why default to Sv39? Why not probe and set up the max mode allowable by the hardware? The overhead for doing so is minimal, just two more pages (to accommodate Sv49 and Sv57).

>From an OS perspective, it would be highly desirable to have firmware start in an MMU mode that is the maximum possible. For example, the first stage bootloader for ESXi configures the RT address space using SetVirtualAddressSpace, and those addresses would be OS addresses - if the OS runs in a larger address mode than Sv39, then there's a problem here and the first stage BL would have to build its own set of PTs...

A

-----Original Message-----
From: Tuan Phan <tphan@ventanamicro.com> 
Sent: Monday, March 6, 2023 11:33 AM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; sunilvl@ventanamicro.com; git@danielschaefer.me; Warkentin, Andrei <andrei.warkentin@intel.com>; Tuan Phan <tphan@ventanamicro.com>
Subject: [PATCH 7/7] OvmfPkg/RiscVVirt: Enable MMU with SV39 mode

As MMU will be enabled in CpuDxe, remove the code that set up satp mode in SEC phase.

Enable SV39 as default mode.

Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
 OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc |  1 +
 OvmfPkg/RiscVVirt/Sec/Memory.c      | 17 -----------------
 2 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
index 731f54f73f81..ef268481ca07 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
+++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
@@ -207,6 +207,7 @@
   gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0   gEfiMdePkgTokenSpaceGuid.PcdSpinLockTimeout|10000000   gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|320+  gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVSatpMode|8    # DEBUG_ASSERT_ENABLED       0x01   # DEBUG_PRINT_ENABLED        0x02diff --git a/OvmfPkg/RiscVVirt/Sec/Memory.c b/OvmfPkg/RiscVVirt/Sec/Memory.c
index 70935b07b56b..0b589cd1d071 100644
--- a/OvmfPkg/RiscVVirt/Sec/Memory.c
+++ b/OvmfPkg/RiscVVirt/Sec/Memory.c
@@ -110,21 +110,6 @@ AddMemoryRangeHob (
   AddMemoryBaseSizeHob (MemoryBase, (UINT64)(MemoryLimit - MemoryBase)); } -/**-  Configure MMU-**/-STATIC-VOID-InitMmu (-  )-{-  //-  // Set supervisor translation mode to Bare mode-  //-  RiscVSetSupervisorAddressTranslationRegister ((UINT64)SATP_MODE_OFF << 60);-  DEBUG ((DEBUG_INFO, "%a: Set Supervisor address mode to bare-metal mode.\n", __FUNCTION__));-}- /**   Publish system RAM and reserve memory regions. @@ -255,8 +240,6 @@ MemoryPeimInitialization (
     }   } -  InitMmu ();-   BuildMemoryTypeInformationHob ();    return EFI_SUCCESS;-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/7] RISC-V: Add MMU support
  2023-03-06 17:33 [PATCH 0/7] RISC-V: Add MMU support Tuan Phan
                   ` (6 preceding siblings ...)
  2023-03-06 17:33 ` [PATCH 7/7] OvmfPkg/RiscVVirt: Enable MMU with SV39 mode Tuan Phan
@ 2023-03-09 19:19 ` Tuan Phan
  2023-03-09 21:34   ` Andrei Warkentin
  7 siblings, 1 reply; 16+ messages in thread
From: Tuan Phan @ 2023-03-09 19:19 UTC (permalink / raw)
  To: devel@edk2.groups.io
  Cc: michael.d.kinney@intel.com, gaoliming@byosoft.com.cn,
	zhiguang.liu@intel.com, sunilvl@ventanamicro.com,
	git@danielschaefer.me, andrei.warkentin@intel.com

[-- Attachment #1: Type: text/html, Size: 5781 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/7] RISC-V: Add MMU support
  2023-03-09 19:19 ` [PATCH 0/7] RISC-V: Add MMU support Tuan Phan
@ 2023-03-09 21:34   ` Andrei Warkentin
  2023-03-10 22:20     ` Tuan Phan
  0 siblings, 1 reply; 16+ messages in thread
From: Andrei Warkentin @ 2023-03-09 21:34 UTC (permalink / raw)
  To: Tuan Phan, devel@edk2.groups.io
  Cc: Kinney, Michael D, Gao, Liming, Liu, Zhiguang,
	sunilvl@ventanamicro.com, git@danielschaefer.me

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

Hi Tuan,

Could you share a GitHub link to a branch with the patch set? Somehow my email client is mangling one of your patches where it’s all one giant line of code.

A

From: Tuan Phan <tphan@ventanamicro.com>
Sent: Thursday, March 9, 2023 1:20 PM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; sunilvl@ventanamicro.com; git@danielschaefer.me; Warkentin, Andrei <andrei.warkentin@intel.com>
Subject: RE: [PATCH 0/7] RISC-V: Add MMU support

Hi All,
Any updates on this series?

Thanks,

From: Tuan Phan<mailto:tphan@ventanamicro.com>
Sent: Monday, March 6, 2023 9:33 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>; gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>; zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>; sunilvl@ventanamicro.com<mailto:sunilvl@ventanamicro.com>; git@danielschaefer.me<mailto:git@danielschaefer.me>; andrei.warkentin@intel.com<mailto:andrei.warkentin@intel.com>; Tuan Phan<mailto:tphan@ventanamicro.com>
Subject: [PATCH 0/7] RISC-V: Add MMU support

This series adds MMU support for RISC-V. Only SV39/48/57 modes
are supported and tested. The MMU is required to support setting
page attribute which is the first basic step to support security
booting on RISC-V.

There are three parts:
1. Add MMU core to UefiCpuPkg. MMU will be enabled during
CpuDxe initialization.
2. Fix OvmfPkg/VirtNorFlashDxe that failed to add flash base
address to GCD if already done.
3. Enable MMU for RiscVVirt platform and populating its device
resources in SEC phase. All resources should be populated in HOB
or added to GCD by driver before accessing them when MMU enabled.

Tuan Phan (7):
  MdePkg/BaseLib: RISC-V: Support getting satp register value
  MdePkg/Register: RISC-V: Add satp mode bits shift definition
  UefiCpuPkg: RISC-V: Support MMU with SV39/48/57 mode
  OvmfPkg/RiscVVirt: VirtNorFlashPlatformLib: Fix wrong flash size
  OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists
  OvmfPkg/RiscVVirt: SEC: Add IO memory resource hob for platform
    devices
  OvmfPkg/RiscVVirt: Enable MMU with SV39 mode

MdePkg/Include/Library/BaseLib.h              |   5 +
.../Include/Register/RiscV64/RiscVEncoding.h  |   7 +-
MdePkg/Library/BaseLib/RiscV64/RiscVMmu.S     |   8 +
.../VirtNorFlashStaticLib.c                   |   3 +-
OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc           |   1 +
OvmfPkg/RiscVVirt/Sec/Memory.c                |  17 -
OvmfPkg/RiscVVirt/Sec/Platform.c              |  62 +++
OvmfPkg/RiscVVirt/Sec/SecMain.inf             |   1 +
OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c     |  25 +-
UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c             |  10 +-
UefiCpuPkg/CpuDxeRiscV64/CpuDxe.h             |   1 +
UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf    |   5 +
UefiCpuPkg/CpuDxeRiscV64/Mmu.c                | 493 ++++++++++++++++++
UefiCpuPkg/CpuDxeRiscV64/Mmu.h                |  33 ++
UefiCpuPkg/CpuDxeRiscV64/MmuCore.S            |  29 ++
UefiCpuPkg/UefiCpuPkg.dec                     |   8 +
16 files changed, 676 insertions(+), 32 deletions(-)
create mode 100644 UefiCpuPkg/CpuDxeRiscV64/Mmu.c
create mode 100644 UefiCpuPkg/CpuDxeRiscV64/Mmu.h
create mode 100644 UefiCpuPkg/CpuDxeRiscV64/MmuCore.S

--
2.25.1



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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/7] RISC-V: Add MMU support
  2023-03-09 21:34   ` Andrei Warkentin
@ 2023-03-10 22:20     ` Tuan Phan
  0 siblings, 0 replies; 16+ messages in thread
From: Tuan Phan @ 2023-03-10 22:20 UTC (permalink / raw)
  To: Warkentin, Andrei
  Cc: devel@edk2.groups.io, Kinney, Michael D, Gao, Liming,
	Liu, Zhiguang, sunilvl@ventanamicro.com, git@danielschaefer.me

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

Hi Andrei,
Here it is: https://github.com/pttuan/edk2/tree/tphan/riscv_mmu

On Thu, Mar 9, 2023 at 1:34 PM Warkentin, Andrei <andrei.warkentin@intel.com>
wrote:

> Hi Tuan,
>
>
>
> Could you share a GitHub link to a branch with the patch set? Somehow my
> email client is mangling one of your patches where it’s all one giant line
> of code.
>
>
>
> A
>
>
>
> *From:* Tuan Phan <tphan@ventanamicro.com>
> *Sent:* Thursday, March 9, 2023 1:20 PM
> *To:* devel@edk2.groups.io
> *Cc:* Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <
> gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> sunilvl@ventanamicro.com; git@danielschaefer.me; Warkentin, Andrei <
> andrei.warkentin@intel.com>
> *Subject:* RE: [PATCH 0/7] RISC-V: Add MMU support
>
>
>
> Hi All,
>
> Any updates on this series?
>
>
>
> Thanks,
>
>
>
> *From: *Tuan Phan <tphan@ventanamicro.com>
> *Sent: *Monday, March 6, 2023 9:33 AM
> *To: *devel@edk2.groups.io
> *Cc: *michael.d.kinney@intel.com; gaoliming@byosoft.com.cn;
> zhiguang.liu@intel.com; sunilvl@ventanamicro.com; git@danielschaefer.me;
> andrei.warkentin@intel.com; Tuan Phan <tphan@ventanamicro.com>
> *Subject: *[PATCH 0/7] RISC-V: Add MMU support
>
>
>
> This series adds MMU support for RISC-V. Only SV39/48/57 modes
>
> are supported and tested. The MMU is required to support setting
>
> page attribute which is the first basic step to support security
>
> booting on RISC-V.
>
>
>
> There are three parts:
>
> 1. Add MMU core to UefiCpuPkg. MMU will be enabled during
>
> CpuDxe initialization.
>
> 2. Fix OvmfPkg/VirtNorFlashDxe that failed to add flash base
>
> address to GCD if already done.
>
> 3. Enable MMU for RiscVVirt platform and populating its device
>
> resources in SEC phase. All resources should be populated in HOB
>
> or added to GCD by driver before accessing them when MMU enabled.
>
>
>
> Tuan Phan (7):
>
>   MdePkg/BaseLib: RISC-V: Support getting satp register value
>
>   MdePkg/Register: RISC-V: Add satp mode bits shift definition
>
>   UefiCpuPkg: RISC-V: Support MMU with SV39/48/57 mode
>
>   OvmfPkg/RiscVVirt: VirtNorFlashPlatformLib: Fix wrong flash size
>
>   OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists
>
>   OvmfPkg/RiscVVirt: SEC: Add IO memory resource hob for platform
>
>     devices
>
>   OvmfPkg/RiscVVirt: Enable MMU with SV39 mode
>
>
>
> MdePkg/Include/Library/BaseLib.h              |   5 +
>
> .../Include/Register/RiscV64/RiscVEncoding.h  |   7 +-
>
> MdePkg/Library/BaseLib/RiscV64/RiscVMmu.S     |   8 +
>
> .../VirtNorFlashStaticLib.c                   |   3 +-
>
> OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc           |   1 +
>
> OvmfPkg/RiscVVirt/Sec/Memory.c                |  17 -
>
> OvmfPkg/RiscVVirt/Sec/Platform.c              |  62 +++
>
> OvmfPkg/RiscVVirt/Sec/SecMain.inf             |   1 +
>
> OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c     |  25 +-
>
> UefiCpuPkg/CpuDxeRiscV64/CpuDxe.c             |  10 +-
>
> UefiCpuPkg/CpuDxeRiscV64/CpuDxe.h             |   1 +
>
> UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf    |   5 +
>
> UefiCpuPkg/CpuDxeRiscV64/Mmu.c                | 493 ++++++++++++++++++
>
> UefiCpuPkg/CpuDxeRiscV64/Mmu.h                |  33 ++
>
> UefiCpuPkg/CpuDxeRiscV64/MmuCore.S            |  29 ++
>
> UefiCpuPkg/UefiCpuPkg.dec                     |   8 +
>
> 16 files changed, 676 insertions(+), 32 deletions(-)
>
> create mode 100644 UefiCpuPkg/CpuDxeRiscV64/Mmu.c
>
> create mode 100644 UefiCpuPkg/CpuDxeRiscV64/Mmu.h
>
> create mode 100644 UefiCpuPkg/CpuDxeRiscV64/MmuCore.S
>
>
>
> --
>
> 2.25.1
>
>
>
>
>

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [edk2-devel] [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists
  2023-03-06 17:53   ` [edk2-devel] " Ard Biesheuvel
@ 2023-05-24 18:13     ` Tuan Phan
  2023-05-25 14:27       ` Ard Biesheuvel
  0 siblings, 1 reply; 16+ messages in thread
From: Tuan Phan @ 2023-05-24 18:13 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: devel, michael.d.kinney, gaoliming, zhiguang.liu, sunilvl, git,
	andrei.warkentin

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

On Mon, Mar 6, 2023 at 9:53 AM Ard Biesheuvel <ardb@kernel.org> wrote:

> On Mon, 6 Mar 2023 at 18:33, Tuan Phan <tphan@ventanamicro.com> wrote:
> >
> > The flash base address can be added to GCD before this driver run.
> > So only add it if it has not been done.
> >
>
> How do you end up in this situation?
>
> You cannot skip this registration, as it is required to get the region
> marked as EFI_MEMORY_RUNTIME, and without that, EFI variables will be
> broken when running under the OS.
>

Ard,
The patch only skips AddMemorySpace if it is already done in the early SEC
phase for RiscV platform.
The EFI_MEMORY_RUNTIME always be set in the next line with
SetMemorySpaceAttributes.


> > Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> > ---
> >  OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c | 25 +++++++++++++++--------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> > index f9a41f6aab0f..8875824f3333 100644
> > --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> > +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> > @@ -372,10 +372,11 @@ NorFlashFvbInitialize (
> >    IN NOR_FLASH_INSTANCE  *Instance
> >    )
> >  {
> > -  EFI_STATUS     Status;
> > -  UINT32         FvbNumLba;
> > -  EFI_BOOT_MODE  BootMode;
> > -  UINTN          RuntimeMmioRegionSize;
> > +  EFI_STATUS                      Status;
> > +  UINT32                          FvbNumLba;
> > +  EFI_BOOT_MODE                   BootMode;
> > +  UINTN                           RuntimeMmioRegionSize;
> > +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
> >
> >    DEBUG ((DEBUG_BLKIO, "NorFlashFvbInitialize\n"));
> >    ASSERT ((Instance != NULL));
> > @@ -390,13 +391,19 @@ NorFlashFvbInitialize (
> >    //       is written as the base of the flash region (ie:
> Instance->DeviceBaseAddress)
> >    RuntimeMmioRegionSize = (Instance->RegionBaseAddress -
> Instance->DeviceBaseAddress) + Instance->Size;
> >
> > -  Status = gDS->AddMemorySpace (
> > -                  EfiGcdMemoryTypeMemoryMappedIo,
> > +  Status = gDS->GetMemorySpaceDescriptor (
> >                    Instance->DeviceBaseAddress,
> > -                  RuntimeMmioRegionSize,
> > -                  EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> > +                  &Desc
> >                    );
> > -  ASSERT_EFI_ERROR (Status);
> > +  if (Status == EFI_NOT_FOUND) {
> > +    Status = gDS->AddMemorySpace (
> > +                    EfiGcdMemoryTypeMemoryMappedIo,
> > +                    Instance->DeviceBaseAddress,
> > +                    RuntimeMmioRegionSize,
> > +                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> > +                    );
> > +    ASSERT_EFI_ERROR (Status);
> > +  }
> >
> >    Status = gDS->SetMemorySpaceAttributes (
> >                    Instance->DeviceBaseAddress,
> > --
> > 2.25.1
> >
> >
> >
> > ------------
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#100754):
> https://edk2.groups.io/g/devel/message/100754
> > Mute This Topic: https://groups.io/mt/97430554/1131722
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb@kernel.org]
> > ------------
> >
> >
>

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [edk2-devel] [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists
  2023-05-24 18:13     ` Tuan Phan
@ 2023-05-25 14:27       ` Ard Biesheuvel
  2023-05-25 14:44         ` Tuan Phan
  0 siblings, 1 reply; 16+ messages in thread
From: Ard Biesheuvel @ 2023-05-25 14:27 UTC (permalink / raw)
  To: devel, tphan
  Cc: michael.d.kinney, gaoliming, zhiguang.liu, sunilvl, git,
	andrei.warkentin

On Wed, 24 May 2023 at 20:13, Tuan Phan <tphan@ventanamicro.com> wrote:
>
>
>
> On Mon, Mar 6, 2023 at 9:53 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Mon, 6 Mar 2023 at 18:33, Tuan Phan <tphan@ventanamicro.com> wrote:
>> >
>> > The flash base address can be added to GCD before this driver run.
>> > So only add it if it has not been done.
>> >
>>
>> How do you end up in this situation?
>>
>> You cannot skip this registration, as it is required to get the region
>> marked as EFI_MEMORY_RUNTIME, and without that, EFI variables will be
>> broken when running under the OS.
>
>
> Ard,
> The patch only skips AddMemorySpace if it is already done in the early SEC phase for RiscV platform.
> The EFI_MEMORY_RUNTIME always be set in the next line with SetMemorySpaceAttributes.
>

So how does the SEC phase create GCD regions to begin with?

This really sounds like there is a problem elsewhere tbh.


>>
>> > Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
>> > ---
>> >  OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c | 25 +++++++++++++++--------
>> >  1 file changed, 16 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
>> > index f9a41f6aab0f..8875824f3333 100644
>> > --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
>> > +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
>> > @@ -372,10 +372,11 @@ NorFlashFvbInitialize (
>> >    IN NOR_FLASH_INSTANCE  *Instance
>> >    )
>> >  {
>> > -  EFI_STATUS     Status;
>> > -  UINT32         FvbNumLba;
>> > -  EFI_BOOT_MODE  BootMode;
>> > -  UINTN          RuntimeMmioRegionSize;
>> > +  EFI_STATUS                      Status;
>> > +  UINT32                          FvbNumLba;
>> > +  EFI_BOOT_MODE                   BootMode;
>> > +  UINTN                           RuntimeMmioRegionSize;
>> > +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
>> >
>> >    DEBUG ((DEBUG_BLKIO, "NorFlashFvbInitialize\n"));
>> >    ASSERT ((Instance != NULL));
>> > @@ -390,13 +391,19 @@ NorFlashFvbInitialize (
>> >    //       is written as the base of the flash region (ie: Instance->DeviceBaseAddress)
>> >    RuntimeMmioRegionSize = (Instance->RegionBaseAddress - Instance->DeviceBaseAddress) + Instance->Size;
>> >
>> > -  Status = gDS->AddMemorySpace (
>> > -                  EfiGcdMemoryTypeMemoryMappedIo,
>> > +  Status = gDS->GetMemorySpaceDescriptor (
>> >                    Instance->DeviceBaseAddress,
>> > -                  RuntimeMmioRegionSize,
>> > -                  EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>> > +                  &Desc
>> >                    );
>> > -  ASSERT_EFI_ERROR (Status);
>> > +  if (Status == EFI_NOT_FOUND) {
>> > +    Status = gDS->AddMemorySpace (
>> > +                    EfiGcdMemoryTypeMemoryMappedIo,
>> > +                    Instance->DeviceBaseAddress,
>> > +                    RuntimeMmioRegionSize,
>> > +                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
>> > +                    );
>> > +    ASSERT_EFI_ERROR (Status);
>> > +  }
>> >
>> >    Status = gDS->SetMemorySpaceAttributes (
>> >                    Instance->DeviceBaseAddress,
>> > --
>> > 2.25.1
>> >
>> >
>> >
>> > ------------
>> > Groups.io Links: You receive all messages sent to this group.
>> > View/Reply Online (#100754): https://edk2.groups.io/g/devel/message/100754
>> > Mute This Topic: https://groups.io/mt/97430554/1131722
>> > Group Owner: devel+owner@edk2.groups.io
>> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb@kernel.org]
>> > ------------
>> >
>> >
>
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [edk2-devel] [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists
  2023-05-25 14:27       ` Ard Biesheuvel
@ 2023-05-25 14:44         ` Tuan Phan
  0 siblings, 0 replies; 16+ messages in thread
From: Tuan Phan @ 2023-05-25 14:44 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: devel, michael.d.kinney, gaoliming, zhiguang.liu, sunilvl, git,
	andrei.warkentin

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

On Thu, May 25, 2023 at 7:27 AM Ard Biesheuvel <ardb@kernel.org> wrote:

> On Wed, 24 May 2023 at 20:13, Tuan Phan <tphan@ventanamicro.com> wrote:
> >
> >
> >
> > On Mon, Mar 6, 2023 at 9:53 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> On Mon, 6 Mar 2023 at 18:33, Tuan Phan <tphan@ventanamicro.com> wrote:
> >> >
> >> > The flash base address can be added to GCD before this driver run.
> >> > So only add it if it has not been done.
> >> >
> >>
> >> How do you end up in this situation?
> >>
> >> You cannot skip this registration, as it is required to get the region
> >> marked as EFI_MEMORY_RUNTIME, and without that, EFI variables will be
> >> broken when running under the OS.
> >
> >
> > Ard,
> > The patch only skips AddMemorySpace if it is already done in the early
> SEC phase for RiscV platform.
> > The EFI_MEMORY_RUNTIME always be set in the next line with
> SetMemorySpaceAttributes.
> >
>
> So how does the SEC phase create GCD regions to begin with?
>
> This really sounds like there is a problem elsewhere tbh.
>

The SEC phase just simply adds that region to the memory hob with
BuildResourceDescriptorHob.
Agree, the problem is VariableRuntimeDxe.efi accessing the region before
this VirtNorFlashDxe starts so when
MMU enabled, edk2 will hang due to page fault exception.

>
>
> >>
> >> > Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> >> > ---
> >> >  OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c | 25
> +++++++++++++++--------
> >> >  1 file changed, 16 insertions(+), 9 deletions(-)
> >> >
> >> > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> >> > index f9a41f6aab0f..8875824f3333 100644
> >> > --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> >> > +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.c
> >> > @@ -372,10 +372,11 @@ NorFlashFvbInitialize (
> >> >    IN NOR_FLASH_INSTANCE  *Instance
> >> >    )
> >> >  {
> >> > -  EFI_STATUS     Status;
> >> > -  UINT32         FvbNumLba;
> >> > -  EFI_BOOT_MODE  BootMode;
> >> > -  UINTN          RuntimeMmioRegionSize;
> >> > +  EFI_STATUS                      Status;
> >> > +  UINT32                          FvbNumLba;
> >> > +  EFI_BOOT_MODE                   BootMode;
> >> > +  UINTN                           RuntimeMmioRegionSize;
> >> > +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
> >> >
> >> >    DEBUG ((DEBUG_BLKIO, "NorFlashFvbInitialize\n"));
> >> >    ASSERT ((Instance != NULL));
> >> > @@ -390,13 +391,19 @@ NorFlashFvbInitialize (
> >> >    //       is written as the base of the flash region (ie:
> Instance->DeviceBaseAddress)
> >> >    RuntimeMmioRegionSize = (Instance->RegionBaseAddress -
> Instance->DeviceBaseAddress) + Instance->Size;
> >> >
> >> > -  Status = gDS->AddMemorySpace (
> >> > -                  EfiGcdMemoryTypeMemoryMappedIo,
> >> > +  Status = gDS->GetMemorySpaceDescriptor (
> >> >                    Instance->DeviceBaseAddress,
> >> > -                  RuntimeMmioRegionSize,
> >> > -                  EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> >> > +                  &Desc
> >> >                    );
> >> > -  ASSERT_EFI_ERROR (Status);
> >> > +  if (Status == EFI_NOT_FOUND) {
> >> > +    Status = gDS->AddMemorySpace (
> >> > +                    EfiGcdMemoryTypeMemoryMappedIo,
> >> > +                    Instance->DeviceBaseAddress,
> >> > +                    RuntimeMmioRegionSize,
> >> > +                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> >> > +                    );
> >> > +    ASSERT_EFI_ERROR (Status);
> >> > +  }
> >> >
> >> >    Status = gDS->SetMemorySpaceAttributes (
> >> >                    Instance->DeviceBaseAddress,
> >> > --
> >> > 2.25.1
> >> >
> >> >
> >> >
> >> > ------------
> >> > Groups.io Links: You receive all messages sent to this group.
> >> > View/Reply Online (#100754):
> https://edk2.groups.io/g/devel/message/100754
> >> > Mute This Topic: https://groups.io/mt/97430554/1131722
> >> > Group Owner: devel+owner@edk2.groups.io
> >> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb@kernel.org]
> >> > ------------
> >> >
> >> >
> >
> > 
>

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2023-05-25 14:44 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-06 17:33 [PATCH 0/7] RISC-V: Add MMU support Tuan Phan
2023-03-06 17:33 ` [PATCH 1/7] MdePkg/BaseLib: RISC-V: Support getting satp register value Tuan Phan
2023-03-06 17:33 ` [PATCH 2/7] MdePkg/Register: RISC-V: Add satp mode bits shift definition Tuan Phan
2023-03-06 17:33 ` [PATCH 3/7] UefiCpuPkg: RISC-V: Support MMU with SV39/48/57 mode Tuan Phan
2023-03-06 17:33 ` [PATCH 4/7] OvmfPkg/RiscVVirt: VirtNorFlashPlatformLib: Fix wrong flash size Tuan Phan
2023-03-06 17:33 ` [PATCH 5/7] OvmfPkg/VirtNorFlashDxe: Not add memory space if it exists Tuan Phan
2023-03-06 17:53   ` [edk2-devel] " Ard Biesheuvel
2023-05-24 18:13     ` Tuan Phan
2023-05-25 14:27       ` Ard Biesheuvel
2023-05-25 14:44         ` Tuan Phan
2023-03-06 17:33 ` [PATCH 6/7] OvmfPkg/RiscVVirt: SEC: Add IO memory resource hob for platform devices Tuan Phan
2023-03-06 17:33 ` [PATCH 7/7] OvmfPkg/RiscVVirt: Enable MMU with SV39 mode Tuan Phan
2023-03-09  0:46   ` Andrei Warkentin
2023-03-09 19:19 ` [PATCH 0/7] RISC-V: Add MMU support Tuan Phan
2023-03-09 21:34   ` Andrei Warkentin
2023-03-10 22:20     ` Tuan Phan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox