From: "Ard Biesheuvel" <ard.biesheuvel@linaro.org>
To: Leif Lindholm <leif@nuviainc.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>
Subject: Re: [PATCH edk2-platforms 1/1] Platform/ARM/ArmShellCmdRunAxf: switch to by-VA cache maintenance
Date: Wed, 4 Mar 2020 12:56:04 +0100 [thread overview]
Message-ID: <CAKv+Gu-c-Hg-JT4iFg-hpWQjEj4JN01O6ckn+buUFuoAh7E3gw@mail.gmail.com> (raw)
In-Reply-To: <CAKv+Gu8S8H_09sEdH8oOXgKhCZ6T9sp9n5woBXCpS6524BW3oQ@mail.gmail.com>
On Wed, 4 Mar 2020 at 12:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Wed, 4 Mar 2020 at 12:47, Leif Lindholm <leif@nuviainc.com> wrote:
> >
> > 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
>
> One thing occurred to me though, please see below
>
> > > ---
> > > 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
> > > +
... and add a
bl ArmDisableDataCache
here
(in both instances)
> > > + 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);
> > > +
>
> Since we are no longer touching the I-cache now, I should do a
>
> InvalidateInstructionCacheRange (FileData, FileSize);
>
> here as well. Mind if I fold that in?
>
>
> > > // 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
> > >
prev parent reply other threads:[~2020-03-04 11:56 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-04 11:42 [PATCH edk2-platforms 1/1] Platform/ARM/ArmShellCmdRunAxf: switch to by-VA cache maintenance Ard Biesheuvel
2020-03-04 11:47 ` Leif Lindholm
2020-03-04 11:52 ` Ard Biesheuvel
2020-03-04 11:56 ` Ard Biesheuvel [this message]
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=CAKv+Gu-c-Hg-JT4iFg-hpWQjEj4JN01O6ckn+buUFuoAh7E3gw@mail.gmail.com \
--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