Re: [PATCH v2] acpi: fix suspend with Xen PV

[Rafael J. Wysocki]

On Tue, Jan 17, 2023 at 4:57 PM Juergen Gross <jgross@xxxxxxxx> wrote:
>
> Commit f1e525009493 ("x86/boot: Skip realmode init code when running as
> Xen PV guest") missed one code path accessing real_mode_header, leading
> to dereferencing NULL when suspending the system under Xen:
>
>     [  348.284004] PM: suspend entry (deep)
>     [  348.289532] Filesystems sync: 0.005 seconds
>     [  348.291545] Freezing user space processes ... (elapsed 0.000 seconds) done.
>     [  348.292457] OOM killer disabled.
>     [  348.292462] Freezing remaining freezable tasks ... (elapsed 0.104 seconds) done.
>     [  348.396612] printk: Suspending console(s) (use no_console_suspend to debug)
>     [  348.749228] PM: suspend devices took 0.352 seconds
>     [  348.769713] ACPI: EC: interrupt blocked
>     [  348.816077] BUG: kernel NULL pointer dereference, address: 000000000000001c
>     [  348.816080] #PF: supervisor read access in kernel mode
>     [  348.816081] #PF: error_code(0x0000) - not-present page
>     [  348.816083] PGD 0 P4D 0
>     [  348.816086] Oops: 0000 [#1] PREEMPT SMP NOPTI
>     [  348.816089] CPU: 0 PID: 6764 Comm: systemd-sleep Not tainted 6.1.3-1.fc32.qubes.x86_64 #1
>     [  348.816092] Hardware name: Star Labs StarBook/StarBook, BIOS 8.01 07/03/2022
>     [  348.816093] RIP: e030:acpi_get_wakeup_address+0xc/0x20
>
> Fix that by adding an optional acpi callback allowing to skip setting
> the wakeup address, as in the Xen PV case this will be handled by the
> hypervisor anyway.
>
> Fixes: f1e525009493 ("x86/boot: Skip realmode init code when running as Xen PV guest")
> Reported-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> V2:
> - new approach, avoid calling acpi_get_wakeup_address()

I'll queue this up for 6.3 if the x86 people don't object.

Thanks!

> ---
>  arch/x86/include/asm/acpi.h | 8 ++++++++
>  drivers/acpi/sleep.c        | 6 +++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> index 65064d9f7fa6..8eb74cf386db 100644
> --- a/arch/x86/include/asm/acpi.h
> +++ b/arch/x86/include/asm/acpi.h
> @@ -14,6 +14,7 @@
>  #include <asm/mmu.h>
>  #include <asm/mpspec.h>
>  #include <asm/x86_init.h>
> +#include <asm/cpufeature.h>
>
>  #ifdef CONFIG_ACPI_APEI
>  # include <asm/pgtable_types.h>
> @@ -63,6 +64,13 @@ extern int (*acpi_suspend_lowlevel)(void);
>  /* Physical address to resume after wakeup */
>  unsigned long acpi_get_wakeup_address(void);
>
> +static inline bool acpi_skip_set_wakeup_address(void)
> +{
> +       return cpu_feature_enabled(X86_FEATURE_XENPV);
> +}
> +
> +#define acpi_skip_set_wakeup_address acpi_skip_set_wakeup_address
> +
>  /*
>   * Check if the CPU can handle C2 and deeper
>   */
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 0b557c0d405e..4ca667251272 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -60,13 +60,17 @@ static struct notifier_block tts_notifier = {
>         .priority       = 0,
>  };
>
> +#ifndef acpi_skip_set_wakeup_address
> +#define acpi_skip_set_wakeup_address() false
> +#endif
> +
>  static int acpi_sleep_prepare(u32 acpi_state)
>  {
>  #ifdef CONFIG_ACPI_SLEEP
>         unsigned long acpi_wakeup_address;
>
>         /* do we have a wakeup address for S2 and S3? */
> -       if (acpi_state == ACPI_STATE_S3) {
> +       if (acpi_state == ACPI_STATE_S3 && !acpi_skip_set_wakeup_address()) {
>                 acpi_wakeup_address = acpi_get_wakeup_address();
>                 if (!acpi_wakeup_address)
>                         return -EFAULT;
> --