| Message ID | 1409324757-12607-2-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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);
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 > >
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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
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.
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).
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.
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.
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
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);
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(+)