Re: [PATCH] ArmPkg/ArmSmcPsciResetSystemLib: implement fallback for warm reboot

[Laszlo Ersek <lersek@redhat.com>]

On 06/06/18 14:37, Ard Biesheuvel wrote:
> Implement ResetSystemLib's EnterS3WithImmediateWake() routine using
> a jump back to the PEI entry point with interrupts and MMU+caches
> disabled. This is only possible at boot time, when we are sure that
> the current CPU is the only one up and running. Also, it depends on
> the platform whether the PEI code is preserved in memory (it may be
> copied to DRAM rather than execute in place), so also add a feature
> PCD to selectively enable this feature.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/ArmPkg.dec                                                    |  4 ++++
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c   | 21 ++++++++++++++++++--
>  ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf |  9 +++++++++
>  3 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index debe066b6f7b..3aa229fe2ec9 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -85,6 +85,10 @@ [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.c b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> index d6d26bce5009..cb75e32771c2 100644
> --- a/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> +++ b/ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.c
> @@ -15,10 +15,13 @@
>  
>  #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/ArmSmcLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiRuntimeLib.h>
>  
>  #include <IndustryStandard/ArmStdSmc.h>
>  
> @@ -89,7 +92,21 @@ EnterS3WithImmediateWake (
>    VOID
>    )
>  {
> -  // Not implemented
> +  VOID (*Reset)(VOID);
> +
> +  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.
> +    //
> +    Reset = (VOID *)(UINTN)FixedPcdGet64 (PcdFvBaseAddress);

ISO C99 6.3.2.3 Pointers

  1 A pointer to void may be converted to or from a pointer to any
    incomplete or object type. [...]

  [...]

  5 An integer may be converted to any pointer type. Except as
    previously specified, the result is implementation-defined, might
    not be correctly aligned, might not point to an entity of the
    referenced type, and might be a trap representation. [...]

  [...]

  8 A pointer to a function of one type may be converted to a pointer to
    a function of another type and back again; [...]

The point is, converting pointer-to-void to pointer-to-function is
undefined, and I expect at least clang will yell at us for it. However,
converting an integer to pointer-to-function is implementation-defined;
i.e., the C language implementation must support it to an extent, and
must document how it works. (The "previously specified" part is about
null pointer constants, in paragraph 3, which I'm not quoting now.)

Thus, I suggest either

  typedef VOID (*RESET_FUNCTION_PTR) (VOID);
  RESET_FUNCTION_PTR Reset;

  Reset = (RESET_FUNCTION_PTR)(UINTN)FixedPcdGet64 (PcdFvBaseAddress);

or else (for kicks and giggles regarding the syntax), just

  Reset = (VOID (*)(VOID))(UINTN)FixedPcdGet64 (PcdFvBaseAddress);

Thanks
Laszlo