Re: [patch] x86/smpboot: Fix the parallel bringup decision

[Tom Lendacky <thomas.lendacky@amd.com>]

On 5/31/23 02:44, Thomas Gleixner wrote:
> The decision to allow parallel bringup of secondary CPUs checks
> CC_ATTR_GUEST_STATE_ENCRYPT to detect encrypted guests. Those cannot use
> parallel bootup because accessing the local APIC is intercepted and raises
> a #VC or #VE, which cannot be handled at that point.
> 
> The check works correctly, but only for AMD encrypted guests. TDX does not
> set that flag.
> 
> As there is no real connection between CC attributes and the inability to
> support parallel bringup, replace this with a generic control flag in
> x86_cpuinit and let SEV-ES and TDX init code disable it.
> 
> Fixes: 0c7ffa32dbd6 ("x86/smpboot/64: Implement arch_cpuhp_init_parallel_bringup() and enable it")
> Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Still works for SEV-ES/SEV-SNP with parallel boot properly disabled.

Tested-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>   arch/x86/coco/tdx/tdx.c         |   11 +++++++++++
>   arch/x86/include/asm/x86_init.h |    3 +++
>   arch/x86/kernel/smpboot.c       |   19 ++-----------------
>   arch/x86/kernel/x86_init.c      |    1 +
>   arch/x86/mm/mem_encrypt_amd.c   |   15 +++++++++++++++
>   5 files changed, 32 insertions(+), 17 deletions(-)
> 
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -871,5 +871,16 @@ void __init tdx_early_init(void)
>   	x86_platform.guest.enc_tlb_flush_required   = tdx_tlb_flush_required;
>   	x86_platform.guest.enc_status_change_finish = tdx_enc_status_changed;
>   
> +	/*
> +	 * TDX intercepts the RDMSR to read the X2APIC ID in the parallel
> +	 * bringup low level code. That raises #VE which cannot be handled
> +	 * there.
> +	 *
> +	 * Intel-TDX has a secure RDMSR hypercall, but that needs to be
> +	 * implemented seperately in the low level startup ASM code.
> +	 * Until that is in place, disable parallel bringup for TDX.
> +	 */
> +	x86_cpuinit.parallel_bringup = false;
> +
>   	pr_info("Guest detected\n");
>   }
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -177,11 +177,14 @@ struct x86_init_ops {
>    * struct x86_cpuinit_ops - platform specific cpu hotplug setups
>    * @setup_percpu_clockev:	set up the per cpu clock event device
>    * @early_percpu_clock_init:	early init of the per cpu clock event device
> + * @fixup_cpu_id:		fixup function for cpuinfo_x86::phys_proc_id
> + * @parallel_bringup:		Parallel bringup control
>    */
>   struct x86_cpuinit_ops {
>   	void (*setup_percpu_clockev)(void);
>   	void (*early_percpu_clock_init)(void);
>   	void (*fixup_cpu_id)(struct cpuinfo_x86 *c, int node);
> +	bool parallel_bringup;
>   };
>   
>   struct timespec64;
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1267,23 +1267,8 @@ void __init smp_prepare_cpus_common(void
>   /* Establish whether parallel bringup can be supported. */
>   bool __init arch_cpuhp_init_parallel_bringup(void)
>   {
> -	/*
> -	 * Encrypted guests require special handling. They enforce X2APIC
> -	 * mode but the RDMSR to read the APIC ID is intercepted and raises
> -	 * #VC or #VE which cannot be handled in the early startup code.
> -	 *
> -	 * AMD-SEV does not provide a RDMSR GHCB protocol so the early
> -	 * startup code cannot directly communicate with the secure
> -	 * firmware. The alternative solution to retrieve the APIC ID via
> -	 * CPUID(0xb), which is covered by the GHCB protocol, is not viable
> -	 * either because there is no enforcement of the CPUID(0xb)
> -	 * provided "initial" APIC ID to be the same as the real APIC ID.
> -	 *
> -	 * Intel-TDX has a secure RDMSR hypercall, but that needs to be
> -	 * implemented seperately in the low level startup ASM code.
> -	 */
> -	if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) {
> -		pr_info("Parallel CPU startup disabled due to guest state encryption\n");
> +	if (!x86_cpuinit.parallel_bringup) {
> +		pr_info("Parallel CPU startup disabled by the platform\n");
>   		return false;
>   	}
>   
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -126,6 +126,7 @@ struct x86_init_ops x86_init __initdata
>   struct x86_cpuinit_ops x86_cpuinit = {
>   	.early_percpu_clock_init	= x86_init_noop,
>   	.setup_percpu_clockev		= setup_secondary_APIC_clock,
> +	.parallel_bringup		= true,
>   };
>   
>   static void default_nmi_init(void) { };
> --- a/arch/x86/mm/mem_encrypt_amd.c
> +++ b/arch/x86/mm/mem_encrypt_amd.c
> @@ -501,6 +501,21 @@ void __init sme_early_init(void)
>   	x86_platform.guest.enc_status_change_finish  = amd_enc_status_change_finish;
>   	x86_platform.guest.enc_tlb_flush_required    = amd_enc_tlb_flush_required;
>   	x86_platform.guest.enc_cache_flush_required  = amd_enc_cache_flush_required;
> +
> +	/*
> +	 * AMD-SEV-ES intercepts the RDMSR to read the X2APIC ID in the
> +	 * parallel bringup low level code. That raises #VC which cannot be
> +	 * handled there.
> +	 * It does not provide a RDMSR GHCB protocol so the early startup
> +	 * code cannot directly communicate with the secure firmware. The
> +	 * alternative solution to retrieve the APIC ID via CPUID(0xb),
> +	 * which is covered by the GHCB protocol, is not viable either
> +	 * because there is no enforcement of the CPUID(0xb) provided
> +	 * "initial" APIC ID to be the same as the real APIC ID.
> +	 * Disable parallel bootup.
> +	 */
> +	if (sev_status & MSR_AMD64_SEV_ES_ENABLED)
> +		x86_cpuinit.parallel_bringup = false;
>   }
>   
>   void __init mem_encrypt_free_decrypted_mem(void)