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

[Ingo Molnar]

* Mario Limonciello <mario.limonciello@xxxxxxx> 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@xxxxxxxxxxxxxxx # 6.5+
> Fixes: 0c7ffa32dbd6 ("x86/smpboot/64: Implement arch_cpuhp_init_parallel_bringup() and enable it")
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
>  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