From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by mx.groups.io with SMTP id smtpd.web09.10040.1583322978075418960 for ; Wed, 04 Mar 2020 03:56:18 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=kEWhmPfq; spf=pass (domain: linaro.org, ip: 209.85.128.68, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wm1-f68.google.com with SMTP id m3so1581924wmi.0 for ; Wed, 04 Mar 2020 03:56:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=yOn3XpWmpaSh9X18x3IFzWtplN1xMEH2nwo0pcpLRoo=; b=kEWhmPfqwbW7M8ZontNhzoEAs3SDSLbd2ytbEyL0NOVJ+UsJ5oVNWHUfebof1Vg741 Ki3O12sQfo1OZjNmYS6ESXcqXR/dyM4zNncyTiPj1FDhoU6RYCdxt5ZP9PdAR7iYWGv/ qN0QX2jtFSFymric8+wDR05QKH3CY8Y2Ty8Kpb9Sc3tuuB26fw+XmTZi8hMpubPjFyYD n27L/Qr+4Z9yKVke3GoJlA2ylTOyGYL6/bZPqtB0XdAhklydUVtlAkQ/iBq7ZF2URqpB XJNVLBg7nOq9retftbCslzPJENUL7TzHkdqasxTzh/olToeGWZcdGhMXp4QqX/qkLQpD OkNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=yOn3XpWmpaSh9X18x3IFzWtplN1xMEH2nwo0pcpLRoo=; b=l/Ix3X1Jcn5D2ZdSpGFnycIU+8bBEYSTc5qsEjx4AJVjTYHri+yipirLfP9hD8OdmO GePOUETeRrLybVYDqHODzAyooLmuud/ICo6WWlL0RGy+dsIRjzEcF9N+tq/6C41qOAzD RVVFxh7W5sqsP+fyS/GXO/q5uYIdEs31wwRJwuZbwisF6xvE5zqEgsvQ8zZ5v2+K85U5 cYw7QnhbNUdfytNYOZZgRPpHJT7OCUyiLUBkwLa+loTUpzCRJegfPdcVMuwl9f9FXJkf BucbAztRDSVa8xnv1MjVtaxdnWVQzCH4tDJUpT4xp0ijWXEVY49/xPwQ55CtHre7yxzC 0aRQ== X-Gm-Message-State: ANhLgQ0NJmHHT8ZRVsjMdCnoZhmAqlRoaMZC5vscZkyF2FBJhNEQc2in fabEWUimdQSho8dUPQrKNvnRQfV+1FgFHbctVaPdPg== X-Google-Smtp-Source: ADFU+vs0LH2sN1kszyHyTRhRaJ/HyNd0vTyoj+EYsn3F11QwHcbWTTMJe5N7c1GYhf8Xc4cIRWibuufqQLL6W1lHw9M= X-Received: by 2002:a05:600c:24b:: with SMTP id 11mr3316131wmj.1.1583322975570; Wed, 04 Mar 2020 03:56:15 -0800 (PST) MIME-Version: 1.0 References: <20200304114250.6540-1-ard.biesheuvel@linaro.org> <20200304114735.GR23627@bivouac.eciton.net> In-Reply-To: From: "Ard Biesheuvel" Date: Wed, 4 Mar 2020 12:56:04 +0100 Message-ID: Subject: Re: [PATCH edk2-platforms 1/1] Platform/ARM/ArmShellCmdRunAxf: switch to by-VA cache maintenance To: Leif Lindholm Cc: edk2-devel-groups-io Content-Type: text/plain; charset="UTF-8" On Wed, 4 Mar 2020 at 12:52, Ard Biesheuvel wrote: > > On Wed, 4 Mar 2020 at 12:47, Leif Lindholm 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 > > > > Reviewed-by: Leif Lindholm > > 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 > > > + > > > +// 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 > > > + > > > +// 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 > > > > > > +#include > > > #include > > > #include > > > #include > > > @@ -20,6 +21,8 @@ > > > > > > #include > > > > > > +#include > > > + > > > #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 > > >