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

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

On 6 June 2018 at 15:29, Laszlo Ersek <lersek@redhat.com> wrote:
> 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

I will go for option #2 unless Leif has any objections to that?