public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ard Biesheuvel" <ardb@kernel.org>
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb@kernel.org>,
	Leif Lindholm <quic_llindhol@quicinc.com>,
	Alexander Graf <agraf@csgraf.de>
Subject: [PATCH v3 06/16] ArmPkg/ArmMmuLib: Disable and re-enable MMU only when needed
Date: Mon, 26 Sep 2022 10:25:01 +0200	[thread overview]
Message-ID: <20220926082511.2110797-7-ardb@kernel.org> (raw)
In-Reply-To: <20220926082511.2110797-1-ardb@kernel.org>

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


  parent reply	other threads:[~2022-09-26  8:25 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 ` Ard Biesheuvel [this message]
2022-09-26 23:28   ` [PATCH v3 06/16] ArmPkg/ArmMmuLib: Disable and re-enable MMU only when needed 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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220926082511.2110797-7-ardb@kernel.org \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox