Re: [PATCH] PM / hibernate: Honour ACPI hardware signature by default for virtual guests

[Rafael J. Wysocki]

On Fri, Mar 11, 2022 at 8:20 PM David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote:
>
> From: David Woodhouse <dwmw@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >
> > 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!