[PATCH 1/2] efi/arm64: use UEFI for system poweroff

[PATCH 1/2] efi/arm64: use UEFI for system poweroff

[Ard Biesheuvel]

If UEFI Runtime Services are available, they are preferred over direct
PSCI calls or other methods to power down the system.

Enable the existing efi_reboot() code by supplying a definition of
efi_poweroff_required() that returns true.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/efi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index e72f3100958f..78c85c4252bc 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -475,3 +475,11 @@ err_unmap:
 	return -1;
 }
 early_initcall(arm64_enter_virtual_mode);
+
+/*
+ * Override the weak definition in drivers/firmware/efi/reboot.c
+ */
+bool efi_poweroff_required(void)
+{
+	return true;
+}
-- 
1.8.3.2

[PATCH 2/2] efi/arm64: use UEFI for system reset

[Ard Biesheuvel]

If UEFI Runtime Services are available, they are preferred over direct
PSCI calls or other methods to reset the system.

For the reset case, we need to hook into machine_restart(), as the
arm_pm_restart function pointer may be overwritten by modules.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/process.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 1309d64aa926..335a93da5eeb 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -177,6 +177,13 @@ void machine_restart(char *cmd)
 	local_irq_disable();
 	smp_send_stop();
 
+	/*
+	 * arm_pm_restart is exported to modules, so the only way to supersede
+	 * it with efi_reboot() is to call it here.
+	 */
+	if (IS_ENABLED(CONFIG_EFI) && efi_enabled(EFI_RUNTIME_SERVICES))
+		efi_reboot(REBOOT_WARM, NULL);
+
 	/* Now call the architecture specific reboot code. */
 	if (arm_pm_restart)
 		arm_pm_restart(reboot_mode, cmd);
-- 
1.8.3.2

[PATCH 2/2] efi/arm64: use UEFI for system reset

[Mark Salter]

On Fri, 2014-08-29 at 17:05 +0200, Ard Biesheuvel wrote:
> If UEFI Runtime Services are available, they are preferred over direct
> PSCI calls or other methods to reset the system.
> 
> For the reset case, we need to hook into machine_restart(), as the
> arm_pm_restart function pointer may be overwritten by modules.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/process.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 1309d64aa926..335a93da5eeb 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c

I get:

  arch/arm64/kernel/process.c: In function ?machine_restart?:
  arch/arm64/kernel/process.c:188:2: error: implicit declaration of function ?efi_reboot?

unless I add

  #include <linux/efi.h>


> @@ -177,6 +177,13 @@ void machine_restart(char *cmd)
>  	local_irq_disable();
>  	smp_send_stop();
>  
> +	/*
> +	 * arm_pm_restart is exported to modules, so the only way to supersede
> +	 * it with efi_reboot() is to call it here.
> +	 */
> +	if (IS_ENABLED(CONFIG_EFI) && efi_enabled(EFI_RUNTIME_SERVICES))
This test isn't necessary.

> +		efi_reboot(REBOOT_WARM, NULL);

Why not pass reboot_mode instead?

> +
>  	/* Now call the architecture specific reboot code. */
>  	if (arm_pm_restart)
>  		arm_pm_restart(reboot_mode, cmd);

[PATCH 1/2] efi/arm64: use UEFI for system poweroff

[Mark Rutland]

Hi Ard,

On Fri, Aug 29, 2014 at 04:05:56PM +0100, Ard Biesheuvel wrote:
> If UEFI Runtime Services are available, they are preferred over direct
> PSCI calls or other methods to power down the system.

Could you elaborate on why they should be preferred?

Mark.

> Enable the existing efi_reboot() code by supplying a definition of
> efi_poweroff_required() that returns true.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/efi.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index e72f3100958f..78c85c4252bc 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -475,3 +475,11 @@ err_unmap:
>  	return -1;
>  }
>  early_initcall(arm64_enter_virtual_mode);
> +
> +/*
> + * Override the weak definition in drivers/firmware/efi/reboot.c
> + */
> +bool efi_poweroff_required(void)
> +{
> +	return true;
> +}
> -- 
> 1.8.3.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 2/2] efi/arm64: use UEFI for system reset

[Catalin Marinas]

On Fri, Aug 29, 2014 at 04:05:57PM +0100, Ard Biesheuvel wrote:
> If UEFI Runtime Services are available, they are preferred over direct
> PSCI calls or other methods to reset the system.
> 
> For the reset case, we need to hook into machine_restart(), as the
> arm_pm_restart function pointer may be overwritten by modules.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/process.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 1309d64aa926..335a93da5eeb 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -177,6 +177,13 @@ void machine_restart(char *cmd)
>  	local_irq_disable();
>  	smp_send_stop();
>  
> +	/*
> +	 * arm_pm_restart is exported to modules, so the only way to supersede
> +	 * it with efi_reboot() is to call it here.
> +	 */

Why do you make this the preferred method? Is there a risk that UEFI is
broken and we want to override it with a SoC-specific driver (I wouldn't
like it but it's still an option).

There are also some patches here that try to get rid of arm_pm_restart:

https://lkml.org/lkml/2014/8/19/665

Once there are no drivers for arm64 setting arm_pm_restart explicitly
I'll remove it.

> +	if (IS_ENABLED(CONFIG_EFI) && efi_enabled(EFI_RUNTIME_SERVICES))

efi_enabled always returns false if !IS_ENABLED(CONFIG_EFI).

> +		efi_reboot(REBOOT_WARM, NULL);
> +
>  	/* Now call the architecture specific reboot code. */
>  	if (arm_pm_restart)
>  		arm_pm_restart(reboot_mode, cmd);
> -- 
> 1.8.3.2
> 
> 

-- 
Catalin

[PATCH 2/2] efi/arm64: use UEFI for system reset

[Mark Rutland]

Hi Ard,

On Fri, Aug 29, 2014 at 04:05:57PM +0100, Ard Biesheuvel wrote:
> If UEFI Runtime Services are available, they are preferred over direct
> PSCI calls or other methods to reset the system.
> 
> For the reset case, we need to hook into machine_restart(), as the
> arm_pm_restart function pointer may be overwritten by modules.

This is changing with the restart handler callchain series [1]. I think
you need to rework this atop of that.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280519.html

> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/process.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 1309d64aa926..335a93da5eeb 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -177,6 +177,13 @@ void machine_restart(char *cmd)
>  	local_irq_disable();
>  	smp_send_stop();
>  
> +	/*
> +	 * arm_pm_restart is exported to modules, so the only way to supersede
> +	 * it with efi_reboot() is to call it here.
> +	 */
> +	if (IS_ENABLED(CONFIG_EFI) && efi_enabled(EFI_RUNTIME_SERVICES))
> +		efi_reboot(REBOOT_WARM, NULL);
> +
>  	/* Now call the architecture specific reboot code. */
>  	if (arm_pm_restart)
>  		arm_pm_restart(reboot_mode, cmd);
> -- 
> 1.8.3.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 2/2] efi/arm64: use UEFI for system reset

[Ard Biesheuvel]

On 29 August 2014 18:04, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Fri, Aug 29, 2014 at 04:05:57PM +0100, Ard Biesheuvel wrote:
>> If UEFI Runtime Services are available, they are preferred over direct
>> PSCI calls or other methods to reset the system.
>>
>> For the reset case, we need to hook into machine_restart(), as the
>> arm_pm_restart function pointer may be overwritten by modules.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/kernel/process.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> index 1309d64aa926..335a93da5eeb 100644
>> --- a/arch/arm64/kernel/process.c
>> +++ b/arch/arm64/kernel/process.c
>> @@ -177,6 +177,13 @@ void machine_restart(char *cmd)
>>       local_irq_disable();
>>       smp_send_stop();
>>
>> +     /*
>> +      * arm_pm_restart is exported to modules, so the only way to supersede
>> +      * it with efi_reboot() is to call it here.
>> +      */
>
> Why do you make this the preferred method? Is there a risk that UEFI is
> broken and we want to override it with a SoC-specific driver (I wouldn't
> like it but it's still an option).
>

For poweroff (the other patch), it may not make a huge difference, but
the SBBR does state it explicitly.

For reboot, we *have* to use EFI reboot in order to support capsules:
when using capsules (for instance, for updating the firmware), you
need to use EFI reboot as you need to pass the return code of
UpdateCapsule() (a runtime service) to ResetSystem()

> There are also some patches here that try to get rid of arm_pm_restart:
>
> https://lkml.org/lkml/2014/8/19/665
>
> Once there are no drivers for arm64 setting arm_pm_restart explicitly
> I'll remove it.
>

OK, good to know. I will revisit this once that stuff is in.

-- 
Ard.

>> +     if (IS_ENABLED(CONFIG_EFI) && efi_enabled(EFI_RUNTIME_SERVICES))
>
> efi_enabled always returns false if !IS_ENABLED(CONFIG_EFI).
>
>> +             efi_reboot(REBOOT_WARM, NULL);
>> +
>>       /* Now call the architecture specific reboot code. */
>>       if (arm_pm_restart)
>>               arm_pm_restart(reboot_mode, cmd);
>> --
>> 1.8.3.2
>>
>>
>
> --
> Catalin

[PATCH 2/2] efi/arm64: use UEFI for system reset

[Catalin Marinas]

On Fri, Aug 29, 2014 at 05:12:48PM +0100, Ard Biesheuvel wrote:
> On 29 August 2014 18:04, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Fri, Aug 29, 2014 at 04:05:57PM +0100, Ard Biesheuvel wrote:
> >> If UEFI Runtime Services are available, they are preferred over direct
> >> PSCI calls or other methods to reset the system.
> >>
> >> For the reset case, we need to hook into machine_restart(), as the
> >> arm_pm_restart function pointer may be overwritten by modules.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  arch/arm64/kernel/process.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> >> index 1309d64aa926..335a93da5eeb 100644
> >> --- a/arch/arm64/kernel/process.c
> >> +++ b/arch/arm64/kernel/process.c
> >> @@ -177,6 +177,13 @@ void machine_restart(char *cmd)
> >>       local_irq_disable();
> >>       smp_send_stop();
> >>
> >> +     /*
> >> +      * arm_pm_restart is exported to modules, so the only way to supersede
> >> +      * it with efi_reboot() is to call it here.
> >> +      */
> >
> > Why do you make this the preferred method? Is there a risk that UEFI is
> > broken and we want to override it with a SoC-specific driver (I wouldn't
> > like it but it's still an option).
> >
> 
> For poweroff (the other patch), it may not make a huge difference, but
> the SBBR does state it explicitly.

The SBBR scope reads like this:

  This document defines the boot and Runtime Services that are expected
  by an enterprise platform Operating System or hypervisor, for an ARM
  AArch64 server, which is SBSA-compliant and follows the UEFI and ACPI
  specifications.

Which means that it does not apply to non-SBSA or SBSA systems that use
UEFI but not ACPI?

> For reboot, we *have* to use EFI reboot in order to support capsules:
> when using capsules (for instance, for updating the firmware), you
> need to use EFI reboot as you need to pass the return code of
> UpdateCapsule() (a runtime service) to ResetSystem()

That's a better argument ;). But I think with the restart patches you
could set some priority and if absolutely needed on a platform as
workaround we could register a driver with a higher priority (and losing
the above features).

-- 
Catalin

[PATCH 2/2] efi/arm64: use UEFI for system reset

[Ard Biesheuvel]

On 29 August 2014 18:25, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Fri, Aug 29, 2014 at 05:12:48PM +0100, Ard Biesheuvel wrote:
>> On 29 August 2014 18:04, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> > On Fri, Aug 29, 2014 at 04:05:57PM +0100, Ard Biesheuvel wrote:
>> >> If UEFI Runtime Services are available, they are preferred over direct
>> >> PSCI calls or other methods to reset the system.
>> >>
>> >> For the reset case, we need to hook into machine_restart(), as the
>> >> arm_pm_restart function pointer may be overwritten by modules.
>> >>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> ---
>> >>  arch/arm64/kernel/process.c | 7 +++++++
>> >>  1 file changed, 7 insertions(+)
>> >>
>> >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> >> index 1309d64aa926..335a93da5eeb 100644
>> >> --- a/arch/arm64/kernel/process.c
>> >> +++ b/arch/arm64/kernel/process.c
>> >> @@ -177,6 +177,13 @@ void machine_restart(char *cmd)
>> >>       local_irq_disable();
>> >>       smp_send_stop();
>> >>
>> >> +     /*
>> >> +      * arm_pm_restart is exported to modules, so the only way to supersede
>> >> +      * it with efi_reboot() is to call it here.
>> >> +      */
>> >
>> > Why do you make this the preferred method? Is there a risk that UEFI is
>> > broken and we want to override it with a SoC-specific driver (I wouldn't
>> > like it but it's still an option).
>> >
>>
>> For poweroff (the other patch), it may not make a huge difference, but
>> the SBBR does state it explicitly.
>
> The SBBR scope reads like this:
>
>   This document defines the boot and Runtime Services that are expected
>   by an enterprise platform Operating System or hypervisor, for an ARM
>   AArch64 server, which is SBSA-compliant and follows the UEFI and ACPI
>   specifications.
>
> Which means that it does not apply to non-SBSA or SBSA systems that use
> UEFI but not ACPI?
>

Good question.

>> For reboot, we *have* to use EFI reboot in order to support capsules:
>> when using capsules (for instance, for updating the firmware), you
>> need to use EFI reboot as you need to pass the return code of
>> UpdateCapsule() (a runtime service) to ResetSystem()
>
> That's a better argument ;). But I think with the restart patches you
> could set some priority and if absolutely needed on a platform as
> workaround we could register a driver with a higher priority (and losing
> the above features).
>

I will wait for those patches to land then.

-- 
Ard.

[PATCH 2/2] efi/arm64: use UEFI for system reset

[Mark Rutland]

On Fri, Aug 29, 2014 at 05:12:48PM +0100, Ard Biesheuvel wrote:
> On 29 August 2014 18:04, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Fri, Aug 29, 2014 at 04:05:57PM +0100, Ard Biesheuvel wrote:
> >> If UEFI Runtime Services are available, they are preferred over direct
> >> PSCI calls or other methods to reset the system.
> >>
> >> For the reset case, we need to hook into machine_restart(), as the
> >> arm_pm_restart function pointer may be overwritten by modules.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  arch/arm64/kernel/process.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> >> index 1309d64aa926..335a93da5eeb 100644
> >> --- a/arch/arm64/kernel/process.c
> >> +++ b/arch/arm64/kernel/process.c
> >> @@ -177,6 +177,13 @@ void machine_restart(char *cmd)
> >>       local_irq_disable();
> >>       smp_send_stop();
> >>
> >> +     /*
> >> +      * arm_pm_restart is exported to modules, so the only way to supersede
> >> +      * it with efi_reboot() is to call it here.
> >> +      */
> >
> > Why do you make this the preferred method? Is there a risk that UEFI is
> > broken and we want to override it with a SoC-specific driver (I wouldn't
> > like it but it's still an option).
> >
> 
> For poweroff (the other patch), it may not make a huge difference, but
> the SBBR does state it explicitly.
> 
> For reboot, we *have* to use EFI reboot in order to support capsules:
> when using capsules (for instance, for updating the firmware), you
> need to use EFI reboot as you need to pass the return code of
> UpdateCapsule() (a runtime service) to ResetSystem()

Ah. Could you drop that in the commit message next time around? That's a
far clearer (and stronger) argument than saying that the runtime
services are preferred.

Cheers,
Mark.

[PATCH 2/2] efi/arm64: use UEFI for system reset

[Leif Lindholm]

On Fri, Aug 29, 2014 at 05:25:34PM +0100, Catalin Marinas wrote:
> > For poweroff (the other patch), it may not make a huge difference, but
> > the SBBR does state it explicitly.
> 
> The SBBR scope reads like this:
> 
>   This document defines the boot and Runtime Services that are expected
>   by an enterprise platform Operating System or hypervisor, for an ARM
>   AArch64 server, which is SBSA-compliant and follows the UEFI and ACPI
>   specifications.
> 
> Which means that it does not apply to non-SBSA or SBSA systems that use
> UEFI but not ACPI?

It applies to all UEFI systems, really. But the SBBR explicitly
enumerates certain features already covered by the spec, to reduce
risk of misinterpretation (i.e. not reading the whole UEFI spec when
implementing).

> > For reboot, we *have* to use EFI reboot in order to support capsules:
> > when using capsules (for instance, for updating the firmware), you
> > need to use EFI reboot as you need to pass the return code of
> > UpdateCapsule() (a runtime service) to ResetSystem()
> 
> That's a better argument ;). But I think with the restart patches you
> could set some priority and if absolutely needed on a platform as
> workaround we could register a driver with a higher priority (and losing
> the above features).

That sounds sensible to me.
But the kernel should probably be very noisy on the console if that
override was done. And we would also want to store that fact
somwehere, to prevent any capsule runtime service calls being made.

/
    Leif