| Message ID | 87ilc9gd2d.ffs@tglx (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ff3cfcb0d46adc541283a507560f88b7d7114dbe |
| Headers | show |
| Series | x86/smpboot: Fix the parallel bringup decision | expand |
On Wed, May 31, 2023 at 09:44:26AM +0200, 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> Tested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.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)
--- 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)
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> --- 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(-)