public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] ArmPkg/ArmSmcPsciResetSystemLib: add missing call to ExitBootServices()
@ 2018-11-20 14:33 Ard Biesheuvel
  2018-11-20 14:34 ` Ard Biesheuvel
  2018-11-20 14:48 ` Leif Lindholm
  0 siblings, 2 replies; 3+ messages in thread
From: Ard Biesheuvel @ 2018-11-20 14:33 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>
---
 ArmPkg/Include/Library/ArmLib.h                                    |  1 +
 ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c | 51 ++++++++++++++++++--
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index e46df447b33d..ffda50e9d767 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -59,6 +59,7 @@ typedef enum {
 
 typedef struct {
   EFI_PHYSICAL_ADDRESS          PhysicalBase;
+  EFI_VIRTUAL_ADDRESS           VirtualBase;
   UINT64                        Length;
   ARM_MEMORY_REGION_ATTRIBUTES  Attributes;
 } ARM_MEMORY_REGION_DESCRIPTOR;
diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
index 10ceafd14d5d..c9c42ab3b244 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
@@ -92,7 +92,13 @@ EnterS3WithImmediateWake (
   VOID
   )
 {
-  VOID (*Reset)(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 ()) {
@@ -103,7 +109,46 @@ EnterS3WithImmediateWake (
     //
     Reset = (VOID (*)(VOID))(UINTN)FixedPcdGet64 (PcdFvBaseAddress);
 
-    gBS->RaiseTPL (TPL_HIGH_LEVEL);
+    //
+    // Obtain the size of the memory map
+    //
+    MemMapSize = 0;
+    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);
+    }
+
     ArmDisableMmu ();
     Reset ();
   }
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] ArmPkg/ArmSmcPsciResetSystemLib: add missing call to ExitBootServices()
  2018-11-20 14:33 [PATCH] ArmPkg/ArmSmcPsciResetSystemLib: add missing call to ExitBootServices() Ard Biesheuvel
@ 2018-11-20 14:34 ` Ard Biesheuvel
  2018-11-20 14:48 ` Leif Lindholm
  1 sibling, 0 replies; 3+ messages in thread
From: Ard Biesheuvel @ 2018-11-20 14:34 UTC (permalink / raw)
  To: edk2-devel@lists.01.org

On Tue, 20 Nov 2018 at 15:33, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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>
> ---
>  ArmPkg/Include/Library/ArmLib.h                                    |  1 +
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c | 51 ++++++++++++++++++--
>  2 files changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
> index e46df447b33d..ffda50e9d767 100644
> --- a/ArmPkg/Include/Library/ArmLib.h
> +++ b/ArmPkg/Include/Library/ArmLib.h
> @@ -59,6 +59,7 @@ typedef enum {
>
>  typedef struct {
>    EFI_PHYSICAL_ADDRESS          PhysicalBase;
> +  EFI_VIRTUAL_ADDRESS           VirtualBase;
>    UINT64                        Length;
>    ARM_MEMORY_REGION_ATTRIBUTES  Attributes;
>  } ARM_MEMORY_REGION_DESCRIPTOR;

This first hunk was included by accident - please disregard.

> diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> index 10ceafd14d5d..c9c42ab3b244 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
> @@ -92,7 +92,13 @@ EnterS3WithImmediateWake (
>    VOID
>    )
>  {
> -  VOID (*Reset)(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 ()) {
> @@ -103,7 +109,46 @@ EnterS3WithImmediateWake (
>      //
>      Reset = (VOID (*)(VOID))(UINTN)FixedPcdGet64 (PcdFvBaseAddress);
>
> -    gBS->RaiseTPL (TPL_HIGH_LEVEL);
> +    //
> +    // Obtain the size of the memory map
> +    //
> +    MemMapSize = 0;
> +    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);
> +    }
> +
>      ArmDisableMmu ();
>      Reset ();
>    }
> --
> 2.17.1
>


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] ArmPkg/ArmSmcPsciResetSystemLib: add missing call to ExitBootServices()
  2018-11-20 14:33 [PATCH] ArmPkg/ArmSmcPsciResetSystemLib: add missing call to ExitBootServices() Ard Biesheuvel
  2018-11-20 14:34 ` Ard Biesheuvel
@ 2018-11-20 14:48 ` Leif Lindholm
  1 sibling, 0 replies; 3+ messages in thread
From: Leif Lindholm @ 2018-11-20 14:48 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel

On Tue, Nov 20, 2018 at 03:33:00PM +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>
> ---
>  ArmPkg/Include/Library/ArmLib.h                                    |  1 +
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c | 51 ++++++++++++++++++--
>  2 files changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
> index e46df447b33d..ffda50e9d767 100644
> --- a/ArmPkg/Include/Library/ArmLib.h
> +++ b/ArmPkg/Include/Library/ArmLib.h
> @@ -59,6 +59,7 @@ typedef enum {
>  
>  typedef struct {
>    EFI_PHYSICAL_ADDRESS          PhysicalBase;
> +  EFI_VIRTUAL_ADDRESS           VirtualBase;
>    UINT64                        Length;
>    ARM_MEMORY_REGION_ATTRIBUTES  Attributes;
>  } ARM_MEMORY_REGION_DESCRIPTOR;

Without the above hunk:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> index 10ceafd14d5d..c9c42ab3b244 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
> @@ -92,7 +92,13 @@ EnterS3WithImmediateWake (
>    VOID
>    )
>  {
> -  VOID (*Reset)(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 ()) {
> @@ -103,7 +109,46 @@ EnterS3WithImmediateWake (
>      //
>      Reset = (VOID (*)(VOID))(UINTN)FixedPcdGet64 (PcdFvBaseAddress);
>  
> -    gBS->RaiseTPL (TPL_HIGH_LEVEL);
> +    //
> +    // Obtain the size of the memory map
> +    //
> +    MemMapSize = 0;
> +    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);
> +    }
> +
>      ArmDisableMmu ();
>      Reset ();
>    }
> -- 
> 2.17.1
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-11-20 14:48 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-20 14:33 [PATCH] ArmPkg/ArmSmcPsciResetSystemLib: add missing call to ExitBootServices() Ard Biesheuvel
2018-11-20 14:34 ` Ard Biesheuvel
2018-11-20 14:48 ` Leif Lindholm

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox