ArmPkg/ArmMmuLib: Disable and re-enable MMU only when needed

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>
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h
index 7538a8274a72e70d51afc274920c64e894b0372d..b745e2230e7e178cea7afe7a0dc0a662b29867c7 100644 (file)
--- a/ArmPkg/Include/Library/ArmMmuLib.h
+++ b/ArmPkg/Include/Library/ArmMmuLib.h
@@ -52,9 +52,10 @@ ArmClearMemoryRegionReadOnly (
 VOID\r
 EFIAPI\r
 ArmReplaceLiveTranslationEntry (\r
-  IN  UINT64  *Entry,\r
-  IN  UINT64  Value,\r
-  IN  UINT64  RegionStart\r
+  IN  UINT64   *Entry,\r
+  IN  UINT64   Value,\r
+  IN  UINT64   RegionStart,\r
+  IN  BOOLEAN  DisableMmu\r
   );\r
 \r
 EFI_STATUS\r

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 34f1031c4de316611476b52ae43fd42887663458..4d75788ed2b2356180058d7e3b0a8d0d2e7c5503 100644 (file)
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -18,6 +18,17 @@
 #include <Library/ArmMmuLib.h>\r
 #include <Library/BaseLib.h>\r
 #include <Library/DebugLib.h>\r
+#include <Library/HobLib.h>\r
+\r
+STATIC\r
+VOID (\r
+  EFIAPI  *mReplaceLiveEntryFunc\r
+  )(\r
+    IN  UINT64  *Entry,\r
+    IN  UINT64  Value,\r
+    IN  UINT64  RegionStart,\r
+    IN  BOOLEAN DisableMmu\r
+    ) = ArmReplaceLiveTranslationEntry;\r
 \r
 STATIC\r
 UINT64\r
@@ -83,14 +94,40 @@ ReplaceTableEntry (
   IN  UINT64   *Entry,\r
   IN  UINT64   Value,\r
   IN  UINT64   RegionStart,\r
+  IN  UINT64   BlockMask,\r
   IN  BOOLEAN  IsLiveBlockMapping\r
   )\r
 {\r
-  if (!ArmMmuEnabled () || !IsLiveBlockMapping) {\r
+  BOOLEAN  DisableMmu;\r
+\r
+  //\r
+  // Replacing a live block entry with a table entry (or vice versa) requires a\r
+  // break-before-make sequence as per the architecture. This means the mapping\r
+  // must be made invalid and cleaned from the TLBs first, and this is a bit of\r
+  // a hassle if the mapping in question covers the code that is actually doing\r
+  // the mapping and the unmapping, and so we only bother with this if actually\r
+  // necessary.\r
+  //\r
+\r
+  if (!IsLiveBlockMapping || !ArmMmuEnabled ()) {\r
+    // If the mapping is not a live block mapping, or the MMU is not on yet, we\r
+    // can simply overwrite the entry.\r
     *Entry = Value;\r
     ArmUpdateTranslationTableEntry (Entry, (VOID *)(UINTN)RegionStart);\r
   } else {\r
-    ArmReplaceLiveTranslationEntry (Entry, Value, RegionStart);\r
+    // If the mapping in question does not cover the code that updates the\r
+    // entry in memory, or the entry that we are intending to update, we can\r
+    // use an ordinary break before make. Otherwise, we will need to\r
+    // temporarily disable the MMU.\r
+    DisableMmu = FALSE;\r
+    if ((((RegionStart ^ (UINTN)ArmReplaceLiveTranslationEntry) & ~BlockMask) == 0) ||\r
+        (((RegionStart ^ (UINTN)Entry) & ~BlockMask) == 0))\r
+    {\r
+      DisableMmu = TRUE;\r
+      DEBUG ((DEBUG_WARN, "%a: splitting block entry with MMU disabled\n", __FUNCTION__));\r
+    }\r
+\r
+    ArmReplaceLiveTranslationEntry (Entry, Value, RegionStart, DisableMmu);\r
   }\r
 }\r
 \r
@@ -155,12 +192,13 @@ IsTableEntry (
 STATIC\r
 EFI_STATUS\r
 UpdateRegionMappingRecursive (\r
-  IN  UINT64  RegionStart,\r
-  IN  UINT64  RegionEnd,\r
-  IN  UINT64  AttributeSetMask,\r
-  IN  UINT64  AttributeClearMask,\r
-  IN  UINT64  *PageTable,\r
-  IN  UINTN   Level\r
+  IN  UINT64   RegionStart,\r
+  IN  UINT64   RegionEnd,\r
+  IN  UINT64   AttributeSetMask,\r
+  IN  UINT64   AttributeClearMask,\r
+  IN  UINT64   *PageTable,\r
+  IN  UINTN    Level,\r
+  IN  BOOLEAN  TableIsLive\r
   )\r
 {\r
   UINTN       BlockShift;\r
@@ -170,6 +208,7 @@ UpdateRegionMappingRecursive (
   UINT64      EntryValue;\r
   VOID        *TranslationTable;\r
   EFI_STATUS  Status;\r
+  BOOLEAN     NextTableIsLive;\r
 \r
   ASSERT (((RegionStart | RegionEnd) & EFI_PAGE_MASK) == 0);\r
 \r
@@ -198,7 +237,14 @@ UpdateRegionMappingRecursive (
     // the next level. No block mappings are allowed at all at level 0,\r
     // so in that case, we have to recurse unconditionally.\r
     //\r
+    // One special case to take into account is any region that covers the page\r
+    // table itself: if we'd cover such a region with block mappings, we are\r
+    // more likely to end up in the situation later where we need to disable\r
+    // the MMU in order to update page table entries safely, so prefer page\r
+    // mappings in that particular case.\r
+    //\r
     if ((Level == 0) || (((RegionStart | BlockEnd) & BlockMask) != 0) ||\r
+        ((Level < 3) && (((UINT64)PageTable & ~BlockMask) == RegionStart)) ||\r
         IsTableEntry (*Entry, Level))\r
     {\r
       ASSERT (Level < 3);\r
@@ -234,7 +280,8 @@ UpdateRegionMappingRecursive (
                      *Entry & TT_ATTRIBUTES_MASK,\r
                      0,\r
                      TranslationTable,\r
-                     Level + 1\r
+                     Level + 1,\r
+                     FALSE\r
                      );\r
           if (EFI_ERROR (Status)) {\r
             //\r
@@ -246,8 +293,11 @@ UpdateRegionMappingRecursive (
             return Status;\r
           }\r
         }\r
+\r
+        NextTableIsLive = FALSE;\r
       } else {\r
         TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);\r
+        NextTableIsLive  = TableIsLive;\r
       }\r
 \r
       //\r
@@ -259,7 +309,8 @@ UpdateRegionMappingRecursive (
                  AttributeSetMask,\r
                  AttributeClearMask,\r
                  TranslationTable,\r
-                 Level + 1\r
+                 Level + 1,\r
+                 NextTableIsLive\r
                  );\r
       if (EFI_ERROR (Status)) {\r
         if (!IsTableEntry (*Entry, Level)) {\r
@@ -282,7 +333,8 @@ UpdateRegionMappingRecursive (
           Entry,\r
           EntryValue,\r
           RegionStart,\r
-          IsBlockEntry (*Entry, Level)\r
+          BlockMask,\r
+          TableIsLive && IsBlockEntry (*Entry, Level)\r
           );\r
       }\r
     } else {\r
@@ -291,7 +343,7 @@ UpdateRegionMappingRecursive (
       EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3\r
                                  : TT_TYPE_BLOCK_ENTRY;\r
 \r
-      ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);\r
+      ReplaceTableEntry (Entry, EntryValue, RegionStart, BlockMask, FALSE);\r
     }\r
   }\r
 \r
@@ -301,10 +353,11 @@ UpdateRegionMappingRecursive (
 STATIC\r
 EFI_STATUS\r
 UpdateRegionMapping (\r
-  IN  UINT64  RegionStart,\r
-  IN  UINT64  RegionLength,\r
-  IN  UINT64  AttributeSetMask,\r
-  IN  UINT64  AttributeClearMask\r
+  IN  UINT64   RegionStart,\r
+  IN  UINT64   RegionLength,\r
+  IN  UINT64   AttributeSetMask,\r
+  IN  UINT64   AttributeClearMask,\r
+  IN  BOOLEAN  TableIsLive\r
   )\r
 {\r
   UINTN  T0SZ;\r
@@ -321,7 +374,8 @@ UpdateRegionMapping (
            AttributeSetMask,\r
            AttributeClearMask,\r
            ArmGetTTBR0BaseAddress (),\r
-           GetRootTableLevel (T0SZ)\r
+           GetRootTableLevel (T0SZ),\r
+           TableIsLive\r
            );\r
 }\r
 \r
@@ -336,7 +390,8 @@ FillTranslationTable (
            MemoryRegion->VirtualBase,\r
            MemoryRegion->Length,\r
            ArmMemoryAttributeToPageAttribute (MemoryRegion->Attributes) | TT_AF,\r
-           0\r
+           0,\r
+           FALSE\r
            );\r
 }\r
 \r
@@ -410,7 +465,8 @@ ArmSetMemoryAttributes (
            BaseAddress,\r
            Length,\r
            PageAttributes,\r
-           PageAttributeMask\r
+           PageAttributeMask,\r
+           TRUE\r
            );\r
 }\r
 \r
@@ -423,7 +479,13 @@ SetMemoryRegionAttribute (
   IN  UINT64                BlockEntryMask\r
   )\r
 {\r
-  return UpdateRegionMapping (BaseAddress, Length, Attributes, BlockEntryMask);\r
+  return UpdateRegionMapping (\r
+           BaseAddress,\r
+           Length,\r
+           Attributes,\r
+           BlockEntryMask,\r
+           TRUE\r
+           );\r
 }\r
 \r
 EFI_STATUS\r

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
index 66ebca571e63d06da88cdd054cbdb500f9c50bef..e936a5be4e112504e7ec37577219432b4124d238 100644 (file)
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
@@ -12,6 +12,14 @@
 \r
   .macro __replace_entry, el\r
 \r
+  // check whether we should disable the MMU\r
+  cbz   x3, .L1_\@\r
+\r
+  // clean and invalidate first so that we don't clobber\r
+  // adjacent entries that are dirty in the caches\r
+  dc    civac, x0\r
+  dsb   nsh\r
+\r
   // disable the MMU\r
   mrs   x8, sctlr_el\el\r
   bic   x9, x8, #CTRL_M_BIT\r
@@ -38,8 +46,33 @@
   // re-enable the MMU\r
   msr   sctlr_el\el, x8\r
   isb\r
+  b     .L2_\@\r
+\r
+.L1_\@:\r
+  // write invalid entry\r
+  str   xzr, [x0]\r
+  dsb   nshst\r
+\r
+  // flush translations for the target address from the TLBs\r
+  lsr   x2, x2, #12\r
+  .if   \el == 1\r
+  tlbi  vaae1, x2\r
+  .else\r
+  tlbi  vae\el, x2\r
+  .endif\r
+  dsb   nsh\r
+\r
+  // write updated entry\r
+  str   x1, [x0]\r
+  dsb   nshst\r
+\r
+.L2_\@:\r
   .endm\r
 \r
+  // Align this routine to a log2 upper bound of its size, so that it is\r
+  // guaranteed not to cross a page or block boundary.\r
+  .balign 0x200\r
+\r
 //VOID\r
 //ArmReplaceLiveTranslationEntry (\r
 //  IN  UINT64  *Entry,\r
@@ -53,12 +86,7 @@ ASM_FUNC(ArmReplaceLiveTranslationEntry)
   msr   daifset, #0xf\r
   isb\r
 \r
-  // clean and invalidate first so that we don't clobber\r
-  // adjacent entries that are dirty in the caches\r
-  dc    civac, x0\r
-  dsb   nsh\r
-\r
-  EL1_OR_EL2_OR_EL3(x3)\r
+  EL1_OR_EL2_OR_EL3(x5)\r
 1:__replace_entry 1\r
   b     4f\r
 2:__replace_entry 2\r
@@ -72,3 +100,6 @@ ASM_GLOBAL ASM_PFX(ArmReplaceLiveTranslationEntrySize)
 \r
 ASM_PFX(ArmReplaceLiveTranslationEntrySize):\r
   .long   . - ArmReplaceLiveTranslationEntry\r
+\r
+  // Double check that we did not overrun the assumed maximum size\r
+  .org    ArmReplaceLiveTranslationEntry + 0x200\r