Re: [PATCH v6 6/6] arm64: Use SYSTEM_OFF2 PSCI call to power off for hibernate

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Sat, Oct 19, 2024 at 06:15:47PM +0100, David Woodhouse wrote:

[...]

> +#ifdef CONFIG_HIBERNATION
> +static int psci_sys_hibernate(struct sys_off_data *data)
> +{
> +	/*
> +	 * Zero is an acceptable alternative to PSCI_1_3_OFF_TYPE_HIBERNATE_OFF
> +	 * and is supported by hypervisors implementing an earlier version
> +	 * of the pSCI v1.3 spec.
> +	 */

It is obvious but with this patch applied a host kernel would start executing
SYSTEM_OFF2 too if supported in firmware to hibernate, it is not a hypervisor
only code path.

Related to that: is it now always safe to override

commit 60c0d45a7f7a ("efi/arm64: use UEFI for system reset and poweroff")

for hibernation ? It is not very clear to me why overriding PSCI for
poweroff was the right thing to do - tried to follow that patch history but
the question remains (it is related to UpdateCapsule() but I don't know
how that applies to the hibernation use case).

As far as type == 0 is concerned:

I don't think the issue here is really PSCI 1.3 ALP1 vs PSCI 1.3 Issue
F.b, by reading the PSCI 1.3 Issue F.b specs (note (e) page 96) it looks
like there was a (superseded) PSCI 1.3 Issue F (september 2024 -
superseded in October 2024 - just reading the specs timeline) that allowed an
implementation to return INVALID_PARAMETERS if type == 0 - there should
be no firwmare out there that followed it - it was short lived.

Lorenzo

> +	if (system_entering_hibernation())
> +		invoke_psci_fn(PSCI_FN_NATIVE(1_3, SYSTEM_OFF2),
> +			       0 /*PSCI_1_3_OFF_TYPE_HIBERNATE_OFF*/, 0, 0);
> +	return NOTIFY_DONE;
> +}
> +
> +static int __init psci_hibernate_init(void)
> +{
> +	if (psci_system_off2_hibernate_supported) {
> +		/* Higher priority than EFI shutdown, but only for hibernate */
> +		register_sys_off_handler(SYS_OFF_MODE_POWER_OFF,
> +					 SYS_OFF_PRIO_FIRMWARE + 2,
> +					 psci_sys_hibernate, NULL);
> +	}
> +	return 0;
> +}
> +subsys_initcall(psci_hibernate_init);
> +#endif
> +
>  static int psci_features(u32 psci_func_id)
>  {
>  	return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
> @@ -364,6 +392,7 @@ static const struct {
>  	PSCI_ID_NATIVE(1_1, SYSTEM_RESET2),
>  	PSCI_ID(1_1, MEM_PROTECT),
>  	PSCI_ID_NATIVE(1_1, MEM_PROTECT_CHECK_RANGE),
> +	PSCI_ID_NATIVE(1_3, SYSTEM_OFF2),
>  };
>  
>  static int psci_debugfs_read(struct seq_file *s, void *data)
> @@ -525,6 +554,18 @@ static void __init psci_init_system_reset2(void)
>  		psci_system_reset2_supported = true;
>  }
>  
> +static void __init psci_init_system_off2(void)
> +{
> +	int ret;
> +
> +	ret = psci_features(PSCI_FN_NATIVE(1_3, SYSTEM_OFF2));
> +	if (ret < 0)
> +		return;
> +
> +	if (ret & PSCI_1_3_OFF_TYPE_HIBERNATE_OFF)
> +		psci_system_off2_hibernate_supported = true;
> +}
> +
>  static void __init psci_init_system_suspend(void)
>  {
>  	int ret;
> @@ -655,6 +696,7 @@ static int __init psci_probe(void)
>  		psci_init_cpu_suspend();
>  		psci_init_system_suspend();
>  		psci_init_system_reset2();
> +		psci_init_system_off2();
>  		kvm_init_hyp_services();
>  	}
>  
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index e35829d36039..1f87aa01ba44 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -685,8 +685,11 @@ static void power_down(void)
>  		}
>  		fallthrough;
>  	case HIBERNATION_SHUTDOWN:
> -		if (kernel_can_power_off())
> +		if (kernel_can_power_off()) {
> +			entering_platform_hibernation = true;
>  			kernel_power_off();
> +			entering_platform_hibernation = false;
> +		}
>  		break;
>  	}
>  	kernel_halt();
> -- 
> 2.44.0
>