public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining
@ 2022-09-26  8:24 Ard Biesheuvel
  2022-09-26  8:24 ` [PATCH v3 01/16] ArmVirtPkg: remove EbcDxe from all platforms Ard Biesheuvel
                   ` (15 more replies)
  0 siblings, 16 replies; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:24 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

We currently do a substantial amount of processing before enabling the
MMU and caches, which is bad for performance, but also fragile, as it
requires cache coherency to be managed in software.

It also means that when running under virtualization, the hypervisor
must do a non-trivial amount of work to ensure that the host's cached
view of memory is consistent with the guest's uncached view.

So let's update the ArmVirtQemu early boot sequence to improve the
situation:
- modify the page table building logic to avoid the MMU disable/enable
  unless really necessary, i.e., only when the entry in question maps
  itself, or the code that performs the actual update;
- map any regions that cover page tables in memory eagerly down to
  pages, so that we will not need to split them later, and be forced to
  go through the MMU-off path to unmap and remap them;
- allow the asm helper routine that lives in the MemoryInit XIP PEIM to
  be exposed via a HOB so we can fall back to it from DXE; 
- use a compile time generated ID map that covers the first bank of NOR
  flash, the first MMIO region (for the UART), and the first 128 MiB of
  DRAM, and switch to it straight out of reset.

The resulting build no longer performs any non-coherent memory accesses
via the data side, and only relies on instruction fetches before the MMU
is enabled. It also avoids any cache maintenance to the PoC.

Changes since v2:
- drop shadow page table approach - it only works at EL1, and is a bit
  more intrusive than needed; instead, do a proper break-before-make
  (BBM) unless the break unmaps the page table itself or the code that
  is modifying it;
- add a couple of only tangentially related performance streamlining
  changes, to avoid dispatching and shadowing drivers that we don't need

Changes since v1:
- coding style tweaks to placate our CI overlord
- drop -mstrict-align which is no longer needed now that all C code runs
  with the MMU and caches on

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Alexander Graf <agraf@csgraf.de>

Ard Biesheuvel (16):
  ArmVirtPkg: remove EbcDxe from all platforms
  ArmVirtPkg: do not enable iSCSI driver by default
  ArmVirtPkg: make EFI_LOADER_DATA non-executable
  ArmVirtPkg/ArmVirtQemu: wire up timeout PCD to Timeout variable
  ArmPkg/ArmMmuLib: don't replace table entries with block entries
  ArmPkg/ArmMmuLib: Disable and re-enable MMU only when needed
  ArmPkg/ArmMmuLib: permit initial configuration with MMU enabled
  ArmPkg/ArmMmuLib: Reuse XIP MMU routines when splitting entries
  ArmPlatformPkg/PrePeiCore: permit entry with the MMU enabled
  ArmVirtPkg/ArmVirtQemu: implement ArmPlatformLib with static ID map
  ArmVirtPkg/ArmVirtQemu: use first 128 MiB as permanent PEI memory
  ArmVirtPkg/ArmVirtQemu: enable initial ID map at early boot
  ArmVirtPkg/ArmVirtQemu: Drop unused variable PEIM
  ArmVirtPkg/ArmVirtQemu: avoid shadowing PEIMs unless necessary
  ArmVirtPkg/QemuVirtMemInfoLib: use HOB not PCD to record the memory
    size
  ArmVirtPkg/ArmVirtQemu: omit PCD PEIM unless TPM support is enabled

 ArmPkg/ArmPkg.dec                                                                                            |   2 +
 ArmPkg/Include/Library/ArmMmuLib.h                                                                           |   7 +-
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c                                                             | 191 +++++++++++++-------
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S                                                     |  43 ++++-
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c                                                   |  17 ++
 ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf                                                                   |   4 +
 ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf                                                                    |   4 +
 ArmPlatformPkg/PrePeiCore/PrePeiCore.c                                                                       |  22 ++-
 ArmVirtPkg/ArmVirt.dsc.inc                                                                                   |   7 +-
 ArmVirtPkg/ArmVirtCloudHv.fdf                                                                                |   5 -
 ArmVirtPkg/ArmVirtPkg.dec                                                                                    |   1 +
 ArmVirtPkg/ArmVirtQemu.dsc                                                                                   |  53 ++++--
 ArmVirtPkg/ArmVirtQemu.fdf                                                                                   |   5 +-
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc                                                                         |   5 -
 ArmVirtPkg/ArmVirtQemuKernel.dsc                                                                             |   1 -
 ArmVirtPkg/ArmVirtXen.fdf                                                                                    |   5 -
 ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S                                            | 115 ++++++++++++
 ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.c                                                   |  64 +++++++
 ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.inf                                                 |  40 ++++
 ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S                                                                |  57 ++++++
 ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c                                         |  14 +-
 ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf                                       |   1 +
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c                                                   |  35 +++-
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf                                                 |   5 +-
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf                                              |   8 +-
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c                                     |  30 +--
 ArmVirtPkg/MemoryInitPei/MemoryInitPeim.c                                                                    | 104 +++++++++++
 ArmVirtPkg/{Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf => MemoryInitPei/MemoryInitPeim.inf} |  36 ++--
 28 files changed, 714 insertions(+), 167 deletions(-)
 create mode 100644 ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
 create mode 100644 ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.c
 create mode 100644 ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.inf
 create mode 100644 ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S
 create mode 100644 ArmVirtPkg/MemoryInitPei/MemoryInitPeim.c
 copy ArmVirtPkg/{Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf => MemoryInitPei/MemoryInitPeim.inf} (64%)

-- 
2.35.1


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

* [PATCH v3 01/16] ArmVirtPkg: remove EbcDxe from all platforms
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
@ 2022-09-26  8:24 ` Ard Biesheuvel
  2022-09-26  8:24 ` [PATCH v3 02/16] ArmVirtPkg: do not enable iSCSI driver by default Ard Biesheuvel
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:24 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

The EBC interpreter is rarely, if ever, used on ARM, and is especially
pointless on virtual machines. So let's drop it from the builds.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirt.dsc.inc           | 5 -----
 ArmVirtPkg/ArmVirtCloudHv.fdf        | 5 -----
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 5 -----
 ArmVirtPkg/ArmVirtXen.fdf            | 5 -----
 4 files changed, 20 deletions(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index c39e2506a3ea..34575585adbb 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -422,8 +422,3 @@ [Components.AARCH64]
     <LibraryClasses>
       NULL|EmbeddedPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.inf
   }
-
-  #
-  # EBC support
-  #
-  MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
diff --git a/ArmVirtPkg/ArmVirtCloudHv.fdf b/ArmVirtPkg/ArmVirtCloudHv.fdf
index 81c539590a76..a5f172d79bfc 100644
--- a/ArmVirtPkg/ArmVirtCloudHv.fdf
+++ b/ArmVirtPkg/ArmVirtCloudHv.fdf
@@ -195,11 +195,6 @@ [FV.FvMain]
   INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
   INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
   INF ArmVirtPkg/CloudHvAcpiPlatformDxe/CloudHvAcpiPlatformDxe.inf
-
-  #
-  # EBC support
-  #
-  INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
 !endif
 
   #
diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
index d4df6dede0fe..787286133095 100644
--- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
+++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
@@ -146,11 +146,6 @@ [FV.FvMain]
   INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
   INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
   INF OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
-
-  #
-  # EBC support
-  #
-  INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
 !endif
 
   #
diff --git a/ArmVirtPkg/ArmVirtXen.fdf b/ArmVirtPkg/ArmVirtXen.fdf
index 132480f03059..770fbf7289be 100644
--- a/ArmVirtPkg/ArmVirtXen.fdf
+++ b/ArmVirtPkg/ArmVirtXen.fdf
@@ -201,11 +201,6 @@ [FV.FvMain]
 !if $(ARCH) == AARCH64
   INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
   INF ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.inf
-
-  #
-  # EBC support
-  #
-  INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
 !endif
 
  #
-- 
2.35.1


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

* [PATCH v3 02/16] ArmVirtPkg: do not enable iSCSI driver by default
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
  2022-09-26  8:24 ` [PATCH v3 01/16] ArmVirtPkg: remove EbcDxe from all platforms Ard Biesheuvel
@ 2022-09-26  8:24 ` Ard Biesheuvel
  2022-09-26  8:24 ` [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable Ard Biesheuvel
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:24 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

The iSCSI driver slows down the boot on a pristine variable store flash
image, as it creates a couple of large EFI non-volatile variables to
preserve state between boots.

Since iSCSI boot for VMs is kind of niche anyway, let's default to
disabled. If someone needs it in their build, they can use the -D build
command option to re-enable it on the fly.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc       | 1 -
 ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 -
 2 files changed, 2 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 9369a88858fd..45c4a8fc84e0 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -40,7 +40,6 @@ [Defines]
   DEFINE NETWORK_SNP_ENABLE              = FALSE
   DEFINE NETWORK_TLS_ENABLE              = FALSE
   DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS  = TRUE
-  DEFINE NETWORK_ISCSI_ENABLE            = TRUE
 
 !if $(NETWORK_SNP_ENABLE) == TRUE
   !error "NETWORK_SNP_ENABLE is IA32/X64/EBC only"
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index 7f7d15d6eee3..66039f07f41b 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -38,7 +38,6 @@ [Defines]
   DEFINE NETWORK_SNP_ENABLE              = FALSE
   DEFINE NETWORK_TLS_ENABLE              = FALSE
   DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS  = TRUE
-  DEFINE NETWORK_ISCSI_ENABLE            = TRUE
 
 !if $(NETWORK_SNP_ENABLE) == TRUE
   !error "NETWORK_SNP_ENABLE is IA32/X64/EBC only"
-- 
2.35.1


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

* [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
  2022-09-26  8:24 ` [PATCH v3 01/16] ArmVirtPkg: remove EbcDxe from all platforms Ard Biesheuvel
  2022-09-26  8:24 ` [PATCH v3 02/16] ArmVirtPkg: do not enable iSCSI driver by default Ard Biesheuvel
@ 2022-09-26  8:24 ` Ard Biesheuvel
  2022-09-26 22:28   ` [edk2-devel] " Leif Lindholm
  2022-11-28 15:46   ` Gerd Hoffmann
  2022-09-26  8:24 ` [PATCH v3 04/16] ArmVirtPkg/ArmVirtQemu: wire up timeout PCD to Timeout variable Ard Biesheuvel
                   ` (12 subsequent siblings)
  15 siblings, 2 replies; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:24 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

When the memory protections were implemented and enabled on ArmVirtQemu
5+ years ago, we had to work around the fact that GRUB at the time
expected EFI_LOADER_DATA to be executable, as that is the memory type it
allocates when loading its modules.

This has been fixed in GRUB in August 2017, so by now, we should be able
to tighten this, and remove execute permissions from EFI_LOADER_DATA
allocations.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirt.dsc.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 34575585adbb..462073517a22 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -368,7 +368,7 @@ [PcdsFixedAtBuild.common]
   # reserved ones, with the exception of LoaderData regions, of which OS loaders
   # (i.e., GRUB) may assume that its contents are executable.
   #
-  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD5
 
 [Components.common]
   #
-- 
2.35.1


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

* [PATCH v3 04/16] ArmVirtPkg/ArmVirtQemu: wire up timeout PCD to Timeout variable
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
                   ` (2 preceding siblings ...)
  2022-09-26  8:24 ` [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable Ard Biesheuvel
@ 2022-09-26  8:24 ` Ard Biesheuvel
  2022-09-26  8:25 ` [PATCH v3 05/16] ArmPkg/ArmMmuLib: don't replace table entries with block entries Ard Biesheuvel
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:24 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

Use the appropriate PCD definition in the ArmVirtQemu DSC so that the
boot timeout is taken from the Timeout variable automatically, which is
what Linux tools such as efibootmgr expect.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 45c4a8fc84e0..302c0d2a4e29 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -292,6 +292,8 @@ [PcdsDynamicHii]
   gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
 !endif
 
+  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|5
+
 ################################################################################
 #
 # Components Section - list of all EDK II Modules needed by this Platform
-- 
2.35.1


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

* [PATCH v3 05/16] ArmPkg/ArmMmuLib: don't replace table entries with block entries
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
                   ` (3 preceding siblings ...)
  2022-09-26  8:24 ` [PATCH v3 04/16] ArmVirtPkg/ArmVirtQemu: wire up timeout PCD to Timeout variable Ard Biesheuvel
@ 2022-09-26  8:25 ` Ard Biesheuvel
  2022-09-26 22:32   ` Leif Lindholm
  2022-09-26  8:25 ` [PATCH v3 06/16] ArmPkg/ArmMmuLib: Disable and re-enable MMU only when needed Ard Biesheuvel
                   ` (10 subsequent siblings)
  15 siblings, 1 reply; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:25 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

Drop the optimization that replaces table entries with block entries and
frees the page tables in the subhierarchy that is being replaced. This
rarely occurs in practice anyway, and will require more elaborate TLB
maintenance once we switch to a different approach when running at EL1,
where we no longer disable the MMU and nuke the TLB entirely every time
we update a descriptor in a way that requires break-before-make (BBM).

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index e5ecc7375153..34f1031c4de3 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -197,12 +197,9 @@ UpdateRegionMappingRecursive (
     // 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 we are changing a table entry and the AttributeClearMask is non-zero,
-    // we cannot replace it with a block entry without potentially losing
-    // attribute information, so keep the table entry in that case.
     //
     if ((Level == 0) || (((RegionStart | BlockEnd) & BlockMask) != 0) ||
-        (IsTableEntry (*Entry, Level) && (AttributeClearMask != 0)))
+        IsTableEntry (*Entry, Level))
     {
       ASSERT (Level < 3);
 
@@ -294,20 +291,7 @@ UpdateRegionMappingRecursive (
       EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
                                  : TT_TYPE_BLOCK_ENTRY;
 
-      if (IsTableEntry (*Entry, Level)) {
-        //
-        // We are replacing a table entry with a block entry. This is only
-        // possible if we are keeping none of the original attributes.
-        // We can free the table entry's page table, and all the ones below
-        // it, since we are dropping the only possible reference to it.
-        //
-        ASSERT (AttributeClearMask == 0);
-        TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
-        ReplaceTableEntry (Entry, EntryValue, RegionStart, TRUE);
-        FreePageTablesRecursive (TranslationTable, Level + 1);
-      } else {
-        ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
-      }
+      ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
     }
   }
 
-- 
2.35.1


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

* [PATCH v3 06/16] ArmPkg/ArmMmuLib: Disable and re-enable MMU only when needed
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
                   ` (4 preceding siblings ...)
  2022-09-26  8:25 ` [PATCH v3 05/16] ArmPkg/ArmMmuLib: don't replace table entries with block entries Ard Biesheuvel
@ 2022-09-26  8:25 ` Ard Biesheuvel
  2022-09-26 23:28   ` Leif Lindholm
  2022-09-26  8:25 ` [PATCH v3 07/16] ArmPkg/ArmMmuLib: permit initial configuration with MMU enabled Ard Biesheuvel
                   ` (9 subsequent siblings)
  15 siblings, 1 reply; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:25 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

When updating a page table descriptor in a way that requires break
before make, we temporarily disable the MMU to ensure that we don't
unmap the memory region that the code itself is executing from.

However, this is a condition we can check in a straight-forward manner,
and if the regions are disjoint, we don't have to bother with the MMU
controls, and we can just perform an ordinary break before make.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Include/Library/ArmMmuLib.h                       |   7 +-
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c         | 102 ++++++++++++++++----
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S |  43 +++++++--
 3 files changed, 123 insertions(+), 29 deletions(-)

diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h
index 7538a8274a72..b745e2230e7e 100644
--- a/ArmPkg/Include/Library/ArmMmuLib.h
+++ b/ArmPkg/Include/Library/ArmMmuLib.h
@@ -52,9 +52,10 @@ ArmClearMemoryRegionReadOnly (
 VOID
 EFIAPI
 ArmReplaceLiveTranslationEntry (
-  IN  UINT64  *Entry,
-  IN  UINT64  Value,
-  IN  UINT64  RegionStart
+  IN  UINT64   *Entry,
+  IN  UINT64   Value,
+  IN  UINT64   RegionStart,
+  IN  BOOLEAN  DisableMmu
   );
 
 EFI_STATUS
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 34f1031c4de3..4d75788ed2b2 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -18,6 +18,17 @@
 #include <Library/ArmMmuLib.h>
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
+#include <Library/HobLib.h>
+
+STATIC
+VOID (
+  EFIAPI  *mReplaceLiveEntryFunc
+  )(
+    IN  UINT64  *Entry,
+    IN  UINT64  Value,
+    IN  UINT64  RegionStart,
+    IN  BOOLEAN DisableMmu
+    ) = ArmReplaceLiveTranslationEntry;
 
 STATIC
 UINT64
@@ -83,14 +94,40 @@ ReplaceTableEntry (
   IN  UINT64   *Entry,
   IN  UINT64   Value,
   IN  UINT64   RegionStart,
+  IN  UINT64   BlockMask,
   IN  BOOLEAN  IsLiveBlockMapping
   )
 {
-  if (!ArmMmuEnabled () || !IsLiveBlockMapping) {
+  BOOLEAN  DisableMmu;
+
+  //
+  // Replacing a live block entry with a table entry (or vice versa) requires a
+  // break-before-make sequence as per the architecture. This means the mapping
+  // must be made invalid and cleaned from the TLBs first, and this is a bit of
+  // a hassle if the mapping in question covers the code that is actually doing
+  // the mapping and the unmapping, and so we only bother with this if actually
+  // necessary.
+  //
+
+  if (!IsLiveBlockMapping || !ArmMmuEnabled ()) {
+    // If the mapping is not a live block mapping, or the MMU is not on yet, we
+    // can simply overwrite the entry.
     *Entry = Value;
     ArmUpdateTranslationTableEntry (Entry, (VOID *)(UINTN)RegionStart);
   } else {
-    ArmReplaceLiveTranslationEntry (Entry, Value, RegionStart);
+    // If the mapping in question does not cover the code that updates the
+    // entry in memory, or the entry that we are intending to update, we can
+    // use an ordinary break before make. Otherwise, we will need to
+    // temporarily disable the MMU.
+    DisableMmu = FALSE;
+    if ((((RegionStart ^ (UINTN)ArmReplaceLiveTranslationEntry) & ~BlockMask) == 0) ||
+        (((RegionStart ^ (UINTN)Entry) & ~BlockMask) == 0))
+    {
+      DisableMmu = TRUE;
+      DEBUG ((DEBUG_WARN, "%a: splitting block entry with MMU disabled\n", __FUNCTION__));
+    }
+
+    ArmReplaceLiveTranslationEntry (Entry, Value, RegionStart, DisableMmu);
   }
 }
 
@@ -155,12 +192,13 @@ IsTableEntry (
 STATIC
 EFI_STATUS
 UpdateRegionMappingRecursive (
-  IN  UINT64  RegionStart,
-  IN  UINT64  RegionEnd,
-  IN  UINT64  AttributeSetMask,
-  IN  UINT64  AttributeClearMask,
-  IN  UINT64  *PageTable,
-  IN  UINTN   Level
+  IN  UINT64   RegionStart,
+  IN  UINT64   RegionEnd,
+  IN  UINT64   AttributeSetMask,
+  IN  UINT64   AttributeClearMask,
+  IN  UINT64   *PageTable,
+  IN  UINTN    Level,
+  IN  BOOLEAN  TableIsLive
   )
 {
   UINTN       BlockShift;
@@ -170,6 +208,7 @@ UpdateRegionMappingRecursive (
   UINT64      EntryValue;
   VOID        *TranslationTable;
   EFI_STATUS  Status;
+  BOOLEAN     NextTableIsLive;
 
   ASSERT (((RegionStart | RegionEnd) & EFI_PAGE_MASK) == 0);
 
@@ -198,7 +237,14 @@ UpdateRegionMappingRecursive (
     // the next level. No block mappings are allowed at all at level 0,
     // so in that case, we have to recurse unconditionally.
     //
+    // One special case to take into account is any region that covers the page
+    // table itself: if we'd cover such a region with block mappings, we are
+    // more likely to end up in the situation later where we need to disable
+    // the MMU in order to update page table entries safely, so prefer page
+    // mappings in that particular case.
+    //
     if ((Level == 0) || (((RegionStart | BlockEnd) & BlockMask) != 0) ||
+        ((Level < 3) && (((UINT64)PageTable & ~BlockMask) == RegionStart)) ||
         IsTableEntry (*Entry, Level))
     {
       ASSERT (Level < 3);
@@ -234,7 +280,8 @@ UpdateRegionMappingRecursive (
                      *Entry & TT_ATTRIBUTES_MASK,
                      0,
                      TranslationTable,
-                     Level + 1
+                     Level + 1,
+                     FALSE
                      );
           if (EFI_ERROR (Status)) {
             //
@@ -246,8 +293,11 @@ UpdateRegionMappingRecursive (
             return Status;
           }
         }
+
+        NextTableIsLive = FALSE;
       } else {
         TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
+        NextTableIsLive  = TableIsLive;
       }
 
       //
@@ -259,7 +309,8 @@ UpdateRegionMappingRecursive (
                  AttributeSetMask,
                  AttributeClearMask,
                  TranslationTable,
-                 Level + 1
+                 Level + 1,
+                 NextTableIsLive
                  );
       if (EFI_ERROR (Status)) {
         if (!IsTableEntry (*Entry, Level)) {
@@ -282,7 +333,8 @@ UpdateRegionMappingRecursive (
           Entry,
           EntryValue,
           RegionStart,
-          IsBlockEntry (*Entry, Level)
+          BlockMask,
+          TableIsLive && IsBlockEntry (*Entry, Level)
           );
       }
     } else {
@@ -291,7 +343,7 @@ UpdateRegionMappingRecursive (
       EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
                                  : TT_TYPE_BLOCK_ENTRY;
 
-      ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
+      ReplaceTableEntry (Entry, EntryValue, RegionStart, BlockMask, FALSE);
     }
   }
 
@@ -301,10 +353,11 @@ UpdateRegionMappingRecursive (
 STATIC
 EFI_STATUS
 UpdateRegionMapping (
-  IN  UINT64  RegionStart,
-  IN  UINT64  RegionLength,
-  IN  UINT64  AttributeSetMask,
-  IN  UINT64  AttributeClearMask
+  IN  UINT64   RegionStart,
+  IN  UINT64   RegionLength,
+  IN  UINT64   AttributeSetMask,
+  IN  UINT64   AttributeClearMask,
+  IN  BOOLEAN  TableIsLive
   )
 {
   UINTN  T0SZ;
@@ -321,7 +374,8 @@ UpdateRegionMapping (
            AttributeSetMask,
            AttributeClearMask,
            ArmGetTTBR0BaseAddress (),
-           GetRootTableLevel (T0SZ)
+           GetRootTableLevel (T0SZ),
+           TableIsLive
            );
 }
 
@@ -336,7 +390,8 @@ FillTranslationTable (
            MemoryRegion->VirtualBase,
            MemoryRegion->Length,
            ArmMemoryAttributeToPageAttribute (MemoryRegion->Attributes) | TT_AF,
-           0
+           0,
+           FALSE
            );
 }
 
@@ -410,7 +465,8 @@ ArmSetMemoryAttributes (
            BaseAddress,
            Length,
            PageAttributes,
-           PageAttributeMask
+           PageAttributeMask,
+           TRUE
            );
 }
 
@@ -423,7 +479,13 @@ SetMemoryRegionAttribute (
   IN  UINT64                BlockEntryMask
   )
 {
-  return UpdateRegionMapping (BaseAddress, Length, Attributes, BlockEntryMask);
+  return UpdateRegionMapping (
+           BaseAddress,
+           Length,
+           Attributes,
+           BlockEntryMask,
+           TRUE
+           );
 }
 
 EFI_STATUS
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
index 66ebca571e63..e936a5be4e11 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
@@ -12,6 +12,14 @@
 
   .macro __replace_entry, el
 
+  // check whether we should disable the MMU
+  cbz   x3, .L1_\@
+
+  // clean and invalidate first so that we don't clobber
+  // adjacent entries that are dirty in the caches
+  dc    civac, x0
+  dsb   nsh
+
   // disable the MMU
   mrs   x8, sctlr_el\el
   bic   x9, x8, #CTRL_M_BIT
@@ -38,8 +46,33 @@
   // re-enable the MMU
   msr   sctlr_el\el, x8
   isb
+  b     .L2_\@
+
+.L1_\@:
+  // write invalid entry
+  str   xzr, [x0]
+  dsb   nshst
+
+  // flush translations for the target address from the TLBs
+  lsr   x2, x2, #12
+  .if   \el == 1
+  tlbi  vaae1, x2
+  .else
+  tlbi  vae\el, x2
+  .endif
+  dsb   nsh
+
+  // write updated entry
+  str   x1, [x0]
+  dsb   nshst
+
+.L2_\@:
   .endm
 
+  // Align this routine to a log2 upper bound of its size, so that it is
+  // guaranteed not to cross a page or block boundary.
+  .balign 0x200
+
 //VOID
 //ArmReplaceLiveTranslationEntry (
 //  IN  UINT64  *Entry,
@@ -53,12 +86,7 @@ ASM_FUNC(ArmReplaceLiveTranslationEntry)
   msr   daifset, #0xf
   isb
 
-  // clean and invalidate first so that we don't clobber
-  // adjacent entries that are dirty in the caches
-  dc    civac, x0
-  dsb   nsh
-
-  EL1_OR_EL2_OR_EL3(x3)
+  EL1_OR_EL2_OR_EL3(x5)
 1:__replace_entry 1
   b     4f
 2:__replace_entry 2
@@ -72,3 +100,6 @@ ASM_GLOBAL ASM_PFX(ArmReplaceLiveTranslationEntrySize)
 
 ASM_PFX(ArmReplaceLiveTranslationEntrySize):
   .long   . - ArmReplaceLiveTranslationEntry
+
+  // Double check that we did not overrun the assumed maximum size
+  .org    ArmReplaceLiveTranslationEntry + 0x200
-- 
2.35.1


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

* [PATCH v3 07/16] ArmPkg/ArmMmuLib: permit initial configuration with MMU enabled
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
                   ` (5 preceding siblings ...)
  2022-09-26  8:25 ` [PATCH v3 06/16] ArmPkg/ArmMmuLib: Disable and re-enable MMU only when needed Ard Biesheuvel
@ 2022-09-26  8:25 ` Ard Biesheuvel
  2022-09-26 22:35   ` Leif Lindholm
  2022-09-26  8:25 ` [PATCH v3 08/16] ArmPkg/ArmMmuLib: Reuse XIP MMU routines when splitting entries Ard Biesheuvel
                   ` (8 subsequent siblings)
  15 siblings, 1 reply; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:25 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

Permit the use of this library with the MMU and caches already enabled.
This removes the need for any cache maintenance for coherency, and is
generally better for robustness and performance, especially when running
under virtualization.

Note that this means we have to defer assignment of TTBR0 until the
page tables are ready to be used, and so UpdateRegionMapping() can no
longer read back TTBR0 directly to discover the root table address.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 48 +++++++++++---------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 4d75788ed2b2..ae59e9a7d04e 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -357,6 +357,7 @@ UpdateRegionMapping (
   IN  UINT64   RegionLength,
   IN  UINT64   AttributeSetMask,
   IN  UINT64   AttributeClearMask,
+  IN  UINT64   *RootTable,
   IN  BOOLEAN  TableIsLive
   )
 {
@@ -373,7 +374,7 @@ UpdateRegionMapping (
            RegionStart + RegionLength,
            AttributeSetMask,
            AttributeClearMask,
-           ArmGetTTBR0BaseAddress (),
+           RootTable,
            GetRootTableLevel (T0SZ),
            TableIsLive
            );
@@ -391,6 +392,7 @@ FillTranslationTable (
            MemoryRegion->Length,
            ArmMemoryAttributeToPageAttribute (MemoryRegion->Attributes) | TT_AF,
            0,
+           RootTable,
            FALSE
            );
 }
@@ -466,6 +468,7 @@ ArmSetMemoryAttributes (
            Length,
            PageAttributes,
            PageAttributeMask,
+           ArmGetTTBR0BaseAddress (),
            TRUE
            );
 }
@@ -484,6 +487,7 @@ SetMemoryRegionAttribute (
            Length,
            Attributes,
            BlockEntryMask,
+           ArmGetTTBR0BaseAddress (),
            TRUE
            );
 }
@@ -675,14 +679,6 @@ ArmConfigureMmu (
     return EFI_OUT_OF_RESOURCES;
   }
 
-  //
-  // We set TTBR0 just after allocating the table to retrieve its location from
-  // the subsequent functions without needing to pass this value across the
-  // functions. The MMU is only enabled after the translation tables are
-  // populated.
-  //
-  ArmSetTTBR0 (TranslationTable);
-
   if (TranslationTableBase != NULL) {
     *TranslationTableBase = TranslationTable;
   }
@@ -691,14 +687,17 @@ ArmConfigureMmu (
     *TranslationTableSize = RootTableEntryCount * sizeof (UINT64);
   }
 
-  //
-  // Make sure we are not inadvertently hitting in the caches
-  // when populating the page tables.
-  //
-  InvalidateDataCacheRange (
-    TranslationTable,
-    RootTableEntryCount * sizeof (UINT64)
-    );
+  if (!ArmMmuEnabled ()) {
+    //
+    // Make sure we are not inadvertently hitting in the caches
+    // when populating the page tables.
+    //
+    InvalidateDataCacheRange (
+      TranslationTable,
+      RootTableEntryCount * sizeof (UINT64)
+      );
+  }
+
   ZeroMem (TranslationTable, RootTableEntryCount * sizeof (UINT64));
 
   while (MemoryTable->Length != 0) {
@@ -723,12 +722,17 @@ ArmConfigureMmu (
     MAIR_ATTR (TT_ATTR_INDX_MEMORY_WRITE_BACK, MAIR_ATTR_NORMAL_MEMORY_WRITE_BACK)
     );
 
-  ArmDisableAlignmentCheck ();
-  ArmEnableStackAlignmentCheck ();
-  ArmEnableInstructionCache ();
-  ArmEnableDataCache ();
+  ArmSetTTBR0 (TranslationTable);
+
+  if (!ArmMmuEnabled ()) {
+    ArmDisableAlignmentCheck ();
+    ArmEnableStackAlignmentCheck ();
+    ArmEnableInstructionCache ();
+    ArmEnableDataCache ();
+
+    ArmEnableMmu ();
+  }
 
-  ArmEnableMmu ();
   return EFI_SUCCESS;
 
 FreeTranslationTable:
-- 
2.35.1


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

* [PATCH v3 08/16] ArmPkg/ArmMmuLib: Reuse XIP MMU routines when splitting entries
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
                   ` (6 preceding siblings ...)
  2022-09-26  8:25 ` [PATCH v3 07/16] ArmPkg/ArmMmuLib: permit initial configuration with MMU enabled Ard Biesheuvel
@ 2022-09-26  8:25 ` Ard Biesheuvel
  2022-09-26 22:38   ` Leif Lindholm
  2022-09-26  8:25 ` [PATCH v3 09/16] ArmPlatformPkg/PrePeiCore: permit entry with the MMU enabled Ard Biesheuvel
                   ` (7 subsequent siblings)
  15 siblings, 1 reply; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:25 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

In order to reduce the likelihood that we will need to rely on the logic
that disables and re-enables the MMU for updating a page table entry
safely, expose the XIP version of the helper routine via a HOB and use
it instead of the one that is copied into DRAM. Since the XIP copy is
already clean to the PoC, and will never end up getting unmapped during
a block entry split, we can use it safely without any cache maintenance,
and without running the risk of pulling the rug from under our feet when
updating an entry by going through an invalid mapping.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/ArmPkg.dec                                          |  2 ++
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c           | 27 ++++++++++++--------
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c | 17 ++++++++++++
 ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf                 |  4 +++
 ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf                  |  4 +++
 5 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index 9da1bbc9f216..cfb6fe602485 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -99,6 +99,8 @@ [Guids.common]
   # Include/Guid/ArmMpCoreInfo.h
   gArmMpCoreInfoGuid = { 0xa4ee0728, 0xe5d7, 0x4ac5,  {0xb2, 0x1e, 0x65, 0x8e, 0xd8, 0x57, 0xe8, 0x34} }
 
+  gArmMmuReplaceLiveTranslationEntryFuncGuid = { 0xa8b50ff3, 0x08ec, 0x4dd3, {0xbf, 0x04, 0x28, 0xbf, 0x71, 0x75, 0xc7, 0x4a} }
+
 [Protocols.common]
   ## Arm System Control and Management Interface(SCMI) Base protocol
   ## ArmPkg/Include/Protocol/ArmScmiBaseProtocol.h
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index ae59e9a7d04e..764c7d362e2e 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -10,6 +10,7 @@
 **/
 
 #include <Uefi.h>
+#include <Pi/PiMultiPhase.h>
 #include <Chipset/AArch64.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/CacheMaintenanceLib.h>
@@ -120,14 +121,14 @@ ReplaceTableEntry (
     // use an ordinary break before make. Otherwise, we will need to
     // temporarily disable the MMU.
     DisableMmu = FALSE;
-    if ((((RegionStart ^ (UINTN)ArmReplaceLiveTranslationEntry) & ~BlockMask) == 0) ||
+    if ((((RegionStart ^ (UINTN)mReplaceLiveEntryFunc) & ~BlockMask) == 0) ||
         (((RegionStart ^ (UINTN)Entry) & ~BlockMask) == 0))
     {
       DisableMmu = TRUE;
       DEBUG ((DEBUG_WARN, "%a: splitting block entry with MMU disabled\n", __FUNCTION__));
     }
 
-    ArmReplaceLiveTranslationEntry (Entry, Value, RegionStart, DisableMmu);
+    mReplaceLiveEntryFunc (Entry, Value, RegionStart, DisableMmu);
   }
 }
 
@@ -747,15 +748,21 @@ ArmMmuBaseLibConstructor (
   )
 {
   extern UINT32  ArmReplaceLiveTranslationEntrySize;
+  VOID           *Hob;
 
-  //
-  // The ArmReplaceLiveTranslationEntry () helper function may be invoked
-  // with the MMU off so we have to ensure that it gets cleaned to the PoC
-  //
-  WriteBackDataCacheRange (
-    (VOID *)(UINTN)ArmReplaceLiveTranslationEntry,
-    ArmReplaceLiveTranslationEntrySize
-    );
+  Hob = GetFirstGuidHob (&gArmMmuReplaceLiveTranslationEntryFuncGuid);
+  if (Hob != NULL) {
+    mReplaceLiveEntryFunc = *(VOID **)GET_GUID_HOB_DATA (Hob);
+  } else {
+    //
+    // The ArmReplaceLiveTranslationEntry () helper function may be invoked
+    // with the MMU off so we have to ensure that it gets cleaned to the PoC
+    //
+    WriteBackDataCacheRange (
+      (VOID *)(UINTN)ArmReplaceLiveTranslationEntry,
+      ArmReplaceLiveTranslationEntrySize
+      );
+  }
 
   return RETURN_SUCCESS;
 }
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
index caace2c17cdc..5f50a605a338 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
@@ -12,6 +12,7 @@
 #include <Library/ArmMmuLib.h>
 #include <Library/CacheMaintenanceLib.h>
 #include <Library/DebugLib.h>
+#include <Library/HobLib.h>
 
 EFI_STATUS
 EFIAPI
@@ -21,6 +22,8 @@ ArmMmuPeiLibConstructor (
   )
 {
   extern UINT32  ArmReplaceLiveTranslationEntrySize;
+  VOID           *ArmReplaceLiveTranslationEntryFunc;
+  VOID           *Hob;
 
   EFI_FV_FILE_INFO  FileInfo;
   EFI_STATUS        Status;
@@ -42,6 +45,20 @@ ArmMmuPeiLibConstructor (
        (UINTN)ArmReplaceLiveTranslationEntry + ArmReplaceLiveTranslationEntrySize))
   {
     DEBUG ((DEBUG_INFO, "ArmMmuLib: skipping cache maintenance on XIP PEIM\n"));
+
+    //
+    // Expose the XIP version of the ArmReplaceLiveTranslationEntry() routine
+    // via a HOB so we can fall back to it later when we need to split block
+    // mappings in a way that adheres to break-before-make requirements.
+    //
+    ArmReplaceLiveTranslationEntryFunc = ArmReplaceLiveTranslationEntry;
+
+    Hob = BuildGuidDataHob (
+            &gArmMmuReplaceLiveTranslationEntryFuncGuid,
+            &ArmReplaceLiveTranslationEntryFunc,
+            sizeof ArmReplaceLiveTranslationEntryFunc
+            );
+    ASSERT (Hob != NULL);
   } else {
     DEBUG ((DEBUG_INFO, "ArmMmuLib: performing cache maintenance on shadowed PEIM\n"));
     //
diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf b/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
index 3d78e7dabf47..57cb71f90ee3 100644
--- a/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
+++ b/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
@@ -36,7 +36,11 @@ [Packages]
 [LibraryClasses]
   ArmLib
   CacheMaintenanceLib
+  HobLib
   MemoryAllocationLib
 
+[Guids]
+  gArmMmuReplaceLiveTranslationEntryFuncGuid
+
 [Pcd.ARM]
   gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride
diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf b/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
index ce9674ea99ef..02f874a1a994 100644
--- a/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
+++ b/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
@@ -29,4 +29,8 @@ [Packages]
 [LibraryClasses]
   ArmLib
   CacheMaintenanceLib
+  HobLib
   MemoryAllocationLib
+
+[Guids]
+  gArmMmuReplaceLiveTranslationEntryFuncGuid
-- 
2.35.1


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

* [PATCH v3 09/16] ArmPlatformPkg/PrePeiCore: permit entry with the MMU enabled
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
                   ` (7 preceding siblings ...)
  2022-09-26  8:25 ` [PATCH v3 08/16] ArmPkg/ArmMmuLib: Reuse XIP MMU routines when splitting entries Ard Biesheuvel
@ 2022-09-26  8:25 ` Ard Biesheuvel
  2022-09-26 22:39   ` [edk2-devel] " Leif Lindholm
  2022-09-26  8:25 ` [PATCH v3 10/16] ArmVirtPkg/ArmVirtQemu: implement ArmPlatformLib with static ID map Ard Biesheuvel
                   ` (6 subsequent siblings)
  15 siblings, 1 reply; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:25 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

Some platforms may set up a preliminary ID map in flash and enter EFI
with the MMU and caches enabled, as this removes a lot of the complexity
around cache coherency. Let's take this into account, and avoid touching
the MMU controls or perform cache invalidation when the MMU is enabled
at entry.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPlatformPkg/PrePeiCore/PrePeiCore.c | 22 +++++++++++---------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
index 9c4b25df953d..8b86c6e69abd 100644
--- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
+++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
@@ -58,17 +58,19 @@ CEntryPoint (
   IN  EFI_PEI_CORE_ENTRY_POINT  PeiCoreEntryPoint
   )
 {
-  // Data Cache enabled on Primary core when MMU is enabled.
-  ArmDisableDataCache ();
-  // Invalidate instruction cache
-  ArmInvalidateInstructionCache ();
-  // Enable Instruction Caches on all cores.
-  ArmEnableInstructionCache ();
+  if (!ArmMmuEnabled ()) {
+    // Data Cache enabled on Primary core when MMU is enabled.
+    ArmDisableDataCache ();
+    // Invalidate instruction cache
+    ArmInvalidateInstructionCache ();
+    // Enable Instruction Caches on all cores.
+    ArmEnableInstructionCache ();
 
-  InvalidateDataCacheRange (
-    (VOID *)(UINTN)PcdGet64 (PcdCPUCoresStackBase),
-    PcdGet32 (PcdCPUCorePrimaryStackSize)
-    );
+    InvalidateDataCacheRange (
+      (VOID *)(UINTN)PcdGet64 (PcdCPUCoresStackBase),
+      PcdGet32 (PcdCPUCorePrimaryStackSize)
+      );
+  }
 
   //
   // Note: Doesn't have to Enable CPU interface in non-secure world,
-- 
2.35.1


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

* [PATCH v3 10/16] ArmVirtPkg/ArmVirtQemu: implement ArmPlatformLib with static ID map
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
                   ` (8 preceding siblings ...)
  2022-09-26  8:25 ` [PATCH v3 09/16] ArmPlatformPkg/PrePeiCore: permit entry with the MMU enabled Ard Biesheuvel
@ 2022-09-26  8:25 ` Ard Biesheuvel
  2022-09-26  8:25 ` [PATCH v3 11/16] ArmVirtPkg/ArmVirtQemu: use first 128 MiB as permanent PEI memory Ard Biesheuvel
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:25 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

To substantially reduce the amount of processing that takes place with
the MMU and caches off, implement a version of ArmPlatformLib specific
for QEMU/mach-virt in AArch64 mode that carries a statically allocated
and populated ID map that covers the NOR flash and device region, and
128 MiB of DRAM at the base of memory (0x4000_0000).

Note that 128 MiB has always been the minimum amount of DRAM we support
for this configuration, and the existing code already ASSERT()s in DEBUG
mode when booting with less.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S | 115 ++++++++++++++++++++
 ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.c        |  64 +++++++++++
 ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.inf      |  40 +++++++
 ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S                     |  57 ++++++++++
 4 files changed, 276 insertions(+)

diff --git a/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S b/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
new file mode 100644
index 000000000000..05ccc7f9f043
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
@@ -0,0 +1,115 @@
+//
+//  Copyright (c) 2022, Google LLC. All rights reserved.
+//
+//  SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+//
+
+#include <AsmMacroIoLibV8.h>
+
+  .macro mov_i, reg:req, imm:req
+  movz   \reg, :abs_g3:\imm
+  movk   \reg, :abs_g2_nc:\imm
+  movk   \reg, :abs_g1_nc:\imm
+  movk   \reg, :abs_g0_nc:\imm
+  .endm
+
+ .set    MAIR_DEV_nGnRnE,     0x00
+ .set    MAIR_MEM_NC,         0x44
+ .set    MAIR_MEM_WT,         0xbb
+ .set    MAIR_MEM_WBWA,       0xff
+ .set    mairval, MAIR_DEV_nGnRnE | (MAIR_MEM_NC << 8) | (MAIR_MEM_WT << 16) | (MAIR_MEM_WBWA << 24)
+
+ .set    TCR_TG0_4KB,         0x0 << 14
+ .set    TCR_TG1_4KB,         0x2 << 30
+ .set    TCR_IPS_SHIFT,       32
+ .set    TCR_EPD1,            0x1 << 23
+ .set    TCR_SH_INNER,        0x3 << 12
+ .set    TCR_RGN_OWB,         0x1 << 10
+ .set    TCR_RGN_IWB,         0x1 << 8
+ .set    tcrval, TCR_TG0_4KB | TCR_TG1_4KB | TCR_EPD1 | TCR_RGN_OWB
+ .set    tcrval, tcrval | TCR_RGN_IWB | TCR_SH_INNER
+
+ .set    SCTLR_ELx_I,         0x1 << 12
+ .set    SCTLR_ELx_SA,        0x1 << 3
+ .set    SCTLR_ELx_C,         0x1 << 2
+ .set    SCTLR_ELx_M,         0x1 << 0
+ .set    SCTLR_EL1_SPAN,      0x1 << 23
+ .set    SCTLR_EL1_WXN,       0x1 << 19
+ .set    SCTLR_EL1_SED,       0x1 << 8
+ .set    SCTLR_EL1_ITD,       0x1 << 7
+ .set    SCTLR_EL1_RES1,      (0x1 << 11) | (0x1 << 20) | (0x1 << 22) | (0x1 << 28) | (0x1 << 29)
+ .set    sctlrval, SCTLR_ELx_M | SCTLR_ELx_C | SCTLR_ELx_SA | SCTLR_EL1_ITD | SCTLR_EL1_SED
+ .set    sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES1
+
+
+ASM_FUNC(ArmPlatformPeiBootAction)
+  mrs    x0, CurrentEL           // check current exception level
+  tbz    x0, #3, 0f              // bail if above EL1
+  ret
+
+0:mov_i  x0, mairval
+  mov_i  x1, tcrval
+  adrp   x2, idmap
+  orr    x2, x2, #0xff << 48     // set non-zero ASID
+  mov_i  x3, sctlrval
+
+  mrs    x6, id_aa64mmfr0_el1    // get the supported PA range
+  and    x6, x6, #0xf            // isolate PArange bits
+  cmp    x6, #6                  // 0b0110 == 52 bits
+  sub    x6, x6, #1              // subtract 1
+  cinc   x6, x6, ne              // add back 1 unless PArange == 52 bits
+  bfi    x1, x6, #32, #3         // copy updated PArange into TCR_EL1.IPS
+
+  cmp    x6, #3                  // 0b0011 == 42 bits
+  sub    x6, x6, #1              // subtract 1
+  cinc   x6, x6, lt              // add back 1 unless VA range >= 42
+
+  mov    x7, #32
+  sub    x6, x7, x6, lsl #2      // T0SZ for PArange != 42
+  mov    x7, #64 - 42            // T0SZ for PArange == 42
+  csel   x6, x6, x7, ne
+  orr    x1, x1, x6              // set T0SZ field in TCR
+
+  cmp    x6, #64 - 40            // VA size < 40 bits?
+  add    x4, x2, #0x1000         // advance to level 1 descriptor
+  csel   x2, x4, x2, gt
+
+  msr    mair_el1, x0            // set up the 1:1 mapping
+  msr    tcr_el1, x1
+  msr    ttbr0_el1, x2
+  isb
+
+  tlbi   vmalle1                 // invalidate any cached translations
+  ic     iallu                   // invalidate the I-cache
+  dsb    nsh
+  isb
+
+  msr    sctlr_el1, x3           // enable MMU and caches
+  isb
+  ret
+
+//UINTN
+//ArmPlatformGetCorePosition (
+//  IN UINTN MpId
+//  );
+// With this function: CorePos = (ClusterId * 4) + CoreId
+ASM_FUNC(ArmPlatformGetCorePosition)
+  mov   x0, xzr
+  ret
+
+//UINTN
+//ArmPlatformGetPrimaryCoreMpId (
+//  VOID
+//  );
+ASM_FUNC(ArmPlatformGetPrimaryCoreMpId)
+  MOV32  (w0, FixedPcdGet32 (PcdArmPrimaryCore))
+  ret
+
+//UINTN
+//ArmPlatformIsPrimaryCore (
+//  IN UINTN MpId
+//  );
+ASM_FUNC(ArmPlatformIsPrimaryCore)
+  mov   x0, #1
+  ret
diff --git a/ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.c b/ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.c
new file mode 100644
index 000000000000..1de80422ee4c
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.c
@@ -0,0 +1,64 @@
+/** @file
+
+  Copyright (c) 2011-2012, ARM Limited. All rights reserved.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Library/ArmLib.h>
+#include <Library/ArmPlatformLib.h>
+
+/**
+  Return the current Boot Mode.
+
+  This function returns the boot reason on the platform
+
+  @return   Return the current Boot Mode of the platform
+
+**/
+EFI_BOOT_MODE
+ArmPlatformGetBootMode (
+  VOID
+  )
+{
+  return BOOT_WITH_FULL_CONFIGURATION;
+}
+
+/**
+  Initialize controllers that must setup in the normal world.
+
+  This function is called by the ArmPlatformPkg/PrePi or
+  ArmPlatformPkg/PlatformPei in the PEI phase.
+
+  @param[in]     MpId               ID of the calling CPU
+
+  @return        RETURN_SUCCESS unless the operation failed
+**/
+RETURN_STATUS
+ArmPlatformInitialize (
+  IN  UINTN  MpId
+  )
+{
+  return RETURN_SUCCESS;
+}
+
+/**
+  Return the Platform specific PPIs.
+
+  This function exposes the Platform Specific PPIs. They can be used by any
+  PrePi modules or passed to the PeiCore by PrePeiCore.
+
+  @param[out]   PpiListSize         Size in Bytes of the Platform PPI List
+  @param[out]   PpiList             Platform PPI List
+
+**/
+VOID
+ArmPlatformGetPlatformPpiList (
+  OUT UINTN                   *PpiListSize,
+  OUT EFI_PEI_PPI_DESCRIPTOR  **PpiList
+  )
+{
+  *PpiListSize = 0;
+  *PpiList     = NULL;
+}
diff --git a/ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.inf b/ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.inf
new file mode 100644
index 000000000000..b2ecdfa061cb
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.inf
@@ -0,0 +1,40 @@
+## @file
+#  ArmPlatformLib implementation for QEMU/mach-virt on AArch64 that contains a
+#  statically allocated 1:1 mapping of the first 128 MiB of DRAM, as well as
+#  the NOR flash and the device region
+#
+#  Copyright (c) 2011-2012, ARM Limited. All rights reserved.
+#  Copyright (c) 2022, Google LLC. All rights reserved.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 1.27
+  BASE_NAME                      = ArmPlatformLibQemu
+  FILE_GUID                      = 40af3a25-f02c-4aef-94ef-7ac0282d21d4
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = ArmPlatformLib
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  ArmPkg/ArmPkg.dec
+  ArmPlatformPkg/ArmPlatformPkg.dec
+
+[LibraryClasses]
+  ArmLib
+  DebugLib
+
+[Sources.common]
+  ArmPlatformLibQemu.c
+  IdMap.S
+
+[Sources.AArch64]
+  AArch64/ArmPlatformHelper.S
+
+[FixedPcd]
+  gArmTokenSpaceGuid.PcdArmPrimaryCoreMask
+  gArmTokenSpaceGuid.PcdArmPrimaryCore
diff --git a/ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S b/ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S
new file mode 100644
index 000000000000..4a4b7b77ed83
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+// Copyright 2022 Google LLC
+// Author: Ard Biesheuvel <ardb@google.com>
+
+  .set      TT_TYPE_BLOCK, 0x1
+  .set      TT_TYPE_PAGE,  0x3
+  .set      TT_TYPE_TABLE, 0x3
+
+  .set      TT_AF, 0x1 << 10
+  .set      TT_NG, 0x1 << 11
+  .set      TT_RO, 0x2 << 6
+  .set      TT_XN, 0x3 << 53
+
+  .set      TT_MT_DEV, 0x0 << 2                 // MAIR #0
+  .set      TT_MT_MEM, (0x3 << 2) | (0x3 << 8)  // MAIR #3
+
+  .set      PAGE_XIP,  TT_TYPE_PAGE  | TT_MT_MEM | TT_AF | TT_RO | TT_NG
+  .set      BLOCK_DEV, TT_TYPE_BLOCK | TT_MT_DEV | TT_AF | TT_XN | TT_NG
+  .set      BLOCK_MEM, TT_TYPE_BLOCK | TT_MT_MEM | TT_AF | TT_XN | TT_NG
+
+  .globl    idmap
+  .section  ".rodata.idmap", "a", %progbits
+  .align    12
+
+idmap:      /* level 0 */
+  .quad     1f + TT_TYPE_TABLE
+  .fill     511, 8, 0x0
+
+1:          /* level 1 */
+  .quad     20f + TT_TYPE_TABLE           // 1 GB of flash and device mappings
+  .quad     21f + TT_TYPE_TABLE           // up to 1 GB of DRAM
+  .fill     510, 8, 0x0                   // 510 GB of remaining VA space
+
+20:         /* level 2 */
+  .quad     3f + TT_TYPE_TABLE            // up to 2 MB of flash
+  .fill     63, 8, 0x0                    // 126 MB of unused flash
+  .set      idx, 64
+  .rept     448
+  .quad     BLOCK_DEV | (idx << 21)       // 896 MB of RW- device mappings
+  .set      idx, idx + 1
+  .endr
+
+21:         /* level 2 */
+  .set      idx, 0x40000000 >> 21
+  .rept     64
+  .quad     BLOCK_MEM | (idx << 21)       // 128 MB of RW- memory mappings
+  .set      idx, idx + 1
+  .endr
+  .fill     448, 8, 0x0
+
+3:          /* level 3 */
+  .quad     0x0                           // omit first 4k page
+  .set      idx, 1
+  .rept     511
+  .quad     PAGE_XIP | (idx << 12)        // 2044 KiB of R-X flash mappings
+  .set      idx, idx + 1
+  .endr
-- 
2.35.1


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

* [PATCH v3 11/16] ArmVirtPkg/ArmVirtQemu: use first 128 MiB as permanent PEI memory
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
                   ` (9 preceding siblings ...)
  2022-09-26  8:25 ` [PATCH v3 10/16] ArmVirtPkg/ArmVirtQemu: implement ArmPlatformLib with static ID map Ard Biesheuvel
@ 2022-09-26  8:25 ` Ard Biesheuvel
  2022-09-26  8:25 ` [PATCH v3 12/16] ArmVirtPkg/ArmVirtQemu: enable initial ID map at early boot Ard Biesheuvel
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:25 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

In order to allow booting with the MMU and caches enabled really early,
we need to ensure that the code that populates the page tables can
access those page tables with the statically defined ID map active.

So let's put the permanent PEI RAM in the first 128 MiB of memory, which
we will cover with this initial ID map (as it is the minimum supported
DRAM size for ArmVirtQemu).

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/MemoryInitPei/MemoryInitPeim.c   | 104 ++++++++++++++++++++
 ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf |  59 +++++++++++
 2 files changed, 163 insertions(+)

diff --git a/ArmVirtPkg/MemoryInitPei/MemoryInitPeim.c b/ArmVirtPkg/MemoryInitPei/MemoryInitPeim.c
new file mode 100644
index 000000000000..ef88a9df1d62
--- /dev/null
+++ b/ArmVirtPkg/MemoryInitPei/MemoryInitPeim.c
@@ -0,0 +1,104 @@
+/** @file
+
+  Copyright (c) 2011, ARM Limited. All rights reserved.
+  Copyright (c) 2022, Google LLC. All rights reserved.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <PiPei.h>
+#include <Library/ArmPlatformLib.h>
+#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
+#include <Library/PeimEntryPoint.h>
+#include <Library/PeiServicesLib.h>
+#include <Library/PcdLib.h>
+#include <Guid/MemoryTypeInformation.h>
+
+EFI_STATUS
+EFIAPI
+MemoryPeim (
+  IN EFI_PHYSICAL_ADDRESS  UefiMemoryBase,
+  IN UINT64                UefiMemorySize
+  );
+
+/**
+  Build the memory type information HOB that describes how many pages of each
+  type to preallocate when initializing the GCD memory map.
+**/
+VOID
+EFIAPI
+BuildMemoryTypeInformationHob (
+  VOID
+  )
+{
+  EFI_MEMORY_TYPE_INFORMATION  Info[10];
+
+  Info[0].Type          = EfiACPIReclaimMemory;
+  Info[0].NumberOfPages = FixedPcdGet32 (PcdMemoryTypeEfiACPIReclaimMemory);
+  Info[1].Type          = EfiACPIMemoryNVS;
+  Info[1].NumberOfPages = FixedPcdGet32 (PcdMemoryTypeEfiACPIMemoryNVS);
+  Info[2].Type          = EfiReservedMemoryType;
+  Info[2].NumberOfPages = FixedPcdGet32 (PcdMemoryTypeEfiReservedMemoryType);
+  Info[3].Type          = EfiRuntimeServicesData;
+  Info[3].NumberOfPages = FixedPcdGet32 (PcdMemoryTypeEfiRuntimeServicesData);
+  Info[4].Type          = EfiRuntimeServicesCode;
+  Info[4].NumberOfPages = FixedPcdGet32 (PcdMemoryTypeEfiRuntimeServicesCode);
+  Info[5].Type          = EfiBootServicesCode;
+  Info[5].NumberOfPages = FixedPcdGet32 (PcdMemoryTypeEfiBootServicesCode);
+  Info[6].Type          = EfiBootServicesData;
+  Info[6].NumberOfPages = FixedPcdGet32 (PcdMemoryTypeEfiBootServicesData);
+  Info[7].Type          = EfiLoaderCode;
+  Info[7].NumberOfPages = FixedPcdGet32 (PcdMemoryTypeEfiLoaderCode);
+  Info[8].Type          = EfiLoaderData;
+  Info[8].NumberOfPages = FixedPcdGet32 (PcdMemoryTypeEfiLoaderData);
+
+  // Terminator for the list
+  Info[9].Type          = EfiMaxMemoryType;
+  Info[9].NumberOfPages = 0;
+
+  BuildGuidDataHob (&gEfiMemoryTypeInformationGuid, &Info, sizeof (Info));
+}
+
+/**
+  Module entry point.
+
+  @param[in]    FileHandle    Handle of the file being invoked.
+  @param[in]    PeiServices   Describes the list of possible PEI Services.
+
+  @return       EFI_SUCCESS unless the operation failed.
+**/
+EFI_STATUS
+EFIAPI
+InitializeMemory (
+  IN       EFI_PEI_FILE_HANDLE  FileHandle,
+  IN CONST EFI_PEI_SERVICES     **PeiServices
+  )
+{
+  UINTN       UefiMemoryBase;
+  EFI_STATUS  Status;
+
+  ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) < (UINT64)MAX_ALLOC_ADDRESS);
+
+  //
+  // Put the permanent PEI memory in the first 128 MiB of DRAM so that
+  // it is covered by the statically configured ID map.
+  //
+  UefiMemoryBase = (UINTN)FixedPcdGet64 (PcdSystemMemoryBase) + SIZE_128MB
+                   - FixedPcdGet32 (PcdSystemMemoryUefiRegionSize);
+
+  Status = PeiServicesInstallPeiMemory (
+             UefiMemoryBase,
+             FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
+             );
+  ASSERT_EFI_ERROR (Status);
+
+  Status = MemoryPeim (
+             UefiMemoryBase,
+             FixedPcdGet32 (PcdSystemMemoryUefiRegionSize)
+             );
+  ASSERT_EFI_ERROR (Status);
+
+  return Status;
+}
diff --git a/ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf b/ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf
new file mode 100644
index 000000000000..2039f71a0ebe
--- /dev/null
+++ b/ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf
@@ -0,0 +1,59 @@
+## @file
+#  Implementation of MemoryInitPeim that uses the first 128 MiB at the base of
+#  DRAM as permanent PEI memory
+#
+#  Copyright (c) 2011-2014, ARM Ltd. All rights reserved.<BR>
+#  Copyright (c) 2022, Google LLC. All rights reserved.<BR>
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 1.27
+  BASE_NAME                      = MemoryInit
+  FILE_GUID                      = 0fbffd44-f98f-4e1c-9922-e9b21f13c3f8
+  MODULE_TYPE                    = PEIM
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = InitializeMemory
+
+[Sources]
+  MemoryInitPeim.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  EmbeddedPkg/EmbeddedPkg.dec
+  ArmPkg/ArmPkg.dec
+  ArmPlatformPkg/ArmPlatformPkg.dec
+
+[LibraryClasses]
+  PeimEntryPoint
+  DebugLib
+  HobLib
+  ArmLib
+  ArmPlatformLib
+  MemoryInitPeiLib
+
+[Guids]
+  gEfiMemoryTypeInformationGuid
+
+[FeaturePcd]
+  gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob
+
+[FixedPcd]
+  gArmTokenSpaceGuid.PcdSystemMemoryBase
+  gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize
+
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesCode
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode
+  gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData
+
+[Depex]
+  TRUE
-- 
2.35.1


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

* [PATCH v3 12/16] ArmVirtPkg/ArmVirtQemu: enable initial ID map at early boot
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
                   ` (10 preceding siblings ...)
  2022-09-26  8:25 ` [PATCH v3 11/16] ArmVirtPkg/ArmVirtQemu: use first 128 MiB as permanent PEI memory Ard Biesheuvel
@ 2022-09-26  8:25 ` Ard Biesheuvel
  2022-12-29 21:10   ` [edk2-devel] " dann frazier
  2022-09-26  8:25 ` [PATCH v3 13/16] ArmVirtPkg/ArmVirtQemu: Drop unused variable PEIM Ard Biesheuvel
                   ` (3 subsequent siblings)
  15 siblings, 1 reply; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:25 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

Now that we have all the pieces in place, switch the AArch64 version of
ArmVirtQemu to a mode where the first thing it does out of reset is
enable a preliminary ID map that covers the NOR flash and sufficient
DRAM to create the UEFI page tables as usual.

The advantage of this is that no manipulation of memory occurs any
longer before the MMU is enabled, which removes the need for explicit
coherency management, which is cumbersome and bad for performance.

It also means we no longer need to build all components that may execute
with the MMU off (including BASE libraries) with strict alignment.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc | 17 ++++++++++++++---
 ArmVirtPkg/ArmVirtQemu.fdf |  2 +-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 302c0d2a4e29..21a321e35794 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -63,8 +63,6 @@ [LibraryClasses.common]
   QemuFwCfgSimpleParserLib|OvmfPkg/Library/QemuFwCfgSimpleParserLib/QemuFwCfgSimpleParserLib.inf
   QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
 
-  ArmPlatformLib|ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf
-
   TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
   NorFlashPlatformLib|ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
 
@@ -92,6 +90,12 @@ [LibraryClasses.common]
   TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf
 !endif
 
+[LibraryClasses.AARCH64]
+  ArmPlatformLib|ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.inf
+
+[LibraryClasses.ARM]
+  ArmPlatformLib|ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf
+
 [LibraryClasses.common.PEIM]
   ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
 
@@ -112,6 +116,8 @@ [LibraryClasses.common.UEFI_DRIVER]
   UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
 
 [BuildOptions]
+  GCC:*_*_AARCH64_CC_XIPFLAGS = -mno-strict-align
+
 !include NetworkPkg/NetworkBuildOptions.dsc.inc
 
 ################################################################################
@@ -310,7 +316,12 @@ [Components.common]
       PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
   }
   ArmPlatformPkg/PlatformPei/PlatformPeim.inf
-  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
+  ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf {
+    <LibraryClasses>
+!if $(ARCH) == AARCH64
+      ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
+!endif
+  }
   ArmPkg/Drivers/CpuPei/CpuPei.inf
 
   MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
index b5e2253295fe..7f17aeb3ad0d 100644
--- a/ArmVirtPkg/ArmVirtQemu.fdf
+++ b/ArmVirtPkg/ArmVirtQemu.fdf
@@ -107,7 +107,7 @@ [FV.FVMAIN_COMPACT]
   INF ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
   INF MdeModulePkg/Core/Pei/PeiMain.inf
   INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
-  INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
+  INF ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf
   INF ArmPkg/Drivers/CpuPei/CpuPei.inf
   INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
   INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
-- 
2.35.1


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

* [PATCH v3 13/16] ArmVirtPkg/ArmVirtQemu: Drop unused variable PEIM
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
                   ` (11 preceding siblings ...)
  2022-09-26  8:25 ` [PATCH v3 12/16] ArmVirtPkg/ArmVirtQemu: enable initial ID map at early boot Ard Biesheuvel
@ 2022-09-26  8:25 ` Ard Biesheuvel
  2022-09-26  8:25 ` [PATCH v3 14/16] ArmVirtPkg/ArmVirtQemu: avoid shadowing PEIMs unless necessary Ard Biesheuvel
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:25 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

The variable PEIM is included in the build but its runtime prerequisites
are absent so it is never dispatched. Just drop it.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc | 2 --
 ArmVirtPkg/ArmVirtQemu.fdf | 1 -
 2 files changed, 3 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 21a321e35794..de34481673ea 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -324,8 +324,6 @@ [Components.common]
   }
   ArmPkg/Drivers/CpuPei/CpuPei.inf
 
-  MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
-
 !if $(TPM2_ENABLE) == TRUE
   MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf {
     <LibraryClasses>
diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
index 7f17aeb3ad0d..c85e36b185d3 100644
--- a/ArmVirtPkg/ArmVirtQemu.fdf
+++ b/ArmVirtPkg/ArmVirtQemu.fdf
@@ -110,7 +110,6 @@ [FV.FVMAIN_COMPACT]
   INF ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf
   INF ArmPkg/Drivers/CpuPei/CpuPei.inf
   INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
-  INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
   INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
 
 !if $(TPM2_ENABLE) == TRUE
-- 
2.35.1


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

* [PATCH v3 14/16] ArmVirtPkg/ArmVirtQemu: avoid shadowing PEIMs unless necessary
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
                   ` (12 preceding siblings ...)
  2022-09-26  8:25 ` [PATCH v3 13/16] ArmVirtPkg/ArmVirtQemu: Drop unused variable PEIM Ard Biesheuvel
@ 2022-09-26  8:25 ` Ard Biesheuvel
  2022-09-26  8:25 ` [PATCH v3 15/16] ArmVirtPkg/QemuVirtMemInfoLib: use HOB not PCD to record the memory size Ard Biesheuvel
  2022-09-26  8:25 ` [PATCH v3 16/16] ArmVirtPkg/ArmVirtQemu: omit PCD PEIM unless TPM support is enabled Ard Biesheuvel
  15 siblings, 0 replies; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:25 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

Some PEIMs register for shadow execution explicitly, but others exist
that don't care and can happily execute in place. Since the emulated NOR
flash is just RAM, shadowing has no performance benefits so let's only
do this if needed.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index de34481673ea..c3d264077bce 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -217,6 +217,9 @@ [PcdsFixedAtBuild.common]
   gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|3
   gEfiShellPkgTokenSpaceGuid.PcdShellFileOperationSize|0x20000
 
+  # Shadowing PEI modules is absolutely pointless when the NOR flash is emulated
+  gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot|FALSE
+
 [PcdsFixedAtBuild.AARCH64]
   # Clearing BIT0 in this PCD prevents installing a 32-bit SMBIOS entry point,
   # if the entry point version is >= 3.0. AARCH64 OSes cannot assume the
-- 
2.35.1


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

* [PATCH v3 15/16] ArmVirtPkg/QemuVirtMemInfoLib: use HOB not PCD to record the memory size
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
                   ` (13 preceding siblings ...)
  2022-09-26  8:25 ` [PATCH v3 14/16] ArmVirtPkg/ArmVirtQemu: avoid shadowing PEIMs unless necessary Ard Biesheuvel
@ 2022-09-26  8:25 ` Ard Biesheuvel
  2022-09-26  8:25 ` [PATCH v3 16/16] ArmVirtPkg/ArmVirtQemu: omit PCD PEIM unless TPM support is enabled Ard Biesheuvel
  15 siblings, 0 replies; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:25 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

Due to the way we inherited the formerly fixed PCDs to describe the
system memory base and size from ArmPlatformPkg, we ended up with a
MemoryInit PEIM that relies on dynamic PCDs to communicate the size of
system memory between the constructur of one of its library dependencies
and the core module. This is unnecessary, and forces us to incorporate
the PCD PEIM as well, for no good reason. So instead, let's use a HOB.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirtPkg.dec                                                |  1 +
 ArmVirtPkg/ArmVirtQemu.dsc                                               |  6 ++--
 ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c     | 14 ++++++--
 ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf   |  1 +
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c               | 35 ++++++++++++++++++--
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf             |  5 ++-
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf          |  8 ++---
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c | 30 ++++++++++-------
 8 files changed, 75 insertions(+), 25 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index 4e165f6cd845..89d21ec3a364 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -32,6 +32,7 @@ [Guids.common]
   gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E, 0x2E, 0x36, 0x5B, 0x80, 0x63, 0x66 } }
   gEarlyPL011BaseAddressGuid       = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80, 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } }
   gEarly16550UartBaseAddressGuid   = { 0xea67ca3e, 0x1f54, 0x436b, { 0x97, 0x88, 0xd4, 0xeb, 0x29, 0xc3, 0x42, 0x67 } }
+  gArmVirtSystemMemorySizeGuid     = { 0x504eccb9, 0x1bf0, 0x4420, { 0x86, 0x5d, 0xdc, 0x66, 0x06, 0xd4, 0x13, 0xbf } }
 
   gArmVirtVariableGuid   = { 0x50bea1e5, 0xa2c5, 0x46e9, { 0x9b, 0x3a, 0x59, 0x59, 0x65, 0x16, 0xb0, 0x0a } }
 
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index c3d264077bce..43e19f605084 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -220,6 +220,9 @@ [PcdsFixedAtBuild.common]
   # Shadowing PEI modules is absolutely pointless when the NOR flash is emulated
   gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot|FALSE
 
+  # System Memory Size -- 128 MB initially, actual size will be fetched from DT
+  gArmTokenSpaceGuid.PcdSystemMemorySize|0x8000000
+
 [PcdsFixedAtBuild.AARCH64]
   # Clearing BIT0 in this PCD prevents installing a 32-bit SMBIOS entry point,
   # if the entry point version is >= 3.0. AARCH64 OSes cannot assume the
@@ -236,9 +239,6 @@ [PcdsDynamicDefault.common]
   #  enumeration to complete before installing ACPI tables.
   gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration|TRUE
 
-  # System Memory Size -- 1 MB initially, actual size will be fetched from DT
-  gArmTokenSpaceGuid.PcdSystemMemorySize|0x00100000
-
   gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum|0x0
   gArmTokenSpaceGuid.PcdArmArchTimerIntrNum|0x0
   gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum|0x0
diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
index 98d90ad4200d..72e5c65af79e 100644
--- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
+++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
@@ -52,10 +52,19 @@ MemoryPeim (
 {
   EFI_RESOURCE_ATTRIBUTE_TYPE  ResourceAttributes;
   UINT64                       SystemMemoryTop;
+  UINT64                       SystemMemorySize;
+  VOID                         *Hob;
 
   // Ensure PcdSystemMemorySize has been set
   ASSERT (PcdGet64 (PcdSystemMemorySize) != 0);
 
+  SystemMemorySize = PcdGet64 (PcdSystemMemorySize);
+
+  Hob = GetFirstGuidHob (&gArmVirtSystemMemorySizeGuid);
+  if (Hob != NULL) {
+    SystemMemorySize = *(UINT64 *)GET_GUID_HOB_DATA (Hob);
+  }
+
   //
   // Now, the permanent memory has been installed, we can call AllocatePages()
   //
@@ -66,8 +75,7 @@ MemoryPeim (
                         EFI_RESOURCE_ATTRIBUTE_TESTED
                         );
 
-  SystemMemoryTop = PcdGet64 (PcdSystemMemoryBase) +
-                    PcdGet64 (PcdSystemMemorySize);
+  SystemMemoryTop = PcdGet64 (PcdSystemMemoryBase) + SystemMemorySize;
 
   if (SystemMemoryTop - 1 > MAX_ALLOC_ADDRESS) {
     BuildResourceDescriptorHob (
@@ -87,7 +95,7 @@ MemoryPeim (
       EFI_RESOURCE_SYSTEM_MEMORY,
       ResourceAttributes,
       PcdGet64 (PcdSystemMemoryBase),
-      PcdGet64 (PcdSystemMemorySize)
+      SystemMemorySize
       );
   }
 
diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
index 21327f79f4b9..48d9c66b22db 100644
--- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
+++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
@@ -34,6 +34,7 @@ [LibraryClasses]
   CacheMaintenanceLib
 
 [Guids]
+  gArmVirtSystemMemorySizeGuid
   gEfiMemoryTypeInformationGuid
 
 [FeaturePcd]
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
index cf569bed99c4..9cf43f06c073 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
@@ -6,10 +6,12 @@
 
 **/
 
-#include <Base.h>
+#include <Uefi.h>
+#include <Pi/PiMultiPhase.h>
 #include <Library/ArmLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
+#include <Library/HobLib.h>
 #include <Library/MemoryAllocationLib.h>
 
 // Number of Virtual Memory Map Descriptors
@@ -24,6 +26,28 @@
 #define MACH_VIRT_PERIPH_BASE  0x08000000
 #define MACH_VIRT_PERIPH_SIZE  SIZE_128MB
 
+/**
+  Default library constructur that obtains the memory size from a PCD.
+
+  @return  Always returns RETURN_SUCCESS
+
+**/
+RETURN_STATUS
+EFIAPI
+QemuVirtMemInfoLibConstructor (
+  VOID
+  )
+{
+  UINT64  Size;
+  VOID    *Hob;
+
+  Size = PcdGet64 (PcdSystemMemorySize);
+  Hob  = BuildGuidDataHob (&gArmVirtSystemMemorySizeGuid, &Size, sizeof Size);
+  ASSERT (Hob != NULL);
+
+  return RETURN_SUCCESS;
+}
+
 /**
   Return the Virtual Memory Map of your platform
 
@@ -43,9 +67,16 @@ ArmVirtGetMemoryMap (
   )
 {
   ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
+  VOID                          *MemorySizeHob;
 
   ASSERT (VirtualMemoryMap != NULL);
 
+  MemorySizeHob = GetFirstGuidHob (&gArmVirtSystemMemorySizeGuid);
+  ASSERT (MemorySizeHob != NULL);
+  if (MemorySizeHob == NULL) {
+    return;
+  }
+
   VirtualMemoryTable = AllocatePool (
                          sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *
                          MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS
@@ -59,7 +90,7 @@ ArmVirtGetMemoryMap (
   // System DRAM
   VirtualMemoryTable[0].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
   VirtualMemoryTable[0].VirtualBase  = VirtualMemoryTable[0].PhysicalBase;
-  VirtualMemoryTable[0].Length       = PcdGet64 (PcdSystemMemorySize);
+  VirtualMemoryTable[0].Length       = *(UINT64 *)GET_GUID_HOB_DATA (MemorySizeHob);
   VirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
 
   DEBUG ((
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
index 7150de6c10d0..6acad8bbd7f3 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
@@ -14,6 +14,7 @@ [Defines]
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
   LIBRARY_CLASS                  = ArmVirtMemInfoLib
+  CONSTRUCTOR                    = QemuVirtMemInfoLibConstructor
 
 [Sources]
   QemuVirtMemInfoLib.c
@@ -30,7 +31,9 @@ [LibraryClasses]
   BaseMemoryLib
   DebugLib
   MemoryAllocationLib
-  PcdLib
+
+[Guids]
+  gArmVirtSystemMemorySizeGuid
 
 [Pcd]
   gArmTokenSpaceGuid.PcdFvBaseAddress
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
index 7ecf6dfbb786..f045e39a4131 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
@@ -32,16 +32,16 @@ [LibraryClasses]
   BaseMemoryLib
   DebugLib
   FdtLib
-  PcdLib
   MemoryAllocationLib
 
-[Pcd]
+[Guids]
+  gArmVirtSystemMemorySizeGuid
+
+[FixedPcd]
   gArmTokenSpaceGuid.PcdFdBaseAddress
   gArmTokenSpaceGuid.PcdFvBaseAddress
   gArmTokenSpaceGuid.PcdSystemMemoryBase
   gArmTokenSpaceGuid.PcdSystemMemorySize
-
-[FixedPcd]
   gArmTokenSpaceGuid.PcdFdSize
   gArmTokenSpaceGuid.PcdFvSize
   gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c
index 33d3597d57ab..c47ab8296622 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c
@@ -6,9 +6,10 @@
 
 **/
 
-#include <Base.h>
+#include <Uefi.h>
+#include <Pi/PiMultiPhase.h>
 #include <Library/DebugLib.h>
-#include <Library/PcdLib.h>
+#include <Library/HobLib.h>
 #include <libfdt.h>
 
 RETURN_STATUS
@@ -17,14 +18,14 @@ QemuVirtMemInfoPeiLibConstructor (
   VOID
   )
 {
-  VOID           *DeviceTreeBase;
-  INT32          Node, Prev;
-  UINT64         NewBase, CurBase;
-  UINT64         NewSize, CurSize;
-  CONST CHAR8    *Type;
-  INT32          Len;
-  CONST UINT64   *RegProp;
-  RETURN_STATUS  PcdStatus;
+  VOID          *DeviceTreeBase;
+  INT32         Node, Prev;
+  UINT64        NewBase, CurBase;
+  UINT64        NewSize, CurSize;
+  CONST CHAR8   *Type;
+  INT32         Len;
+  CONST UINT64  *RegProp;
+  VOID          *Hob;
 
   NewBase = 0;
   NewSize = 0;
@@ -86,8 +87,13 @@ QemuVirtMemInfoPeiLibConstructor (
   // Make sure the start of DRAM matches our expectation
   //
   ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
-  PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
-  ASSERT_RETURN_ERROR (PcdStatus);
+
+  Hob = BuildGuidDataHob (
+          &gArmVirtSystemMemorySizeGuid,
+          &NewSize,
+          sizeof NewSize
+          );
+  ASSERT (Hob != NULL);
 
   //
   // We need to make sure that the machine we are running on has at least
-- 
2.35.1


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

* [PATCH v3 16/16] ArmVirtPkg/ArmVirtQemu: omit PCD PEIM unless TPM support is enabled
  2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
                   ` (14 preceding siblings ...)
  2022-09-26  8:25 ` [PATCH v3 15/16] ArmVirtPkg/QemuVirtMemInfoLib: use HOB not PCD to record the memory size Ard Biesheuvel
@ 2022-09-26  8:25 ` Ard Biesheuvel
  15 siblings, 0 replies; 49+ messages in thread
From: Ard Biesheuvel @ 2022-09-26  8:25 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Leif Lindholm, Alexander Graf

The TPM discovery code relies on a dynamic PCD to communicate the TPM
base address to other components. But no other code relies on dynamic
PCDs in the PEI phase so let's drop the PCD PEIM when TPM support is not
enabled.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc | 22 +++++++++++++++-----
 ArmVirtPkg/ArmVirtQemu.fdf |  2 +-
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 43e19f605084..842a298e0435 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -287,10 +287,15 @@ [PcdsDynamicDefault.common]
   #
   # TPM2 support
   #
-  gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress|0x0
 !if $(TPM2_ENABLE) == TRUE
+  gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress|0x0
   gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
   gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask|0
+!else
+[PcdsPatchableInModule]
+  # make this PCD patchable instead of dynamic when TPM support is not enabled
+  # this permits setting the PCD in unreachable code without pulling in dynamic PCD support
+  gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress|0x0
 !endif
 
 [PcdsDynamicHii]
@@ -303,6 +308,13 @@ [PcdsDynamicHii]
 
   gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|5
 
+[LibraryClasses.common.PEI_CORE, LibraryClasses.common.PEIM]
+!if $(TPM2_ENABLE) == TRUE
+  PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
+!else
+  PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+!endif
+
 ################################################################################
 #
 # Components Section - list of all EDK II Modules needed by this Platform
@@ -314,10 +326,6 @@ [Components.common]
   #
   ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
   MdeModulePkg/Core/Pei/PeiMain.inf
-  MdeModulePkg/Universal/PCD/Pei/Pcd.inf {
-    <LibraryClasses>
-      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
-  }
   ArmPlatformPkg/PlatformPei/PlatformPeim.inf
   ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf {
     <LibraryClasses>
@@ -328,6 +336,10 @@ [Components.common]
   ArmPkg/Drivers/CpuPei/CpuPei.inf
 
 !if $(TPM2_ENABLE) == TRUE
+  MdeModulePkg/Universal/PCD/Pei/Pcd.inf {
+    <LibraryClasses>
+      PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+  }
   MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf {
     <LibraryClasses>
       ResetSystemLib|ArmVirtPkg/Library/ArmVirtPsciResetSystemPeiLib/ArmVirtPsciResetSystemPeiLib.inf
diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
index c85e36b185d3..764f652afd0e 100644
--- a/ArmVirtPkg/ArmVirtQemu.fdf
+++ b/ArmVirtPkg/ArmVirtQemu.fdf
@@ -109,10 +109,10 @@ [FV.FVMAIN_COMPACT]
   INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
   INF ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf
   INF ArmPkg/Drivers/CpuPei/CpuPei.inf
-  INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
   INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
 
 !if $(TPM2_ENABLE) == TRUE
+  INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
   INF MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf
   INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
   INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
-- 
2.35.1


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2022-09-26  8:24 ` [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable Ard Biesheuvel
@ 2022-09-26 22:28   ` Leif Lindholm
  2022-11-28 15:46   ` Gerd Hoffmann
  1 sibling, 0 replies; 49+ messages in thread
From: Leif Lindholm @ 2022-09-26 22:28 UTC (permalink / raw)
  To: devel, ardb; +Cc: Alexander Graf

On 2022-09-26 01:24, Ard Biesheuvel wrote:
> When the memory protections were implemented and enabled on ArmVirtQemu
> 5+ years ago, we had to work around the fact that GRUB at the time
> expected EFI_LOADER_DATA to be executable, as that is the memory type it
> allocates when loading its modules.
> 
> This has been fixed in GRUB in August 2017, so by now, we should be able
> to tighten this, and remove execute permissions from EFI_LOADER_DATA
> allocations.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>



> ---
>   ArmVirtPkg/ArmVirt.dsc.inc | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 34575585adbb..462073517a22 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -368,7 +368,7 @@ [PcdsFixedAtBuild.common]
>     # reserved ones, with the exception of LoaderData regions, of which OS loaders
> 
>     # (i.e., GRUB) may assume that its contents are executable.
> 

Should the comment be updated too ("old versions of GRUB")?

Regardless:
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

/
     Leif

>     #
> 
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
> 
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD5
> 
>   
> 
>   [Components.common]
> 
>     #
> 


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

* Re: [PATCH v3 05/16] ArmPkg/ArmMmuLib: don't replace table entries with block entries
  2022-09-26  8:25 ` [PATCH v3 05/16] ArmPkg/ArmMmuLib: don't replace table entries with block entries Ard Biesheuvel
@ 2022-09-26 22:32   ` Leif Lindholm
  0 siblings, 0 replies; 49+ messages in thread
From: Leif Lindholm @ 2022-09-26 22:32 UTC (permalink / raw)
  To: Ard Biesheuvel, devel; +Cc: Alexander Graf

On 2022-09-26 01:25, Ard Biesheuvel wrote:
> Drop the optimization that replaces table entries with block entries and
> frees the page tables in the subhierarchy that is being replaced. This
> rarely occurs in practice anyway, and will require more elaborate TLB
> maintenance once we switch to a different approach when running at EL1,
> where we no longer disable the MMU and nuke the TLB entirely every time
> we update a descriptor in a way that requires break-before-make (BBM).
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

/
     Leif

> ---
>   ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 20 ++------------------
>   1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index e5ecc7375153..34f1031c4de3 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -197,12 +197,9 @@ UpdateRegionMappingRecursive (
>       // 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 we are changing a table entry and the AttributeClearMask is non-zero,
> 
> -    // we cannot replace it with a block entry without potentially losing
> 
> -    // attribute information, so keep the table entry in that case.
> 
>       //
> 
>       if ((Level == 0) || (((RegionStart | BlockEnd) & BlockMask) != 0) ||
> 
> -        (IsTableEntry (*Entry, Level) && (AttributeClearMask != 0)))
> 
> +        IsTableEntry (*Entry, Level))
> 
>       {
> 
>         ASSERT (Level < 3);
> 
>   
> 
> @@ -294,20 +291,7 @@ UpdateRegionMappingRecursive (
>         EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
> 
>                                    : TT_TYPE_BLOCK_ENTRY;
> 
>   
> 
> -      if (IsTableEntry (*Entry, Level)) {
> 
> -        //
> 
> -        // We are replacing a table entry with a block entry. This is only
> 
> -        // possible if we are keeping none of the original attributes.
> 
> -        // We can free the table entry's page table, and all the ones below
> 
> -        // it, since we are dropping the only possible reference to it.
> 
> -        //
> 
> -        ASSERT (AttributeClearMask == 0);
> 
> -        TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
> 
> -        ReplaceTableEntry (Entry, EntryValue, RegionStart, TRUE);
> 
> -        FreePageTablesRecursive (TranslationTable, Level + 1);
> 
> -      } else {
> 
> -        ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> 
> -      }
> 
> +      ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> 
>       }
> 
>     }
> 
>   
> 


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

* Re: [PATCH v3 07/16] ArmPkg/ArmMmuLib: permit initial configuration with MMU enabled
  2022-09-26  8:25 ` [PATCH v3 07/16] ArmPkg/ArmMmuLib: permit initial configuration with MMU enabled Ard Biesheuvel
@ 2022-09-26 22:35   ` Leif Lindholm
  0 siblings, 0 replies; 49+ messages in thread
From: Leif Lindholm @ 2022-09-26 22:35 UTC (permalink / raw)
  To: Ard Biesheuvel, devel; +Cc: Alexander Graf

On 2022-09-26 01:25, Ard Biesheuvel wrote:
> Permit the use of this library with the MMU and caches already enabled.
> This removes the need for any cache maintenance for coherency, and is
> generally better for robustness and performance, especially when running
> under virtualization.
> 
> Note that this means we have to defer assignment of TTBR0 until the
> page tables are ready to be used, and so UpdateRegionMapping() can no
> longer read back TTBR0 directly to discover the root table address.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

/
     Leif

> ---
>   ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 48 +++++++++++---------
>   1 file changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 4d75788ed2b2..ae59e9a7d04e 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -357,6 +357,7 @@ UpdateRegionMapping (
>     IN  UINT64   RegionLength,
> 
>     IN  UINT64   AttributeSetMask,
> 
>     IN  UINT64   AttributeClearMask,
> 
> +  IN  UINT64   *RootTable,
> 
>     IN  BOOLEAN  TableIsLive
> 
>     )
> 
>   {
> 
> @@ -373,7 +374,7 @@ UpdateRegionMapping (
>              RegionStart + RegionLength,
> 
>              AttributeSetMask,
> 
>              AttributeClearMask,
> 
> -           ArmGetTTBR0BaseAddress (),
> 
> +           RootTable,
> 
>              GetRootTableLevel (T0SZ),
> 
>              TableIsLive
> 
>              );
> 
> @@ -391,6 +392,7 @@ FillTranslationTable (
>              MemoryRegion->Length,
> 
>              ArmMemoryAttributeToPageAttribute (MemoryRegion->Attributes) | TT_AF,
> 
>              0,
> 
> +           RootTable,
> 
>              FALSE
> 
>              );
> 
>   }
> 
> @@ -466,6 +468,7 @@ ArmSetMemoryAttributes (
>              Length,
> 
>              PageAttributes,
> 
>              PageAttributeMask,
> 
> +           ArmGetTTBR0BaseAddress (),
> 
>              TRUE
> 
>              );
> 
>   }
> 
> @@ -484,6 +487,7 @@ SetMemoryRegionAttribute (
>              Length,
> 
>              Attributes,
> 
>              BlockEntryMask,
> 
> +           ArmGetTTBR0BaseAddress (),
> 
>              TRUE
> 
>              );
> 
>   }
> 
> @@ -675,14 +679,6 @@ ArmConfigureMmu (
>       return EFI_OUT_OF_RESOURCES;
> 
>     }
> 
>   
> 
> -  //
> 
> -  // We set TTBR0 just after allocating the table to retrieve its location from
> 
> -  // the subsequent functions without needing to pass this value across the
> 
> -  // functions. The MMU is only enabled after the translation tables are
> 
> -  // populated.
> 
> -  //
> 
> -  ArmSetTTBR0 (TranslationTable);
> 
> -
> 
>     if (TranslationTableBase != NULL) {
> 
>       *TranslationTableBase = TranslationTable;
> 
>     }
> 
> @@ -691,14 +687,17 @@ ArmConfigureMmu (
>       *TranslationTableSize = RootTableEntryCount * sizeof (UINT64);
> 
>     }
> 
>   
> 
> -  //
> 
> -  // Make sure we are not inadvertently hitting in the caches
> 
> -  // when populating the page tables.
> 
> -  //
> 
> -  InvalidateDataCacheRange (
> 
> -    TranslationTable,
> 
> -    RootTableEntryCount * sizeof (UINT64)
> 
> -    );
> 
> +  if (!ArmMmuEnabled ()) {
> 
> +    //
> 
> +    // Make sure we are not inadvertently hitting in the caches
> 
> +    // when populating the page tables.
> 
> +    //
> 
> +    InvalidateDataCacheRange (
> 
> +      TranslationTable,
> 
> +      RootTableEntryCount * sizeof (UINT64)
> 
> +      );
> 
> +  }
> 
> +
> 
>     ZeroMem (TranslationTable, RootTableEntryCount * sizeof (UINT64));
> 
>   
> 
>     while (MemoryTable->Length != 0) {
> 
> @@ -723,12 +722,17 @@ ArmConfigureMmu (
>       MAIR_ATTR (TT_ATTR_INDX_MEMORY_WRITE_BACK, MAIR_ATTR_NORMAL_MEMORY_WRITE_BACK)
> 
>       );
> 
>   
> 
> -  ArmDisableAlignmentCheck ();
> 
> -  ArmEnableStackAlignmentCheck ();
> 
> -  ArmEnableInstructionCache ();
> 
> -  ArmEnableDataCache ();
> 
> +  ArmSetTTBR0 (TranslationTable);
> 
> +
> 
> +  if (!ArmMmuEnabled ()) {
> 
> +    ArmDisableAlignmentCheck ();
> 
> +    ArmEnableStackAlignmentCheck ();
> 
> +    ArmEnableInstructionCache ();
> 
> +    ArmEnableDataCache ();
> 
> +
> 
> +    ArmEnableMmu ();
> 
> +  }
> 
>   
> 
> -  ArmEnableMmu ();
> 
>     return EFI_SUCCESS;
> 
>   
> 
>   FreeTranslationTable:
> 


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

* Re: [PATCH v3 08/16] ArmPkg/ArmMmuLib: Reuse XIP MMU routines when splitting entries
  2022-09-26  8:25 ` [PATCH v3 08/16] ArmPkg/ArmMmuLib: Reuse XIP MMU routines when splitting entries Ard Biesheuvel
@ 2022-09-26 22:38   ` Leif Lindholm
  0 siblings, 0 replies; 49+ messages in thread
From: Leif Lindholm @ 2022-09-26 22:38 UTC (permalink / raw)
  To: Ard Biesheuvel, devel; +Cc: Alexander Graf

On 2022-09-26 01:25, Ard Biesheuvel wrote:
> In order to reduce the likelihood that we will need to rely on the logic
> that disables and re-enables the MMU for updating a page table entry
> safely, expose the XIP version of the helper routine via a HOB and use
> it instead of the one that is copied into DRAM. Since the XIP copy is
> already clean to the PoC, and will never end up getting unmapped during
> a block entry split, we can use it safely without any cache maintenance,
> and without running the risk of pulling the rug from under our feet when
> updating an entry by going through an invalid mapping.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Acked-by: Leif Lindholm <quic_llindhol@quicinc.com>

/
     Leif

> ---
>   ArmPkg/ArmPkg.dec                                          |  2 ++
>   ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c           | 27 ++++++++++++--------
>   ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c | 17 ++++++++++++
>   ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf                 |  4 +++
>   ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf                  |  4 +++
>   5 files changed, 44 insertions(+), 10 deletions(-)
> 
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index 9da1bbc9f216..cfb6fe602485 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -99,6 +99,8 @@ [Guids.common]
>     # Include/Guid/ArmMpCoreInfo.h
> 
>     gArmMpCoreInfoGuid = { 0xa4ee0728, 0xe5d7, 0x4ac5,  {0xb2, 0x1e, 0x65, 0x8e, 0xd8, 0x57, 0xe8, 0x34} }
> 
>   
> 
> +  gArmMmuReplaceLiveTranslationEntryFuncGuid = { 0xa8b50ff3, 0x08ec, 0x4dd3, {0xbf, 0x04, 0x28, 0xbf, 0x71, 0x75, 0xc7, 0x4a} }
> 
> +
> 
>   [Protocols.common]
> 
>     ## Arm System Control and Management Interface(SCMI) Base protocol
> 
>     ## ArmPkg/Include/Protocol/ArmScmiBaseProtocol.h
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index ae59e9a7d04e..764c7d362e2e 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -10,6 +10,7 @@
>   **/
> 
>   
> 
>   #include <Uefi.h>
> 
> +#include <Pi/PiMultiPhase.h>
> 
>   #include <Chipset/AArch64.h>
> 
>   #include <Library/BaseMemoryLib.h>
> 
>   #include <Library/CacheMaintenanceLib.h>
> 
> @@ -120,14 +121,14 @@ ReplaceTableEntry (
>       // use an ordinary break before make. Otherwise, we will need to
> 
>       // temporarily disable the MMU.
> 
>       DisableMmu = FALSE;
> 
> -    if ((((RegionStart ^ (UINTN)ArmReplaceLiveTranslationEntry) & ~BlockMask) == 0) ||
> 
> +    if ((((RegionStart ^ (UINTN)mReplaceLiveEntryFunc) & ~BlockMask) == 0) ||
> 
>           (((RegionStart ^ (UINTN)Entry) & ~BlockMask) == 0))
> 
>       {
> 
>         DisableMmu = TRUE;
> 
>         DEBUG ((DEBUG_WARN, "%a: splitting block entry with MMU disabled\n", __FUNCTION__));
> 
>       }
> 
>   
> 
> -    ArmReplaceLiveTranslationEntry (Entry, Value, RegionStart, DisableMmu);
> 
> +    mReplaceLiveEntryFunc (Entry, Value, RegionStart, DisableMmu);
> 
>     }
> 
>   }
> 
>   
> 
> @@ -747,15 +748,21 @@ ArmMmuBaseLibConstructor (
>     )
> 
>   {
> 
>     extern UINT32  ArmReplaceLiveTranslationEntrySize;
> 
> +  VOID           *Hob;
> 
>   
> 
> -  //
> 
> -  // The ArmReplaceLiveTranslationEntry () helper function may be invoked
> 
> -  // with the MMU off so we have to ensure that it gets cleaned to the PoC
> 
> -  //
> 
> -  WriteBackDataCacheRange (
> 
> -    (VOID *)(UINTN)ArmReplaceLiveTranslationEntry,
> 
> -    ArmReplaceLiveTranslationEntrySize
> 
> -    );
> 
> +  Hob = GetFirstGuidHob (&gArmMmuReplaceLiveTranslationEntryFuncGuid);
> 
> +  if (Hob != NULL) {
> 
> +    mReplaceLiveEntryFunc = *(VOID **)GET_GUID_HOB_DATA (Hob);
> 
> +  } else {
> 
> +    //
> 
> +    // The ArmReplaceLiveTranslationEntry () helper function may be invoked
> 
> +    // with the MMU off so we have to ensure that it gets cleaned to the PoC
> 
> +    //
> 
> +    WriteBackDataCacheRange (
> 
> +      (VOID *)(UINTN)ArmReplaceLiveTranslationEntry,
> 
> +      ArmReplaceLiveTranslationEntrySize
> 
> +      );
> 
> +  }
> 
>   
> 
>     return RETURN_SUCCESS;
> 
>   }
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
> index caace2c17cdc..5f50a605a338 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
> @@ -12,6 +12,7 @@
>   #include <Library/ArmMmuLib.h>
> 
>   #include <Library/CacheMaintenanceLib.h>
> 
>   #include <Library/DebugLib.h>
> 
> +#include <Library/HobLib.h>
> 
>   
> 
>   EFI_STATUS
> 
>   EFIAPI
> 
> @@ -21,6 +22,8 @@ ArmMmuPeiLibConstructor (
>     )
> 
>   {
> 
>     extern UINT32  ArmReplaceLiveTranslationEntrySize;
> 
> +  VOID           *ArmReplaceLiveTranslationEntryFunc;
> 
> +  VOID           *Hob;
> 
>   
> 
>     EFI_FV_FILE_INFO  FileInfo;
> 
>     EFI_STATUS        Status;
> 
> @@ -42,6 +45,20 @@ ArmMmuPeiLibConstructor (
>          (UINTN)ArmReplaceLiveTranslationEntry + ArmReplaceLiveTranslationEntrySize))
> 
>     {
> 
>       DEBUG ((DEBUG_INFO, "ArmMmuLib: skipping cache maintenance on XIP PEIM\n"));
> 
> +
> 
> +    //
> 
> +    // Expose the XIP version of the ArmReplaceLiveTranslationEntry() routine
> 
> +    // via a HOB so we can fall back to it later when we need to split block
> 
> +    // mappings in a way that adheres to break-before-make requirements.
> 
> +    //
> 
> +    ArmReplaceLiveTranslationEntryFunc = ArmReplaceLiveTranslationEntry;
> 
> +
> 
> +    Hob = BuildGuidDataHob (
> 
> +            &gArmMmuReplaceLiveTranslationEntryFuncGuid,
> 
> +            &ArmReplaceLiveTranslationEntryFunc,
> 
> +            sizeof ArmReplaceLiveTranslationEntryFunc
> 
> +            );
> 
> +    ASSERT (Hob != NULL);
> 
>     } else {
> 
>       DEBUG ((DEBUG_INFO, "ArmMmuLib: performing cache maintenance on shadowed PEIM\n"));
> 
>       //
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf b/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
> index 3d78e7dabf47..57cb71f90ee3 100644
> --- a/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
> +++ b/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
> @@ -36,7 +36,11 @@ [Packages]
>   [LibraryClasses]
> 
>     ArmLib
> 
>     CacheMaintenanceLib
> 
> +  HobLib
> 
>     MemoryAllocationLib
> 
>   
> 
> +[Guids]
> 
> +  gArmMmuReplaceLiveTranslationEntryFuncGuid
> 
> +
> 
>   [Pcd.ARM]
> 
>     gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf b/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
> index ce9674ea99ef..02f874a1a994 100644
> --- a/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
> +++ b/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
> @@ -29,4 +29,8 @@ [Packages]
>   [LibraryClasses]
> 
>     ArmLib
> 
>     CacheMaintenanceLib
> 
> +  HobLib
> 
>     MemoryAllocationLib
> 
> +
> 
> +[Guids]
> 
> +  gArmMmuReplaceLiveTranslationEntryFuncGuid
> 


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

* Re: [edk2-devel] [PATCH v3 09/16] ArmPlatformPkg/PrePeiCore: permit entry with the MMU enabled
  2022-09-26  8:25 ` [PATCH v3 09/16] ArmPlatformPkg/PrePeiCore: permit entry with the MMU enabled Ard Biesheuvel
@ 2022-09-26 22:39   ` Leif Lindholm
  0 siblings, 0 replies; 49+ messages in thread
From: Leif Lindholm @ 2022-09-26 22:39 UTC (permalink / raw)
  To: devel, ardb; +Cc: Alexander Graf

On 2022-09-26 01:25, Ard Biesheuvel wrote:
> Some platforms may set up a preliminary ID map in flash and enter EFI
> with the MMU and caches enabled, as this removes a lot of the complexity
> around cache coherency. Let's take this into account, and avoid touching
> the MMU controls or perform cache invalidation when the MMU is enabled
> at entry.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

/
     Leif

> ---
>   ArmPlatformPkg/PrePeiCore/PrePeiCore.c | 22 +++++++++++---------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> index 9c4b25df953d..8b86c6e69abd 100644
> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> @@ -58,17 +58,19 @@ CEntryPoint (
>     IN  EFI_PEI_CORE_ENTRY_POINT  PeiCoreEntryPoint
> 
>     )
> 
>   {
> 
> -  // Data Cache enabled on Primary core when MMU is enabled.
> 
> -  ArmDisableDataCache ();
> 
> -  // Invalidate instruction cache
> 
> -  ArmInvalidateInstructionCache ();
> 
> -  // Enable Instruction Caches on all cores.
> 
> -  ArmEnableInstructionCache ();
> 
> +  if (!ArmMmuEnabled ()) {
> 
> +    // Data Cache enabled on Primary core when MMU is enabled.
> 
> +    ArmDisableDataCache ();
> 
> +    // Invalidate instruction cache
> 
> +    ArmInvalidateInstructionCache ();
> 
> +    // Enable Instruction Caches on all cores.
> 
> +    ArmEnableInstructionCache ();
> 
>   
> 
> -  InvalidateDataCacheRange (
> 
> -    (VOID *)(UINTN)PcdGet64 (PcdCPUCoresStackBase),
> 
> -    PcdGet32 (PcdCPUCorePrimaryStackSize)
> 
> -    );
> 
> +    InvalidateDataCacheRange (
> 
> +      (VOID *)(UINTN)PcdGet64 (PcdCPUCoresStackBase),
> 
> +      PcdGet32 (PcdCPUCorePrimaryStackSize)
> 
> +      );
> 
> +  }
> 
>   
> 
>     //
> 
>     // Note: Doesn't have to Enable CPU interface in non-secure world,
> 


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

* Re: [PATCH v3 06/16] ArmPkg/ArmMmuLib: Disable and re-enable MMU only when needed
  2022-09-26  8:25 ` [PATCH v3 06/16] ArmPkg/ArmMmuLib: Disable and re-enable MMU only when needed Ard Biesheuvel
@ 2022-09-26 23:28   ` Leif Lindholm
  0 siblings, 0 replies; 49+ messages in thread
From: Leif Lindholm @ 2022-09-26 23:28 UTC (permalink / raw)
  To: Ard Biesheuvel, devel; +Cc: Alexander Graf

On 2022-09-26 01:25, Ard Biesheuvel wrote:
> When updating a page table descriptor in a way that requires break
> before make, we temporarily disable the MMU to ensure that we don't
> unmap the memory region that the code itself is executing from.
> 
> However, this is a condition we can check in a straight-forward manner,
> and if the regions are disjoint, we don't have to bother with the MMU
> controls, and we can just perform an ordinary break before make.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Acked-by: Leif Lindholm <quic_llindhol@quicinc.com>

/
     Leif

> ---
>   ArmPkg/Include/Library/ArmMmuLib.h                       |   7 +-
>   ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c         | 102 ++++++++++++++++----
>   ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S |  43 +++++++--
>   3 files changed, 123 insertions(+), 29 deletions(-)
> 
> diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h
> index 7538a8274a72..b745e2230e7e 100644
> --- a/ArmPkg/Include/Library/ArmMmuLib.h
> +++ b/ArmPkg/Include/Library/ArmMmuLib.h
> @@ -52,9 +52,10 @@ ArmClearMemoryRegionReadOnly (
>   VOID
> 
>   EFIAPI
> 
>   ArmReplaceLiveTranslationEntry (
> 
> -  IN  UINT64  *Entry,
> 
> -  IN  UINT64  Value,
> 
> -  IN  UINT64  RegionStart
> 
> +  IN  UINT64   *Entry,
> 
> +  IN  UINT64   Value,
> 
> +  IN  UINT64   RegionStart,
> 
> +  IN  BOOLEAN  DisableMmu
> 
>     );
> 
>   
> 
>   EFI_STATUS
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 34f1031c4de3..4d75788ed2b2 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -18,6 +18,17 @@
>   #include <Library/ArmMmuLib.h>
> 
>   #include <Library/BaseLib.h>
> 
>   #include <Library/DebugLib.h>
> 
> +#include <Library/HobLib.h>
> 
> +
> 
> +STATIC
> 
> +VOID (
> 
> +  EFIAPI  *mReplaceLiveEntryFunc
> 
> +  )(
> 
> +    IN  UINT64  *Entry,
> 
> +    IN  UINT64  Value,
> 
> +    IN  UINT64  RegionStart,
> 
> +    IN  BOOLEAN DisableMmu
> 
> +    ) = ArmReplaceLiveTranslationEntry;
> 
>   
> 
>   STATIC
> 
>   UINT64
> 
> @@ -83,14 +94,40 @@ ReplaceTableEntry (
>     IN  UINT64   *Entry,
> 
>     IN  UINT64   Value,
> 
>     IN  UINT64   RegionStart,
> 
> +  IN  UINT64   BlockMask,
> 
>     IN  BOOLEAN  IsLiveBlockMapping
> 
>     )
> 
>   {
> 
> -  if (!ArmMmuEnabled () || !IsLiveBlockMapping) {
> 
> +  BOOLEAN  DisableMmu;
> 
> +
> 
> +  //
> 
> +  // Replacing a live block entry with a table entry (or vice versa) requires a
> 
> +  // break-before-make sequence as per the architecture. This means the mapping
> 
> +  // must be made invalid and cleaned from the TLBs first, and this is a bit of
> 
> +  // a hassle if the mapping in question covers the code that is actually doing
> 
> +  // the mapping and the unmapping, and so we only bother with this if actually
> 
> +  // necessary.
> 
> +  //
> 
> +
> 
> +  if (!IsLiveBlockMapping || !ArmMmuEnabled ()) {
> 
> +    // If the mapping is not a live block mapping, or the MMU is not on yet, we
> 
> +    // can simply overwrite the entry.
> 
>       *Entry = Value;
> 
>       ArmUpdateTranslationTableEntry (Entry, (VOID *)(UINTN)RegionStart);
> 
>     } else {
> 
> -    ArmReplaceLiveTranslationEntry (Entry, Value, RegionStart);
> 
> +    // If the mapping in question does not cover the code that updates the
> 
> +    // entry in memory, or the entry that we are intending to update, we can
> 
> +    // use an ordinary break before make. Otherwise, we will need to
> 
> +    // temporarily disable the MMU.
> 
> +    DisableMmu = FALSE;
> 
> +    if ((((RegionStart ^ (UINTN)ArmReplaceLiveTranslationEntry) & ~BlockMask) == 0) ||
> 
> +        (((RegionStart ^ (UINTN)Entry) & ~BlockMask) == 0))
> 
> +    {
> 
> +      DisableMmu = TRUE;
> 
> +      DEBUG ((DEBUG_WARN, "%a: splitting block entry with MMU disabled\n", __FUNCTION__));
> 
> +    }
> 
> +
> 
> +    ArmReplaceLiveTranslationEntry (Entry, Value, RegionStart, DisableMmu);
> 
>     }
> 
>   }
> 
>   
> 
> @@ -155,12 +192,13 @@ IsTableEntry (
>   STATIC
> 
>   EFI_STATUS
> 
>   UpdateRegionMappingRecursive (
> 
> -  IN  UINT64  RegionStart,
> 
> -  IN  UINT64  RegionEnd,
> 
> -  IN  UINT64  AttributeSetMask,
> 
> -  IN  UINT64  AttributeClearMask,
> 
> -  IN  UINT64  *PageTable,
> 
> -  IN  UINTN   Level
> 
> +  IN  UINT64   RegionStart,
> 
> +  IN  UINT64   RegionEnd,
> 
> +  IN  UINT64   AttributeSetMask,
> 
> +  IN  UINT64   AttributeClearMask,
> 
> +  IN  UINT64   *PageTable,
> 
> +  IN  UINTN    Level,
> 
> +  IN  BOOLEAN  TableIsLive
> 
>     )
> 
>   {
> 
>     UINTN       BlockShift;
> 
> @@ -170,6 +208,7 @@ UpdateRegionMappingRecursive (
>     UINT64      EntryValue;
> 
>     VOID        *TranslationTable;
> 
>     EFI_STATUS  Status;
> 
> +  BOOLEAN     NextTableIsLive;
> 
>   
> 
>     ASSERT (((RegionStart | RegionEnd) & EFI_PAGE_MASK) == 0);
> 
>   
> 
> @@ -198,7 +237,14 @@ UpdateRegionMappingRecursive (
>       // the next level. No block mappings are allowed at all at level 0,
> 
>       // so in that case, we have to recurse unconditionally.
> 
>       //
> 
> +    // One special case to take into account is any region that covers the page
> 
> +    // table itself: if we'd cover such a region with block mappings, we are
> 
> +    // more likely to end up in the situation later where we need to disable
> 
> +    // the MMU in order to update page table entries safely, so prefer page
> 
> +    // mappings in that particular case.
> 
> +    //
> 
>       if ((Level == 0) || (((RegionStart | BlockEnd) & BlockMask) != 0) ||
> 
> +        ((Level < 3) && (((UINT64)PageTable & ~BlockMask) == RegionStart)) ||
> 
>           IsTableEntry (*Entry, Level))
> 
>       {
> 
>         ASSERT (Level < 3);
> 
> @@ -234,7 +280,8 @@ UpdateRegionMappingRecursive (
>                        *Entry & TT_ATTRIBUTES_MASK,
> 
>                        0,
> 
>                        TranslationTable,
> 
> -                     Level + 1
> 
> +                     Level + 1,
> 
> +                     FALSE
> 
>                        );
> 
>             if (EFI_ERROR (Status)) {
> 
>               //
> 
> @@ -246,8 +293,11 @@ UpdateRegionMappingRecursive (
>               return Status;
> 
>             }
> 
>           }
> 
> +
> 
> +        NextTableIsLive = FALSE;
> 
>         } else {
> 
>           TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
> 
> +        NextTableIsLive  = TableIsLive;
> 
>         }
> 
>   
> 
>         //
> 
> @@ -259,7 +309,8 @@ UpdateRegionMappingRecursive (
>                    AttributeSetMask,
> 
>                    AttributeClearMask,
> 
>                    TranslationTable,
> 
> -                 Level + 1
> 
> +                 Level + 1,
> 
> +                 NextTableIsLive
> 
>                    );
> 
>         if (EFI_ERROR (Status)) {
> 
>           if (!IsTableEntry (*Entry, Level)) {
> 
> @@ -282,7 +333,8 @@ UpdateRegionMappingRecursive (
>             Entry,
> 
>             EntryValue,
> 
>             RegionStart,
> 
> -          IsBlockEntry (*Entry, Level)
> 
> +          BlockMask,
> 
> +          TableIsLive && IsBlockEntry (*Entry, Level)
> 
>             );
> 
>         }
> 
>       } else {
> 
> @@ -291,7 +343,7 @@ UpdateRegionMappingRecursive (
>         EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
> 
>                                    : TT_TYPE_BLOCK_ENTRY;
> 
>   
> 
> -      ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> 
> +      ReplaceTableEntry (Entry, EntryValue, RegionStart, BlockMask, FALSE);
> 
>       }
> 
>     }
> 
>   
> 
> @@ -301,10 +353,11 @@ UpdateRegionMappingRecursive (
>   STATIC
> 
>   EFI_STATUS
> 
>   UpdateRegionMapping (
> 
> -  IN  UINT64  RegionStart,
> 
> -  IN  UINT64  RegionLength,
> 
> -  IN  UINT64  AttributeSetMask,
> 
> -  IN  UINT64  AttributeClearMask
> 
> +  IN  UINT64   RegionStart,
> 
> +  IN  UINT64   RegionLength,
> 
> +  IN  UINT64   AttributeSetMask,
> 
> +  IN  UINT64   AttributeClearMask,
> 
> +  IN  BOOLEAN  TableIsLive
> 
>     )
> 
>   {
> 
>     UINTN  T0SZ;
> 
> @@ -321,7 +374,8 @@ UpdateRegionMapping (
>              AttributeSetMask,
> 
>              AttributeClearMask,
> 
>              ArmGetTTBR0BaseAddress (),
> 
> -           GetRootTableLevel (T0SZ)
> 
> +           GetRootTableLevel (T0SZ),
> 
> +           TableIsLive
> 
>              );
> 
>   }
> 
>   
> 
> @@ -336,7 +390,8 @@ FillTranslationTable (
>              MemoryRegion->VirtualBase,
> 
>              MemoryRegion->Length,
> 
>              ArmMemoryAttributeToPageAttribute (MemoryRegion->Attributes) | TT_AF,
> 
> -           0
> 
> +           0,
> 
> +           FALSE
> 
>              );
> 
>   }
> 
>   
> 
> @@ -410,7 +465,8 @@ ArmSetMemoryAttributes (
>              BaseAddress,
> 
>              Length,
> 
>              PageAttributes,
> 
> -           PageAttributeMask
> 
> +           PageAttributeMask,
> 
> +           TRUE
> 
>              );
> 
>   }
> 
>   
> 
> @@ -423,7 +479,13 @@ SetMemoryRegionAttribute (
>     IN  UINT64                BlockEntryMask
> 
>     )
> 
>   {
> 
> -  return UpdateRegionMapping (BaseAddress, Length, Attributes, BlockEntryMask);
> 
> +  return UpdateRegionMapping (
> 
> +           BaseAddress,
> 
> +           Length,
> 
> +           Attributes,
> 
> +           BlockEntryMask,
> 
> +           TRUE
> 
> +           );
> 
>   }
> 
>   
> 
>   EFI_STATUS
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> index 66ebca571e63..e936a5be4e11 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> @@ -12,6 +12,14 @@
>   
> 
>     .macro __replace_entry, el
> 
>   
> 
> +  // check whether we should disable the MMU
> 
> +  cbz   x3, .L1_\@
> 
> +
> 
> +  // clean and invalidate first so that we don't clobber
> 
> +  // adjacent entries that are dirty in the caches
> 
> +  dc    civac, x0
> 
> +  dsb   nsh
> 
> +
> 
>     // disable the MMU
> 
>     mrs   x8, sctlr_el\el
> 
>     bic   x9, x8, #CTRL_M_BIT
> 
> @@ -38,8 +46,33 @@
>     // re-enable the MMU
> 
>     msr   sctlr_el\el, x8
> 
>     isb
> 
> +  b     .L2_\@
> 
> +
> 
> +.L1_\@:
> 
> +  // write invalid entry
> 
> +  str   xzr, [x0]
> 
> +  dsb   nshst
> 
> +
> 
> +  // flush translations for the target address from the TLBs
> 
> +  lsr   x2, x2, #12
> 
> +  .if   \el == 1
> 
> +  tlbi  vaae1, x2
> 
> +  .else
> 
> +  tlbi  vae\el, x2
> 
> +  .endif
> 
> +  dsb   nsh
> 
> +
> 
> +  // write updated entry
> 
> +  str   x1, [x0]
> 
> +  dsb   nshst
> 
> +
> 
> +.L2_\@:
> 
>     .endm
> 
>   
> 
> +  // Align this routine to a log2 upper bound of its size, so that it is
> 
> +  // guaranteed not to cross a page or block boundary.
> 
> +  .balign 0x200
> 
> +
> 
>   //VOID
> 
>   //ArmReplaceLiveTranslationEntry (
> 
>   //  IN  UINT64  *Entry,
> 
> @@ -53,12 +86,7 @@ ASM_FUNC(ArmReplaceLiveTranslationEntry)
>     msr   daifset, #0xf
> 
>     isb
> 
>   
> 
> -  // clean and invalidate first so that we don't clobber
> 
> -  // adjacent entries that are dirty in the caches
> 
> -  dc    civac, x0
> 
> -  dsb   nsh
> 
> -
> 
> -  EL1_OR_EL2_OR_EL3(x3)
> 
> +  EL1_OR_EL2_OR_EL3(x5)
> 
>   1:__replace_entry 1
> 
>     b     4f
> 
>   2:__replace_entry 2
> 
> @@ -72,3 +100,6 @@ ASM_GLOBAL ASM_PFX(ArmReplaceLiveTranslationEntrySize)
>   
> 
>   ASM_PFX(ArmReplaceLiveTranslationEntrySize):
> 
>     .long   . - ArmReplaceLiveTranslationEntry
> 
> +
> 
> +  // Double check that we did not overrun the assumed maximum size
> 
> +  .org    ArmReplaceLiveTranslationEntry + 0x200
> 


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2022-09-26  8:24 ` [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable Ard Biesheuvel
  2022-09-26 22:28   ` [edk2-devel] " Leif Lindholm
@ 2022-11-28 15:46   ` Gerd Hoffmann
  2022-12-29 18:00     ` dann frazier
  1 sibling, 1 reply; 49+ messages in thread
From: Gerd Hoffmann @ 2022-11-28 15:46 UTC (permalink / raw)
  To: devel, ardb; +Cc: Leif Lindholm, Alexander Graf

On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
> When the memory protections were implemented and enabled on ArmVirtQemu
> 5+ years ago, we had to work around the fact that GRUB at the time
> expected EFI_LOADER_DATA to be executable, as that is the memory type it
> allocates when loading its modules.
> 
> This has been fixed in GRUB in August 2017, so by now, we should be able
> to tighten this, and remove execute permissions from EFI_LOADER_DATA
> allocations.

Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
tl;dr: fedora 37 grub.efi is still broken.

take care,
  Gerd


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2022-11-28 15:46   ` Gerd Hoffmann
@ 2022-12-29 18:00     ` dann frazier
  2023-01-03  9:59       ` Ard Biesheuvel
  0 siblings, 1 reply; 49+ messages in thread
From: dann frazier @ 2022-12-29 18:00 UTC (permalink / raw)
  To: devel, kraxel; +Cc: ardb, Leif Lindholm, Alexander Graf

On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
> On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
> > When the memory protections were implemented and enabled on ArmVirtQemu
> > 5+ years ago, we had to work around the fact that GRUB at the time
> > expected EFI_LOADER_DATA to be executable, as that is the memory type it
> > allocates when loading its modules.
> > 
> > This has been fixed in GRUB in August 2017, so by now, we should be able
> > to tighten this, and remove execute permissions from EFI_LOADER_DATA
> > allocations.
> 
> Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
> tl;dr: fedora 37 grub.efi is still broken.

This is also the case with existing Ubuntu releases, as well as
AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert this
patch in Debian/Ubuntu until it is more ubiquitous. Do you want to do
the same upstream? I'm not sure at what point it would make sense to
reintroduce it, given we can't force users to upgrade their bootloaders.

   -dann

[*] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1025656
[**] https://git.launchpad.net/~ubuntu-core-dev/grub/+git/ubuntu/commit/?id=a0ee822f1c85fcf55066996ab51c5cf77e2728b2)

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

* Re: [edk2-devel] [PATCH v3 12/16] ArmVirtPkg/ArmVirtQemu: enable initial ID map at early boot
  2022-09-26  8:25 ` [PATCH v3 12/16] ArmVirtPkg/ArmVirtQemu: enable initial ID map at early boot Ard Biesheuvel
@ 2022-12-29 21:10   ` dann frazier
  2023-01-03  9:02     ` Ard Biesheuvel
  0 siblings, 1 reply; 49+ messages in thread
From: dann frazier @ 2022-12-29 21:10 UTC (permalink / raw)
  To: devel, ardb; +Cc: Leif Lindholm, Alexander Graf

On Mon, Sep 26, 2022 at 10:25:07AM +0200, Ard Biesheuvel wrote:
> Now that we have all the pieces in place, switch the AArch64 version of
> ArmVirtQemu to a mode where the first thing it does out of reset is
> enable a preliminary ID map that covers the NOR flash and sufficient
> DRAM to create the UEFI page tables as usual.
> 
> The advantage of this is that no manipulation of memory occurs any
> longer before the MMU is enabled, which removes the need for explicit
> coherency management, which is cumbersome and bad for performance.
> 
> It also means we no longer need to build all components that may execute
> with the MMU off (including BASE libraries) with strict alignment.

After this switch, I'm seeing a Synchronous Exception when launching a
VM, though only on old Cavium ThunderX (CN88XX) systems. I used print
debugging to narrow it down to ArmSetTTBR0(). Initially I thought it
might be related to Cavium Erratum 27456, but that doesn't seem to
make sense because the instruction cache isn't enabled until
later. I tried implementing the same workaround as Linux does anyway
(flush caches after the setting ttbr0) without any luck.

Any idea what is going on there?

  -dann


> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  ArmVirtPkg/ArmVirtQemu.dsc | 17 ++++++++++++++---
>  ArmVirtPkg/ArmVirtQemu.fdf |  2 +-
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 302c0d2a4e29..21a321e35794 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -63,8 +63,6 @@ [LibraryClasses.common]
>    QemuFwCfgSimpleParserLib|OvmfPkg/Library/QemuFwCfgSimpleParserLib/QemuFwCfgSimpleParserLib.inf
>    QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
>  
> -  ArmPlatformLib|ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf
> -
>    TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
>    NorFlashPlatformLib|ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
>  
> @@ -92,6 +90,12 @@ [LibraryClasses.common]
>    TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf
>  !endif
>  
> +[LibraryClasses.AARCH64]
> +  ArmPlatformLib|ArmVirtPkg/Library/ArmPlatformLibQemu/ArmPlatformLibQemu.inf
> +
> +[LibraryClasses.ARM]
> +  ArmPlatformLib|ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf
> +
>  [LibraryClasses.common.PEIM]
>    ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
>  
> @@ -112,6 +116,8 @@ [LibraryClasses.common.UEFI_DRIVER]
>    UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
>  
>  [BuildOptions]
> +  GCC:*_*_AARCH64_CC_XIPFLAGS = -mno-strict-align
> +
>  !include NetworkPkg/NetworkBuildOptions.dsc.inc
>  
>  ################################################################################
> @@ -310,7 +316,12 @@ [Components.common]
>        PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>    }
>    ArmPlatformPkg/PlatformPei/PlatformPeim.inf
> -  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> +  ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf {
> +    <LibraryClasses>
> +!if $(ARCH) == AARCH64
> +      ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
> +!endif
> +  }
>    ArmPkg/Drivers/CpuPei/CpuPei.inf
>  
>    MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
> index b5e2253295fe..7f17aeb3ad0d 100644
> --- a/ArmVirtPkg/ArmVirtQemu.fdf
> +++ b/ArmVirtPkg/ArmVirtQemu.fdf
> @@ -107,7 +107,7 @@ [FV.FVMAIN_COMPACT]
>    INF ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
>    INF MdeModulePkg/Core/Pei/PeiMain.inf
>    INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
> -  INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> +  INF ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf
>    INF ArmPkg/Drivers/CpuPei/CpuPei.inf
>    INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
>    INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf

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

* Re: [edk2-devel] [PATCH v3 12/16] ArmVirtPkg/ArmVirtQemu: enable initial ID map at early boot
  2022-12-29 21:10   ` [edk2-devel] " dann frazier
@ 2023-01-03  9:02     ` Ard Biesheuvel
  2023-01-03 19:38       ` dann frazier
  0 siblings, 1 reply; 49+ messages in thread
From: Ard Biesheuvel @ 2023-01-03  9:02 UTC (permalink / raw)
  To: dann frazier; +Cc: devel, Leif Lindholm, Alexander Graf

On Thu, 29 Dec 2022 at 22:10, dann frazier <dann.frazier@canonical.com> wrote:
>
> On Mon, Sep 26, 2022 at 10:25:07AM +0200, Ard Biesheuvel wrote:
> > Now that we have all the pieces in place, switch the AArch64 version of
> > ArmVirtQemu to a mode where the first thing it does out of reset is
> > enable a preliminary ID map that covers the NOR flash and sufficient
> > DRAM to create the UEFI page tables as usual.
> >
> > The advantage of this is that no manipulation of memory occurs any
> > longer before the MMU is enabled, which removes the need for explicit
> > coherency management, which is cumbersome and bad for performance.
> >
> > It also means we no longer need to build all components that may execute
> > with the MMU off (including BASE libraries) with strict alignment.
>
> After this switch, I'm seeing a Synchronous Exception when launching a
> VM, though only on old Cavium ThunderX (CN88XX) systems. I used print
> debugging to narrow it down to ArmSetTTBR0(). Initially I thought it
> might be related to Cavium Erratum 27456, but that doesn't seem to
> make sense because the instruction cache isn't enabled until
> later. I tried implementing the same workaround as Linux does anyway
> (flush caches after the setting ttbr0) without any luck.
>
> Any idea what is going on there?
>

I suspect it is in fact the same erratum - the I-cache does get
enabled almost immediately after reset, and the use of ASIDs for EL1
mappings looks suspiciously like failure mode that required us to
disable KPTI for ThunderX on Linux.

It is a bit disappointing to have to add workarounds for obsolete
platforms in new code, so if I fix this, it will be gated by a -D
build option - is that acceptable to you?

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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2022-12-29 18:00     ` dann frazier
@ 2023-01-03  9:59       ` Ard Biesheuvel
  2023-01-03 19:39         ` Alexander Graf
  0 siblings, 1 reply; 49+ messages in thread
From: Ard Biesheuvel @ 2023-01-03  9:59 UTC (permalink / raw)
  To: dann frazier; +Cc: devel, kraxel, Leif Lindholm, Alexander Graf

On Thu, 29 Dec 2022 at 19:00, dann frazier <dann.frazier@canonical.com> wrote:
>
> On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
> > On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
> > > When the memory protections were implemented and enabled on ArmVirtQemu
> > > 5+ years ago, we had to work around the fact that GRUB at the time
> > > expected EFI_LOADER_DATA to be executable, as that is the memory type it
> > > allocates when loading its modules.
> > >
> > > This has been fixed in GRUB in August 2017, so by now, we should be able
> > > to tighten this, and remove execute permissions from EFI_LOADER_DATA
> > > allocations.
> >
> > Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
> > tl;dr: fedora 37 grub.efi is still broken.
>
> This is also the case with existing Ubuntu releases, as well as
> AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
> the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert this
> patch in Debian/Ubuntu until it is more ubiquitous. Do you want to do
> the same upstream? I'm not sure at what point it would make sense to
> reintroduce it, given we can't force users to upgrade their bootloaders.
>

Thanks for the report.

You can override PCDs on the build command line, so I suggest you use
that for building these images as long as it is needed.

E.g,, append this to the build.sh command line

--pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1

to undo the effects of this patch.

I do not intend to revert this patch - the trend under EFI is towards
much stricter memory permissions, also on the MS side, and this is
especially important under CC scenarios. And if 5+ years is not
sufficient for out-of-tree GRUB to catch up, what is the point of
waiting for it?

Thanks,
Ard.

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

* Re: [edk2-devel] [PATCH v3 12/16] ArmVirtPkg/ArmVirtQemu: enable initial ID map at early boot
  2023-01-03  9:02     ` Ard Biesheuvel
@ 2023-01-03 19:38       ` dann frazier
  0 siblings, 0 replies; 49+ messages in thread
From: dann frazier @ 2023-01-03 19:38 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: devel, Leif Lindholm, Alexander Graf

On Tue, Jan 03, 2023 at 10:02:27AM +0100, Ard Biesheuvel wrote:
> On Thu, 29 Dec 2022 at 22:10, dann frazier <dann.frazier@canonical.com> wrote:
> >
> > On Mon, Sep 26, 2022 at 10:25:07AM +0200, Ard Biesheuvel wrote:
> > > Now that we have all the pieces in place, switch the AArch64 version of
> > > ArmVirtQemu to a mode where the first thing it does out of reset is
> > > enable a preliminary ID map that covers the NOR flash and sufficient
> > > DRAM to create the UEFI page tables as usual.
> > >
> > > The advantage of this is that no manipulation of memory occurs any
> > > longer before the MMU is enabled, which removes the need for explicit
> > > coherency management, which is cumbersome and bad for performance.
> > >
> > > It also means we no longer need to build all components that may execute
> > > with the MMU off (including BASE libraries) with strict alignment.
> >
> > After this switch, I'm seeing a Synchronous Exception when launching a
> > VM, though only on old Cavium ThunderX (CN88XX) systems. I used print
> > debugging to narrow it down to ArmSetTTBR0(). Initially I thought it
> > might be related to Cavium Erratum 27456, but that doesn't seem to
> > make sense because the instruction cache isn't enabled until
> > later. I tried implementing the same workaround as Linux does anyway
> > (flush caches after the setting ttbr0) without any luck.
> >
> > Any idea what is going on there?
> >
> 
> I suspect it is in fact the same erratum - the I-cache does get
> enabled almost immediately after reset, and the use of ASIDs for EL1
> mappings looks suspiciously like failure mode that required us to
> disable KPTI for ThunderX on Linux.
> 
> It is a bit disappointing to have to add workarounds for obsolete
> platforms in new code, so if I fix this, it will be gated by a -D
> build option - is that acceptable to you?

It is. fwiw, here's what I tried w/o luck:

diff --git a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
index ba0ec5682b..1a94a9782c 100644
--- a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
+++ b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
@@ -56,10 +56,30 @@ ASM_FUNC(ArmReadAuxCr)
 ASM_FUNC(ArmSetTTBR0)
   EL1_OR_EL2_OR_EL3(x1)
 1:msr     ttbr0_el1, x0      // Translation Table Base Reg 0 (TTBR0)
+  isb
+  nop
+  nop
+  nop
+  ic iallu
+  dsb nsh
+  isb
   b       4f
 2:msr     ttbr0_el2, x0      // Translation Table Base Reg 0 (TTBR0)
+  isb
+  nop
+  nop
+  nop
+  ic iallu
+  dsb nsh
+  isb
   b       4f
 3:msr     ttbr0_el3, x0      // Translation Table Base Reg 0 (TTBR0)
+  isb
+  nop
+  nop
+  nop
+  ic iallu
+  dsb nsh
 4:isb
   ret
 

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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-03  9:59       ` Ard Biesheuvel
@ 2023-01-03 19:39         ` Alexander Graf
  2023-01-03 22:47           ` dann frazier
  0 siblings, 1 reply; 49+ messages in thread
From: Alexander Graf @ 2023-01-03 19:39 UTC (permalink / raw)
  To: Ard Biesheuvel, dann frazier; +Cc: devel, kraxel, Leif Lindholm

Hey Ard,

On 03.01.23 10:59, Ard Biesheuvel wrote:
> On Thu, 29 Dec 2022 at 19:00, dann frazier <dann.frazier@canonical.com> wrote:
>> On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
>>> On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
>>>> When the memory protections were implemented and enabled on ArmVirtQemu
>>>> 5+ years ago, we had to work around the fact that GRUB at the time
>>>> expected EFI_LOADER_DATA to be executable, as that is the memory type it
>>>> allocates when loading its modules.
>>>>
>>>> This has been fixed in GRUB in August 2017, so by now, we should be able
>>>> to tighten this, and remove execute permissions from EFI_LOADER_DATA
>>>> allocations.
>>> Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
>>> tl;dr: fedora 37 grub.efi is still broken.
>> This is also the case with existing Ubuntu releases, as well as
>> AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
>> the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert this
>> patch in Debian/Ubuntu until it is more ubiquitous. Do you want to do
>> the same upstream? I'm not sure at what point it would make sense to
>> reintroduce it, given we can't force users to upgrade their bootloaders.
>>
> Thanks for the report.
>
> You can override PCDs on the build command line, so I suggest you use
> that for building these images as long as it is needed.
>
> E.g,, append this to the build.sh command line
>
> --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
>
> to undo the effects of this patch.
>
> I do not intend to revert this patch - the trend under EFI is towards
> much stricter memory permissions, also on the MS side, and this is
> especially important under CC scenarios. And if 5+ years is not
> sufficient for out-of-tree GRUB to catch up, what is the point of
> waiting for it?


I think we need to be smarter here. ArmVirtPkg is used by a lot of 
virtualization software - such as QEMU. Typically, users (and 
developers) expect that an update to a newer version will preserve 
compatibility.

Let me contrive an example: In 1 year, QEMU updates to the latest AAVMF. 
It ships that as part of its pc-bios directory. Suddenly, we 
accidentally break old (immutable!) iso images that used to work. So 
someone that updates QEMU to a newer version will have a regression in 
running a Fedora 37 installation. Or RHEL 8.7 installation. Not good :).

Outside of OSVs providing new iso images, there is also not much you can 
do about this. The grub binary here runs way before any updater code 
that could pull a fixed version from the internet.

What alternatives do we have? Assuming grub is the only offender we have 
to worry about, is there a way we can identify that the allocation came 
from an unpatched version? Or have a database of hashes (with all known 
grub binaries that have this bug in existing isos) that would force 
disable NX protection for it if we hit it? Or disable NX when Secure 
Boot is disabled?

I really think we need to be a bit more creative than make NX mandatory 
in all cases when we know the are immutable images out there that won't 
work with it, otherwise we break very legit use cases.


Alex


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-03 19:39         ` Alexander Graf
@ 2023-01-03 22:47           ` dann frazier
  2023-01-04  9:35             ` Ard Biesheuvel
  0 siblings, 1 reply; 49+ messages in thread
From: dann frazier @ 2023-01-03 22:47 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Ard Biesheuvel, devel, kraxel, Leif Lindholm

On Tue, Jan 03, 2023 at 08:39:24PM +0100, Alexander Graf wrote:
> Hey Ard,
> 
> On 03.01.23 10:59, Ard Biesheuvel wrote:
> > On Thu, 29 Dec 2022 at 19:00, dann frazier <dann.frazier@canonical.com> wrote:
> > > On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
> > > > On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
> > > > > When the memory protections were implemented and enabled on ArmVirtQemu
> > > > > 5+ years ago, we had to work around the fact that GRUB at the time
> > > > > expected EFI_LOADER_DATA to be executable, as that is the memory type it
> > > > > allocates when loading its modules.
> > > > > 
> > > > > This has been fixed in GRUB in August 2017, so by now, we should be able
> > > > > to tighten this, and remove execute permissions from EFI_LOADER_DATA
> > > > > allocations.
> > > > Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
> > > > tl;dr: fedora 37 grub.efi is still broken.
> > > This is also the case with existing Ubuntu releases, as well as
> > > AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
> > > the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert this
> > > patch in Debian/Ubuntu until it is more ubiquitous. Do you want to do
> > > the same upstream? I'm not sure at what point it would make sense to
> > > reintroduce it, given we can't force users to upgrade their bootloaders.
> > > 
> > Thanks for the report.
> > 
> > You can override PCDs on the build command line, so I suggest you use
> > that for building these images as long as it is needed.
> > 
> > E.g,, append this to the build.sh command line
> > 
> > --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
> > 
> > to undo the effects of this patch.
> > 
> > I do not intend to revert this patch - the trend under EFI is towards
> > much stricter memory permissions, also on the MS side, and this is
> > especially important under CC scenarios. And if 5+ years is not
> > sufficient for out-of-tree GRUB to catch up, what is the point of
> > waiting for it?
> 
> 
> I think we need to be smarter here. ArmVirtPkg is used by a lot of
> virtualization software - such as QEMU. Typically, users (and developers)
> expect that an update to a newer version will preserve compatibility.
> 
> Let me contrive an example: In 1 year, QEMU updates to the latest AAVMF. It
> ships that as part of its pc-bios directory. Suddenly, we accidentally break
> old (immutable!) iso images that used to work. So someone that updates QEMU
> to a newer version will have a regression in running a Fedora 37
> installation. Or RHEL 8.7 installation. Not good :).
> 
> Outside of OSVs providing new iso images, there is also not much you can do
> about this. The grub binary here runs way before any updater code that could
> pull a fixed version from the internet.
> 
> What alternatives do we have? Assuming grub is the only offender we have to
> worry about, is there a way we can identify that the allocation came from an
> unpatched version? Or have a database of hashes (with all known grub
> binaries that have this bug in existing isos) that would force disable NX
> protection for it if we hit it? Or disable NX when Secure Boot is disabled?
> 
> I really think we need to be a bit more creative than make NX mandatory in
> all cases when we know the are immutable images out there that won't work
> with it, otherwise we break very legit use cases.

While it has its own issues, I suppose another way to go about it is
for distributors to provide multiple AAVMF_CODE images, and perhaps
invent a "feature" flag for the json descriptor for management tools
to select an appropriate one.

  -dann

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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-03 22:47           ` dann frazier
@ 2023-01-04  9:35             ` Ard Biesheuvel
  2023-01-04 11:11               ` Gerd Hoffmann
  2023-01-04 13:13               ` Alexander Graf
  0 siblings, 2 replies; 49+ messages in thread
From: Ard Biesheuvel @ 2023-01-04  9:35 UTC (permalink / raw)
  To: dann frazier; +Cc: Alexander Graf, devel, kraxel, Leif Lindholm

On Tue, 3 Jan 2023 at 23:47, dann frazier <dann.frazier@canonical.com> wrote:
>
> On Tue, Jan 03, 2023 at 08:39:24PM +0100, Alexander Graf wrote:
> > Hey Ard,
> >
> > On 03.01.23 10:59, Ard Biesheuvel wrote:
> > > On Thu, 29 Dec 2022 at 19:00, dann frazier <dann.frazier@canonical.com> wrote:
> > > > On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
> > > > > On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
> > > > > > When the memory protections were implemented and enabled on ArmVirtQemu
> > > > > > 5+ years ago, we had to work around the fact that GRUB at the time
> > > > > > expected EFI_LOADER_DATA to be executable, as that is the memory type it
> > > > > > allocates when loading its modules.
> > > > > >
> > > > > > This has been fixed in GRUB in August 2017, so by now, we should be able
> > > > > > to tighten this, and remove execute permissions from EFI_LOADER_DATA
> > > > > > allocations.
> > > > > Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
> > > > > tl;dr: fedora 37 grub.efi is still broken.
> > > > This is also the case with existing Ubuntu releases, as well as
> > > > AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
> > > > the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert this
> > > > patch in Debian/Ubuntu until it is more ubiquitous. Do you want to do
> > > > the same upstream? I'm not sure at what point it would make sense to
> > > > reintroduce it, given we can't force users to upgrade their bootloaders.
> > > >
> > > Thanks for the report.
> > >
> > > You can override PCDs on the build command line, so I suggest you use
> > > that for building these images as long as it is needed.
> > >
> > > E.g,, append this to the build.sh command line
> > >
> > > --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
> > >
> > > to undo the effects of this patch.
> > >
> > > I do not intend to revert this patch - the trend under EFI is towards
> > > much stricter memory permissions, also on the MS side, and this is
> > > especially important under CC scenarios. And if 5+ years is not
> > > sufficient for out-of-tree GRUB to catch up, what is the point of
> > > waiting for it?
> >
> >
> > I think we need to be smarter here. ArmVirtPkg is used by a lot of
> > virtualization software - such as QEMU. Typically, users (and developers)
> > expect that an update to a newer version will preserve compatibility.
> >
> > Let me contrive an example: In 1 year, QEMU updates to the latest AAVMF. It
> > ships that as part of its pc-bios directory. Suddenly, we accidentally break
> > old (immutable!) iso images that used to work. So someone that updates QEMU
> > to a newer version will have a regression in running a Fedora 37
> > installation. Or RHEL 8.7 installation. Not good :).
> >
> > Outside of OSVs providing new iso images, there is also not much you can do
> > about this. The grub binary here runs way before any updater code that could
> > pull a fixed version from the internet.
> >
> > What alternatives do we have? Assuming grub is the only offender we have to
> > worry about, is there a way we can identify that the allocation came from an
> > unpatched version? Or have a database of hashes (with all known grub
> > binaries that have this bug in existing isos) that would force disable NX
> > protection for it if we hit it? Or disable NX when Secure Boot is disabled?
> >
> > I really think we need to be a bit more creative than make NX mandatory in
> > all cases when we know the are immutable images out there that won't work
> > with it, otherwise we break very legit use cases.
>
> While it has its own issues, I suppose another way to go about it is
> for distributors to provide multiple AAVMF_CODE images, and perhaps
> invent a "feature" flag for the json descriptor for management tools
> to select an appropriate one.
>

I don't think having different versions of the image makes sense, tbh,
but of course, this is up to the distros.

Compatibility with ancient downstream GRUB builds is not a goal of the
EDK2 upstream, so as long as distros can tweak the build to their
needs, I don't see a reason to revert this change upstream.

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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-04  9:35             ` Ard Biesheuvel
@ 2023-01-04 11:11               ` Gerd Hoffmann
  2023-01-04 12:04                 ` Ard Biesheuvel
  2023-01-06  9:55                 ` Laszlo Ersek
  2023-01-04 13:13               ` Alexander Graf
  1 sibling, 2 replies; 49+ messages in thread
From: Gerd Hoffmann @ 2023-01-04 11:11 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: dann frazier, Alexander Graf, devel, Leif Lindholm

  Hi,

> > > > You can override PCDs on the build command line, so I suggest you use
> > > > that for building these images as long as it is needed.
> > > >
> > > > E.g,, append this to the build.sh command line
> > > >
> > > > --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
> > > >
> > > > to undo the effects of this patch.

Can this also be flipped at runtime?
Does this pcd work the same way on all architectures?

> I don't think having different versions of the image makes sense, tbh,
> but of course, this is up to the distros.

Fedora has reverted the patch for now, and I don't see how we can enable
that anytime soon given that RHEL-8,9 with loooooong support times ship
broken grub binaries today.

> Compatibility with ancient downstream GRUB builds is not a goal of the
> EDK2 upstream, so as long as distros can tweak the build to their
> needs, I don't see a reason to revert this change upstream.

The versions are not that ancient.  The problem is more that upstream
grub is really slow on integrating patches so every distro does carry
a huge pile of downstream patches.  And they seem to re-introduce the
bug ...

But, yes, just reverting upstream too doesn't look like a good option
either, we need at least a little pressure to get things fixed.

take care,
  Gerd


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-04 11:11               ` Gerd Hoffmann
@ 2023-01-04 12:04                 ` Ard Biesheuvel
  2023-01-04 12:56                   ` Gerd Hoffmann
  2023-01-06  9:55                 ` Laszlo Ersek
  1 sibling, 1 reply; 49+ messages in thread
From: Ard Biesheuvel @ 2023-01-04 12:04 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: dann frazier, Alexander Graf, devel, Leif Lindholm

On Wed, 4 Jan 2023 at 12:11, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > > > You can override PCDs on the build command line, so I suggest you use
> > > > > that for building these images as long as it is needed.
> > > > >
> > > > > E.g,, append this to the build.sh command line
> > > > >
> > > > > --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
> > > > >
> > > > > to undo the effects of this patch.
>
> Can this also be flipped at runtime?

Currently, it is fixed or patchable, which means that you can override
it at build time only. I don't think making this a dynamic PCD would
be difficult, and on QEMU, we can set the value early enough if we key
it off fw_cfg or something like that.

But that implies that you need a 'permissive' mode to invoke QEMU,
which ends up being always enabled, most likely, so I'm not sure this
is an improvement.

> Does this pcd work the same way on all architectures?
>

In principle, yes. However, I cannot vouch for the X86 code not doing
dodgy things with data regions, so whether the same *value* works
reliably across all architectures is a separate matter.

> > I don't think having different versions of the image makes sense, tbh,
> > but of course, this is up to the distros.
>
> Fedora has reverted the patch for now, and I don't see how we can enable
> that anytime soon given that RHEL-8,9 with loooooong support times ship
> broken grub binaries today.
>

Yeah. This is really disappointing.

> > Compatibility with ancient downstream GRUB builds is not a goal of the
> > EDK2 upstream, so as long as distros can tweak the build to their
> > needs, I don't see a reason to revert this change upstream.
>
> The versions are not that ancient.  The problem is more that upstream
> grub is really slow on integrating patches so every distro does carry
> a huge pile of downstream patches.  And they seem to re-introduce the
> bug ...
>
> But, yes, just reverting upstream too doesn't look like a good option
> either, we need at least a little pressure to get things fixed.
>

Indeed.

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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-04 12:04                 ` Ard Biesheuvel
@ 2023-01-04 12:56                   ` Gerd Hoffmann
  0 siblings, 0 replies; 49+ messages in thread
From: Gerd Hoffmann @ 2023-01-04 12:56 UTC (permalink / raw)
  To: devel, ardb; +Cc: dann frazier, Alexander Graf, Leif Lindholm

On Wed, Jan 04, 2023 at 01:04:41PM +0100, Ard Biesheuvel wrote:
> On Wed, 4 Jan 2023 at 12:11, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >   Hi,
> >
> > > > > > --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
> >
> > Can this also be flipped at runtime?
> 
> Currently, it is fixed or patchable, which means that you can override
> it at build time only. I don't think making this a dynamic PCD would
> be difficult, and on QEMU, we can set the value early enough if we key
> it off fw_cfg or something like that.
> 
> But that implies that you need a 'permissive' mode to invoke QEMU,
> which ends up being always enabled, most likely, so I'm not sure this
> is an improvement.

It works both ways.  Being able to enable nx protection at runtime on
builds which have it disabled by default would be quite useful.  Write
test cases.  Write reproducer instructions which don't include building
edk2 yourself.

take care,
  Gerd


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-04  9:35             ` Ard Biesheuvel
  2023-01-04 11:11               ` Gerd Hoffmann
@ 2023-01-04 13:13               ` Alexander Graf
  2023-01-05  0:09                 ` Alexander Graf
  1 sibling, 1 reply; 49+ messages in thread
From: Alexander Graf @ 2023-01-04 13:13 UTC (permalink / raw)
  To: Ard Biesheuvel, dann frazier; +Cc: devel, kraxel, Leif Lindholm


On 04.01.23 10:35, Ard Biesheuvel wrote:
> On Tue, 3 Jan 2023 at 23:47, dann frazier <dann.frazier@canonical.com> wrote:
>> On Tue, Jan 03, 2023 at 08:39:24PM +0100, Alexander Graf wrote:
>>> Hey Ard,
>>>
>>> On 03.01.23 10:59, Ard Biesheuvel wrote:
>>>> On Thu, 29 Dec 2022 at 19:00, dann frazier <dann.frazier@canonical.com> wrote:
>>>>> On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
>>>>>> On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
>>>>>>> When the memory protections were implemented and enabled on ArmVirtQemu
>>>>>>> 5+ years ago, we had to work around the fact that GRUB at the time
>>>>>>> expected EFI_LOADER_DATA to be executable, as that is the memory type it
>>>>>>> allocates when loading its modules.
>>>>>>>
>>>>>>> This has been fixed in GRUB in August 2017, so by now, we should be able
>>>>>>> to tighten this, and remove execute permissions from EFI_LOADER_DATA
>>>>>>> allocations.
>>>>>> Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
>>>>>> tl;dr: fedora 37 grub.efi is still broken.
>>>>> This is also the case with existing Ubuntu releases, as well as
>>>>> AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
>>>>> the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert this
>>>>> patch in Debian/Ubuntu until it is more ubiquitous. Do you want to do
>>>>> the same upstream? I'm not sure at what point it would make sense to
>>>>> reintroduce it, given we can't force users to upgrade their bootloaders.
>>>>>
>>>> Thanks for the report.
>>>>
>>>> You can override PCDs on the build command line, so I suggest you use
>>>> that for building these images as long as it is needed.
>>>>
>>>> E.g,, append this to the build.sh command line
>>>>
>>>> --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
>>>>
>>>> to undo the effects of this patch.
>>>>
>>>> I do not intend to revert this patch - the trend under EFI is towards
>>>> much stricter memory permissions, also on the MS side, and this is
>>>> especially important under CC scenarios. And if 5+ years is not
>>>> sufficient for out-of-tree GRUB to catch up, what is the point of
>>>> waiting for it?
>>>
>>> I think we need to be smarter here. ArmVirtPkg is used by a lot of
>>> virtualization software - such as QEMU. Typically, users (and developers)
>>> expect that an update to a newer version will preserve compatibility.
>>>
>>> Let me contrive an example: In 1 year, QEMU updates to the latest AAVMF. It
>>> ships that as part of its pc-bios directory. Suddenly, we accidentally break
>>> old (immutable!) iso images that used to work. So someone that updates QEMU
>>> to a newer version will have a regression in running a Fedora 37
>>> installation. Or RHEL 8.7 installation. Not good :).
>>>
>>> Outside of OSVs providing new iso images, there is also not much you can do
>>> about this. The grub binary here runs way before any updater code that could
>>> pull a fixed version from the internet.
>>>
>>> What alternatives do we have? Assuming grub is the only offender we have to
>>> worry about, is there a way we can identify that the allocation came from an
>>> unpatched version? Or have a database of hashes (with all known grub
>>> binaries that have this bug in existing isos) that would force disable NX
>>> protection for it if we hit it? Or disable NX when Secure Boot is disabled?
>>>
>>> I really think we need to be a bit more creative than make NX mandatory in
>>> all cases when we know the are immutable images out there that won't work
>>> with it, otherwise we break very legit use cases.
>> While it has its own issues, I suppose another way to go about it is
>> for distributors to provide multiple AAVMF_CODE images, and perhaps
>> invent a "feature" flag for the json descriptor for management tools
>> to select an appropriate one.
>>
> I don't think having different versions of the image makes sense, tbh,
> but of course, this is up to the distros.
>
> Compatibility with ancient downstream GRUB builds is not a goal of the
> EDK2 upstream, so as long as distros can tweak the build to their
> needs, I don't see a reason to revert this change upstream.


First of all, I don't think we should revert this change. We should 
augment it to give users the out-of-the-box experience they expect.

On top of that, I don't think it's a problem of "distros". Every 
consumer of AAVMF will run into this problem as soon as their users will 
want to run any Red Hat OS (installer / image) all the way into 2022. 
That's  very likely >90% of the user base. Because of that, I'm pretty 
sure no Cloud vendor will be able to enable NX in its current shape for 
example.

I'm very happy to see NX proliferate through the stack, but let's please 
make sure we do it compatibly by default, otherwise the net result is 
that *everyone* who compiles AAVMF will disable NX by default again - 
and all you end up with is more frustration and more downstream code / 
forks.

IMHO the most obvious approach would be a fingerprint based override. 
There should be a finite number of known broken grub binaries. If we 
just maintain a database with them and then apply some magic when we 
detect such a binary gets loaded, we'll have tightened security by 
default, without breaking backwards compat.

For environments that know they're running in environments with CC 
requirements, we can automatically disable the fingerprint override :).


Alex


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-04 13:13               ` Alexander Graf
@ 2023-01-05  0:09                 ` Alexander Graf
  2023-01-05  8:11                   ` Gerd Hoffmann
  0 siblings, 1 reply; 49+ messages in thread
From: Alexander Graf @ 2023-01-05  0:09 UTC (permalink / raw)
  To: Ard Biesheuvel, dann frazier; +Cc: devel, kraxel, Leif Lindholm


On 04.01.23 14:13, Alexander Graf wrote:
>
> On 04.01.23 10:35, Ard Biesheuvel wrote:
>> On Tue, 3 Jan 2023 at 23:47, dann frazier 
>> <dann.frazier@canonical.com> wrote:
>>> On Tue, Jan 03, 2023 at 08:39:24PM +0100, Alexander Graf wrote:
>>>> Hey Ard,
>>>>
>>>> On 03.01.23 10:59, Ard Biesheuvel wrote:
>>>>> On Thu, 29 Dec 2022 at 19:00, dann frazier 
>>>>> <dann.frazier@canonical.com> wrote:
>>>>>> On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
>>>>>>> On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
>>>>>>>> When the memory protections were implemented and enabled on 
>>>>>>>> ArmVirtQemu
>>>>>>>> 5+ years ago, we had to work around the fact that GRUB at the time
>>>>>>>> expected EFI_LOADER_DATA to be executable, as that is the 
>>>>>>>> memory type it
>>>>>>>> allocates when loading its modules.
>>>>>>>>
>>>>>>>> This has been fixed in GRUB in August 2017, so by now, we 
>>>>>>>> should be able
>>>>>>>> to tighten this, and remove execute permissions from 
>>>>>>>> EFI_LOADER_DATA
>>>>>>>> allocations.
>>>>>>> Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
>>>>>>> tl;dr: fedora 37 grub.efi is still broken.
>>>>>> This is also the case with existing Ubuntu releases, as well as
>>>>>> AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
>>>>>> the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert 
>>>>>> this
>>>>>> patch in Debian/Ubuntu until it is more ubiquitous. Do you want 
>>>>>> to do
>>>>>> the same upstream? I'm not sure at what point it would make sense to
>>>>>> reintroduce it, given we can't force users to upgrade their 
>>>>>> bootloaders.
>>>>>>
>>>>> Thanks for the report.
>>>>>
>>>>> You can override PCDs on the build command line, so I suggest you use
>>>>> that for building these images as long as it is needed.
>>>>>
>>>>> E.g,, append this to the build.sh command line
>>>>>
>>>>> --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
>>>>>
>>>>> to undo the effects of this patch.
>>>>>
>>>>> I do not intend to revert this patch - the trend under EFI is towards
>>>>> much stricter memory permissions, also on the MS side, and this is
>>>>> especially important under CC scenarios. And if 5+ years is not
>>>>> sufficient for out-of-tree GRUB to catch up, what is the point of
>>>>> waiting for it?
>>>>
>>>> I think we need to be smarter here. ArmVirtPkg is used by a lot of
>>>> virtualization software - such as QEMU. Typically, users (and 
>>>> developers)
>>>> expect that an update to a newer version will preserve compatibility.
>>>>
>>>> Let me contrive an example: In 1 year, QEMU updates to the latest 
>>>> AAVMF. It
>>>> ships that as part of its pc-bios directory. Suddenly, we 
>>>> accidentally break
>>>> old (immutable!) iso images that used to work. So someone that 
>>>> updates QEMU
>>>> to a newer version will have a regression in running a Fedora 37
>>>> installation. Or RHEL 8.7 installation. Not good :).
>>>>
>>>> Outside of OSVs providing new iso images, there is also not much 
>>>> you can do
>>>> about this. The grub binary here runs way before any updater code 
>>>> that could
>>>> pull a fixed version from the internet.
>>>>
>>>> What alternatives do we have? Assuming grub is the only offender we 
>>>> have to
>>>> worry about, is there a way we can identify that the allocation 
>>>> came from an
>>>> unpatched version? Or have a database of hashes (with all known grub
>>>> binaries that have this bug in existing isos) that would force 
>>>> disable NX
>>>> protection for it if we hit it? Or disable NX when Secure Boot is 
>>>> disabled?
>>>>
>>>> I really think we need to be a bit more creative than make NX 
>>>> mandatory in
>>>> all cases when we know the are immutable images out there that 
>>>> won't work
>>>> with it, otherwise we break very legit use cases.
>>> While it has its own issues, I suppose another way to go about it is
>>> for distributors to provide multiple AAVMF_CODE images, and perhaps
>>> invent a "feature" flag for the json descriptor for management tools
>>> to select an appropriate one.
>>>
>> I don't think having different versions of the image makes sense, tbh,
>> but of course, this is up to the distros.
>>
>> Compatibility with ancient downstream GRUB builds is not a goal of the
>> EDK2 upstream, so as long as distros can tweak the build to their
>> needs, I don't see a reason to revert this change upstream.
>
>
> First of all, I don't think we should revert this change. We should 
> augment it to give users the out-of-the-box experience they expect.
>
> On top of that, I don't think it's a problem of "distros". Every 
> consumer of AAVMF will run into this problem as soon as their users 
> will want to run any Red Hat OS (installer / image) all the way into 
> 2022. That's  very likely >90% of the user base. Because of that, I'm 
> pretty sure no Cloud vendor will be able to enable NX in its current 
> shape for example.
>
> I'm very happy to see NX proliferate through the stack, but let's 
> please make sure we do it compatibly by default, otherwise the net 
> result is that *everyone* who compiles AAVMF will disable NX by 
> default again - and all you end up with is more frustration and more 
> downstream code / forks.
>
> IMHO the most obvious approach would be a fingerprint based override. 
> There should be a finite number of known broken grub binaries. If we 
> just maintain a database with them and then apply some magic when we 
> detect such a binary gets loaded, we'll have tightened security by 
> default, without breaking backwards compat.
>
> For environments that know they're running in environments with CC 
> requirements, we can automatically disable the fingerprint override :).


To clarify, I mean something like the patch below, but with an 
additional callback notification similar to the Emu one in LoadImage(), 
so that we can make sure we only enable the quirk when we load a 
known-bad grub binary. That way we still force distros to ship fixed 
versions of their code, but enable old code to continue running.


Alex


diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c 
b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 3ad1ecd9d2..365eb1c157 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -902,6 +902,25 @@ PlatformBootManagerBeforeConsole (
    FilterAndProcess (&gEfiPciIoProtocolGuid, IsVirtioPciRng, Connect);
  }

+static EFI_ALLOCATE_PAGES RealAllocatePages;
+
+static EFI_STATUS EFIAPI AllocatePagesForceLoaderCode(
+  IN     EFI_ALLOCATE_TYPE            Type,
+  IN     EFI_MEMORY_TYPE              MemoryType,
+  IN     UINTN                        Pages,
+  IN OUT EFI_PHYSICAL_ADDRESS         *Memory
+)
+{
+    /*
+     * Broken grub versions do LoaderData allocations for code. Let's patch
+     * them into LoaderCode allocations instead.
+     */
+    if (MemoryType == EfiLoaderData)
+        MemoryType = EfiLoaderCode;
+
+    return RealAllocatePages(Type, MemoryType, Pages, Memory);
+}
+
  /**
    Do the platform specific action after the console is ready
    Possible things that can be done in PlatformBootManagerAfterConsole:
@@ -964,6 +983,14 @@ PlatformBootManagerAfterConsole (
    SetBootOrderFromQemu ();

    PlatformBmPrintScRegisterHandler ();
+
+  /* TODO: Only run this as part of a notify callback in ImageLoad() 
when we
+           load a grub binary with a known-broken hash */
+  BOOLEAN is_broken_grub = TRUE;
+  if (is_broken_grub) {
+    RealAllocatePages = gBS->AllocatePages;
+    gBS->AllocatePages = AllocatePagesForceLoaderCode;
+  }
  }

  /**


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-05  0:09                 ` Alexander Graf
@ 2023-01-05  8:11                   ` Gerd Hoffmann
  2023-01-05  8:43                     ` Alexander Graf
  0 siblings, 1 reply; 49+ messages in thread
From: Gerd Hoffmann @ 2023-01-05  8:11 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Ard Biesheuvel, dann frazier, devel, Leif Lindholm

  Hi,

> To clarify, I mean something like the patch below, but with an additional
> callback notification similar to the Emu one in LoadImage(), so that we can
> make sure we only enable the quirk when we load a known-bad grub binary.
> That way we still force distros to ship fixed versions of their code, but
> enable old code to continue running.

> +  /* TODO: Only run this as part of a notify callback in ImageLoad() when
> we
> +           load a grub binary with a known-broken hash */
> +  BOOLEAN is_broken_grub = TRUE;
> +  if (is_broken_grub) {
> +    RealAllocatePages = gBS->AllocatePages;
> +    gBS->AllocatePages = AllocatePagesForceLoaderCode;
> +  }

You left out the hard part, which is the list of hashes.  And I suspect
you underestimate the number of broken grub binaries in the wild ...

take care,
  Gerd


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-05  8:11                   ` Gerd Hoffmann
@ 2023-01-05  8:43                     ` Alexander Graf
  2023-01-05  9:41                       ` Ard Biesheuvel
  0 siblings, 1 reply; 49+ messages in thread
From: Alexander Graf @ 2023-01-05  8:43 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Ard Biesheuvel, dann frazier, devel, Leif Lindholm



> Am 05.01.2023 um 09:11 schrieb Gerd Hoffmann <kraxel@redhat.com>:
> 
>   Hi,
> 
>> To clarify, I mean something like the patch below, but with an additional
>> callback notification similar to the Emu one in LoadImage(), so that we can
>> make sure we only enable the quirk when we load a known-bad grub binary.
>> That way we still force distros to ship fixed versions of their code, but
>> enable old code to continue running.
> 
>> +  /* TODO: Only run this as part of a notify callback in ImageLoad() when
>> we
>> +           load a grub binary with a known-broken hash */
>> +  BOOLEAN is_broken_grub = TRUE;
>> +  if (is_broken_grub) {
>> +    RealAllocatePages = gBS->AllocatePages;
>> +    gBS->AllocatePages = AllocatePagesForceLoaderCode;
>> +  }
> 
> You left out the hard part, which is the list of hashes.

Yes, I'd crowd source that list. If someone has vested interest to keep their old grub binaries working, they can send an upstream patch to add their hash :). At least we'd have a path forward to make things work that is not "revert NX enablement".

>  And I suspect
> you underestimate the number of broken grub binaries in the wild ...

What number would you expect? I'd hope that we get to <100 realistically.

I'm happy to hear about alternatives to this approach. I'm very confident that forcing NX on always is going to have the opposite effect of what we want: Everyone who ships AAVMF binaries will disable NX because they eventually get bug reports from users that their shiny update regressed some legit use case.

The only alternative I can think of would be logic similar to the patch I sent without any grub hash check: Exclude AllocatePages for LoaderData from the NX logic. Keep NX for any other non-code memory type as well as LoaderData pool allocations.


Alex


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-05  8:43                     ` Alexander Graf
@ 2023-01-05  9:41                       ` Ard Biesheuvel
  2023-01-05 11:19                         ` Gerd Hoffmann
  0 siblings, 1 reply; 49+ messages in thread
From: Ard Biesheuvel @ 2023-01-05  9:41 UTC (permalink / raw)
  To: Alexander Graf; +Cc: Gerd Hoffmann, dann frazier, devel, Leif Lindholm

On Thu, 5 Jan 2023 at 09:43, Alexander Graf <agraf@csgraf.de> wrote:
>
>
>
> > Am 05.01.2023 um 09:11 schrieb Gerd Hoffmann <kraxel@redhat.com>:
> >
> >   Hi,
> >
> >> To clarify, I mean something like the patch below, but with an additional
> >> callback notification similar to the Emu one in LoadImage(), so that we can
> >> make sure we only enable the quirk when we load a known-bad grub binary.
> >> That way we still force distros to ship fixed versions of their code, but
> >> enable old code to continue running.
> >
> >> +  /* TODO: Only run this as part of a notify callback in ImageLoad() when
> >> we
> >> +           load a grub binary with a known-broken hash */
> >> +  BOOLEAN is_broken_grub = TRUE;
> >> +  if (is_broken_grub) {
> >> +    RealAllocatePages = gBS->AllocatePages;
> >> +    gBS->AllocatePages = AllocatePagesForceLoaderCode;
> >> +  }
> >
> > You left out the hard part, which is the list of hashes.
>
> Yes, I'd crowd source that list. If someone has vested interest to keep their old grub binaries working, they can send an upstream patch to add their hash :). At least we'd have a path forward to make things work that is not "revert NX enablement".
>
> >  And I suspect
> > you underestimate the number of broken grub binaries in the wild ...
>
> What number would you expect? I'd hope that we get to <100 realistically.
>
> I'm happy to hear about alternatives to this approach. I'm very confident that forcing NX on always is going to have the opposite effect of what we want: Everyone who ships AAVMF binaries will disable NX because they eventually get bug reports from users that their shiny update regressed some legit use case.
>
> The only alternative I can think of would be logic similar to the patch I sent without any grub hash check: Exclude AllocatePages for LoaderData from the NX logic. Keep NX for any other non-code memory type as well as LoaderData pool allocations.
>

Another thing we might consider is trapping exec permission violations
and switching the pages in question from rw- to r-x.

Does GRUB generally load/map executable modules at page granularity?

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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-05  9:41                       ` Ard Biesheuvel
@ 2023-01-05 11:19                         ` Gerd Hoffmann
  2023-01-05 11:44                           ` Ard Biesheuvel
  0 siblings, 1 reply; 49+ messages in thread
From: Gerd Hoffmann @ 2023-01-05 11:19 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Alexander Graf, dann frazier, devel, Leif Lindholm

  Hi,

> > What number would you expect? I'd hope that we get to <100 realistically.
> >
> > I'm happy to hear about alternatives to this approach. I'm very confident that forcing NX on always is going to have the opposite effect of what we want: Everyone who ships AAVMF binaries will disable NX because they eventually get bug reports from users that their shiny update regressed some legit use case.
> >
> > The only alternative I can think of would be logic similar to the patch I sent without any grub hash check: Exclude AllocatePages for LoaderData from the NX logic. Keep NX for any other non-code memory type as well as LoaderData pool allocations.

> Another thing we might consider is trapping exec permission violations
> and switching the pages in question from rw- to r-x.

That sounds neat, especially as we can print a big'n'fat warning in that
case, so the problem gets attention without actually breaking users.

Looking at the efi calls it looks like edk2 doesn't track the owner of
an allocation (say by image handle), so I suspect it is not possible to
automatically figure who is to blame?

> Does GRUB generally load/map executable modules at page granularity?

I don't think so, at least the code handles modules not being page
aligned.  But I think it's not grub modules, that fix was actually
picked up meanwhile.  But there are downstream patches for image
loader code which look suspicious to me ...

take care,
  Gerd


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-05 11:19                         ` Gerd Hoffmann
@ 2023-01-05 11:44                           ` Ard Biesheuvel
  2023-01-05 15:12                             ` Gerd Hoffmann
  2023-01-05 23:37                             ` Alexander Graf
  0 siblings, 2 replies; 49+ messages in thread
From: Ard Biesheuvel @ 2023-01-05 11:44 UTC (permalink / raw)
  To: devel, kraxel; +Cc: Alexander Graf, dann frazier, Leif Lindholm

On Thu, 5 Jan 2023 at 12:19, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > What number would you expect? I'd hope that we get to <100 realistically.
> > >
> > > I'm happy to hear about alternatives to this approach. I'm very confident that forcing NX on always is going to have the opposite effect of what we want: Everyone who ships AAVMF binaries will disable NX because they eventually get bug reports from users that their shiny update regressed some legit use case.
> > >
> > > The only alternative I can think of would be logic similar to the patch I sent without any grub hash check: Exclude AllocatePages for LoaderData from the NX logic. Keep NX for any other non-code memory type as well as LoaderData pool allocations.
>
> > Another thing we might consider is trapping exec permission violations
> > and switching the pages in question from rw- to r-x.
>
> That sounds neat, especially as we can print a big'n'fat warning in that
> case, so the problem gets attention without actually breaking users.
>

That, and a sleep(5)

> Looking at the efi calls it looks like edk2 doesn't track the owner of
> an allocation (say by image handle), so I suspect it is not possible to
> automatically figure who is to blame?
>
> > Does GRUB generally load/map executable modules at page granularity?
>
> I don't think so, at least the code handles modules not being page
> aligned.  But I think it's not grub modules, that fix was actually
> picked up meanwhile.  But there are downstream patches for image
> loader code which look suspicious to me ...
>

OK, so the GRUB PE/COFF loader strikes again :-(

So shim may be broken in the exact same way, and the things shim loads
may not adhere to page alignment for the sections. Loading the kernel
itself this way should be fine, though - we always had section
alignment in the EFI stub kernel.

The downside of this approach is that it can only be done on a
page-by-page basis, given that the EFI_LOADER_DATA region in question
will cover both the .text/.rodata and the .data/.bss of the loaded
image.

Could someone check/confirm whether shim builds need to be take into
account here? Thanks.

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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-05 11:44                           ` Ard Biesheuvel
@ 2023-01-05 15:12                             ` Gerd Hoffmann
  2023-01-05 19:58                               ` Gerd Hoffmann
  2023-01-05 23:37                             ` Alexander Graf
  1 sibling, 1 reply; 49+ messages in thread
From: Gerd Hoffmann @ 2023-01-05 15:12 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: devel, Alexander Graf, dann frazier, Leif Lindholm

  Hi,

> > That sounds neat, especially as we can print a big'n'fat warning in that
> > case, so the problem gets attention without actually breaking users.
> >
> 
> That, and a sleep(5)
> 
> > Looking at the efi calls it looks like edk2 doesn't track the owner of
> > an allocation (say by image handle), so I suspect it is not possible to
> > automatically figure who is to blame?
> >
> > > Does GRUB generally load/map executable modules at page granularity?
> >
> > I don't think so, at least the code handles modules not being page
> > aligned.  But I think it's not grub modules, that fix was actually
> > picked up meanwhile.  But there are downstream patches for image
> > loader code which look suspicious to me ...
> 
> OK, so the GRUB PE/COFF loader strikes again :-(

Yep.

> Could someone check/confirm whether shim builds need to be take into
> account here? Thanks.

Tried booting grub.efi directly and via shim.efi, on Fedora 37 GA.

In both cases I get a pagefault on linux kernel boot (before any other
message is printed), which I guess happens because the loader places the
linux kernel efi stub in EfiLoaderData memory.

I'd say that confirms grub.efi being buggy.

Not sure about shim.efi.  It managed to run grub.efi without hitting a
fault, which is good.  But it also installs efi protocols for the boot
loader to call, so it could be involved too.  But maybe that happens
only in case secure boot is active, which is not supported by
ArmVirtPkg.

take care,
  Gerd


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-05 15:12                             ` Gerd Hoffmann
@ 2023-01-05 19:58                               ` Gerd Hoffmann
  2023-01-06  2:19                                 ` Sean
  0 siblings, 1 reply; 49+ messages in thread
From: Gerd Hoffmann @ 2023-01-05 19:58 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Alexander Graf, dann frazier, Leif Lindholm

  Hi,

> Tried booting grub.efi directly and via shim.efi, on Fedora 37 GA.
> 
> In both cases I get a pagefault on linux kernel boot (before any other
> message is printed), which I guess happens because the loader places the
> linux kernel efi stub in EfiLoaderData memory.

When compiling ovmf with the same pcd value I get the same behavior
on x64, so it's consistently buggy across architectures.

take care,
  Gerd


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-05 11:44                           ` Ard Biesheuvel
  2023-01-05 15:12                             ` Gerd Hoffmann
@ 2023-01-05 23:37                             ` Alexander Graf
  1 sibling, 0 replies; 49+ messages in thread
From: Alexander Graf @ 2023-01-05 23:37 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: devel, kraxel, dann frazier, Leif Lindholm



> Am 05.01.2023 um 12:44 schrieb Ard Biesheuvel <ardb@kernel.org>:
> 
> On Thu, 5 Jan 2023 at 12:19, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> 
>>  Hi,
>> 
>>>> What number would you expect? I'd hope that we get to <100 realistically.
>>>> 
>>>> I'm happy to hear about alternatives to this approach. I'm very confident that forcing NX on always is going to have the opposite effect of what we want: Everyone who ships AAVMF binaries will disable NX because they eventually get bug reports from users that their shiny update regressed some legit use case.
>>>> 
>>>> The only alternative I can think of would be logic similar to the patch I sent without any grub hash check: Exclude AllocatePages for LoaderData from the NX logic. Keep NX for any other non-code memory type as well as LoaderData pool allocations.
>> 
>>> Another thing we might consider is trapping exec permission violations
>>> and switching the pages in question from rw- to r-x.
>> 
>> That sounds neat, especially as we can print a big'n'fat warning in that
>> case, so the problem gets attention without actually breaking users.
>> 
> 
> That, and a sleep(5)

I like the direction this is moving :)

> 
>> Looking at the efi calls it looks like edk2 doesn't track the owner of
>> an allocation (say by image handle), so I suspect it is not possible to
>> automatically figure who is to blame?
>> 
>>> Does GRUB generally load/map executable modules at page granularity?
>> 
>> I don't think so, at least the code handles modules not being page
>> aligned.  But I think it's not grub modules, that fix was actually
>> picked up meanwhile.  But there are downstream patches for image
>> loader code which look suspicious to me ...
>> 
> 
> OK, so the GRUB PE/COFF loader strikes again :-(
> 
> So shim may be broken in the exact same way, and the things shim loads
> may not adhere to page alignment for the sections. Loading the kernel
> itself this way should be fine, though - we always had section
> alignment in the EFI stub kernel.
> 
> The downside of this approach is that it can only be done on a
> page-by-page basis, given that the EFI_LOADER_DATA region in question
> will cover both the .text/.rodata and the .data/.bss of the loaded
> image.

Does it have to be r-x instead of rwx? If we add the warning and sleep(5), that should hopefully give enough incentive already to OSVs to fix their grub binaries :). And then we don't even need to think about alignment.

If I map a region as LoaderCode, I get rwx as well, no? So we effectively make LoaderData behave like LoaderCode plus warning plus sleep.


Alex

> 
> Could someone check/confirm whether shim builds need to be take into
> account here? Thanks.

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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-05 19:58                               ` Gerd Hoffmann
@ 2023-01-06  2:19                                 ` Sean
  2023-01-06  8:44                                   ` Gerd Hoffmann
  0 siblings, 1 reply; 49+ messages in thread
From: Sean @ 2023-01-06  2:19 UTC (permalink / raw)
  To: devel, kraxel; +Cc: Ard Biesheuvel, Alexander Graf, dann frazier, Leif Lindholm


[-- Attachment #1.1: Type: text/plain, Size: 3598 bytes --]

Gerd/Ard,

This is a great topic and shows some of the challenges of tightening up 
security/enforcing correctness in UEFI.

I wanted to let you know that we have been doing a lot of work in both 
edk2 firmware and discussing with partners in the Linux community and PC 
ecosystem.  The Shim and Grub teams have been directly engaged and have 
code patches that correct a number of problems as well as make their 
code compliant with even tighter restrictions.   Engineers from redhat, 
canonical, oracle, and others have all been involved.  Also note 
Microsoft's UEFI CA now requires submissions to be compliant with a 
strict set of memory mitigations. UEFI CA Memory Mitigation Requirements 
for Signing - Windows drivers | Microsoft Learn 
<https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/uefi-ca-memory-mitigation-requirements> 
and UPDATED: UEFI Signing Requirements - Microsoft Community Hub 
<https://techcommunity.microsoft.com/t5/hardware-dev-center/updated-uefi-signing-requirements/ba-p/1062916>.  
This means any new shim submitted must now meet these requirements. How 
long it takes for distros to update to a new shim is still unknown.


Hopefully sometime in the next few weeks we can prepare a comprehensive 
set of patches and changes needed in edk2 to implement this strict 
environment.  One of the relevant changes to this discussion and patch 
series is we switched the configuration from PCD to hob 
(mu_basecore/DxeMemoryProtectionSettings.h at release/202208 · 
microsoft/mu_basecore (github.com) 
<https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModulePkg/Include/Guid/DxeMemoryProtectionSettings.h>). 
This allows our platforms complete control of the config per boot.  Some 
platforms have compatibility requirements and have implemented code so 
that when a PF is triggered by incompatible software, it is recorded and 
then the system rebooted.  On the next boot the platform changes the HOB 
configuration to be in a more compatible mode (this mode could be 
measured in a PCR and/or other hints to the user/system of degraded 
security).  We hope this balance makes compatibility possible but 
inconvenient and with a less than desirable user experience.  Will this 
help move the industry, I don't know?


Anyway, rather than a patchwork of changes going into different 
platforms and components of edk2 I would like to see alignment between 
x86/arm64 in edk2 and a complete set of changes for edk2. We have 
developed a tool and some tests that can capture the memory environment 
in DXE and export it to then allow a developer to review.  This has 
identified dozens of problems in edk2 code as well as platform code.  
See attached a report file showing a passing report for our q35 qemu 
based platform. mu_tiano_platforms/Platforms/QemuQ35Pkg at main · 
microsoft/mu_tiano_platforms (github.com) 
<https://github.com/microsoft/mu_tiano_platforms/tree/main/Platforms/QemuQ35Pkg>  
We are still working on getting the equivalent for arm64.   Happy to 
discuss more if anyone else is interested.


Thanks

sean



On 1/5/2023 11:58 AM, Gerd Hoffmann wrote:
>    Hi,
>
>> Tried booting grub.efi directly and via shim.efi, on Fedora 37 GA.
>>
>> In both cases I get a pagefault on linux kernel boot (before any other
>> message is printed), which I guess happens because the loader places the
>> linux kernel efi stub in EfiLoaderData memory.
> When compiling ovmf with the same pcd value I get the same behavior
> on x64, so it's consistently buggy across architectures.
>
> take care,
>    Gerd
>
>
>
> 
>
>

[-- Attachment #1.2: Type: text/html, Size: 4596 bytes --]

[-- Attachment #2: Q35_Paging_Audit_With_Stack_Null.html --]
[-- Type: text/html, Size: 4052714 bytes --]

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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-06  2:19                                 ` Sean
@ 2023-01-06  8:44                                   ` Gerd Hoffmann
  0 siblings, 0 replies; 49+ messages in thread
From: Gerd Hoffmann @ 2023-01-06  8:44 UTC (permalink / raw)
  To: Sean Brogan
  Cc: devel, Ard Biesheuvel, Alexander Graf, dann frazier,
	Leif Lindholm

  Hi,

> Hopefully sometime in the next few weeks we can prepare a comprehensive set
> of patches and changes needed in edk2 to implement this strict environment. 
> One of the relevant changes to this discussion and patch series is we
> switched the configuration from PCD to hob
> (mu_basecore/DxeMemoryProtectionSettings.h at release/202208 ·
> microsoft/mu_basecore (github.com) <https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModulePkg/Include/Guid/DxeMemoryProtectionSettings.h>).
> This allows our platforms complete control of the config per boot.

Why a HOB?  I guess because dynamic PCDs are available too late in the
boot process?

> Some platforms have compatibility requirements and have implemented
> code so that when a PF is triggered by incompatible software, it is
> recorded and then the system rebooted.  On the next boot the platform
> changes the HOB configuration to be in a more compatible mode (this
> mode could be measured in a PCR and/or other hints to the user/system
> of degraded security).

Where is the configuration stored?

> Anyway, rather than a patchwork of changes going into different platforms
> and components of edk2 I would like to see alignment between x86/arm64 in
> edk2 and a complete set of changes for edk2.

Sure, it totally makes sense to have that as core edk2 feature instead
of adding this to each platform individually.

Looking forward to see the patches.

take care,
  Gerd


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-04 11:11               ` Gerd Hoffmann
  2023-01-04 12:04                 ` Ard Biesheuvel
@ 2023-01-06  9:55                 ` Laszlo Ersek
  2023-01-06 10:06                   ` Laszlo Ersek
  1 sibling, 1 reply; 49+ messages in thread
From: Laszlo Ersek @ 2023-01-06  9:55 UTC (permalink / raw)
  To: devel, kraxel, Ard Biesheuvel
  Cc: dann frazier, Alexander Graf, Leif Lindholm, Sean Brogan

On 1/4/23 12:11, Gerd Hoffmann wrote:

> The versions are not that ancient.  The problem is more that upstream
> grub is really slow on integrating patches so every distro does carry
> a huge pile of downstream patches.  And they seem to re-introduce the
> bug ...

See also: commit d20b06a3afdf ("OvmfPkg: disable no-exec DXE stack by
default", 2015-09-15). That was more than seven years ago, and it's what

  git blame master -- OvmfPkg/OvmfPkgX64.dsc | grep PcdSetNxForStack

reports today.

Laszlo


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

* Re: [edk2-devel] [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable
  2023-01-06  9:55                 ` Laszlo Ersek
@ 2023-01-06 10:06                   ` Laszlo Ersek
  0 siblings, 0 replies; 49+ messages in thread
From: Laszlo Ersek @ 2023-01-06 10:06 UTC (permalink / raw)
  To: devel, kraxel, Ard Biesheuvel
  Cc: dann frazier, Alexander Graf, Leif Lindholm, Sean Brogan

On 1/6/23 10:55, Laszlo Ersek wrote:
> On 1/4/23 12:11, Gerd Hoffmann wrote:
> 
>> The versions are not that ancient.  The problem is more that upstream
>> grub is really slow on integrating patches so every distro does carry
>> a huge pile of downstream patches.  And they seem to re-introduce the
>> bug ...
> 
> See also: commit d20b06a3afdf ("OvmfPkg: disable no-exec DXE stack by
> default", 2015-09-15). That was more than seven years ago, and it's what
> 
>   git blame master -- OvmfPkg/OvmfPkgX64.dsc | grep PcdSetNxForStack
> 
> reports today.

On a more constructive note.

By the book, this guest OS-level quirk should be exposed from the
firmware up to libosinfo / osinfo-db.

Starting with a dynamic PCD or HOB exposed via fw_cfg (with the fw_cfg
filename conforming to "docs/specs/fw_cfg.rst" in QEMU), handled by
libvirtd and other management applications (domain XML and other config
formats, matching code, etc), and ultimately recorded in a "w^x"
capability entry in the libosinfo schema and the osinfo database.

All other guest OS compatibility settings are tracked in osinfo
nowadays, security related or not, and they are so important that recent
virt-install even refuses (by default) to install a domain if it doesn't
recognize (and the user doesn't say) what the guest OS is.

https://github.com/virt-manager/virt-manager/commit/26ecf8a5e3e4721488159605afd10e39f68e6382

Those settings control various CPU vulnerability mitigations even, IIUC,
so it's almost certainly the right place to implement this new quirk too.

Let us not sweep it under the carpet, and heap on more technical debt.
Storing grub hashes in the firmware is similar to Windows video drivers
tailoring themselves to the game the user happens to start. "Tailoring"
is fine, but not from the bottom up.

Here's what we could do for, and in, ArmVirtQemu *upstream*:

- file the proper RFEs for the above-described components,

- get their maintainers publicly commit to implementing them (that will
take a while),

- once each RFE has been committed to, and we think the whole picture is
covered, downgrade the "w^x" default in ArmVirtQemu as follows:

- list the ticket links near the code that does the downgrade

- *gate* the downgrade on the platform RTC reading a date that's before
a specific, open-coded constant. We'll forget about the downgrade, but
the RTC won't forget about time passing. This will make us revise the
concession in time (unlike how we've completely forgotten about
PcdSetNxForStack). Once all the RFEs have been fixed upstream, and
widely shipped in products, we can remove the downgrade.

Laszlo


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

end of thread, other threads:[~2023-01-06 10:07 UTC | newest]

Thread overview: 49+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-26  8:24 [PATCH v3 00/16] ArmVirtPkg/ArmVirtQemu: Performance streamlining Ard Biesheuvel
2022-09-26  8:24 ` [PATCH v3 01/16] ArmVirtPkg: remove EbcDxe from all platforms Ard Biesheuvel
2022-09-26  8:24 ` [PATCH v3 02/16] ArmVirtPkg: do not enable iSCSI driver by default Ard Biesheuvel
2022-09-26  8:24 ` [PATCH v3 03/16] ArmVirtPkg: make EFI_LOADER_DATA non-executable Ard Biesheuvel
2022-09-26 22:28   ` [edk2-devel] " Leif Lindholm
2022-11-28 15:46   ` Gerd Hoffmann
2022-12-29 18:00     ` dann frazier
2023-01-03  9:59       ` Ard Biesheuvel
2023-01-03 19:39         ` Alexander Graf
2023-01-03 22:47           ` dann frazier
2023-01-04  9:35             ` Ard Biesheuvel
2023-01-04 11:11               ` Gerd Hoffmann
2023-01-04 12:04                 ` Ard Biesheuvel
2023-01-04 12:56                   ` Gerd Hoffmann
2023-01-06  9:55                 ` Laszlo Ersek
2023-01-06 10:06                   ` Laszlo Ersek
2023-01-04 13:13               ` Alexander Graf
2023-01-05  0:09                 ` Alexander Graf
2023-01-05  8:11                   ` Gerd Hoffmann
2023-01-05  8:43                     ` Alexander Graf
2023-01-05  9:41                       ` Ard Biesheuvel
2023-01-05 11:19                         ` Gerd Hoffmann
2023-01-05 11:44                           ` Ard Biesheuvel
2023-01-05 15:12                             ` Gerd Hoffmann
2023-01-05 19:58                               ` Gerd Hoffmann
2023-01-06  2:19                                 ` Sean
2023-01-06  8:44                                   ` Gerd Hoffmann
2023-01-05 23:37                             ` Alexander Graf
2022-09-26  8:24 ` [PATCH v3 04/16] ArmVirtPkg/ArmVirtQemu: wire up timeout PCD to Timeout variable Ard Biesheuvel
2022-09-26  8:25 ` [PATCH v3 05/16] ArmPkg/ArmMmuLib: don't replace table entries with block entries Ard Biesheuvel
2022-09-26 22:32   ` Leif Lindholm
2022-09-26  8:25 ` [PATCH v3 06/16] ArmPkg/ArmMmuLib: Disable and re-enable MMU only when needed Ard Biesheuvel
2022-09-26 23:28   ` Leif Lindholm
2022-09-26  8:25 ` [PATCH v3 07/16] ArmPkg/ArmMmuLib: permit initial configuration with MMU enabled Ard Biesheuvel
2022-09-26 22:35   ` Leif Lindholm
2022-09-26  8:25 ` [PATCH v3 08/16] ArmPkg/ArmMmuLib: Reuse XIP MMU routines when splitting entries Ard Biesheuvel
2022-09-26 22:38   ` Leif Lindholm
2022-09-26  8:25 ` [PATCH v3 09/16] ArmPlatformPkg/PrePeiCore: permit entry with the MMU enabled Ard Biesheuvel
2022-09-26 22:39   ` [edk2-devel] " Leif Lindholm
2022-09-26  8:25 ` [PATCH v3 10/16] ArmVirtPkg/ArmVirtQemu: implement ArmPlatformLib with static ID map Ard Biesheuvel
2022-09-26  8:25 ` [PATCH v3 11/16] ArmVirtPkg/ArmVirtQemu: use first 128 MiB as permanent PEI memory Ard Biesheuvel
2022-09-26  8:25 ` [PATCH v3 12/16] ArmVirtPkg/ArmVirtQemu: enable initial ID map at early boot Ard Biesheuvel
2022-12-29 21:10   ` [edk2-devel] " dann frazier
2023-01-03  9:02     ` Ard Biesheuvel
2023-01-03 19:38       ` dann frazier
2022-09-26  8:25 ` [PATCH v3 13/16] ArmVirtPkg/ArmVirtQemu: Drop unused variable PEIM Ard Biesheuvel
2022-09-26  8:25 ` [PATCH v3 14/16] ArmVirtPkg/ArmVirtQemu: avoid shadowing PEIMs unless necessary Ard Biesheuvel
2022-09-26  8:25 ` [PATCH v3 15/16] ArmVirtPkg/QemuVirtMemInfoLib: use HOB not PCD to record the memory size Ard Biesheuvel
2022-09-26  8:25 ` [PATCH v3 16/16] ArmVirtPkg/ArmVirtQemu: omit PCD PEIM unless TPM support is enabled Ard Biesheuvel

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