Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64

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

On 3/7/23 14:06, Tom Lendacky wrote:
> On 3/7/23 13:18, David Woodhouse wrote:
>> On Tue, 2023-03-07 at 10:45 -0600, Tom Lendacky wrote:
>>> On 3/7/23 08:42, David Woodhouse wrote:
>>>> On Thu, 2023-03-02 at 11:12 +0000, Usama Arif wrote:
>>>>> The main code change over v12 is to fix the build error when
>>>>> CONFIG_FORCE_NR_CPUS is present.
>>>>>
>>>>> The commit message for removing initial stack has also been improved, 
>>>>> typos
>>>>> have been fixed and extra comments have been added to make code clearer.
>>>>
>>>> Might something like this make it work in parallel with SEV-SNP? If so,
>>>> I can clean it up and adjust the C code to actually invoke it...
>>>
>>> This should be ok for both SEV-ES and SEV-SNP.
>>
>> Thanks. So... something like this then?
>>
>> Is static_branch_unlikely(&sev_es_enable_key) the right thing to use,
>> and does that cover SNP too?
> 
> Yes, that will cover SNP, too.
> 
>>
>> Pushed to
>> https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-6.2-rc8-v12bis
>>
>>  From d03aed91e5cfe840f4b18820fe6aed1765687321 Mon Sep 17 00:00:00 2001
>> From: David Woodhouse <dwmw@amazon.co.uk>
>> Date: Tue, 7 Mar 2023 19:06:50 +0000
>> Subject: [PATCH] x86/smpboot: Allow parallel bringup for SEV-ES
>>
>> Enable parallel bringup for SEV-ES guests. The APs can't actually
>> execute the CPUID instruction directly during early startup, but they
>> can make the GHCB call directly instead, just as the VC trap handler
>> would do.
>>
>> Factor out a prepare_parallel_bringup() function to help reduce the level
>> of complexity by allowing a simple 'return false' in the bail-out cases/
>>
>> Thanks to Sabin for talking me through the way this works.
>>
>> Suggested-by: Sabin Rapan <sabrapan@amazon.com>
>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

I did some Qemu/KVM testing. One thing I noticed is that on AMD, CPUID 0xB 
EAX will be non-zero only if SMT is enabled. So just booting some guests 
without CPU topology never did parallel booting ("smpboot: Disabling 
parallel bringup because CPUID 0xb looks untrustworthy"). I would imagine 
a bare-metal system that has diabled SMT will not do parallel booting, too 
(but I haven't had time to test that).

I did have to change "vmgexit" to a "rep; vmmcall" based on my binutils 
and the IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT) change I mentioned before. But 
with that, I was able to successfully boot 64 vCPU SEV-ES and SEV-SNP 
guests using parallel booting.

Thanks,
Tom

>> ---
>>   arch/x86/include/asm/sev-common.h |   3 +
>>   arch/x86/include/asm/smp.h        |   3 +-
>>   arch/x86/kernel/head_64.S         |  27 ++++++-
>>   arch/x86/kernel/smpboot.c         | 112 ++++++++++++++++++------------
>>   4 files changed, 98 insertions(+), 47 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/sev-common.h 
>> b/arch/x86/include/asm/sev-common.h
>> index b8357d6ecd47..f25df4bd318e 100644
>> --- a/arch/x86/include/asm/sev-common.h
>> +++ b/arch/x86/include/asm/sev-common.h
>> @@ -70,6 +70,7 @@
>>       /* GHCBData[63:12] */                \
>>       (((u64)(v) & GENMASK_ULL(63, 12)) >> 12)
>> +#ifndef __ASSEMBLY__
>>   /*
>>    * SNP Page State Change Operation
>>    *
>> @@ -160,6 +161,8 @@ struct snp_psc_desc {
>>   #define GHCB_RESP_CODE(v)        ((v) & GHCB_MSR_INFO_MASK)
>> +#endif /* __ASSEMBLY__ */
>> +
>>   /*
>>    * Error codes related to GHCB input that can be communicated back to 
>> the guest
>>    * by setting the lower 32-bits of the GHCB SW_EXITINFO1 field to 2.
>> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
>> index defe76ee9e64..b3f67a764bfa 100644
>> --- a/arch/x86/include/asm/smp.h
>> +++ b/arch/x86/include/asm/smp.h
>> @@ -204,7 +204,8 @@ extern unsigned int smpboot_control;
>>   /* Control bits for startup_64 */
>>   #define STARTUP_APICID_CPUID_0B    0x80000000
>>   #define STARTUP_APICID_CPUID_01    0x40000000
>> +#define STARTUP_APICID_SEV_ES    0x20000000
>> -#define STARTUP_PARALLEL_MASK (STARTUP_APICID_CPUID_01 | 
>> STARTUP_APICID_CPUID_0B)
>> +#define STARTUP_PARALLEL_MASK (STARTUP_APICID_CPUID_01 | 
>> STARTUP_APICID_CPUID_0B | STARTUP_APICID_SEV_ES)
>>   #endif /* _ASM_X86_SMP_H */
>> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
>> index c35f7c173832..3f5904eab678 100644
>> --- a/arch/x86/kernel/head_64.S
>> +++ b/arch/x86/kernel/head_64.S
>> @@ -26,7 +26,7 @@
>>   #include <asm/nospec-branch.h>
>>   #include <asm/fixmap.h>
>>   #include <asm/smp.h>
>> -
>> +#include <asm/sev-common.h>
>>   /*
>>    * We are not able to switch in one step to the final KERNEL ADDRESS 
>> SPACE
>>    * because we need identity-mapped pages.
>> @@ -242,6 +242,7 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, 
>> SYM_L_GLOBAL)
>>        *
>>        * Bit 31    STARTUP_APICID_CPUID_0B flag (use CPUID 0x0b)
>>        * Bit 30    STARTUP_APICID_CPUID_01 flag (use CPUID 0x01)
>> +     * Bit 29    STARTUP_APICID_SEV_ES flag (CPUID 0x0b via GHCB MSR)
>>        * Bit 0-24    CPU# if STARTUP_APICID_CPUID_xx flags are not set
>>        */
>>       movl    smpboot_control(%rip), %ecx
>> @@ -249,6 +250,8 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, 
>> SYM_L_GLOBAL)
>>       jnz    .Luse_cpuid_0b
>>       testl    $STARTUP_APICID_CPUID_01, %ecx
>>       jnz    .Luse_cpuid_01
>> +    testl    $STARTUP_APICID_SEV_ES, %ecx
>> +    jnz    .Luse_sev_cpuid_0b
>>       andl    $0x0FFFFFFF, %ecx
>>       jmp    .Lsetup_cpu
>> @@ -259,6 +262,28 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, 
>> SYM_L_GLOBAL)
>>       shr    $24, %edx
>>       jmp    .Lsetup_AP
>> +.Luse_sev_cpuid_0b:
>> +    /* Set the GHCB MSR to request CPUID 0xB_EDX */
>> +    movl    $MSR_AMD64_SEV_ES_GHCB, %ecx
>> +    movl    $(GHCB_CPUID_REQ_EDX << 30) | GHCB_MSR_CPUID_REQ, %eax
>> +    movl    $0x0B, %edx
>> +    wrmsr
>> +
>> +    /* Perform GHCB MSR protocol */
>> +    vmgexit
>> +
>> +    /*
>> +     * Get the result. After the RDMSR:
>> +     *   EAX should be 0xc0000005
>> +     *   EDX should have the CPUID register value and since EDX
>> +     *   is the target register, no need to move the result.
>> +     */
>> +    rdmsr
>> +    andl    $GHCB_MSR_INFO_MASK, %eax
>> +    cmpl    $GHCB_MSR_CPUID_RESP, %eax
>> +    jne    1f
>> +    jmp    .Lsetup_AP
>> +
>>   .Luse_cpuid_0b:
>>       mov    $0x0B, %eax
>>       xorl    %ecx, %ecx
>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>> index 9d956571ecc1..d194c4ffeef8 100644
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -1510,6 +1510,71 @@ void __init smp_prepare_cpus_common(void)
>>       set_cpu_sibling_map(0);
>>   }
>> +
>> +/*
>> + * We can do 64-bit AP bringup in parallel if the CPU reports its APIC
>> + * ID in CPUID (either leaf 0x0B if we need the full APIC ID in X2APIC
>> + * mode, or leaf 0x01 if 8 bits are sufficient). Otherwise it's too
>> + * hard. And not for SEV-ES guests because they can't use CPUID that
>> + * early.
>> + */
>> +static bool __init prepare_parallel_bringup(void)
>> +{
>> +    if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 1)
>> +        return false;
>> +
>> +    if (x2apic_mode) {
>> +        unsigned int eax, ebx, ecx, edx;
>> +
>> +        if (boot_cpu_data.cpuid_level < 0xb)
>> +            return false;
>> +
>> +        /*
>> +         * To support parallel bringup in x2apic mode, the AP will need
>> +         * to obtain its APIC ID from CPUID 0x0B, since CPUID 0x01 has
>> +         * only 8 bits. Check that it is present and seems correct.
>> +         */
>> +        cpuid_count(0xb, 0, &eax, &ebx, &ecx, &edx);
>> +
>> +        /*
>> +         * AMD says that if executed with an umimplemented level in
>> +         * ECX, then it will return all zeroes in EAX. Intel says it
>> +         * will return zeroes in both EAX and EBX. Checking only EAX
>> +         * should be sufficient.
>> +         */
>> +        if (!eax) {
>> +            pr_info("Disabling parallel bringup because CPUID 0xb looks 
>> untrustworthy\n");
>> +            return false;
>> +        }
>> +
>> +        if (IS_ENABLED(AMD_MEM_ENCRYPT) && 
>> static_branch_unlikely(&sev_es_enable_key)) {
> 
> This should be IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)
> 
> Let me take this patch and run some tests to confirm that everything works 
> as expected.
> 
> Thanks!
> Tom
> 
>> +            pr_debug("Using SEV-ES CPUID 0xb for parallel CPU startup\n");
>> +            smpboot_control = STARTUP_APICID_SEV_ES;
>> +        } else if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) {
>> +            /*
>> +             * Other forms of memory encryption need to implement a way of
>> +             * finding the APs' APIC IDs that early.
>> +             */
>> +            return false;
>> +        } else {
>> +            pr_debug("Using CPUID 0xb for parallel CPU startup\n");
>> +            smpboot_control = STARTUP_APICID_CPUID_0B;
>> +        }
>> +    } else {
>> +        if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
>> +            return false;
>> +
>> +        /* Without X2APIC, what's in CPUID 0x01 should suffice. */
>> +        pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
>> +        smpboot_control = STARTUP_APICID_CPUID_01;
>> +    }
>> +
>> +    cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
>> +                  native_cpu_kick, NULL);
>> +
>> +    return true;
>> +}
>> +
>>   /*
>>    * Prepare for SMP bootup.
>>    * @max_cpus: configured maximum number of CPUs, It is a legacy parameter
>> @@ -1550,51 +1615,8 @@ void __init native_smp_prepare_cpus(unsigned int 
>> max_cpus)
>>       speculative_store_bypass_ht_init();
>> -    /*
>> -     * We can do 64-bit AP bringup in parallel if the CPU reports
>> -     * its APIC ID in CPUID (either leaf 0x0B if we need the full
>> -     * APIC ID in X2APIC mode, or leaf 0x01 if 8 bits are
>> -     * sufficient). Otherwise it's too hard. And not for SEV-ES
>> -     * guests because they can't use CPUID that early.
>> -     */
>> -    if (IS_ENABLED(CONFIG_X86_32) || boot_cpu_data.cpuid_level < 1 ||
>> -        (x2apic_mode && boot_cpu_data.cpuid_level < 0xb) ||
>> -        cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
>> -        do_parallel_bringup = false;
>> -
>> -    if (do_parallel_bringup && x2apic_mode) {
>> -        unsigned int eax, ebx, ecx, edx;
>> -
>> -        /*
>> -         * To support parallel bringup in x2apic mode, the AP will need
>> -         * to obtain its APIC ID from CPUID 0x0B, since CPUID 0x01 has
>> -         * only 8 bits. Check that it is present and seems correct.
>> -         */
>> -        cpuid_count(0xb, 0, &eax, &ebx, &ecx, &edx);
>> -
>> -        /*
>> -         * AMD says that if executed with an umimplemented level in
>> -         * ECX, then it will return all zeroes in EAX. Intel says it
>> -         * will return zeroes in both EAX and EBX. Checking only EAX
>> -         * should be sufficient.
>> -         */
>> -        if (eax) {
>> -            pr_debug("Using CPUID 0xb for parallel CPU startup\n");
>> -            smpboot_control = STARTUP_APICID_CPUID_0B;
>> -        } else {
>> -            pr_info("Disabling parallel bringup because CPUID 0xb looks 
>> untrustworthy\n");
>> -            do_parallel_bringup = false;
>> -        }
>> -    } else if (do_parallel_bringup) {
>> -        /* Without X2APIC, what's in CPUID 0x01 should suffice. */
>> -        pr_debug("Using CPUID 0x1 for parallel CPU startup\n");
>> -        smpboot_control = STARTUP_APICID_CPUID_01;
>> -    }
>> -
>> -    if (do_parallel_bringup) {
>> -        cpuhp_setup_state_nocalls(CPUHP_BP_PARALLEL_DYN, "x86/cpu:kick",
>> -                      native_cpu_kick, NULL);
>> -    }
>> +    if (do_parallel_bringup)
>> +        do_parallel_bringup = prepare_parallel_bringup();
>>       snp_set_wakeup_secondary_cpu();
>>   }