* [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