* [PATCH 0/2] Fixes for s3 with parallel bootup @ 2023-10-23 16:00 Mario Limonciello 2023-10-23 16:00 ` [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously Mario Limonciello 2023-10-23 16:00 ` [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() Mario Limonciello 0 siblings, 2 replies; 18+ messages in thread From: Mario Limonciello @ 2023-10-23 16:00 UTC (permalink / raw) To: Peter Zijlstra, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin Cc: linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter, Mario Limonciello Parallel bootup on systems that use x2apic broke suspend to ram. This series ensures x2apic is re-enabled at startup and fixes an exposed pre-emption issue. Mario Limonciello (2): x86: Enable x2apic during resume from suspend if used previously perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() arch/x86/events/amd/lbr.c | 3 ++- arch/x86/include/asm/smp.h | 1 + arch/x86/kernel/acpi/sleep.c | 12 ++++++++---- arch/x86/kernel/head_64.S | 15 +++++++++++++++ 4 files changed, 26 insertions(+), 5 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously 2023-10-23 16:00 [PATCH 0/2] Fixes for s3 with parallel bootup Mario Limonciello @ 2023-10-23 16:00 ` Mario Limonciello 2023-10-24 8:36 ` Ingo Molnar 2023-10-23 16:00 ` [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() Mario Limonciello 1 sibling, 1 reply; 18+ messages in thread From: Mario Limonciello @ 2023-10-23 16:00 UTC (permalink / raw) To: Peter Zijlstra, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin Cc: linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter, Mario Limonciello If x2apic was enabled during boot with parallel startup it will be needed during resume from suspend to ram as well. Store whether to enable into the smpboot_control global variable and during startup re-enable it if necessary. Cc: stable@vger.kernel.org # 6.5+ Fixes: 0c7ffa32dbd6 ("x86/smpboot/64: Implement arch_cpuhp_init_parallel_bringup() and enable it") Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- arch/x86/include/asm/smp.h | 1 + arch/x86/kernel/acpi/sleep.c | 12 ++++++++---- arch/x86/kernel/head_64.S | 15 +++++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h index c31c633419fe..86584ffaebc3 100644 --- a/arch/x86/include/asm/smp.h +++ b/arch/x86/include/asm/smp.h @@ -190,6 +190,7 @@ extern unsigned long apic_mmio_base; #endif /* !__ASSEMBLY__ */ /* Control bits for startup_64 */ +#define STARTUP_ENABLE_X2APIC 0x40000000 #define STARTUP_READ_APICID 0x80000000 /* Top 8 bits are reserved for control */ diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c index 6dfecb27b846..29734a1299f6 100644 --- a/arch/x86/kernel/acpi/sleep.c +++ b/arch/x86/kernel/acpi/sleep.c @@ -11,6 +11,7 @@ #include <linux/dmi.h> #include <linux/cpumask.h> #include <linux/pgtable.h> +#include <asm/apic.h> #include <asm/segment.h> #include <asm/desc.h> #include <asm/cacheflush.h> @@ -129,11 +130,14 @@ int x86_acpi_suspend_lowlevel(void) */ current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack); /* - * Ensure the CPU knows which one it is when it comes back, if - * it isn't in parallel mode and expected to work that out for - * itself. + * Ensure x2apic is re-enabled if necessary and the CPU knows which + * one it is when it comes back, if it isn't in parallel mode and + * expected to work that out for itself. */ - if (!(smpboot_control & STARTUP_PARALLEL_MASK)) + if (smpboot_control & STARTUP_PARALLEL_MASK) { + if (x2apic_enabled()) + smpboot_control |= STARTUP_ENABLE_X2APIC; + } else smpboot_control = smp_processor_id(); #endif initial_code = (unsigned long)wakeup_long64; diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S index ea6995920b7a..fcfa79105928 100644 --- a/arch/x86/kernel/head_64.S +++ b/arch/x86/kernel/head_64.S @@ -236,10 +236,15 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL) * used to look up the CPU number. For booting a single CPU, the * CPU number is encoded in smpboot_control. * + * Bit 30 STARTUP_ENABLE_X2APIC (Enable X2APIC mode) * Bit 31 STARTUP_READ_APICID (Read APICID from APIC) * Bit 0-23 CPU# if STARTUP_xx flags are not set */ movl smpboot_control(%rip), %ecx + + testl $STARTUP_ENABLE_X2APIC, %ecx + jnz .Lenable_x2apic + testl $STARTUP_READ_APICID, %ecx jnz .Lread_apicid /* @@ -249,6 +254,16 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL) andl $(~STARTUP_PARALLEL_MASK), %ecx jmp .Lsetup_cpu +.Lenable_x2apic: + /* Enable X2APIC if disabled */ + mov $MSR_IA32_APICBASE, %ecx + rdmsr + testl $X2APIC_ENABLE, %eax + jnz .Lread_apicid_msr + orl $X2APIC_ENABLE, %eax + wrmsr + jmp .Lread_apicid_msr + .Lread_apicid: /* Check whether X2APIC mode is already enabled */ mov $MSR_IA32_APICBASE, %ecx -- 2.34.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously 2023-10-23 16:00 ` [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously Mario Limonciello @ 2023-10-24 8:36 ` Ingo Molnar 2023-10-24 15:36 ` Mario Limonciello 0 siblings, 1 reply; 18+ messages in thread From: Ingo Molnar @ 2023-10-24 8:36 UTC (permalink / raw) To: Mario Limonciello Cc: Peter Zijlstra, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter * Mario Limonciello <mario.limonciello@amd.com> wrote: > If x2apic was enabled during boot with parallel startup > it will be needed during resume from suspend to ram as well. > > Store whether to enable into the smpboot_control global variable > and during startup re-enable it if necessary. > > Cc: stable@vger.kernel.org # 6.5+ > Fixes: 0c7ffa32dbd6 ("x86/smpboot/64: Implement arch_cpuhp_init_parallel_bringup() and enable it") > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > arch/x86/include/asm/smp.h | 1 + > arch/x86/kernel/acpi/sleep.c | 12 ++++++++---- > arch/x86/kernel/head_64.S | 15 +++++++++++++++ > 3 files changed, 24 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h > index c31c633419fe..86584ffaebc3 100644 > --- a/arch/x86/include/asm/smp.h > +++ b/arch/x86/include/asm/smp.h > @@ -190,6 +190,7 @@ extern unsigned long apic_mmio_base; > #endif /* !__ASSEMBLY__ */ > > /* Control bits for startup_64 */ > +#define STARTUP_ENABLE_X2APIC 0x40000000 > #define STARTUP_READ_APICID 0x80000000 > > /* Top 8 bits are reserved for control */ > diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c > index 6dfecb27b846..29734a1299f6 100644 > --- a/arch/x86/kernel/acpi/sleep.c > +++ b/arch/x86/kernel/acpi/sleep.c > @@ -11,6 +11,7 @@ > #include <linux/dmi.h> > #include <linux/cpumask.h> > #include <linux/pgtable.h> > +#include <asm/apic.h> > #include <asm/segment.h> > #include <asm/desc.h> > #include <asm/cacheflush.h> > @@ -129,11 +130,14 @@ int x86_acpi_suspend_lowlevel(void) > */ > current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack); > /* > - * Ensure the CPU knows which one it is when it comes back, if > - * it isn't in parallel mode and expected to work that out for > - * itself. > + * Ensure x2apic is re-enabled if necessary and the CPU knows which > + * one it is when it comes back, if it isn't in parallel mode and > + * expected to work that out for itself. > */ > - if (!(smpboot_control & STARTUP_PARALLEL_MASK)) > + if (smpboot_control & STARTUP_PARALLEL_MASK) { > + if (x2apic_enabled()) > + smpboot_control |= STARTUP_ENABLE_X2APIC; > + } else > smpboot_control = smp_processor_id(); Yeah, so instead of adding further kludges to the 'parallel bringup is possible' code path, which is arguably a functional feature that shouldn't have hardware-management coupled to it, would it be possible to fix parallel bringup to AMD-SEV systems, so that this code path isn't a quirk-dependent "parallel boot" codepath, but simply the "x86 SMP boot codepath", where all SMP x86 systems do a parallel bootup? The original commit by Thomas says: 0c7ffa32dbd6 ("x86/smpboot/64: Implement arch_cpuhp_init_parallel_bringup() and enable it") | Unfortunately there is no RDMSR GHCB protocol at the moment, so enabling | AMD-SEV guests for parallel startup needs some more thought. But that was half a year ago, isn't there RDMSR GHCB access code available now? This code would all read a lot more natural if it was the regular x86 SMP bootup path - which it is 'almost' today already, modulo quirk. Obviously coupling functional features with hardware quirks is fragile, for example your patch extending x86 SMP parallel bringup doesn't extend the AMD-SEV case, which may or may not matter in practice. So, if it's possible, it would be nice to fix AMD-SEV systems as well and remove this artificial coupling. Also, side note #1: curly braces should be balanced. > #endif > initial_code = (unsigned long)wakeup_long64; > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S > index ea6995920b7a..fcfa79105928 100644 > --- a/arch/x86/kernel/head_64.S > +++ b/arch/x86/kernel/head_64.S > @@ -236,10 +236,15 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL) > * used to look up the CPU number. For booting a single CPU, the > * CPU number is encoded in smpboot_control. > * > + * Bit 30 STARTUP_ENABLE_X2APIC (Enable X2APIC mode) > * Bit 31 STARTUP_READ_APICID (Read APICID from APIC) > * Bit 0-23 CPU# if STARTUP_xx flags are not set Side note #2: you mixed up the comment ordering here. Thanks, Ingo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously 2023-10-24 8:36 ` Ingo Molnar @ 2023-10-24 15:36 ` Mario Limonciello 2023-10-24 17:01 ` Ingo Molnar 2023-10-24 17:30 ` Tom Lendacky 0 siblings, 2 replies; 18+ messages in thread From: Mario Limonciello @ 2023-10-24 15:36 UTC (permalink / raw) To: Ingo Molnar, Tom Lendacky Cc: Peter Zijlstra, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter +Tom On 10/24/2023 03:36, Ingo Molnar wrote: > > * Mario Limonciello <mario.limonciello@amd.com> wrote: > >> If x2apic was enabled during boot with parallel startup >> it will be needed during resume from suspend to ram as well. >> >> Store whether to enable into the smpboot_control global variable >> and during startup re-enable it if necessary. >> >> Cc: stable@vger.kernel.org # 6.5+ >> Fixes: 0c7ffa32dbd6 ("x86/smpboot/64: Implement arch_cpuhp_init_parallel_bringup() and enable it") >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> arch/x86/include/asm/smp.h | 1 + >> arch/x86/kernel/acpi/sleep.c | 12 ++++++++---- >> arch/x86/kernel/head_64.S | 15 +++++++++++++++ >> 3 files changed, 24 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h >> index c31c633419fe..86584ffaebc3 100644 >> --- a/arch/x86/include/asm/smp.h >> +++ b/arch/x86/include/asm/smp.h >> @@ -190,6 +190,7 @@ extern unsigned long apic_mmio_base; >> #endif /* !__ASSEMBLY__ */ >> >> /* Control bits for startup_64 */ >> +#define STARTUP_ENABLE_X2APIC 0x40000000 >> #define STARTUP_READ_APICID 0x80000000 >> >> /* Top 8 bits are reserved for control */ >> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c >> index 6dfecb27b846..29734a1299f6 100644 >> --- a/arch/x86/kernel/acpi/sleep.c >> +++ b/arch/x86/kernel/acpi/sleep.c >> @@ -11,6 +11,7 @@ >> #include <linux/dmi.h> >> #include <linux/cpumask.h> >> #include <linux/pgtable.h> >> +#include <asm/apic.h> >> #include <asm/segment.h> >> #include <asm/desc.h> >> #include <asm/cacheflush.h> >> @@ -129,11 +130,14 @@ int x86_acpi_suspend_lowlevel(void) >> */ >> current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack); >> /* >> - * Ensure the CPU knows which one it is when it comes back, if >> - * it isn't in parallel mode and expected to work that out for >> - * itself. >> + * Ensure x2apic is re-enabled if necessary and the CPU knows which >> + * one it is when it comes back, if it isn't in parallel mode and >> + * expected to work that out for itself. >> */ >> - if (!(smpboot_control & STARTUP_PARALLEL_MASK)) >> + if (smpboot_control & STARTUP_PARALLEL_MASK) { >> + if (x2apic_enabled()) >> + smpboot_control |= STARTUP_ENABLE_X2APIC; >> + } else >> smpboot_control = smp_processor_id(); > > Yeah, so instead of adding further kludges to the 'parallel bringup is > possible' code path, which is arguably a functional feature that shouldn't > have hardware-management coupled to it, would it be possible to fix > parallel bringup to AMD-SEV systems, so that this code path isn't a > quirk-dependent "parallel boot" codepath, but simply the "x86 SMP boot > codepath", where all SMP x86 systems do a parallel bootup? > > The original commit by Thomas says: > > 0c7ffa32dbd6 ("x86/smpboot/64: Implement arch_cpuhp_init_parallel_bringup() and enable it") > > | Unfortunately there is no RDMSR GHCB protocol at the moment, so enabling > | AMD-SEV guests for parallel startup needs some more thought. > > But that was half a year ago, isn't there RDMSR GHCB access code available now? > > This code would all read a lot more natural if it was the regular x86 SMP > bootup path - which it is 'almost' today already, modulo quirk. > > Obviously coupling functional features with hardware quirks is fragile, for > example your patch extending x86 SMP parallel bringup doesn't extend the > AMD-SEV case, which may or may not matter in practice. > > So, if it's possible, it would be nice to fix AMD-SEV systems as well and > remove this artificial coupling. It probably isn't clear since I didn't mention it in the commit message, but this is not a system that supports AMD-SEV. This is a workstation that supports x2apic. I'll clarify that for V2. I've looped Tom in to comment whether it's possible to improve AMD-SEV as well. > > Also, side note #1: curly braces should be balanced. > >> #endif >> initial_code = (unsigned long)wakeup_long64; >> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S >> index ea6995920b7a..fcfa79105928 100644 >> --- a/arch/x86/kernel/head_64.S >> +++ b/arch/x86/kernel/head_64.S >> @@ -236,10 +236,15 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL) >> * used to look up the CPU number. For booting a single CPU, the >> * CPU number is encoded in smpboot_control. >> * >> + * Bit 30 STARTUP_ENABLE_X2APIC (Enable X2APIC mode) >> * Bit 31 STARTUP_READ_APICID (Read APICID from APIC) >> * Bit 0-23 CPU# if STARTUP_xx flags are not set > > Side note #2: you mixed up the comment ordering here. > > Thanks, > > Ingo Sure, thanks for the feedback. I'll adjust the style for v2. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously 2023-10-24 15:36 ` Mario Limonciello @ 2023-10-24 17:01 ` Ingo Molnar 2023-10-25 19:04 ` Mario Limonciello 2023-10-24 17:30 ` Tom Lendacky 1 sibling, 1 reply; 18+ messages in thread From: Ingo Molnar @ 2023-10-24 17:01 UTC (permalink / raw) To: Mario Limonciello Cc: Tom Lendacky, Peter Zijlstra, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter * Mario Limonciello <mario.limonciello@amd.com> wrote: > +Tom > > On 10/24/2023 03:36, Ingo Molnar wrote: > > > > * Mario Limonciello <mario.limonciello@amd.com> wrote: > > > > > If x2apic was enabled during boot with parallel startup > > > it will be needed during resume from suspend to ram as well. > > > > > > Store whether to enable into the smpboot_control global variable > > > and during startup re-enable it if necessary. > > > > > > Cc: stable@vger.kernel.org # 6.5+ > > > Fixes: 0c7ffa32dbd6 ("x86/smpboot/64: Implement arch_cpuhp_init_parallel_bringup() and enable it") > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > > --- > > > arch/x86/include/asm/smp.h | 1 + > > > arch/x86/kernel/acpi/sleep.c | 12 ++++++++---- > > > arch/x86/kernel/head_64.S | 15 +++++++++++++++ > > > 3 files changed, 24 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h > > > index c31c633419fe..86584ffaebc3 100644 > > > --- a/arch/x86/include/asm/smp.h > > > +++ b/arch/x86/include/asm/smp.h > > > @@ -190,6 +190,7 @@ extern unsigned long apic_mmio_base; > > > #endif /* !__ASSEMBLY__ */ > > > /* Control bits for startup_64 */ > > > +#define STARTUP_ENABLE_X2APIC 0x40000000 > > > #define STARTUP_READ_APICID 0x80000000 > > > /* Top 8 bits are reserved for control */ > > > diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c > > > index 6dfecb27b846..29734a1299f6 100644 > > > --- a/arch/x86/kernel/acpi/sleep.c > > > +++ b/arch/x86/kernel/acpi/sleep.c > > > @@ -11,6 +11,7 @@ > > > #include <linux/dmi.h> > > > #include <linux/cpumask.h> > > > #include <linux/pgtable.h> > > > +#include <asm/apic.h> > > > #include <asm/segment.h> > > > #include <asm/desc.h> > > > #include <asm/cacheflush.h> > > > @@ -129,11 +130,14 @@ int x86_acpi_suspend_lowlevel(void) > > > */ > > > current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack); > > > /* > > > - * Ensure the CPU knows which one it is when it comes back, if > > > - * it isn't in parallel mode and expected to work that out for > > > - * itself. > > > + * Ensure x2apic is re-enabled if necessary and the CPU knows which > > > + * one it is when it comes back, if it isn't in parallel mode and > > > + * expected to work that out for itself. > > > */ > > > - if (!(smpboot_control & STARTUP_PARALLEL_MASK)) > > > + if (smpboot_control & STARTUP_PARALLEL_MASK) { > > > + if (x2apic_enabled()) > > > + smpboot_control |= STARTUP_ENABLE_X2APIC; > > > + } else > > > smpboot_control = smp_processor_id(); > > > > Yeah, so instead of adding further kludges to the 'parallel bringup is > > possible' code path, which is arguably a functional feature that shouldn't > > have hardware-management coupled to it, would it be possible to fix > > parallel bringup to AMD-SEV systems, so that this code path isn't a > > quirk-dependent "parallel boot" codepath, but simply the "x86 SMP boot > > codepath", where all SMP x86 systems do a parallel bootup? > > > > The original commit by Thomas says: > > > > 0c7ffa32dbd6 ("x86/smpboot/64: Implement arch_cpuhp_init_parallel_bringup() and enable it") > > > > | Unfortunately there is no RDMSR GHCB protocol at the moment, so enabling > > | AMD-SEV guests for parallel startup needs some more thought. > > > > But that was half a year ago, isn't there RDMSR GHCB access code available now? > > > > This code would all read a lot more natural if it was the regular x86 SMP > > bootup path - which it is 'almost' today already, modulo quirk. > > > > Obviously coupling functional features with hardware quirks is fragile, for > > example your patch extending x86 SMP parallel bringup doesn't extend the > > AMD-SEV case, which may or may not matter in practice. > > > > So, if it's possible, it would be nice to fix AMD-SEV systems as well and > > remove this artificial coupling. > > It probably isn't clear since I didn't mention it in the commit message, but > this is not a system that supports AMD-SEV. This is a workstation that > supports x2apic. I'll clarify that for V2. Yes, I suspected as much, but that's irrelevant to the arguments I outlined, that extending upon this quirk that makes SMP parallel bringup HW environment dependent, and then coupling s2ram x2apic re-enablement to that functional feature is inviting trouble in the long run. For example, what guarantees that the x2apic will be turned back on after suspend if a system is booted with maxcpus=1? Obviously something very close to your fix is needed. > I've looped Tom in to comment whether it's possible to improve AMD-SEV as > well. Thanks! Ingo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously 2023-10-24 17:01 ` Ingo Molnar @ 2023-10-25 19:04 ` Mario Limonciello 0 siblings, 0 replies; 18+ messages in thread From: Mario Limonciello @ 2023-10-25 19:04 UTC (permalink / raw) To: Ingo Molnar Cc: Tom Lendacky, Peter Zijlstra, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter On 10/24/2023 12:01, Ingo Molnar wrote: > > * Mario Limonciello <mario.limonciello@amd.com> wrote: > >> +Tom >> >> On 10/24/2023 03:36, Ingo Molnar wrote: >>> >>> * Mario Limonciello <mario.limonciello@amd.com> wrote: >>> >>>> If x2apic was enabled during boot with parallel startup >>>> it will be needed during resume from suspend to ram as well. >>>> >>>> Store whether to enable into the smpboot_control global variable >>>> and during startup re-enable it if necessary. >>>> >>>> Cc: stable@vger.kernel.org # 6.5+ >>>> Fixes: 0c7ffa32dbd6 ("x86/smpboot/64: Implement arch_cpuhp_init_parallel_bringup() and enable it") >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>>> --- >>>> arch/x86/include/asm/smp.h | 1 + >>>> arch/x86/kernel/acpi/sleep.c | 12 ++++++++---- >>>> arch/x86/kernel/head_64.S | 15 +++++++++++++++ >>>> 3 files changed, 24 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h >>>> index c31c633419fe..86584ffaebc3 100644 >>>> --- a/arch/x86/include/asm/smp.h >>>> +++ b/arch/x86/include/asm/smp.h >>>> @@ -190,6 +190,7 @@ extern unsigned long apic_mmio_base; >>>> #endif /* !__ASSEMBLY__ */ >>>> /* Control bits for startup_64 */ >>>> +#define STARTUP_ENABLE_X2APIC 0x40000000 >>>> #define STARTUP_READ_APICID 0x80000000 >>>> /* Top 8 bits are reserved for control */ >>>> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c >>>> index 6dfecb27b846..29734a1299f6 100644 >>>> --- a/arch/x86/kernel/acpi/sleep.c >>>> +++ b/arch/x86/kernel/acpi/sleep.c >>>> @@ -11,6 +11,7 @@ >>>> #include <linux/dmi.h> >>>> #include <linux/cpumask.h> >>>> #include <linux/pgtable.h> >>>> +#include <asm/apic.h> >>>> #include <asm/segment.h> >>>> #include <asm/desc.h> >>>> #include <asm/cacheflush.h> >>>> @@ -129,11 +130,14 @@ int x86_acpi_suspend_lowlevel(void) >>>> */ >>>> current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack); >>>> /* >>>> - * Ensure the CPU knows which one it is when it comes back, if >>>> - * it isn't in parallel mode and expected to work that out for >>>> - * itself. >>>> + * Ensure x2apic is re-enabled if necessary and the CPU knows which >>>> + * one it is when it comes back, if it isn't in parallel mode and >>>> + * expected to work that out for itself. >>>> */ >>>> - if (!(smpboot_control & STARTUP_PARALLEL_MASK)) >>>> + if (smpboot_control & STARTUP_PARALLEL_MASK) { >>>> + if (x2apic_enabled()) >>>> + smpboot_control |= STARTUP_ENABLE_X2APIC; >>>> + } else >>>> smpboot_control = smp_processor_id(); >>> >>> Yeah, so instead of adding further kludges to the 'parallel bringup is >>> possible' code path, which is arguably a functional feature that shouldn't >>> have hardware-management coupled to it, would it be possible to fix >>> parallel bringup to AMD-SEV systems, so that this code path isn't a >>> quirk-dependent "parallel boot" codepath, but simply the "x86 SMP boot >>> codepath", where all SMP x86 systems do a parallel bootup? >>> >>> The original commit by Thomas says: >>> >>> 0c7ffa32dbd6 ("x86/smpboot/64: Implement arch_cpuhp_init_parallel_bringup() and enable it") >>> >>> | Unfortunately there is no RDMSR GHCB protocol at the moment, so enabling >>> | AMD-SEV guests for parallel startup needs some more thought. >>> >>> But that was half a year ago, isn't there RDMSR GHCB access code available now? >>> >>> This code would all read a lot more natural if it was the regular x86 SMP >>> bootup path - which it is 'almost' today already, modulo quirk. >>> >>> Obviously coupling functional features with hardware quirks is fragile, for >>> example your patch extending x86 SMP parallel bringup doesn't extend the >>> AMD-SEV case, which may or may not matter in practice. >>> >>> So, if it's possible, it would be nice to fix AMD-SEV systems as well and >>> remove this artificial coupling. >> >> It probably isn't clear since I didn't mention it in the commit message, but >> this is not a system that supports AMD-SEV. This is a workstation that >> supports x2apic. I'll clarify that for V2. > > Yes, I suspected as much, but that's irrelevant to the arguments I > outlined, that extending upon this quirk that makes SMP parallel bringup HW > environment dependent, and then coupling s2ram x2apic re-enablement to that > functional feature is inviting trouble in the long run. > I spent some more time looking at ways to decouple this, and AFAICT thaw_secondary_cpus() doesn't actually bring CPUs back after resume in parallel mode. To be symmetrical with that, another way to solve this that removes the "HW environment" aspect is to disable parallel boot for resume from sleep entirely. Like this: diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c index 6dfecb27b846..9265d97f497b 100644 --- a/arch/x86/kernel/acpi/sleep.c +++ b/arch/x86/kernel/acpi/sleep.c @@ -128,13 +128,12 @@ int x86_acpi_suspend_lowlevel(void) * value is in the actual %rsp register. */ current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack); - /* - * Ensure the CPU knows which one it is when it comes back, if - * it isn't in parallel mode and expected to work that out for - * itself. + /* + * Don't use parallel startup for resume from sleep. This avoids + * hangs that may occur if x2apic was in use but platform + * has not enabled x2apic on it's own after resume. */ - if (!(smpboot_control & STARTUP_PARALLEL_MASK)) - smpboot_control = smp_processor_id(); + smpboot_control = smp_processor_id(); #endif initial_code = (unsigned long)wakeup_long64; saved_magic = 0x123456789abcdef0L; > For example, what guarantees that the x2apic will be turned back on after > suspend if a system is booted with maxcpus=1? lapic_resume() will do this after the boot CPU makes it up. > > Obviously something very close to your fix is needed. > Given lapic_resume() handles this, I'd think with the style fixups you suggested my patch is appropriate. >> I've looped Tom in to comment whether it's possible to improve AMD-SEV as >> well. > > Thanks! > > Ingo ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously 2023-10-24 15:36 ` Mario Limonciello 2023-10-24 17:01 ` Ingo Molnar @ 2023-10-24 17:30 ` Tom Lendacky 1 sibling, 0 replies; 18+ messages in thread From: Tom Lendacky @ 2023-10-24 17:30 UTC (permalink / raw) To: Mario Limonciello, Ingo Molnar Cc: Peter Zijlstra, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter On 10/24/23 10:36, Mario Limonciello wrote: > +Tom > > On 10/24/2023 03:36, Ingo Molnar wrote: >> >> * Mario Limonciello <mario.limonciello@amd.com> wrote: >> >>> If x2apic was enabled during boot with parallel startup >>> it will be needed during resume from suspend to ram as well. >>> >>> Store whether to enable into the smpboot_control global variable >>> and during startup re-enable it if necessary. >>> >>> Cc: stable@vger.kernel.org # 6.5+ >>> Fixes: 0c7ffa32dbd6 ("x86/smpboot/64: Implement >>> arch_cpuhp_init_parallel_bringup() and enable it") >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>> --- >>> arch/x86/include/asm/smp.h | 1 + >>> arch/x86/kernel/acpi/sleep.c | 12 ++++++++---- >>> arch/x86/kernel/head_64.S | 15 +++++++++++++++ >>> 3 files changed, 24 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h >>> index c31c633419fe..86584ffaebc3 100644 >>> --- a/arch/x86/include/asm/smp.h >>> +++ b/arch/x86/include/asm/smp.h >>> @@ -190,6 +190,7 @@ extern unsigned long apic_mmio_base; >>> #endif /* !__ASSEMBLY__ */ >>> /* Control bits for startup_64 */ >>> +#define STARTUP_ENABLE_X2APIC 0x40000000 >>> #define STARTUP_READ_APICID 0x80000000 >>> /* Top 8 bits are reserved for control */ >>> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c >>> index 6dfecb27b846..29734a1299f6 100644 >>> --- a/arch/x86/kernel/acpi/sleep.c >>> +++ b/arch/x86/kernel/acpi/sleep.c >>> @@ -11,6 +11,7 @@ >>> #include <linux/dmi.h> >>> #include <linux/cpumask.h> >>> #include <linux/pgtable.h> >>> +#include <asm/apic.h> >>> #include <asm/segment.h> >>> #include <asm/desc.h> >>> #include <asm/cacheflush.h> >>> @@ -129,11 +130,14 @@ int x86_acpi_suspend_lowlevel(void) >>> */ >>> current->thread.sp = (unsigned long)temp_stack + sizeof(temp_stack); >>> /* >>> - * Ensure the CPU knows which one it is when it comes back, if >>> - * it isn't in parallel mode and expected to work that out for >>> - * itself. >>> + * Ensure x2apic is re-enabled if necessary and the CPU knows which >>> + * one it is when it comes back, if it isn't in parallel mode and >>> + * expected to work that out for itself. >>> */ >>> - if (!(smpboot_control & STARTUP_PARALLEL_MASK)) >>> + if (smpboot_control & STARTUP_PARALLEL_MASK) { >>> + if (x2apic_enabled()) >>> + smpboot_control |= STARTUP_ENABLE_X2APIC; >>> + } else >>> smpboot_control = smp_processor_id(); >> >> Yeah, so instead of adding further kludges to the 'parallel bringup is >> possible' code path, which is arguably a functional feature that shouldn't >> have hardware-management coupled to it, would it be possible to fix >> parallel bringup to AMD-SEV systems, so that this code path isn't a >> quirk-dependent "parallel boot" codepath, but simply the "x86 SMP boot >> codepath", where all SMP x86 systems do a parallel bootup? >> >> The original commit by Thomas says: >> >> 0c7ffa32dbd6 ("x86/smpboot/64: Implement >> arch_cpuhp_init_parallel_bringup() and enable it") >> >> | Unfortunately there is no RDMSR GHCB protocol at the moment, so >> enabling >> | AMD-SEV guests for parallel startup needs some more thought. >> >> But that was half a year ago, isn't there RDMSR GHCB access code >> available now? That support requires an update to the GHCB specification to add RDMSR/WRMSR access to the GHCB MSR protocol, which hasn't been written, yet. The support would have to be present in both the hypervisor and the guest. Thanks, Tom >> >> This code would all read a lot more natural if it was the regular x86 SMP >> bootup path - which it is 'almost' today already, modulo quirk. >> >> Obviously coupling functional features with hardware quirks is fragile, for >> example your patch extending x86 SMP parallel bringup doesn't extend the >> AMD-SEV case, which may or may not matter in practice. >> >> So, if it's possible, it would be nice to fix AMD-SEV systems as well and >> remove this artificial coupling. > > It probably isn't clear since I didn't mention it in the commit message, > but this is not a system that supports AMD-SEV. This is a workstation > that supports x2apic. I'll clarify that for V2. > > I've looped Tom in to comment whether it's possible to improve AMD-SEV as > well. > >> >> Also, side note #1: curly braces should be balanced. >> >>> #endif >>> initial_code = (unsigned long)wakeup_long64; >>> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S >>> index ea6995920b7a..fcfa79105928 100644 >>> --- a/arch/x86/kernel/head_64.S >>> +++ b/arch/x86/kernel/head_64.S >>> @@ -236,10 +236,15 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, >>> SYM_L_GLOBAL) >>> * used to look up the CPU number. For booting a single CPU, the >>> * CPU number is encoded in smpboot_control. >>> * >>> + * Bit 30 STARTUP_ENABLE_X2APIC (Enable X2APIC mode) >>> * Bit 31 STARTUP_READ_APICID (Read APICID from APIC) >>> * Bit 0-23 CPU# if STARTUP_xx flags are not set >> >> Side note #2: you mixed up the comment ordering here. >> >> Thanks, >> >> Ingo > > Sure, thanks for the feedback. I'll adjust the style for v2. > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() 2023-10-23 16:00 [PATCH 0/2] Fixes for s3 with parallel bootup Mario Limonciello 2023-10-23 16:00 ` [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously Mario Limonciello @ 2023-10-23 16:00 ` Mario Limonciello 2023-10-24 8:02 ` Ingo Molnar 1 sibling, 1 reply; 18+ messages in thread From: Mario Limonciello @ 2023-10-23 16:00 UTC (permalink / raw) To: Peter Zijlstra, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin Cc: linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter, Mario Limonciello Fixes a BUG reported during suspend to ram testing. ``` [ 478.274752] BUG: using smp_processor_id() in preemptible [00000000] code: rtcwake/2948 [ 478.274754] caller is amd_pmu_lbr_reset+0x19/0xc0 ``` Cc: stable@vger.kernel.org # 6.1+ Fixes: ca5b7c0d9621 ("perf/x86/amd/lbr: Add LbrExtV2 branch record support") Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- arch/x86/events/amd/lbr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/x86/events/amd/lbr.c b/arch/x86/events/amd/lbr.c index eb31f850841a..5b98e8c7d8b7 100644 --- a/arch/x86/events/amd/lbr.c +++ b/arch/x86/events/amd/lbr.c @@ -321,7 +321,7 @@ int amd_pmu_lbr_hw_config(struct perf_event *event) void amd_pmu_lbr_reset(void) { - struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); + struct cpu_hw_events *cpuc = get_cpu_ptr(&cpu_hw_events); int i; if (!x86_pmu.lbr_nr) @@ -335,6 +335,7 @@ void amd_pmu_lbr_reset(void) cpuc->last_task_ctx = NULL; cpuc->last_log_id = 0; + put_cpu_ptr(&cpu_hw_events); wrmsrl(MSR_AMD64_LBR_SELECT, 0); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() 2023-10-23 16:00 ` [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() Mario Limonciello @ 2023-10-24 8:02 ` Ingo Molnar 2023-10-24 15:32 ` Mario Limonciello 2023-10-24 16:51 ` Ingo Molnar 0 siblings, 2 replies; 18+ messages in thread From: Ingo Molnar @ 2023-10-24 8:02 UTC (permalink / raw) To: Mario Limonciello Cc: Peter Zijlstra, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter * Mario Limonciello <mario.limonciello@amd.com> wrote: > Fixes a BUG reported during suspend to ram testing. > > ``` > [ 478.274752] BUG: using smp_processor_id() in preemptible [00000000] code: rtcwake/2948 > [ 478.274754] caller is amd_pmu_lbr_reset+0x19/0xc0 > ``` > > Cc: stable@vger.kernel.org # 6.1+ > Fixes: ca5b7c0d9621 ("perf/x86/amd/lbr: Add LbrExtV2 branch record support") > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > arch/x86/events/amd/lbr.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/events/amd/lbr.c b/arch/x86/events/amd/lbr.c > index eb31f850841a..5b98e8c7d8b7 100644 > --- a/arch/x86/events/amd/lbr.c > +++ b/arch/x86/events/amd/lbr.c > @@ -321,7 +321,7 @@ int amd_pmu_lbr_hw_config(struct perf_event *event) > > void amd_pmu_lbr_reset(void) > { > - struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); > + struct cpu_hw_events *cpuc = get_cpu_ptr(&cpu_hw_events); > int i; > > if (!x86_pmu.lbr_nr) > @@ -335,6 +335,7 @@ void amd_pmu_lbr_reset(void) > > cpuc->last_task_ctx = NULL; > cpuc->last_log_id = 0; > + put_cpu_ptr(&cpu_hw_events); > wrmsrl(MSR_AMD64_LBR_SELECT, 0); > } Weird, amd_pmu_lbr_reset() is called from these places: - amd_pmu_lbr_sched_task(): during task sched-in during context-switching, this should already have preemption disabled. - amd_pmu_lbr_add(): this gets indirectly called by amd_pmu::add (amd_pmu_add_event()), called by event_sched_in(), which too should have preemption disabled. I clearly must have missed some additional place it gets called in. Could you please cite the full log of the amd_pmu_lbr_reset() call that caused the critical section warning? Thanks, Ingo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() 2023-10-24 8:02 ` Ingo Molnar @ 2023-10-24 15:32 ` Mario Limonciello 2023-10-24 15:59 ` Peter Zijlstra 2023-10-24 16:51 ` Ingo Molnar 1 sibling, 1 reply; 18+ messages in thread From: Mario Limonciello @ 2023-10-24 15:32 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter On 10/24/2023 03:02, Ingo Molnar wrote: > > * Mario Limonciello <mario.limonciello@amd.com> wrote: > >> Fixes a BUG reported during suspend to ram testing. >> >> ``` >> [ 478.274752] BUG: using smp_processor_id() in preemptible [00000000] code: rtcwake/2948 >> [ 478.274754] caller is amd_pmu_lbr_reset+0x19/0xc0 >> ``` >> >> Cc: stable@vger.kernel.org # 6.1+ >> Fixes: ca5b7c0d9621 ("perf/x86/amd/lbr: Add LbrExtV2 branch record support") >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> arch/x86/events/amd/lbr.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/events/amd/lbr.c b/arch/x86/events/amd/lbr.c >> index eb31f850841a..5b98e8c7d8b7 100644 >> --- a/arch/x86/events/amd/lbr.c >> +++ b/arch/x86/events/amd/lbr.c >> @@ -321,7 +321,7 @@ int amd_pmu_lbr_hw_config(struct perf_event *event) >> >> void amd_pmu_lbr_reset(void) >> { >> - struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); >> + struct cpu_hw_events *cpuc = get_cpu_ptr(&cpu_hw_events); >> int i; >> >> if (!x86_pmu.lbr_nr) >> @@ -335,6 +335,7 @@ void amd_pmu_lbr_reset(void) >> >> cpuc->last_task_ctx = NULL; >> cpuc->last_log_id = 0; >> + put_cpu_ptr(&cpu_hw_events); >> wrmsrl(MSR_AMD64_LBR_SELECT, 0); >> } > > Weird, amd_pmu_lbr_reset() is called from these places: > > - amd_pmu_lbr_sched_task(): during task sched-in during > context-switching, this should already have preemption disabled. > > - amd_pmu_lbr_add(): this gets indirectly called by amd_pmu::add > (amd_pmu_add_event()), called by event_sched_in(), which too should have > preemption disabled. > > I clearly must have missed some additional place it gets called in. > > Could you please cite the full log of the amd_pmu_lbr_reset() call that > caused the critical section warning? > > Thanks, > > Ingo Below is the call trace in case you think it's better to disable preemption by the caller instead. If you think it's better to keep it in amd_pmu_lbr_reset() I'll add this trace to the commit message. Call Trace: <TASK> dump_stack_lvl+0x44/0x60 check_preemption_disabled+0xce/0xf0 ? __pfx_x86_pmu_dead_cpu+0x10/0x10 amd_pmu_lbr_reset+0x19/0xc0 ? __pfx_x86_pmu_dead_cpu+0x10/0x10 amd_pmu_cpu_reset.constprop.0+0x51/0x60 amd_pmu_cpu_dead+0x3e/0x90 x86_pmu_dead_cpu+0x13/0x20 cpuhp_invoke_callback+0x169/0x4b0 ? __pfx_virtnet_cpu_dead+0x10/0x10 __cpuhp_invoke_callback_range+0x76/0xe0 _cpu_down+0x112/0x270 freeze_secondary_cpus+0x8e/0x280 suspend_devices_and_enter+0x342/0x900 pm_suspend+0x2fd/0x690 state_store+0x71/0xd0 kernfs_fop_write_iter+0x128/0x1c0 vfs_write+0x2db/0x400 ksys_write+0x5f/0xe0 do_syscall_64+0x59/0x90 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() 2023-10-24 15:32 ` Mario Limonciello @ 2023-10-24 15:59 ` Peter Zijlstra 2023-10-24 16:04 ` Mario Limonciello 0 siblings, 1 reply; 18+ messages in thread From: Peter Zijlstra @ 2023-10-24 15:59 UTC (permalink / raw) To: Mario Limonciello Cc: Ingo Molnar, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter On Tue, Oct 24, 2023 at 10:32:27AM -0500, Mario Limonciello wrote: > On 10/24/2023 03:02, Ingo Molnar wrote: > > > > * Mario Limonciello <mario.limonciello@amd.com> wrote: > > > > > Fixes a BUG reported during suspend to ram testing. > > > > > > ``` > > > [ 478.274752] BUG: using smp_processor_id() in preemptible [00000000] code: rtcwake/2948 > > > [ 478.274754] caller is amd_pmu_lbr_reset+0x19/0xc0 > > > ``` > > > > > > Cc: stable@vger.kernel.org # 6.1+ > > > Fixes: ca5b7c0d9621 ("perf/x86/amd/lbr: Add LbrExtV2 branch record support") > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > > --- > > > arch/x86/events/amd/lbr.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/events/amd/lbr.c b/arch/x86/events/amd/lbr.c > > > index eb31f850841a..5b98e8c7d8b7 100644 > > > --- a/arch/x86/events/amd/lbr.c > > > +++ b/arch/x86/events/amd/lbr.c > > > @@ -321,7 +321,7 @@ int amd_pmu_lbr_hw_config(struct perf_event *event) > > > void amd_pmu_lbr_reset(void) > > > { > > > - struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); > > > + struct cpu_hw_events *cpuc = get_cpu_ptr(&cpu_hw_events); > > > int i; > > > if (!x86_pmu.lbr_nr) > > > @@ -335,6 +335,7 @@ void amd_pmu_lbr_reset(void) > > > cpuc->last_task_ctx = NULL; > > > cpuc->last_log_id = 0; > > > + put_cpu_ptr(&cpu_hw_events); > > > wrmsrl(MSR_AMD64_LBR_SELECT, 0); > > > } > > > > Weird, amd_pmu_lbr_reset() is called from these places: > > > > - amd_pmu_lbr_sched_task(): during task sched-in during > > context-switching, this should already have preemption disabled. > > > > - amd_pmu_lbr_add(): this gets indirectly called by amd_pmu::add > > (amd_pmu_add_event()), called by event_sched_in(), which too should have > > preemption disabled. > > > > I clearly must have missed some additional place it gets called in. > > > > Could you please cite the full log of the amd_pmu_lbr_reset() call that > > caused the critical section warning? > > > > Thanks, > > > > Ingo > > Below is the call trace in case you think it's better to disable preemption > by the caller instead. If you think it's better to keep it in > amd_pmu_lbr_reset() I'll add this trace to the commit message. You cut too much; what task is running this? IIRC this is the hotplug thread running a teardown function on that CPU itself. It being a strict per-cpu thread should not trip smp_processor_id() wanrs. > > Call Trace: > <TASK> > dump_stack_lvl+0x44/0x60 > check_preemption_disabled+0xce/0xf0 > ? __pfx_x86_pmu_dead_cpu+0x10/0x10 > amd_pmu_lbr_reset+0x19/0xc0 > ? __pfx_x86_pmu_dead_cpu+0x10/0x10 > amd_pmu_cpu_reset.constprop.0+0x51/0x60 > amd_pmu_cpu_dead+0x3e/0x90 > x86_pmu_dead_cpu+0x13/0x20 > cpuhp_invoke_callback+0x169/0x4b0 > ? __pfx_virtnet_cpu_dead+0x10/0x10 > __cpuhp_invoke_callback_range+0x76/0xe0 > _cpu_down+0x112/0x270 > freeze_secondary_cpus+0x8e/0x280 > suspend_devices_and_enter+0x342/0x900 > pm_suspend+0x2fd/0x690 > state_store+0x71/0xd0 > kernfs_fop_write_iter+0x128/0x1c0 > vfs_write+0x2db/0x400 > ksys_write+0x5f/0xe0 > do_syscall_64+0x59/0x90 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() 2023-10-24 15:59 ` Peter Zijlstra @ 2023-10-24 16:04 ` Mario Limonciello 2023-10-24 16:30 ` Peter Zijlstra 0 siblings, 1 reply; 18+ messages in thread From: Mario Limonciello @ 2023-10-24 16:04 UTC (permalink / raw) To: Peter Zijlstra Cc: Ingo Molnar, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter On 10/24/2023 10:59, Peter Zijlstra wrote: > On Tue, Oct 24, 2023 at 10:32:27AM -0500, Mario Limonciello wrote: >> On 10/24/2023 03:02, Ingo Molnar wrote: >>> >>> * Mario Limonciello <mario.limonciello@amd.com> wrote: >>> >>>> Fixes a BUG reported during suspend to ram testing. >>>> >>>> ``` >>>> [ 478.274752] BUG: using smp_processor_id() in preemptible [00000000] code: rtcwake/2948 >>>> [ 478.274754] caller is amd_pmu_lbr_reset+0x19/0xc0 >>>> ``` >>>> >>>> Cc: stable@vger.kernel.org # 6.1+ >>>> Fixes: ca5b7c0d9621 ("perf/x86/amd/lbr: Add LbrExtV2 branch record support") >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>>> --- >>>> arch/x86/events/amd/lbr.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/x86/events/amd/lbr.c b/arch/x86/events/amd/lbr.c >>>> index eb31f850841a..5b98e8c7d8b7 100644 >>>> --- a/arch/x86/events/amd/lbr.c >>>> +++ b/arch/x86/events/amd/lbr.c >>>> @@ -321,7 +321,7 @@ int amd_pmu_lbr_hw_config(struct perf_event *event) >>>> void amd_pmu_lbr_reset(void) >>>> { >>>> - struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); >>>> + struct cpu_hw_events *cpuc = get_cpu_ptr(&cpu_hw_events); >>>> int i; >>>> if (!x86_pmu.lbr_nr) >>>> @@ -335,6 +335,7 @@ void amd_pmu_lbr_reset(void) >>>> cpuc->last_task_ctx = NULL; >>>> cpuc->last_log_id = 0; >>>> + put_cpu_ptr(&cpu_hw_events); >>>> wrmsrl(MSR_AMD64_LBR_SELECT, 0); >>>> } >>> >>> Weird, amd_pmu_lbr_reset() is called from these places: >>> >>> - amd_pmu_lbr_sched_task(): during task sched-in during >>> context-switching, this should already have preemption disabled. >>> >>> - amd_pmu_lbr_add(): this gets indirectly called by amd_pmu::add >>> (amd_pmu_add_event()), called by event_sched_in(), which too should have >>> preemption disabled. >>> >>> I clearly must have missed some additional place it gets called in. >>> >>> Could you please cite the full log of the amd_pmu_lbr_reset() call that >>> caused the critical section warning? >>> >>> Thanks, >>> >>> Ingo >> >> Below is the call trace in case you think it's better to disable preemption >> by the caller instead. If you think it's better to keep it in >> amd_pmu_lbr_reset() I'll add this trace to the commit message. > > You cut too much; what task is running this? > > IIRC this is the hotplug thread running a teardown function on that CPU > itself. It being a strict per-cpu thread should not trip > smp_processor_id() wanrs. > BUG: using smp_processor_id() in preemptible [00000000] code: rtcwake/2960 caller is amd_pmu_lbr_reset+0x19/0xc0 CPU: 104 PID: 2960 Comm: rtcwake Not tainted 6.6.0-rc6-00002-g3e2c7f3ac51f #1025 Call Trace: <TASK> dump_stack_lvl+0x44/0x60 check_preemption_disabled+0xce/0xf0 ? __pfx_x86_pmu_dead_cpu+0x10/0x10 amd_pmu_lbr_reset+0x19/0xc0 ? __pfx_x86_pmu_dead_cpu+0x10/0x10 amd_pmu_cpu_reset.constprop.0+0x51/0x60 amd_pmu_cpu_dead+0x3e/0x90 x86_pmu_dead_cpu+0x13/0x20 cpuhp_invoke_callback+0x169/0x4b0 ? __pfx_virtnet_cpu_dead+0x10/0x10 __cpuhp_invoke_callback_range+0x76/0xe0 _cpu_down+0x112/0x270 freeze_secondary_cpus+0x8e/0x280 suspend_devices_and_enter+0x342/0x900 pm_suspend+0x2fd/0x690 state_store+0x71/0xd0 kernfs_fop_write_iter+0x128/0x1c0 vfs_write+0x2db/0x400 ksys_write+0x5f/0xe0 do_syscall_64+0x59/0x90 ? srso_alias_return_thunk+0x5/0x7f ? count_memcg_events.constprop.0+0x1a/0x30 ? srso_alias_return_thunk+0x5/0x7f ? handle_mm_fault+0x1e9/0x340 ? srso_alias_return_thunk+0x5/0x7f ? preempt_count_add+0x4d/0xa0 ? srso_alias_return_thunk+0x5/0x7f ? up_read+0x38/0x70 ? srso_alias_return_thunk+0x5/0x7f ? do_user_addr_fault+0x343/0x6b0 ? srso_alias_return_thunk+0x5/0x7f ? exc_page_fault+0x74/0x170 entry_SYSCALL_64_after_hwframe+0x6e/0xd8 RIP: 0033:0x7f32f8d14a77 Code: 10 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24 RSP: 002b:00007ffdc648de18 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007f32f8d14a77 RDX: 0000000000000004 RSI: 000055b2fc2a5670 RDI: 0000000000000004 RBP: 000055b2fc2a5670 R08: 0000000000000000 R09: 000055b2fc2a5670 R10: 00007f32f8e1a2f0 R11: 0000000000000246 R12: 0000000000000004 R13: 000055b2fc2a2480 R14: 00007f32f8e16600 R15: 00007f32f8e15a00 </TASK> >> >> Call Trace: >> <TASK> >> dump_stack_lvl+0x44/0x60 >> check_preemption_disabled+0xce/0xf0 >> ? __pfx_x86_pmu_dead_cpu+0x10/0x10 >> amd_pmu_lbr_reset+0x19/0xc0 >> ? __pfx_x86_pmu_dead_cpu+0x10/0x10 >> amd_pmu_cpu_reset.constprop.0+0x51/0x60 >> amd_pmu_cpu_dead+0x3e/0x90 >> x86_pmu_dead_cpu+0x13/0x20 >> cpuhp_invoke_callback+0x169/0x4b0 >> ? __pfx_virtnet_cpu_dead+0x10/0x10 >> __cpuhp_invoke_callback_range+0x76/0xe0 >> _cpu_down+0x112/0x270 >> freeze_secondary_cpus+0x8e/0x280 >> suspend_devices_and_enter+0x342/0x900 >> pm_suspend+0x2fd/0x690 >> state_store+0x71/0xd0 >> kernfs_fop_write_iter+0x128/0x1c0 >> vfs_write+0x2db/0x400 >> ksys_write+0x5f/0xe0 >> do_syscall_64+0x59/0x90 >> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() 2023-10-24 16:04 ` Mario Limonciello @ 2023-10-24 16:30 ` Peter Zijlstra 2023-10-24 16:34 ` Peter Zijlstra 0 siblings, 1 reply; 18+ messages in thread From: Peter Zijlstra @ 2023-10-24 16:30 UTC (permalink / raw) To: Mario Limonciello Cc: Ingo Molnar, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter On Tue, Oct 24, 2023 at 11:04:06AM -0500, Mario Limonciello wrote: > > IIRC this is the hotplug thread running a teardown function on that CPU > > itself. It being a strict per-cpu thread should not trip > > smp_processor_id() wanrs. > > > > BUG: using smp_processor_id() in preemptible [00000000] code: rtcwake/2960 > caller is amd_pmu_lbr_reset+0x19/0xc0 > CPU: 104 PID: 2960 Comm: rtcwake Not tainted 6.6.0-rc6-00002-g3e2c7f3ac51f Very much not the cpuhp/%u thread :/, let me try and figure out how that happens. > #1025 > Call Trace: > <TASK> > dump_stack_lvl+0x44/0x60 > check_preemption_disabled+0xce/0xf0 > ? __pfx_x86_pmu_dead_cpu+0x10/0x10 > amd_pmu_lbr_reset+0x19/0xc0 > ? __pfx_x86_pmu_dead_cpu+0x10/0x10 > amd_pmu_cpu_reset.constprop.0+0x51/0x60 > amd_pmu_cpu_dead+0x3e/0x90 > x86_pmu_dead_cpu+0x13/0x20 > cpuhp_invoke_callback+0x169/0x4b0 > ? __pfx_virtnet_cpu_dead+0x10/0x10 > __cpuhp_invoke_callback_range+0x76/0xe0 > _cpu_down+0x112/0x270 > freeze_secondary_cpus+0x8e/0x280 > suspend_devices_and_enter+0x342/0x900 > pm_suspend+0x2fd/0x690 > state_store+0x71/0xd0 > kernfs_fop_write_iter+0x128/0x1c0 > vfs_write+0x2db/0x400 > ksys_write+0x5f/0xe0 > do_syscall_64+0x59/0x90 > ? srso_alias_return_thunk+0x5/0x7f > ? count_memcg_events.constprop.0+0x1a/0x30 > ? srso_alias_return_thunk+0x5/0x7f > ? handle_mm_fault+0x1e9/0x340 > ? srso_alias_return_thunk+0x5/0x7f > ? preempt_count_add+0x4d/0xa0 > ? srso_alias_return_thunk+0x5/0x7f > ? up_read+0x38/0x70 > ? srso_alias_return_thunk+0x5/0x7f > ? do_user_addr_fault+0x343/0x6b0 > ? srso_alias_return_thunk+0x5/0x7f > ? exc_page_fault+0x74/0x170 > entry_SYSCALL_64_after_hwframe+0x6e/0xd8 > RIP: 0033:0x7f32f8d14a77 > Code: 10 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa > 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff > 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24 > RSP: 002b:00007ffdc648de18 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 > RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007f32f8d14a77 > RDX: 0000000000000004 RSI: 000055b2fc2a5670 RDI: 0000000000000004 > RBP: 000055b2fc2a5670 R08: 0000000000000000 R09: 000055b2fc2a5670 > R10: 00007f32f8e1a2f0 R11: 0000000000000246 R12: 0000000000000004 > R13: 000055b2fc2a2480 R14: 00007f32f8e16600 R15: 00007f32f8e15a00 > </TASK> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() 2023-10-24 16:30 ` Peter Zijlstra @ 2023-10-24 16:34 ` Peter Zijlstra 2023-10-25 11:47 ` Sandipan Das 0 siblings, 1 reply; 18+ messages in thread From: Peter Zijlstra @ 2023-10-24 16:34 UTC (permalink / raw) To: Mario Limonciello Cc: Ingo Molnar, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter On Tue, Oct 24, 2023 at 06:30:38PM +0200, Peter Zijlstra wrote: > On Tue, Oct 24, 2023 at 11:04:06AM -0500, Mario Limonciello wrote: > > > > IIRC this is the hotplug thread running a teardown function on that CPU > > > itself. It being a strict per-cpu thread should not trip > > > smp_processor_id() wanrs. > > > > > > > BUG: using smp_processor_id() in preemptible [00000000] code: rtcwake/2960 > > caller is amd_pmu_lbr_reset+0x19/0xc0 > > CPU: 104 PID: 2960 Comm: rtcwake Not tainted 6.6.0-rc6-00002-g3e2c7f3ac51f > > Very much not the cpuhp/%u thread :/, let me try and figure out how that > happens. Uhh, my bad, these are the PREPARE/DEAD handlers, they run before online and after dying. The CPU is completely dead. Running lbr_reset() here makes no sense. Did that want to be in amd_pmu_cpu_dying() ? > > > #1025 > > Call Trace: > > <TASK> > > dump_stack_lvl+0x44/0x60 > > check_preemption_disabled+0xce/0xf0 > > ? __pfx_x86_pmu_dead_cpu+0x10/0x10 > > amd_pmu_lbr_reset+0x19/0xc0 > > ? __pfx_x86_pmu_dead_cpu+0x10/0x10 > > amd_pmu_cpu_reset.constprop.0+0x51/0x60 > > amd_pmu_cpu_dead+0x3e/0x90 > > x86_pmu_dead_cpu+0x13/0x20 > > cpuhp_invoke_callback+0x169/0x4b0 > > ? __pfx_virtnet_cpu_dead+0x10/0x10 > > __cpuhp_invoke_callback_range+0x76/0xe0 > > _cpu_down+0x112/0x270 > > freeze_secondary_cpus+0x8e/0x280 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() 2023-10-24 16:34 ` Peter Zijlstra @ 2023-10-25 11:47 ` Sandipan Das 0 siblings, 0 replies; 18+ messages in thread From: Sandipan Das @ 2023-10-25 11:47 UTC (permalink / raw) To: Peter Zijlstra, Mario Limonciello Cc: Ingo Molnar, Borislav Petkov, Thomas Gleixner, Dave Hansen, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter On 10/24/2023 10:04 PM, Peter Zijlstra wrote: > On Tue, Oct 24, 2023 at 06:30:38PM +0200, Peter Zijlstra wrote: >> On Tue, Oct 24, 2023 at 11:04:06AM -0500, Mario Limonciello wrote: >> >>>> IIRC this is the hotplug thread running a teardown function on that CPU >>>> itself. It being a strict per-cpu thread should not trip >>>> smp_processor_id() wanrs. >>>> >>> >>> BUG: using smp_processor_id() in preemptible [00000000] code: rtcwake/2960 >>> caller is amd_pmu_lbr_reset+0x19/0xc0 >>> CPU: 104 PID: 2960 Comm: rtcwake Not tainted 6.6.0-rc6-00002-g3e2c7f3ac51f >> >> Very much not the cpuhp/%u thread :/, let me try and figure out how that >> happens. > > Uhh, my bad, these are the PREPARE/DEAD handlers, they run before online > and after dying. The CPU is completely dead. Running lbr_reset() here > makes no sense. > > Did that want to be in amd_pmu_cpu_dying() ? > Agreed, it should have gone into the cpu_dying() callback. lbr_reset() is called once from cpu_starting() so I wonder if its necessary to call it again in the CPU offline path. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() 2023-10-24 8:02 ` Ingo Molnar 2023-10-24 15:32 ` Mario Limonciello @ 2023-10-24 16:51 ` Ingo Molnar 2023-10-24 18:30 ` Mario Limonciello 1 sibling, 1 reply; 18+ messages in thread From: Ingo Molnar @ 2023-10-24 16:51 UTC (permalink / raw) To: Mario Limonciello Cc: Peter Zijlstra, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter * Ingo Molnar <mingo@kernel.org> wrote: > > * Mario Limonciello <mario.limonciello@amd.com> wrote: > > > Fixes a BUG reported during suspend to ram testing. > > > > ``` > > [ 478.274752] BUG: using smp_processor_id() in preemptible [00000000] code: rtcwake/2948 > > [ 478.274754] caller is amd_pmu_lbr_reset+0x19/0xc0 > > ``` > > > > Cc: stable@vger.kernel.org # 6.1+ > > Fixes: ca5b7c0d9621 ("perf/x86/amd/lbr: Add LbrExtV2 branch record support") > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > --- > > arch/x86/events/amd/lbr.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/events/amd/lbr.c b/arch/x86/events/amd/lbr.c > > index eb31f850841a..5b98e8c7d8b7 100644 > > --- a/arch/x86/events/amd/lbr.c > > +++ b/arch/x86/events/amd/lbr.c > > @@ -321,7 +321,7 @@ int amd_pmu_lbr_hw_config(struct perf_event *event) > > > > void amd_pmu_lbr_reset(void) > > { > > - struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); > > + struct cpu_hw_events *cpuc = get_cpu_ptr(&cpu_hw_events); > > int i; > > > > if (!x86_pmu.lbr_nr) > > @@ -335,6 +335,7 @@ void amd_pmu_lbr_reset(void) > > > > cpuc->last_task_ctx = NULL; > > cpuc->last_log_id = 0; > > + put_cpu_ptr(&cpu_hw_events); > > wrmsrl(MSR_AMD64_LBR_SELECT, 0); > > } > > Weird, amd_pmu_lbr_reset() is called from these places: > > - amd_pmu_lbr_sched_task(): during task sched-in during > context-switching, this should already have preemption disabled. > > - amd_pmu_lbr_add(): this gets indirectly called by amd_pmu::add > (amd_pmu_add_event()), called by event_sched_in(), which too should have > preemption disabled. > > I clearly must have missed some additional place it gets called in. Just for completeness, the additional place I missed is amd_pmu_cpu_reset(): static_call(amd_pmu_branch_reset)(); ... and the amd_pmu_branch_reset static call is set up with amd_pmu_lbr_reset, which is why git grep missed it. Anyway, amd_pmu_cpu_reset() is very much something that should run non-preemptable to begin with, so your patch only papers over the real problem AFAICS. Thanks, Ingo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() 2023-10-24 16:51 ` Ingo Molnar @ 2023-10-24 18:30 ` Mario Limonciello 2023-10-24 22:24 ` Peter Zijlstra 0 siblings, 1 reply; 18+ messages in thread From: Mario Limonciello @ 2023-10-24 18:30 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter On 10/24/2023 11:51, Ingo Molnar wrote: > > * Ingo Molnar <mingo@kernel.org> wrote: > >> >> * Mario Limonciello <mario.limonciello@amd.com> wrote: >> >>> Fixes a BUG reported during suspend to ram testing. >>> >>> ``` >>> [ 478.274752] BUG: using smp_processor_id() in preemptible [00000000] code: rtcwake/2948 >>> [ 478.274754] caller is amd_pmu_lbr_reset+0x19/0xc0 >>> ``` >>> >>> Cc: stable@vger.kernel.org # 6.1+ >>> Fixes: ca5b7c0d9621 ("perf/x86/amd/lbr: Add LbrExtV2 branch record support") >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>> --- >>> arch/x86/events/amd/lbr.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/events/amd/lbr.c b/arch/x86/events/amd/lbr.c >>> index eb31f850841a..5b98e8c7d8b7 100644 >>> --- a/arch/x86/events/amd/lbr.c >>> +++ b/arch/x86/events/amd/lbr.c >>> @@ -321,7 +321,7 @@ int amd_pmu_lbr_hw_config(struct perf_event *event) >>> >>> void amd_pmu_lbr_reset(void) >>> { >>> - struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); >>> + struct cpu_hw_events *cpuc = get_cpu_ptr(&cpu_hw_events); >>> int i; >>> >>> if (!x86_pmu.lbr_nr) >>> @@ -335,6 +335,7 @@ void amd_pmu_lbr_reset(void) >>> >>> cpuc->last_task_ctx = NULL; >>> cpuc->last_log_id = 0; >>> + put_cpu_ptr(&cpu_hw_events); >>> wrmsrl(MSR_AMD64_LBR_SELECT, 0); >>> } >> >> Weird, amd_pmu_lbr_reset() is called from these places: >> >> - amd_pmu_lbr_sched_task(): during task sched-in during >> context-switching, this should already have preemption disabled. >> >> - amd_pmu_lbr_add(): this gets indirectly called by amd_pmu::add >> (amd_pmu_add_event()), called by event_sched_in(), which too should have >> preemption disabled. >> >> I clearly must have missed some additional place it gets called in. > > Just for completeness, the additional place I missed is > amd_pmu_cpu_reset(): > > static_call(amd_pmu_branch_reset)(); > > ... and the amd_pmu_branch_reset static call is set up with > amd_pmu_lbr_reset, which is why git grep missed it. > > Anyway, amd_pmu_cpu_reset() is very much something that should run > non-preemptable to begin with, so your patch only papers over the real > problem AFAICS. > > Thanks, > > Ingo In that case - should preemption be disabled for all of x86_pmu_dying_cpu() perhaps? For good measure x86_pmu_starting_cpu() too? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() 2023-10-24 18:30 ` Mario Limonciello @ 2023-10-24 22:24 ` Peter Zijlstra 0 siblings, 0 replies; 18+ messages in thread From: Peter Zijlstra @ 2023-10-24 22:24 UTC (permalink / raw) To: Mario Limonciello Cc: Ingo Molnar, Borislav Petkov, Thomas Gleixner, Dave Hansen, Sandipan Das, H . Peter Anvin, linux-kernel, x86, linux-pm, rafael, pavel, linux-perf-users, Ingo Molnar, Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter On Tue, Oct 24, 2023 at 01:30:59PM -0500, Mario Limonciello wrote: > In that case - should preemption be disabled for all of x86_pmu_dying_cpu() > perhaps? > > For good measure x86_pmu_starting_cpu() too? starting and dying are with IRQs disabled. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-10-25 19:04 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-10-23 16:00 [PATCH 0/2] Fixes for s3 with parallel bootup Mario Limonciello 2023-10-23 16:00 ` [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously Mario Limonciello 2023-10-24 8:36 ` Ingo Molnar 2023-10-24 15:36 ` Mario Limonciello 2023-10-24 17:01 ` Ingo Molnar 2023-10-25 19:04 ` Mario Limonciello 2023-10-24 17:30 ` Tom Lendacky 2023-10-23 16:00 ` [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset() Mario Limonciello 2023-10-24 8:02 ` Ingo Molnar 2023-10-24 15:32 ` Mario Limonciello 2023-10-24 15:59 ` Peter Zijlstra 2023-10-24 16:04 ` Mario Limonciello 2023-10-24 16:30 ` Peter Zijlstra 2023-10-24 16:34 ` Peter Zijlstra 2023-10-25 11:47 ` Sandipan Das 2023-10-24 16:51 ` Ingo Molnar 2023-10-24 18:30 ` Mario Limonciello 2023-10-24 22:24 ` Peter Zijlstra
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.