Re: [PATCH v2] ArmPkg/ArmSmcPsciResetSystemLib: add missing call to ExitBootServices()

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Nov 20, 2018 at 04:44:33PM +0100, Ard Biesheuvel wrote:
> Our poor man's implementation of EnterS3WithImmediateWake () currently
> sets a high TPL level to disable interrupts, and simply calls the
> PEI entrypoint again after disabling the MMU.
> 
> Unfortunately, this is not sufficient: DMA capable devices such as
> network controllers or USB controllers may still be enabled and
> writing to memory, e.g., in response to incoming network packets.
> 
> So instead, do the full ExitBootServices() dance: allocate space and
> get the memory map, call ExitBootServices(), and in case it fails, get
> the memory map again and call ExitBootServices() again. This ensures
> that all cleanup related to DMA capable devices is performed before
> doing the warm reset.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: - add asm routines to ensure that we don't access memory after
>       disabling the MMU and caches

Fun times.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

>     - set MemMap to NULL
> 
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S              | 30 +++++++++++
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.asm            | 35 ++++++++++++
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.S                  | 29 ++++++++++
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.asm                | 34 ++++++++++++
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c   | 57 +++++++++++++++++---
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf |  8 +++
>  6 files changed, 187 insertions(+), 6 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S b/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S
> new file mode 100644
> index 000000000000..1edca941cb8f
> --- /dev/null
> +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S
> @@ -0,0 +1,30 @@
> +/** @file
> +  ResetSystemLib implementation using PSCI calls
> +
> +  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution. The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <AsmMacroIoLibV8.h>
> +
> +ASM_FUNC(DisableMmuAndReenterPei)
> +  stp   x29, x30, [sp, #-16]!
> +  mov   x29, sp
> +
> +  bl    ArmDisableMmu
> +
> +  // no memory accesses after MMU and caches have been disabled
> +
> +  MOV64 (x0, FixedPcdGet64 (PcdFvBaseAddress))
> +  blr   x0
> +
> +  // never returns
> +  nop
> diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.asm b/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.asm
> new file mode 100644
> index 000000000000..b3dca150c150
> --- /dev/null
> +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.asm
> @@ -0,0 +1,35 @@
> +/** @file
> +  ResetSystemLib implementation using PSCI calls
> +
> +  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution. The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +  AREA Reset, CODE, READONLY
> +
> +  EXPORT DisableMmuAndReenterPei
> +  IMPORT ArmDisableMmu
> +
> +DisableMmuAndReenterPei
> +  stp   x29, x30, [sp, #-16]!
> +  mov   x29, sp
> +
> +  bl    ArmDisableMmu
> +
> +  ; no memory accesses after MMU and caches have been disabled
> +
> +  movl  x0, FixedPcdGet64 (PcdFvBaseAddress)
> +  blr   x0
> +
> +  ; never returns
> +  nop
> +
> +  END
> diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.S b/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.S
> new file mode 100644
> index 000000000000..b6fe2bd82baa
> --- /dev/null
> +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.S
> @@ -0,0 +1,29 @@
> +/** @file
> +  ResetSystemLib implementation using PSCI calls
> +
> +  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution. The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <AsmMacroIoLib.h>
> +
> +ASM_FUNC(DisableMmuAndReenterPei)
> +  push  {lr}
> +
> +  bl    ArmDisableMmu
> +
> +  // no memory accesses after MMU and caches have been disabled
> +
> +  MOV32 (r0, FixedPcdGet64 (PcdFvBaseAddress))
> +  blx   r0
> +
> +  // never returns
> +  nop
> diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.asm b/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.asm
> new file mode 100644
> index 000000000000..f098b352418b
> --- /dev/null
> +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.asm
> @@ -0,0 +1,34 @@
> +/** @file
> +  ResetSystemLib implementation using PSCI calls
> +
> +  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution. The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +    INCLUDE AsmMacroExport.inc
> +    PRESERVE8
> +
> +    IMPORT ArmDisableMmu
> +
> +RVCT_ASM_EXPORT DisableMmuAndReenterPei
> +  push  {lr}
> +
> +  bl    ArmDisableMmu
> +
> +  ; no memory accesses after MMU and caches have been disabled
> +
> +  mov32 r0, FixedPcdGet64 (PcdFvBaseAddress)
> +  blx   r0
> +
> +  ; never returns
> +  nop
> +
> +  END
> diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> index 10ceafd14d5d..3f7b8ae0b169 100644
> --- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>    ResetSystemLib implementation using PSCI calls
>  
> -  Copyright (c) 2017, Linaro Ltd. All rights reserved.<BR>
> +  Copyright (c) 2017 - 2018, Linaro Ltd. All rights reserved.<BR>
>  
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD License
> @@ -81,6 +81,8 @@ ResetShutdown (
>    ArmCallSmc (&ArmSmcArgs);
>  }
>  
> +VOID DisableMmuAndReenterPei (VOID);
> +
>  /**
>    This function causes the system to enter S3 and then wake up immediately.
>  
> @@ -92,7 +94,12 @@ EnterS3WithImmediateWake (
>    VOID
>    )
>  {
> -  VOID (*Reset)(VOID);
> +  EFI_PHYSICAL_ADDRESS        Alloc;
> +  EFI_MEMORY_DESCRIPTOR       *MemMap;
> +  UINTN                       MemMapSize;
> +  UINTN                       MapKey, DescriptorSize;
> +  UINT32                      DescriptorVersion;
> +  EFI_STATUS                  Status;
>  
>    if (FeaturePcdGet (PcdArmReenterPeiForCapsuleWarmReboot) &&
>        !EfiAtRuntime ()) {
> @@ -101,11 +108,49 @@ EnterS3WithImmediateWake (
>      // immediate wake (which is used by capsule update) by disabling the MMU
>      // and interrupts, and jumping to the PEI entry point.
>      //
> -    Reset = (VOID (*)(VOID))(UINTN)FixedPcdGet64 (PcdFvBaseAddress);
>  
> -    gBS->RaiseTPL (TPL_HIGH_LEVEL);
> -    ArmDisableMmu ();
> -    Reset ();
> +    //
> +    // Obtain the size of the memory map
> +    //
> +    MemMapSize = 0;
> +    MemMap = NULL;
> +    Status = gBS->GetMemoryMap (&MemMapSize, MemMap, &MapKey, &DescriptorSize,
> +                    &DescriptorVersion);
> +    ASSERT (Status == EFI_BUFFER_TOO_SMALL);
> +
> +    //
> +    // Add some slack to the allocation to cater for changes in the memory
> +    // map if ExitBootServices () fails the first time around.
> +    //
> +    MemMapSize += SIZE_4KB;
> +    Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData,
> +                    EFI_SIZE_TO_PAGES (MemMapSize), &Alloc);
> +    ASSERT_EFI_ERROR (Status);
> +
> +    MemMap = (EFI_MEMORY_DESCRIPTOR *)(UINTN)Alloc;
> +
> +    Status = gBS->GetMemoryMap (&MemMapSize, MemMap, &MapKey, &DescriptorSize,
> +                    &DescriptorVersion);
> +    ASSERT_EFI_ERROR (Status);
> +
> +    Status = gBS->ExitBootServices (gImageHandle, MapKey);
> +    if (EFI_ERROR (Status)) {
> +      //
> +      // ExitBootServices () may fail the first time around if an event fired
> +      // right after the call to GetMemoryMap() which allocated or freed memory.
> +      // Since that first call to ExitBootServices () will disarm the timer,
> +      // this is guaranteed not to happen again, so one additional attempt
> +      // should suffice.
> +      //
> +      Status = gBS->GetMemoryMap (&MemMapSize, MemMap, &MapKey, &DescriptorSize,
> +                      &DescriptorVersion);
> +      ASSERT_EFI_ERROR (Status);
> +
> +      Status = gBS->ExitBootServices (gImageHandle, MapKey);
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +
> +    DisableMmuAndReenterPei ();
>    }
>  }
>  
> diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
> index 19021cd1e8b6..3b8925c6f9cd 100644
> --- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
> +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
> @@ -21,6 +21,14 @@
>    VERSION_STRING                 = 1.0
>    LIBRARY_CLASS                  = ResetSystemLib
>  
> +[Sources.AARCH64]
> +  AArch64/Reset.S   | GCC
> +  AArch64/Reset.asm | MSFT
> +
> +[Sources.ARM]
> +  Arm/Reset.S       | GCC
> +  Arm/Reset.asm     | RVCT
> +
>  [Sources]
>    ArmSmcPsciResetSystemLib.c
>  
> -- 
> 2.17.1
>