| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 7 Mar 2023 16:22:27 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: David Woodhouse <dwmw2@...radead.org>,
Usama Arif <usama.arif@...edance.com>, tglx@...utronix.de,
kim.phillips@....com, brgerst@...il.com,
"Rapan, Sabin" <sabrapan@...zon.com>
Cc: piotrgorski@...hyos.org, oleksandr@...alenko.name,
arjan@...ux.intel.com, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, hpa@...or.com, x86@...nel.org,
pbonzini@...hat.com, paulmck@...nel.org,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
rcu@...r.kernel.org, mimoja@...oja.de, hewenliang4@...wei.com,
seanjc@...gle.com, pmenzel@...gen.mpg.de, fam.zheng@...edance.com,
punit.agrawal@...edance.com, simon.evans@...edance.com,
liangma@...ngbit.com
Subject: Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64
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@...zon.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@...zon.com>
>> Signed-off-by: David Woodhouse <dwmw@...zon.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();
>> }
Powered by blists - more mailing lists