* [PATCH] PM / hibernate: Allow ACPI hardware signature to be honoured
@ 2021-11-08 16:09 David Woodhouse
2021-12-08 15:07 ` Rafael J. Wysocki
0 siblings, 1 reply; 4+ messages in thread
From: David Woodhouse @ 2021-11-08 16:09 UTC (permalink / raw)
To: linux-pm, linux-kernel; +Cc: benh, van der Linden, Frank, Amit Shah
[-- Attachment #1: Type: text/plain, Size: 8944 bytes --]
From: David Woodhouse <dwmw@amazon.co.uk>
Theoretically, when the hardware signature in FACS changes, the OS
is supposed to gracefully decline to attempt to resume from S4:
"If the signature has changed, OSPM will not restore the system
context and can boot from scratch"
In practice, Windows doesn't do this and many laptop vendors do allow
the signature to change especially when docking/undocking, so it would
be a bad idea to simply comply with the specification by default in the
general case.
However, there are use cases where we do want the compliant behaviour
and we know it's safe. Specifically, when resuming virtual machines where
we know the hypervisor has changed sufficiently that resume will fail.
We really want to be able to *tell* the guest kernel not to try, so it
boots cleanly and doesn't just crash. This patch provides a way to opt
in to the spec-compliant behaviour on the command line.
A follow-up patch may do this automatically for certain "known good"
machines based on a DMI match, or perhaps just for all hypervisor
guests since there's no good reason a hypervisor would change the
hardware_signature that it exposes to guests *unless* it wants them
to obey the ACPI specification.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
.../admin-guide/kernel-parameters.txt | 15 ++++++++---
arch/x86/kernel/acpi/sleep.c | 4 ++-
drivers/acpi/sleep.c | 26 +++++++++++++++----
include/linux/acpi.h | 2 +-
include/linux/suspend.h | 1 +
kernel/power/power.h | 1 +
kernel/power/swap.c | 16 ++++++++++--
7 files changed, 53 insertions(+), 12 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 43dc35fe5bc0..209402f4f11d 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -225,14 +225,23 @@
For broken nForce2 BIOS resulting in XT-PIC timer.
acpi_sleep= [HW,ACPI] Sleep options
- Format: { s3_bios, s3_mode, s3_beep, s4_nohwsig,
- old_ordering, nonvs, sci_force_enable, nobl }
+ Format: { s3_bios, s3_mode, s3_beep, s4_hwsig,
+ s4_nohwsig, old_ordering, nonvs,
+ sci_force_enable, nobl }
See Documentation/power/video.rst for information on
s3_bios and s3_mode.
s3_beep is for debugging; it makes the PC's speaker beep
as soon as the kernel's real-mode entry point is called.
+ s4_hwsig causes the kernel to check the ACPI hardware
+ signature during resume from hibernation, and gracefully
+ refuse to resume if it has changed. This complies with
+ the ACPI specification but not with reality, since
+ Windows does not do this and many laptops do change it
+ on docking. So the default behaviour is to allow resume
+ and simply warn when the signature changes, unless the
+ s4_hwsig option is enabled.
s4_nohwsig prevents ACPI hardware signature from being
- used during resume from hibernation.
+ used (or even warned about) during resume.
old_ordering causes the ACPI 1.0 ordering of the _PTS
control method, with respect to putting devices into
low power states, to be enforced (the ACPI 2.0 ordering
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 3f85fcae450c..1e97f944b47d 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -139,8 +139,10 @@ static int __init acpi_sleep_setup(char *str)
if (strncmp(str, "s3_beep", 7) == 0)
acpi_realmode_flags |= 4;
#ifdef CONFIG_HIBERNATION
+ if (strncmp(str, "s4_hwsig", 8) == 0)
+ acpi_check_s4_hw_signature(1);
if (strncmp(str, "s4_nohwsig", 10) == 0)
- acpi_no_s4_hw_signature();
+ acpi_check_s4_hw_signature(0);
#endif
if (strncmp(str, "nonvs", 5) == 0)
acpi_nvs_nosave();
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 3023224515ab..fa27319e5324 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -873,11 +873,11 @@ static inline void acpi_sleep_syscore_init(void) {}
#ifdef CONFIG_HIBERNATION
static unsigned long s4_hardware_signature;
static struct acpi_table_facs *facs;
-static bool nosigcheck;
+static int sigcheck = -1; /* Default behaviour is just to warn */
-void __init acpi_no_s4_hw_signature(void)
+void __init acpi_check_s4_hw_signature(int check)
{
- nosigcheck = true;
+ sigcheck = check;
}
static int acpi_hibernation_begin(pm_message_t stage)
@@ -1005,12 +1005,28 @@ static void acpi_sleep_hibernate_setup(void)
hibernation_set_ops(old_suspend_ordering ?
&acpi_hibernation_ops_old : &acpi_hibernation_ops);
sleep_states[ACPI_STATE_S4] = 1;
- if (nosigcheck)
+ if (!sigcheck)
return;
acpi_get_table(ACPI_SIG_FACS, 1, (struct acpi_table_header **)&facs);
- if (facs)
+ if (facs) {
+ /*
+ * s4_hardware_signature is the local variable which is just
+ * used to warn about mismatch after we're attempting to
+ * resume (in violation of the ACPI specification.)
+ */
s4_hardware_signature = facs->hardware_signature;
+
+ if (sigcheck > 0) {
+ /*
+ * If we're actually obeying the ACPI specification
+ * then the signature is written out as part of the
+ * swsusp header, in order to allow the boot kernel
+ * to gracefully decline to resume.
+ */
+ swsusp_hardware_signature = facs->hardware_signature;
+ }
+ }
}
#else /* !CONFIG_HIBERNATION */
static inline void acpi_sleep_hibernate_setup(void) {}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 974d497a897d..5b6953189912 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -506,7 +506,7 @@ acpi_status acpi_release_memory(acpi_handle handle, struct resource *res,
int acpi_resources_are_enforced(void);
#ifdef CONFIG_HIBERNATION
-void __init acpi_no_s4_hw_signature(void);
+void __init acpi_check_s4_hw_signature(int check);
#endif
#ifdef CONFIG_PM_SLEEP
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 8af13ba60c7e..5785d909c321 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -446,6 +446,7 @@ extern unsigned long get_safe_page(gfp_t gfp_mask);
extern asmlinkage int swsusp_arch_suspend(void);
extern asmlinkage int swsusp_arch_resume(void);
+extern u32 swsusp_hardware_signature;
extern void hibernation_set_ops(const struct platform_hibernation_ops *ops);
extern int hibernate(void);
extern bool system_entering_hibernation(void);
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 778bf431ec02..bb41a1a1c0d3 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -168,6 +168,7 @@ extern int swsusp_swap_in_use(void);
#define SF_PLATFORM_MODE 1
#define SF_NOCOMPRESS_MODE 2
#define SF_CRC32_MODE 4
+#define SF_HW_SIG 8
/* kernel/power/hibernate.c */
extern int swsusp_check(void);
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 3cb89baebc79..58e9641682e8 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -36,6 +36,8 @@
#define HIBERNATE_SIG "S1SUSPEND"
+u32 swsusp_hardware_signature;
+
/*
* When reading an {un,}compressed image, we may restore pages in place,
* in which case some architectures need these pages cleaning before they
@@ -104,7 +106,8 @@ struct swap_map_handle {
struct swsusp_header {
char reserved[PAGE_SIZE - 20 - sizeof(sector_t) - sizeof(int) -
- sizeof(u32)];
+ sizeof(u32) - sizeof(u32)];
+ u32 hw_sig;
u32 crc32;
sector_t image;
unsigned int flags; /* Flags to pass to the "boot" kernel */
@@ -312,7 +315,6 @@ static blk_status_t hib_wait_io(struct hib_bio_batch *hb)
/*
* Saving part
*/
-
static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
{
int error;
@@ -324,6 +326,10 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
memcpy(swsusp_header->orig_sig,swsusp_header->sig, 10);
memcpy(swsusp_header->sig, HIBERNATE_SIG, 10);
swsusp_header->image = handle->first_sector;
+ if (swsusp_hardware_signature) {
+ swsusp_header->hw_sig = swsusp_hardware_signature;
+ flags |= SF_HW_SIG;
+ }
swsusp_header->flags = flags;
if (flags & SF_CRC32_MODE)
swsusp_header->crc32 = handle->crc32;
@@ -1542,6 +1548,12 @@ int swsusp_check(void)
} else {
error = -EINVAL;
}
+ if (!error && swsusp_header->flags & SF_HW_SIG &&
+ swsusp_header->hw_sig != swsusp_hardware_signature) {
+ pr_info("Suspend image hardware signature mismatch (%08x now %08x); aborting resume.\n",
+ swsusp_header->hw_sig, swsusp_hardware_signature);
+ error = -EINVAL;
+ }
put:
if (error)
--
2.25.1
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] PM / hibernate: Allow ACPI hardware signature to be honoured
2021-11-08 16:09 [PATCH] PM / hibernate: Allow ACPI hardware signature to be honoured David Woodhouse
@ 2021-12-08 15:07 ` Rafael J. Wysocki
2022-03-11 19:20 ` [PATCH] PM / hibernate: Honour ACPI hardware signature by default for virtual guests David Woodhouse
0 siblings, 1 reply; 4+ messages in thread
From: Rafael J. Wysocki @ 2021-12-08 15:07 UTC (permalink / raw)
To: David Woodhouse
Cc: linux-pm, linux-kernel, benh, van der Linden, Frank, Amit Shah
On Mon, Nov 8, 2021 at 5:09 PM David Woodhouse <dwmw2@infradead.org> wrote:
>
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> Theoretically, when the hardware signature in FACS changes, the OS
> is supposed to gracefully decline to attempt to resume from S4:
>
> "If the signature has changed, OSPM will not restore the system
> context and can boot from scratch"
>
> In practice, Windows doesn't do this and many laptop vendors do allow
> the signature to change especially when docking/undocking, so it would
> be a bad idea to simply comply with the specification by default in the
> general case.
>
> However, there are use cases where we do want the compliant behaviour
> and we know it's safe. Specifically, when resuming virtual machines where
> we know the hypervisor has changed sufficiently that resume will fail.
> We really want to be able to *tell* the guest kernel not to try, so it
> boots cleanly and doesn't just crash. This patch provides a way to opt
> in to the spec-compliant behaviour on the command line.
>
> A follow-up patch may do this automatically for certain "known good"
> machines based on a DMI match, or perhaps just for all hypervisor
> guests since there's no good reason a hypervisor would change the
> hardware_signature that it exposes to guests *unless* it wants them
> to obey the ACPI specification.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Applied as 5.17 material, sorry for the delay.
Thanks!
> ---
> .../admin-guide/kernel-parameters.txt | 15 ++++++++---
> arch/x86/kernel/acpi/sleep.c | 4 ++-
> drivers/acpi/sleep.c | 26 +++++++++++++++----
> include/linux/acpi.h | 2 +-
> include/linux/suspend.h | 1 +
> kernel/power/power.h | 1 +
> kernel/power/swap.c | 16 ++++++++++--
> 7 files changed, 53 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 43dc35fe5bc0..209402f4f11d 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -225,14 +225,23 @@
> For broken nForce2 BIOS resulting in XT-PIC timer.
>
> acpi_sleep= [HW,ACPI] Sleep options
> - Format: { s3_bios, s3_mode, s3_beep, s4_nohwsig,
> - old_ordering, nonvs, sci_force_enable, nobl }
> + Format: { s3_bios, s3_mode, s3_beep, s4_hwsig,
> + s4_nohwsig, old_ordering, nonvs,
> + sci_force_enable, nobl }
> See Documentation/power/video.rst for information on
> s3_bios and s3_mode.
> s3_beep is for debugging; it makes the PC's speaker beep
> as soon as the kernel's real-mode entry point is called.
> + s4_hwsig causes the kernel to check the ACPI hardware
> + signature during resume from hibernation, and gracefully
> + refuse to resume if it has changed. This complies with
> + the ACPI specification but not with reality, since
> + Windows does not do this and many laptops do change it
> + on docking. So the default behaviour is to allow resume
> + and simply warn when the signature changes, unless the
> + s4_hwsig option is enabled.
> s4_nohwsig prevents ACPI hardware signature from being
> - used during resume from hibernation.
> + used (or even warned about) during resume.
> old_ordering causes the ACPI 1.0 ordering of the _PTS
> control method, with respect to putting devices into
> low power states, to be enforced (the ACPI 2.0 ordering
> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> index 3f85fcae450c..1e97f944b47d 100644
> --- a/arch/x86/kernel/acpi/sleep.c
> +++ b/arch/x86/kernel/acpi/sleep.c
> @@ -139,8 +139,10 @@ static int __init acpi_sleep_setup(char *str)
> if (strncmp(str, "s3_beep", 7) == 0)
> acpi_realmode_flags |= 4;
> #ifdef CONFIG_HIBERNATION
> + if (strncmp(str, "s4_hwsig", 8) == 0)
> + acpi_check_s4_hw_signature(1);
> if (strncmp(str, "s4_nohwsig", 10) == 0)
> - acpi_no_s4_hw_signature();
> + acpi_check_s4_hw_signature(0);
> #endif
> if (strncmp(str, "nonvs", 5) == 0)
> acpi_nvs_nosave();
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 3023224515ab..fa27319e5324 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -873,11 +873,11 @@ static inline void acpi_sleep_syscore_init(void) {}
> #ifdef CONFIG_HIBERNATION
> static unsigned long s4_hardware_signature;
> static struct acpi_table_facs *facs;
> -static bool nosigcheck;
> +static int sigcheck = -1; /* Default behaviour is just to warn */
>
> -void __init acpi_no_s4_hw_signature(void)
> +void __init acpi_check_s4_hw_signature(int check)
> {
> - nosigcheck = true;
> + sigcheck = check;
> }
>
> static int acpi_hibernation_begin(pm_message_t stage)
> @@ -1005,12 +1005,28 @@ static void acpi_sleep_hibernate_setup(void)
> hibernation_set_ops(old_suspend_ordering ?
> &acpi_hibernation_ops_old : &acpi_hibernation_ops);
> sleep_states[ACPI_STATE_S4] = 1;
> - if (nosigcheck)
> + if (!sigcheck)
> return;
>
> acpi_get_table(ACPI_SIG_FACS, 1, (struct acpi_table_header **)&facs);
> - if (facs)
> + if (facs) {
> + /*
> + * s4_hardware_signature is the local variable which is just
> + * used to warn about mismatch after we're attempting to
> + * resume (in violation of the ACPI specification.)
> + */
> s4_hardware_signature = facs->hardware_signature;
> +
> + if (sigcheck > 0) {
> + /*
> + * If we're actually obeying the ACPI specification
> + * then the signature is written out as part of the
> + * swsusp header, in order to allow the boot kernel
> + * to gracefully decline to resume.
> + */
> + swsusp_hardware_signature = facs->hardware_signature;
> + }
> + }
> }
> #else /* !CONFIG_HIBERNATION */
> static inline void acpi_sleep_hibernate_setup(void) {}
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 974d497a897d..5b6953189912 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -506,7 +506,7 @@ acpi_status acpi_release_memory(acpi_handle handle, struct resource *res,
> int acpi_resources_are_enforced(void);
>
> #ifdef CONFIG_HIBERNATION
> -void __init acpi_no_s4_hw_signature(void);
> +void __init acpi_check_s4_hw_signature(int check);
> #endif
>
> #ifdef CONFIG_PM_SLEEP
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 8af13ba60c7e..5785d909c321 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -446,6 +446,7 @@ extern unsigned long get_safe_page(gfp_t gfp_mask);
> extern asmlinkage int swsusp_arch_suspend(void);
> extern asmlinkage int swsusp_arch_resume(void);
>
> +extern u32 swsusp_hardware_signature;
> extern void hibernation_set_ops(const struct platform_hibernation_ops *ops);
> extern int hibernate(void);
> extern bool system_entering_hibernation(void);
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 778bf431ec02..bb41a1a1c0d3 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -168,6 +168,7 @@ extern int swsusp_swap_in_use(void);
> #define SF_PLATFORM_MODE 1
> #define SF_NOCOMPRESS_MODE 2
> #define SF_CRC32_MODE 4
> +#define SF_HW_SIG 8
>
> /* kernel/power/hibernate.c */
> extern int swsusp_check(void);
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 3cb89baebc79..58e9641682e8 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -36,6 +36,8 @@
>
> #define HIBERNATE_SIG "S1SUSPEND"
>
> +u32 swsusp_hardware_signature;
> +
> /*
> * When reading an {un,}compressed image, we may restore pages in place,
> * in which case some architectures need these pages cleaning before they
> @@ -104,7 +106,8 @@ struct swap_map_handle {
>
> struct swsusp_header {
> char reserved[PAGE_SIZE - 20 - sizeof(sector_t) - sizeof(int) -
> - sizeof(u32)];
> + sizeof(u32) - sizeof(u32)];
> + u32 hw_sig;
> u32 crc32;
> sector_t image;
> unsigned int flags; /* Flags to pass to the "boot" kernel */
> @@ -312,7 +315,6 @@ static blk_status_t hib_wait_io(struct hib_bio_batch *hb)
> /*
> * Saving part
> */
> -
> static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
> {
> int error;
> @@ -324,6 +326,10 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
> memcpy(swsusp_header->orig_sig,swsusp_header->sig, 10);
> memcpy(swsusp_header->sig, HIBERNATE_SIG, 10);
> swsusp_header->image = handle->first_sector;
> + if (swsusp_hardware_signature) {
> + swsusp_header->hw_sig = swsusp_hardware_signature;
> + flags |= SF_HW_SIG;
> + }
> swsusp_header->flags = flags;
> if (flags & SF_CRC32_MODE)
> swsusp_header->crc32 = handle->crc32;
> @@ -1542,6 +1548,12 @@ int swsusp_check(void)
> } else {
> error = -EINVAL;
> }
> + if (!error && swsusp_header->flags & SF_HW_SIG &&
> + swsusp_header->hw_sig != swsusp_hardware_signature) {
> + pr_info("Suspend image hardware signature mismatch (%08x now %08x); aborting resume.\n",
> + swsusp_header->hw_sig, swsusp_hardware_signature);
> + error = -EINVAL;
> + }
>
> put:
> if (error)
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] PM / hibernate: Honour ACPI hardware signature by default for virtual guests
2021-12-08 15:07 ` Rafael J. Wysocki
@ 2022-03-11 19:20 ` David Woodhouse
2022-03-16 18:31 ` Rafael J. Wysocki
0 siblings, 1 reply; 4+ messages in thread
From: David Woodhouse @ 2022-03-11 19:20 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-pm, linux-kernel, benh, van der Linden, Frank, Amit Shah
[-- Attachment #1: Type: text/plain, Size: 4976 bytes --]
From: David Woodhouse <dwmw@amazon.co.uk>
The ACPI specification says that OSPM should refuse to restore from
hibernate if the hardware signature changes, and should boot from
scratch. However, real BIOSes often vary the hardware signature in cases
where we *do* want to resume from hibernate, so Linux doesn't follow the
spec by default.
However, in a virtual environment there's no reason for the VMM to vary
the hardware signature *unless* it wants to trigger a clean reboot as
defined by the ACPI spec. So enable the check by default if a hypervisor
is detected.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
On Wed, 2021-12-08 at 16:07 +0100, Rafael J. Wysocki wrote:
> On Mon, Nov 8, 2021 at 5:09 PM David Woodhouse <dwmw2@infradead.org> wrote:
> > A follow-up patch may do this automatically for certain "known good"
> > machines based on a DMI match, or perhaps just for all hypervisor
> > guests since there's no good reason a hypervisor would change the
> > hardware_signature that it exposes to guests *unless* it wants them
> > to obey the ACPI specification.
> >
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>
> Applied as 5.17 material, sorry for the delay.
Here's the threatened follow-up. I think that a blanket enablement for
all hypervisors is sane enough; there's no reason why a virtual
environment would vary the hardware signature *unless* it wanted to
trigger the ACPI defined hibernate behaviour, is there?
arch/x86/kernel/acpi/sleep.c | 23 +++++++++++++++++++++--
drivers/acpi/sleep.c | 11 +++--------
include/linux/acpi.h | 2 +-
3 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 1e97f944b47d..3b7f4cdbf2e0 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -15,6 +15,7 @@
#include <asm/desc.h>
#include <asm/cacheflush.h>
#include <asm/realmode.h>
+#include <asm/hypervisor.h>
#include <linux/ftrace.h>
#include "../../realmode/rm/wakeup.h"
@@ -140,9 +141,9 @@ static int __init acpi_sleep_setup(char *str)
acpi_realmode_flags |= 4;
#ifdef CONFIG_HIBERNATION
if (strncmp(str, "s4_hwsig", 8) == 0)
- acpi_check_s4_hw_signature(1);
+ acpi_check_s4_hw_signature = 1;
if (strncmp(str, "s4_nohwsig", 10) == 0)
- acpi_check_s4_hw_signature(0);
+ acpi_check_s4_hw_signature = 0;
#endif
if (strncmp(str, "nonvs", 5) == 0)
acpi_nvs_nosave();
@@ -160,3 +161,21 @@ static int __init acpi_sleep_setup(char *str)
}
__setup("acpi_sleep=", acpi_sleep_setup);
+
+#if defined(CONFIG_HIBERNATION) && defined(CONFIG_HYPERVISOR_GUEST)
+static int __init init_s4_sigcheck(void)
+{
+ /*
+ * If running on a hypervisor, honour the ACPI specification
+ * by default and trigger a clean reboot when the hardware
+ * signature in FACS is changed after hibernation.
+ */
+ if (acpi_check_s4_hw_signature == -1 &&
+ !hypervisor_is_type(X86_HYPER_NATIVE))
+ acpi_check_s4_hw_signature = 1;
+
+ return 0;
+}
+/* This must happen before acpi_init() which is a subsys initcall */
+arch_initcall(init_s4_sigcheck);
+#endif
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index a60ff5dfed3a..4c498e1051e9 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -874,12 +874,7 @@ static inline void acpi_sleep_syscore_init(void) {}
#ifdef CONFIG_HIBERNATION
static unsigned long s4_hardware_signature;
static struct acpi_table_facs *facs;
-static int sigcheck = -1; /* Default behaviour is just to warn */
-
-void __init acpi_check_s4_hw_signature(int check)
-{
- sigcheck = check;
-}
+int acpi_check_s4_hw_signature = -1; /* Default behaviour is just to warn */
static int acpi_hibernation_begin(pm_message_t stage)
{
@@ -1004,7 +999,7 @@ static void acpi_sleep_hibernate_setup(void)
hibernation_set_ops(old_suspend_ordering ?
&acpi_hibernation_ops_old : &acpi_hibernation_ops);
sleep_states[ACPI_STATE_S4] = 1;
- if (!sigcheck)
+ if (!acpi_check_s4_hw_signature)
return;
acpi_get_table(ACPI_SIG_FACS, 1, (struct acpi_table_header **)&facs);
@@ -1016,7 +1011,7 @@ static void acpi_sleep_hibernate_setup(void)
*/
s4_hardware_signature = facs->hardware_signature;
- if (sigcheck > 0) {
+ if (acpi_check_s4_hw_signature > 0) {
/*
* If we're actually obeying the ACPI specification
* then the signature is written out as part of the
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 6274758648e3..766dbcb82df1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -526,7 +526,7 @@ acpi_status acpi_release_memory(acpi_handle handle, struct resource *res,
int acpi_resources_are_enforced(void);
#ifdef CONFIG_HIBERNATION
-void __init acpi_check_s4_hw_signature(int check);
+extern int acpi_check_s4_hw_signature;
#endif
#ifdef CONFIG_PM_SLEEP
--
2.33.1
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] PM / hibernate: Honour ACPI hardware signature by default for virtual guests
2022-03-11 19:20 ` [PATCH] PM / hibernate: Honour ACPI hardware signature by default for virtual guests David Woodhouse
@ 2022-03-16 18:31 ` Rafael J. Wysocki
0 siblings, 0 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2022-03-16 18:31 UTC (permalink / raw)
To: David Woodhouse
Cc: Rafael J. Wysocki, linux-pm, linux-kernel, benh, van der Linden,
Frank, Amit Shah
On Fri, Mar 11, 2022 at 8:20 PM David Woodhouse <dwmw2@infradead.org> wrote:
>
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> The ACPI specification says that OSPM should refuse to restore from
> hibernate if the hardware signature changes, and should boot from
> scratch. However, real BIOSes often vary the hardware signature in cases
> where we *do* want to resume from hibernate, so Linux doesn't follow the
> spec by default.
>
> However, in a virtual environment there's no reason for the VMM to vary
> the hardware signature *unless* it wants to trigger a clean reboot as
> defined by the ACPI spec. So enable the check by default if a hypervisor
> is detected.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>
> On Wed, 2021-12-08 at 16:07 +0100, Rafael J. Wysocki wrote:
> > On Mon, Nov 8, 2021 at 5:09 PM David Woodhouse <dwmw2@infradead.org> wrote:
> > > A follow-up patch may do this automatically for certain "known good"
> > > machines based on a DMI match, or perhaps just for all hypervisor
> > > guests since there's no good reason a hypervisor would change the
> > > hardware_signature that it exposes to guests *unless* it wants them
> > > to obey the ACPI specification.
> > >
> > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> >
> > Applied as 5.17 material, sorry for the delay.
>
> Here's the threatened follow-up. I think that a blanket enablement for
> all hypervisors is sane enough; there's no reason why a virtual
> environment would vary the hardware signature *unless* it wanted to
> trigger the ACPI defined hibernate behaviour, is there?
I don't think so.
> arch/x86/kernel/acpi/sleep.c | 23 +++++++++++++++++++++--
> drivers/acpi/sleep.c | 11 +++--------
> include/linux/acpi.h | 2 +-
> 3 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> index 1e97f944b47d..3b7f4cdbf2e0 100644
> --- a/arch/x86/kernel/acpi/sleep.c
> +++ b/arch/x86/kernel/acpi/sleep.c
> @@ -15,6 +15,7 @@
> #include <asm/desc.h>
> #include <asm/cacheflush.h>
> #include <asm/realmode.h>
> +#include <asm/hypervisor.h>
>
> #include <linux/ftrace.h>
> #include "../../realmode/rm/wakeup.h"
> @@ -140,9 +141,9 @@ static int __init acpi_sleep_setup(char *str)
> acpi_realmode_flags |= 4;
> #ifdef CONFIG_HIBERNATION
> if (strncmp(str, "s4_hwsig", 8) == 0)
> - acpi_check_s4_hw_signature(1);
> + acpi_check_s4_hw_signature = 1;
> if (strncmp(str, "s4_nohwsig", 10) == 0)
> - acpi_check_s4_hw_signature(0);
> + acpi_check_s4_hw_signature = 0;
> #endif
> if (strncmp(str, "nonvs", 5) == 0)
> acpi_nvs_nosave();
> @@ -160,3 +161,21 @@ static int __init acpi_sleep_setup(char *str)
> }
>
> __setup("acpi_sleep=", acpi_sleep_setup);
> +
> +#if defined(CONFIG_HIBERNATION) && defined(CONFIG_HYPERVISOR_GUEST)
> +static int __init init_s4_sigcheck(void)
> +{
> + /*
> + * If running on a hypervisor, honour the ACPI specification
> + * by default and trigger a clean reboot when the hardware
> + * signature in FACS is changed after hibernation.
> + */
> + if (acpi_check_s4_hw_signature == -1 &&
> + !hypervisor_is_type(X86_HYPER_NATIVE))
> + acpi_check_s4_hw_signature = 1;
> +
> + return 0;
> +}
> +/* This must happen before acpi_init() which is a subsys initcall */
> +arch_initcall(init_s4_sigcheck);
> +#endif
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index a60ff5dfed3a..4c498e1051e9 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -874,12 +874,7 @@ static inline void acpi_sleep_syscore_init(void) {}
> #ifdef CONFIG_HIBERNATION
> static unsigned long s4_hardware_signature;
> static struct acpi_table_facs *facs;
> -static int sigcheck = -1; /* Default behaviour is just to warn */
> -
> -void __init acpi_check_s4_hw_signature(int check)
> -{
> - sigcheck = check;
> -}
> +int acpi_check_s4_hw_signature = -1; /* Default behaviour is just to warn */
>
> static int acpi_hibernation_begin(pm_message_t stage)
> {
> @@ -1004,7 +999,7 @@ static void acpi_sleep_hibernate_setup(void)
> hibernation_set_ops(old_suspend_ordering ?
> &acpi_hibernation_ops_old : &acpi_hibernation_ops);
> sleep_states[ACPI_STATE_S4] = 1;
> - if (!sigcheck)
> + if (!acpi_check_s4_hw_signature)
> return;
>
> acpi_get_table(ACPI_SIG_FACS, 1, (struct acpi_table_header **)&facs);
> @@ -1016,7 +1011,7 @@ static void acpi_sleep_hibernate_setup(void)
> */
> s4_hardware_signature = facs->hardware_signature;
>
> - if (sigcheck > 0) {
> + if (acpi_check_s4_hw_signature > 0) {
> /*
> * If we're actually obeying the ACPI specification
> * then the signature is written out as part of the
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 6274758648e3..766dbcb82df1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -526,7 +526,7 @@ acpi_status acpi_release_memory(acpi_handle handle, struct resource *res,
> int acpi_resources_are_enforced(void);
>
> #ifdef CONFIG_HIBERNATION
> -void __init acpi_check_s4_hw_signature(int check);
> +extern int acpi_check_s4_hw_signature;
> #endif
>
> #ifdef CONFIG_PM_SLEEP
> --
Applied as 5.18 material, thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-16 18:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-08 16:09 [PATCH] PM / hibernate: Allow ACPI hardware signature to be honoured David Woodhouse
2021-12-08 15:07 ` Rafael J. Wysocki
2022-03-11 19:20 ` [PATCH] PM / hibernate: Honour ACPI hardware signature by default for virtual guests David Woodhouse
2022-03-16 18:31 ` Rafael J. Wysocki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.