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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Tue, 20 Nov 2018 at 17:17, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> 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>
>

Thanks

Pushed as 6556224e1f262d1d3d3aecab6e86b14edefe95b7

> >     - 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
> >