Re: [edk2][PATCH V1 1/1] ArmPkg: Handle warm reboot request correctly

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Pranav,

Thank you for this patch.

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar


On 10/03/2022 01:10 PM, Pranav Madhu wrote:
> The warm reboot requests are mapped to cold reboot as the power control
> module was not capable of handling the warm reboot requests in the
> legacy implementation. The support for warm reboot support is added into
> the power control module. To support warm reset, update
> ArmPsciResetSystemLib, and there by invoke the PSCI call with parameters
> for warm reboot.
>
> Signed-off-by: Pranav Madhu <pranav.madhu@arm.com>
> ---
>   ArmPkg/Include/IndustryStandard/ArmStdSmc.h                  | 1 +
>   ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c | 7 +++++--
>   2 files changed, 6 insertions(+), 2 deletions(-)
>
> Link to github branch for this patch -
> https://github.com/Pranav-Madhu/edk2/tree/topics/warm_reboot
>
> diff --git a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
> index 655edc21b205..c9059dead6e9 100644
> --- a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
> +++ b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
> @@ -93,6 +93,7 @@
>   #define ARM_SMC_ID_PSCI_MIGRATE_AARCH32        0x84000005
>   #define ARM_SMC_ID_PSCI_SYSTEM_OFF             0x84000008
>   #define ARM_SMC_ID_PSCI_SYSTEM_RESET           0x84000009
> +#define ARM_SMC_ID_PSCI_SYSTEM_RESET2_AARCH64  0xc4000012
>   
>   /* The current PSCI version is:  0.2 */
>   #define ARM_SMC_PSCI_VERSION_MAJOR  0
> diff --git a/ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c b/ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c
> index 7bcd34849507..27e048ba0f7a 100644
> --- a/ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c
> +++ b/ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.c
> @@ -45,10 +45,13 @@ LibResetSystem (
>     ARM_SMC_ARGS  ArmSmcArgs;
>   
>     switch (ResetType) {
> +    case EfiResetWarm:
> +      ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET2_AARCH64;
> +      ArmSmcArgs.Arg1 = 0;
> +      ArmSmcArgs.Arg2 = 0;
> +      break;
[SAMI] SYSTEM_RESET2 is an optional feature and if not supported would 
return NOT_SUPPORTED. So, if a platform does not support SYSTEM_RESET2, 
should the code here fall back to SYSTEM_RESET?
According to the PSCI specification, it is the responsibility of the OS 
to check that SYSTEM_RESET2 is supported before calling SYSTEM_RESET2 (I 
believe this is applicable for the case where UEFI is not used to boot 
the OS). However, if the runtime service ResetSystem() is invoked by the 
OS requesting a warm reset, is it not the firmware's responsibility to 
ensure that SYSTEM_RESET2 is supported? Any thoughts?
[/SAMI]
>       case EfiResetPlatformSpecific:
>       // Map the platform specific reset as reboot
> -    case EfiResetWarm:
> -    // Map a warm reset into a cold reset
>       case EfiResetCold:
>         // Send a PSCI 0.2 SYSTEM_RESET command
>         ArmSmcArgs.Arg0 = ARM_SMC_ID_PSCI_SYSTEM_RESET;