[edk2-devel] [PATCH 1/2] ArmPkg/ArmSmcPsciResetSystemLib: remove EnterS3WithImmediateWake ()

Ard Biesheuvel posted 2 patches 4 years, 10 months ago
[edk2-devel] [PATCH 1/2] ArmPkg/ArmSmcPsciResetSystemLib: remove EnterS3WithImmediateWake ()
Posted by Ard Biesheuvel 4 years, 10 months ago
EnterS3WithImmediateWake () no longer has any callers, so remove it
from ResetSystemLib. Note that this means the hack to support warm
reboot by jumping to the SEC entry point with the MMU and caches off
is also no longer used, and can be removed as well, along with the PCD
PcdArmReenterPeiForCapsuleWarmReboot that was introduced for this
purpose.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/ArmPkg.dec                                                    |  4 --
 ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf | 17 -----
 ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c   | 66 +-------------------
 ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S              | 24 -------
 ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.asm            | 29 ---------
 ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.S                  | 23 -------
 ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.asm                | 28 ---------
 7 files changed, 2 insertions(+), 189 deletions(-)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index 39ff339c956d..eaf1072d9ef3 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -78,10 +78,6 @@ [PcdsFeatureFlag.common]
   # Define if the GICv3 controller should use the GICv2 legacy
   gArmTokenSpaceGuid.PcdArmGicV3WithV2Legacy|FALSE|BOOLEAN|0x00000042
 
-  # Whether to implement warm reboot for capsule update using a jump back to the
-  # PEI entry point with caches and interrupts disabled.
-  gArmTokenSpaceGuid.PcdArmReenterPeiForCapsuleWarmReboot|FALSE|BOOLEAN|0x0000001F
-
 [PcdsFeatureFlag.ARM]
   # Whether to map normal memory as non-shareable. FALSE is the safe choice, but
   # TRUE may be appropriate to fix performance problems if you don't care about
diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
index fa19bf649131..c17b28cfac79 100644
--- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
+++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
@@ -15,14 +15,6 @@ [Defines]
   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
 
@@ -32,15 +24,6 @@ [Packages]
   MdePkg/MdePkg.dec
 
 [LibraryClasses]
-  ArmMmuLib
   ArmSmcLib
   BaseLib
   DebugLib
-  UefiBootServicesTableLib
-  UefiRuntimeLib
-
-[FeaturePcd]
-  gArmTokenSpaceGuid.PcdArmReenterPeiForCapsuleWarmReboot
-
-[FixedPcd]
-  gArmTokenSpaceGuid.PcdFvBaseAddress
diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
index b2dde9bfc13a..22fcf989e903 100644
--- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
+++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
@@ -10,13 +10,10 @@
 
 #include <PiDxe.h>
 
-#include <Library/ArmMmuLib.h>
-#include <Library/ArmSmcLib.h>
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 #include <Library/ResetSystemLib.h>
-#include <Library/UefiBootServicesTableLib.h>
-#include <Library/UefiRuntimeLib.h>
+#include <Library/ArmSmcLib.h>
 
 #include <IndustryStandard/ArmStdSmc.h>
 
@@ -76,8 +73,6 @@ ResetShutdown (
   ArmCallSmc (&ArmSmcArgs);
 }
 
-VOID DisableMmuAndReenterPei (VOID);
-
 /**
   This function causes the system to enter S3 and then wake up immediately.
 
@@ -89,64 +84,7 @@ EnterS3WithImmediateWake (
   VOID
   )
 {
-  EFI_PHYSICAL_ADDRESS        Alloc;
-  EFI_MEMORY_DESCRIPTOR       *MemMap;
-  UINTN                       MemMapSize;
-  UINTN                       MapKey, DescriptorSize;
-  UINT32                      DescriptorVersion;
-  EFI_STATUS                  Status;
-
-  if (FeaturePcdGet (PcdArmReenterPeiForCapsuleWarmReboot) &&
-      !EfiAtRuntime ()) {
-    //
-    // At boot time, we are the only core running, so we can implement the
-    // immediate wake (which is used by capsule update) by disabling the MMU
-    // and interrupts, and jumping to the PEI entry point.
-    //
-
-    //
-    // 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 ();
-  }
+  // Not implemented
 }
 
 /**
diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S b/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S
deleted file mode 100644
index d0d908b7d70d..000000000000
--- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S
+++ /dev/null
@@ -1,24 +0,0 @@
-/** @file
-  ResetSystemLib implementation using PSCI calls
-
-  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
-
-  SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#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
deleted file mode 100644
index 752df072511b..000000000000
--- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.asm
+++ /dev/null
@@ -1,29 +0,0 @@
-;/** @file
-;  ResetSystemLib implementation using PSCI calls
-;
-;  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
-;
-;  SPDX-License-Identifier: BSD-2-Clause-Patent
-;
-;**/
-
-  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
deleted file mode 100644
index c0c5bcf19723..000000000000
--- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.S
+++ /dev/null
@@ -1,23 +0,0 @@
-/** @file
-  ResetSystemLib implementation using PSCI calls
-
-  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
-
-  SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#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
deleted file mode 100644
index ab7519a5a926..000000000000
--- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.asm
+++ /dev/null
@@ -1,28 +0,0 @@
-;/** @file
-;  ResetSystemLib implementation using PSCI calls
-;
-;  Copyright (c) 2018, Linaro Ltd. All rights reserved.<BR>
-;
-;  SPDX-License-Identifier: BSD-2-Clause-Patent
-;
-;**/
-
-  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
-- 
2.20.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52967): https://edk2.groups.io/g/devel/message/52967
Mute This Topic: https://groups.io/mt/69498790/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] ArmPkg/ArmSmcPsciResetSystemLib: remove EnterS3WithImmediateWake ()
Posted by Leif Lindholm 4 years, 10 months ago
Apologies for late response - only just got email fully back up.
Minor comment below.

On Tue, Jan 07, 2020 at 10:22:14 +0100, Ard Biesheuvel wrote:
> EnterS3WithImmediateWake () no longer has any callers, so remove it
> from ResetSystemLib. Note that this means the hack to support warm
> reboot by jumping to the SEC entry point with the MMU and caches off
> is also no longer used, and can be removed as well, along with the PCD
> PcdArmReenterPeiForCapsuleWarmReboot that was introduced for this
> purpose.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/ArmPkg.dec                                                    |  4 --
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf | 17 -----
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c   | 66 +-------------------
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S              | 24 -------
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.asm            | 29 ---------
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.S                  | 23 -------
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.asm                | 28 ---------
>  7 files changed, 2 insertions(+), 189 deletions(-)
> 

> diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> index b2dde9bfc13a..22fcf989e903 100644
> --- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> @@ -10,13 +10,10 @@
>  
>  #include <PiDxe.h>
>  
> -#include <Library/ArmMmuLib.h>
> -#include <Library/ArmSmcLib.h>
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/ResetSystemLib.h>
> -#include <Library/UefiBootServicesTableLib.h>
> -#include <Library/UefiRuntimeLib.h>
> +#include <Library/ArmSmcLib.h>

Why does ArmSmcLib.h move?
If it is functionally required, then is there a bug in the file?
If not, can it go back to its original spot?
(If it can, and does, Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>)

((I will keep R-b:ing with that address until Maintainers.txt is
updated.))

/
    Leif

>  
>  #include <IndustryStandard/ArmStdSmc.h>
>  
> @@ -76,8 +73,6 @@ ResetShutdown (
>    ArmCallSmc (&ArmSmcArgs);
>  }
>  
> -VOID DisableMmuAndReenterPei (VOID);
> -
>  /**
>    This function causes the system to enter S3 and then wake up immediately.
>  

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53235): https://edk2.groups.io/g/devel/message/53235
Mute This Topic: https://groups.io/mt/69498790/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] ArmPkg/ArmSmcPsciResetSystemLib: remove EnterS3WithImmediateWake ()
Posted by Ard Biesheuvel 4 years, 10 months ago
On Tue, 14 Jan 2020 at 18:18, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> Apologies for late response - only just got email fully back up.
> Minor comment below.
>
> On Tue, Jan 07, 2020 at 10:22:14 +0100, Ard Biesheuvel wrote:
> > EnterS3WithImmediateWake () no longer has any callers, so remove it
> > from ResetSystemLib. Note that this means the hack to support warm
> > reboot by jumping to the SEC entry point with the MMU and caches off
> > is also no longer used, and can be removed as well, along with the PCD
> > PcdArmReenterPeiForCapsuleWarmReboot that was introduced for this
> > purpose.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmPkg/ArmPkg.dec                                                    |  4 --
> >  ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf | 17 -----
> >  ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c   | 66 +-------------------
> >  ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.S              | 24 -------
> >  ArmPkg/Library/ArmSmcPsciResetSystemLib/AArch64/Reset.asm            | 29 ---------
> >  ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.S                  | 23 -------
> >  ArmPkg/Library/ArmSmcPsciResetSystemLib/Arm/Reset.asm                | 28 ---------
> >  7 files changed, 2 insertions(+), 189 deletions(-)
> >
>
> > diff --git a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> > index b2dde9bfc13a..22fcf989e903 100644
> > --- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> > +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> > @@ -10,13 +10,10 @@
> >
> >  #include <PiDxe.h>
> >
> > -#include <Library/ArmMmuLib.h>
> > -#include <Library/ArmSmcLib.h>
> >  #include <Library/BaseLib.h>
> >  #include <Library/DebugLib.h>
> >  #include <Library/ResetSystemLib.h>
> > -#include <Library/UefiBootServicesTableLib.h>
> > -#include <Library/UefiRuntimeLib.h>
> > +#include <Library/ArmSmcLib.h>
>
> Why does ArmSmcLib.h move?
> If it is functionally required, then is there a bug in the file?
> If not, can it go back to its original spot?
> (If it can, and does, Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>)
>

I think that was a side effect of the revert. I'll put it back where it was.

Thanks,

> ((I will keep R-b:ing with that address until Maintainers.txt is
> updated.))
>
> /
>     Leif
>
> >
> >  #include <IndustryStandard/ArmStdSmc.h>
> >
> > @@ -76,8 +73,6 @@ ResetShutdown (
> >    ArmCallSmc (&ArmSmcArgs);
> >  }
> >
> > -VOID DisableMmuAndReenterPei (VOID);
> > -
> >  /**
> >    This function causes the system to enter S3 and then wake up immediately.
> >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53241): https://edk2.groups.io/g/devel/message/53241
Mute This Topic: https://groups.io/mt/69498790/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-