Messages in this thread | | | Date | Wed, 8 Mar 2023 11:27:05 +0000 | Subject | Re: [External] Re: [PATCH v13 00/11] Parallel CPU bringup for x86_64 | From | Usama Arif <> |
| |
On 08/03/2023 09:04, David Woodhouse wrote: > On Tue, 2023-03-07 at 16:55 -0600, Tom Lendacky wrote: >> On 3/7/23 16:27, David Woodhouse wrote: >>> On Tue, 2023-03-07 at 16:22 -0600, Tom Lendacky wrote: >>>> >>>> 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). >>> >>> Interesting, thanks. Should I change to checking for *both* EAX and EBX >>> being zero? That's what I did first, after reading only the Intel SDM. >>> But I changed to only EAX because the AMD doc only says that EAX will >>> be zero for unsupported leaves. >> >> From a baremetal perspective, I think that works. Rome was the first >> generation to support x2apic, and the PPR for Rome states that 0's are >> returned in all 4 registers for undefined function numbers. >> >> For virtualization, at least Qemu/KVM, that also looks to be a safe test. > > At Sean's suggestion, I've switched it to use the existing > check_extended_topology_leaf() which checks for EBX being non-zero, and > CH being 1 (SMT_TYPE). > > I also made it work even if the kernel isn't using x2apic mode (is that > even possible, or does SEV-ES require the MSR-based access anyway?) > > It just looked odd handling SEV-ES in the CPUID 0x0B path but not the > CPUID 0x01 case, and I certainly didn't want to implement the asm side > for handling CPUID 0x01 via the GHCB protocol. And this way I can pull > the check for CC_ATTR_GUEST_STATE_ENCRYPT up above. Which I've kept for > now for the reason described in the comment, but I won't die on that > hill. > > https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/parallel-6.2-v14 > > Looks like this: > > /* > * 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. > */ > static bool prepare_parallel_bringup(void) > { > bool has_sev_es = IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT) && > static_branch_unlikely(&sev_es_enable_key); > > if (IS_ENABLED(CONFIG_X86_32)) > return false; > > /* > * Encrypted guests other than SEV-ES (in the future) will need to > * implement an early way of finding the APIC ID, since they will > * presumably block direct CPUID too. Be kind to our future selves > * by warning here instead of just letting them break. Parallel > * startup doesn't have to be in the first round of enabling patches > * for any such technology. > */ > if (cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT) || !has_sev_es) { > pr_info("Disabling parallel bringup due to guest memory encryption\n"); > return false;
I believe this is still going to enable parallel bringup for TDX? Looking at include/linux/cc_platform.h, it looks like CC_ATTR_GUEST_STATE_ENCRYPT is only set for SEV-ES and TDX guest with x2apic will go on in this function and enable parallel bringup if leaf 0xB is ok. I guess if the apic ID is OK for the TDX guest, then its fine, but just wanted to check if anyone has tested this on TDX guest?
> } > > if (x2apic_mode || has_sev_es) { > if (boot_cpu_data.cpuid_level < 0x0b) > return false; > > if (check_extended_topology_leaf(0x0b) != 0) { > pr_info("Disabling parallel bringup because CPUID 0xb looks untrustworthy\n"); > return false; > } > > if (has_sev_es) { > pr_debug("Using SEV-ES CPUID 0xb for parallel CPU startup\n"); > smpboot_control = STARTUP_APICID_SEV_ES; > } else { > pr_debug("Using CPUID 0xb for parallel CPU startup\n"); > smpboot_control = STARTUP_APICID_CPUID_0B; > } > } else { > /* Without X2APIC, what's in CPUID 0x01 should suffice. */ > if (boot_cpu_data.cpuid_level < 0x01) > return false; > > 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; > } >
| |