Re: [PATCH v3 08/16] ArmPkg/ArmMmuLib: Reuse XIP MMU routines when splitting entries

["Leif Lindholm" <quic_llindhol@quicinc.com>]

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
>