| Message ID | 20230117155724.22940-1-jgross@suse.com |
|---|---|
| State | Accepted |
| Commit | fe0ba8c23f9a35b0307eb662f16dd3a75fcdae41 |
| Headers | show |
| Series | [v2] acpi: fix suspend with Xen PV | expand |
On Tue, Jan 17, 2023 at 4:57 PM Juergen Gross <jgross@suse.com> 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@invisiblethingslab.com> > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > 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; > --
On Tue, Jan 17, 2023 at 5:51 PM Dave Hansen <dave.hansen@intel.com> wrote: > > On 1/17/23 08:47, Rafael J. Wysocki wrote: > >> Fixes: f1e525009493 ("x86/boot: Skip realmode init code when running as Xen PV guest") > >> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > >> Signed-off-by: Juergen Gross <jgross@suse.com> > >> --- > >> V2: > >> - new approach, avoid calling acpi_get_wakeup_address() > > I'll queue this up for 6.3 if the x86 people don't object. > > That 'Fixes:' patch looked to me like it was applied during the 6.2 > merge window. I would have expected a fix to go up for 6.2 final. Right, sorry. Definitely, 6.2-rc material.
On 1/17/23 09:08, Rafael J. Wysocki wrote: > On Tue, Jan 17, 2023 at 5:51 PM Dave Hansen <dave.hansen@intel.com> wrote: >> On 1/17/23 08:47, Rafael J. Wysocki wrote: >>>> Fixes: f1e525009493 ("x86/boot: Skip realmode init code when running as Xen PV guest") >>>> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>> --- >>>> V2: >>>> - new approach, avoid calling acpi_get_wakeup_address() >>> I'll queue this up for 6.3 if the x86 people don't object. >> That 'Fixes:' patch looked to me like it was applied during the 6.2 >> merge window. I would have expected a fix to go up for 6.2 final. > Right, sorry. Definitely, 6.2-rc material. I think the original issue went up in x86/boot, so how about I queue this in x86/urgent? Rafael, should I add an ack from you on it?
On Wed, Jan 18, 2023 at 1:15 AM Dave Hansen <dave.hansen@intel.com> wrote: > > On 1/17/23 09:08, Rafael J. Wysocki wrote: > > On Tue, Jan 17, 2023 at 5:51 PM Dave Hansen <dave.hansen@intel.com> wrote: > >> On 1/17/23 08:47, Rafael J. Wysocki wrote: > >>>> Fixes: f1e525009493 ("x86/boot: Skip realmode init code when running as Xen PV guest") > >>>> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > >>>> Signed-off-by: Juergen Gross <jgross@suse.com> > >>>> --- > >>>> V2: > >>>> - new approach, avoid calling acpi_get_wakeup_address() > >>> I'll queue this up for 6.3 if the x86 people don't object. > >> That 'Fixes:' patch looked to me like it was applied during the 6.2 > >> merge window. I would have expected a fix to go up for 6.2 final. > > Right, sorry. Definitely, 6.2-rc material. > > I think the original issue went up in x86/boot, so how about I queue > this in x86/urgent? Rafael, should I add an ack from you on it? Yes, please. Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
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;
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@invisiblethingslab.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - new approach, avoid calling acpi_get_wakeup_address() --- arch/x86/include/asm/acpi.h | 8 ++++++++ drivers/acpi/sleep.c | 6 +++++- 2 files changed, 13 insertions(+), 1 deletion(-)