* [PATCH v2] ArmPkg/ArmSmcPsciResetSystemLib: add missing call to ExitBootServices()
@ 2018-11-20 15:44 Ard Biesheuvel
2018-11-20 16:17 ` Leif Lindholm
0 siblings, 1 reply; 3+ messages in thread
From: Ard Biesheuvel @ 2018-11-20 15:44 UTC (permalink / raw)
To: edk2-devel
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
- 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] ArmPkg/ArmSmcPsciResetSystemLib: add missing call to ExitBootServices()
2018-11-20 15:44 [PATCH v2] ArmPkg/ArmSmcPsciResetSystemLib: add missing call to ExitBootServices() Ard Biesheuvel
@ 2018-11-20 16:17 ` Leif Lindholm
2018-11-21 11:05 ` Ard Biesheuvel
0 siblings, 1 reply; 3+ messages in thread
From: Leif Lindholm @ 2018-11-20 16:17 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] ArmPkg/ArmSmcPsciResetSystemLib: add missing call to ExitBootServices()
2018-11-20 16:17 ` Leif Lindholm
@ 2018-11-21 11:05 ` Ard Biesheuvel
0 siblings, 0 replies; 3+ messages in thread
From: Ard Biesheuvel @ 2018-11-21 11:05 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.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
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-11-21 11:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-20 15:44 [PATCH v2] ArmPkg/ArmSmcPsciResetSystemLib: add missing call to ExitBootServices() Ard Biesheuvel
2018-11-20 16:17 ` Leif Lindholm
2018-11-21 11:05 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox