Re: [PATCH edk2-platforms 1/1] Platform/ARM/ArmShellCmdRunAxf: switch to by-VA cache maintenance

["Leif Lindholm" <leif@nuviainc.com>]

On Wed, Mar 04, 2020 at 12:42:50 +0100, Ard Biesheuvel wrote:
> Currently, the 'runaxf' shell command that exists only on ARM's own
> development platforms deals with the caches in an unsafe manner, as
> it relies on set/way maintenance to ensure that the ELF image it has
> put into memory, as well as the running image itself is visible in
> memory after the MMU and caches are disabled.
> 
> So let's switch to by-VA maintenance for the currently running image,
> as well as the ELF image, and use a helper in assembly to ensure that
> we are not relying on the stack between the disabling of the MMU and
> the invocation of the ELF image.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif@nuviainc.com>
Thanks!

> ---
>  Platform/ARM/Library/ArmShellCmdRunAxf/AArch64/Pivot.S       | 40 +++++++++
>  Platform/ARM/Library/ArmShellCmdRunAxf/Arm/Pivot.S           | 40 +++++++++
>  Platform/ARM/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf |  8 ++
>  Platform/ARM/Library/ArmShellCmdRunAxf/RunAxf.c              | 91 +++++++++-----------
>  4 files changed, 131 insertions(+), 48 deletions(-)
> 
> diff --git a/Platform/ARM/Library/ArmShellCmdRunAxf/AArch64/Pivot.S b/Platform/ARM/Library/ArmShellCmdRunAxf/AArch64/Pivot.S
> new file mode 100644
> index 000000000000..dd7aa48f7a02
> --- /dev/null
> +++ b/Platform/ARM/Library/ArmShellCmdRunAxf/AArch64/Pivot.S
> @@ -0,0 +1,40 @@
> +//
> +//  Copyright (c) 2020, ARM Limited. All rights reserved.
> +//
> +//  SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +
> +#include <AsmMacroIoLibV8.h>
> +
> +// VOID
> +// RunAxfPivot (
> +//   IN  ELF_ENTRYPOINT  ElfEntry
> +//   IN  UINTN           Arg0,
> +//   IN  UINTN           Arg1,
> +//   IN  UINTN           Arg2,
> +//   IN  UINTN           Arg3
> +//   );
> +ASM_FUNC(RunAxfPivot)
> +  // Preserve ElfEntry() and its arguments
> +  // Since we will not be returning from this function, we can clobber
> +  // callee preserved register instead.
> +  mov   x19, x0
> +  mov   x20, x1
> +  mov   x21, x2
> +  mov   x22, x3
> +  mov   x23, x4
> +
> +  bl    ArmDisableMmu
> +
> +  // Load ElfEntry()'s arguments into x0...x3
> +  mov   x0, x20
> +  mov   x1, x21
> +  mov   x2, x22
> +  mov   x3, x23
> +
> +  // Call ElfEntry()
> +  blr   x19
> +
> +0:wfi
> +  wfe
> +  b     0b
> diff --git a/Platform/ARM/Library/ArmShellCmdRunAxf/Arm/Pivot.S b/Platform/ARM/Library/ArmShellCmdRunAxf/Arm/Pivot.S
> new file mode 100644
> index 000000000000..edb3505e5cb9
> --- /dev/null
> +++ b/Platform/ARM/Library/ArmShellCmdRunAxf/Arm/Pivot.S
> @@ -0,0 +1,40 @@
> +//
> +//  Copyright (c) 2020, ARM Limited. All rights reserved.
> +//
> +//  SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +
> +#include <AsmMacroIoLibV8.h>
> +
> +// VOID
> +// RunAxfPivot (
> +//   IN  ELF_ENTRYPOINT  ElfEntry
> +//   IN  UINTN           Arg0,
> +//   IN  UINTN           Arg1,
> +//   IN  UINTN           Arg2,
> +//   IN  UINTN           Arg3
> +//   );
> +ASM_FUNC(RunAxfPivot)
> +  // Preserve ElfEntry() and its arguments without using the stack.
> +  // Since we will not be returning from this function, we can clobber
> +  // callee preserved register instead.
> +  mov   r4, r0
> +  mov   r5, r1
> +  mov   r6, r2
> +  mov   r7, r3
> +  pop   {r8}
> +
> +  bl    ArmDisableMmu
> +
> +  // Load ElfEntry()'s arguments into x0...x3
> +  mov   r0, r5
> +  mov   r1, r6
> +  mov   r2, r7
> +  mov   r3, r8
> +
> +  // Call ElfEntry()
> +  blx   r4
> +
> +0:wfi
> +  wfe
> +  b     0b
> diff --git a/Platform/ARM/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf b/Platform/ARM/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf
> index 74c6a0e84fdf..7c27a765bd5c 100644
> --- a/Platform/ARM/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf
> +++ b/Platform/ARM/Library/ArmShellCmdRunAxf/ArmShellCmdRunAxf.inf
> @@ -27,6 +27,12 @@ [Sources.common]
>    elf64.h
>    elf_common.h
>  
> +[Sources.AARCH64]
> +  AArch64/Pivot.S
> +
> +[Sources.ARM]
> +  Arm/Pivot.S
> +
>  [Packages]
>    ArmPkg/ArmPkg.dec
>    MdeModulePkg/MdeModulePkg.dec
> @@ -37,11 +43,13 @@ [Packages]
>  [LibraryClasses]
>    ArmLib
>    BaseLib
> +  CacheMaintenanceLib
>    DebugLib
>    HiiLib
>    ShellLib
>  
>  [Protocols]
> +  gEfiLoadedImageProtocolGuid
>    gEfiShellDynamicCommandProtocolGuid
>  
>  [Guids]
> diff --git a/Platform/ARM/Library/ArmShellCmdRunAxf/RunAxf.c b/Platform/ARM/Library/ArmShellCmdRunAxf/RunAxf.c
> index 153aed2ab6bd..efad5c4525ac 100644
> --- a/Platform/ARM/Library/ArmShellCmdRunAxf/RunAxf.c
> +++ b/Platform/ARM/Library/ArmShellCmdRunAxf/RunAxf.c
> @@ -10,6 +10,7 @@
>  
>  #include <Guid/GlobalVariable.h>
>  
> +#include <Library/CacheMaintenanceLib.h>
>  #include <Library/PrintLib.h>
>  #include <Library/HandleParsingLib.h>
>  #include <Library/DevicePathLib.h>
> @@ -20,6 +21,8 @@
>  
>  #include <Library/ArmLib.h>
>  
> +#include <Protocol/LoadedImage.h>
> +
>  #include "ArmShellCmdRunAxf.h"
>  #include "ElfLoader.h"
>  #include "BootMonFsLoader.h"
> @@ -85,37 +88,21 @@ ShutdownUefiBootServices (
>    return Status;
>  }
>  
> -
> -STATIC
> -EFI_STATUS
> -PreparePlatformHardware (
> -  VOID
> -  )
> -{
> -  //Note: Interrupts will be disabled by the GIC driver when ExitBootServices() will be called.
> -
> -  // Clean before Disable else the Stack gets corrupted with old data.
> -  ArmCleanDataCache ();
> -  ArmDisableDataCache ();
> -  // Invalidate all the entries that might have snuck in.
> -  ArmInvalidateDataCache ();
> -
> -  // Disable and invalidate the instruction cache
> -  ArmDisableInstructionCache ();
> -  ArmInvalidateInstructionCache ();
> -
> -  // Turn off MMU
> -  ArmDisableMmu();
> -
> -  return EFI_SUCCESS;
> -}
> -
>  // Process arguments to pass to AXF?
>  STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
>    {NULL, TypeMax}
>  };
>  
>  
> +VOID
> +RunAxfPivot (
> +  IN  ELF_ENTRYPOINT  ElfEntry,
> +  IN  UINTN           Arg0,
> +  IN  UINTN           Arg1,
> +  IN  UINTN           Arg2,
> +  IN  UINTN           Arg3
> +  );
> +
>  /**
>    This is the shell command handler function pointer callback type. This
>    function handles the command when it is invoked in the shell.
> @@ -139,23 +126,23 @@ ShellDynCmdRunAxfHandler (
>    IN EFI_SHELL_PROTOCOL                    *Shell
>    )
>  {
> -  LIST_ENTRY        *ParamPackage;
> -  EFI_STATUS         Status;
> -  SHELL_STATUS       ShellStatus;
> -  SHELL_FILE_HANDLE  FileHandle;
> -  ELF_ENTRYPOINT     StartElf;
> -  CONST CHAR16      *FileName;
> -  EFI_FILE_INFO     *Info;
> -  UINTN              FileSize;
> -  VOID              *FileData;
> -  VOID              *Entrypoint;
> -  LIST_ENTRY         LoadList;
> -  LIST_ENTRY        *Node;
> -  LIST_ENTRY        *NextNode;
> -  RUNAXF_LOAD_LIST  *LoadNode;
> -  CHAR16            *TmpFileName;
> -  CHAR16            *TmpChar16;
> -
> +  LIST_ENTRY                  *ParamPackage;
> +  EFI_STATUS                  Status;
> +  SHELL_STATUS                ShellStatus;
> +  SHELL_FILE_HANDLE           FileHandle;
> +  ELF_ENTRYPOINT              StartElf;
> +  CONST CHAR16                *FileName;
> +  EFI_FILE_INFO               *Info;
> +  UINTN                       FileSize;
> +  VOID                        *FileData;
> +  VOID                        *Entrypoint;
> +  LIST_ENTRY                  LoadList;
> +  LIST_ENTRY                  *Node;
> +  LIST_ENTRY                  *NextNode;
> +  RUNAXF_LOAD_LIST            *LoadNode;
> +  CHAR16                      *TmpFileName;
> +  CHAR16                      *TmpChar16;
> +  EFI_LOADED_IMAGE_PROTOCOL   *LoadedImage;
>  
>    ShellStatus = SHELL_SUCCESS;
>    FileHandle = NULL;
> @@ -291,6 +278,11 @@ ShellDynCmdRunAxfHandler (
>      }
>    }
>  
> +  //
> +  // Clean the loaded file to the PoC
> +  //
> +  WriteBackDataCacheRange (FileData, FileSize);
> +
>    // Program load list created.
>    // Shutdown UEFI, copy and jump to code.
>    if (!IsListEmpty (&LoadList) && !EFI_ERROR (Status)) {
> @@ -315,14 +307,17 @@ ShellDynCmdRunAxfHandler (
>          Node = GetNextNode (&LoadList, Node);
>        }
>  
> -      //
> -      // Switch off interrupts, caches, mmu, etc
> -      //
> -      Status = PreparePlatformHardware ();
> +      Status = gBS->HandleProtocol (gImageHandle, &gEfiLoadedImageProtocolGuid,
> +                      (VOID **)&LoadedImage);
>        ASSERT_EFI_ERROR (Status);
>  
> -      StartElf = (ELF_ENTRYPOINT)Entrypoint;
> -      StartElf (0,0,0,0);
> +      //
> +      // Ensure that the currently running image is clean to the PoC so we can
> +      // safely keep executing it with the MMU and caches off
> +      //
> +      WriteBackDataCacheRange (LoadedImage->ImageBase, LoadedImage->ImageSize);
> +
> +      RunAxfPivot (StartElf, 0, 0, 0, 0);
>  
>        // We should never get here.. But if we do, spin..
>        ASSERT (FALSE);
> -- 
> 2.17.1
>