[PATCH 0/2] Fixes for s3 with parallel bootup

[PATCH 0/2] Fixes for s3 with parallel bootup

[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

[PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously

[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

[PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset()

[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

Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset()

[Ingo Molnar]

* 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

Re: [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously

[Ingo Molnar]

* 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

Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset()

[Mario Limonciello]

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

Re: [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously

[Mario Limonciello]

+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.

Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset()

[Peter Zijlstra]

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
> 

Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset()

[Mario Limonciello]

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
>>

Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset()

[Peter Zijlstra]

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>

Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset()

[Peter Zijlstra]

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

Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset()

[Ingo Molnar]

* 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

Re: [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously

[Ingo Molnar]

* 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

Re: [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously

[Tom Lendacky]

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.
> 
> 

Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset()

[Mario Limonciello]

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?

Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset()

[Peter Zijlstra]

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.

Re: [PATCH 2/2] perf/x86/amd: Don't allow pre-emption in amd_pmu_lbr_reset()

[Sandipan Das]

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.

Re: [PATCH 1/2] x86: Enable x2apic during resume from suspend if used previously

[Mario Limonciello]

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