Re: [External] Re: [PATCH v6 07/11] x86/smpboot: Disable parallel boot for AMD CPUs

From: Usama Arif <usama.arif@bytedance.com>
To: David Woodhouse <dwmw2@infradead.org>,
	Arjan van de Ven <arjan@linux.intel.com>,
	Kim Phillips <kim.phillips@amd.com>,
	tglx@linutronix.de, rja@hpe.com
Cc: mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
	hpa@zytor.com, x86@kernel.org, pbonzini@redhat.com,
	paulmck@kernel.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org, rcu@vger.kernel.org, mimoja@mimoja.de,
	hewenliang4@huawei.com, thomas.lendacky@amd.com,
	seanjc@google.com, pmenzel@molgen.mpg.de,
	fam.zheng@bytedance.com, punit.agrawal@bytedance.com,
	simon.evans@bytedance.com, liangma@liangbit.com,
	Mario Limonciello <Mario.Limonciello@amd.com>
Subject: Re: [External] Re: [PATCH v6 07/11] x86/smpboot: Disable parallel boot for AMD CPUs
Date: Sun, 5 Feb 2023 22:13:42 +0000	[thread overview]
Message-ID: <434b4b74-54ab-68a3-4a81-9cc02ea75e39@bytedance.com> (raw)
In-Reply-To: <E2286684-F8AD-4708-9A3D-74C5EAE183B4@infradead.org>



On 04/02/2023 22:31, David Woodhouse wrote:
> 
> 
> On 4 February 2023 18:18:55 GMT, Arjan van de Ven <arjan@linux.intel.com> wrote:
>>>
>>> However...
>>>
>>> Even though we *can* support non-X2APIC processors, we *might* want to
>>> play it safe and not go back that far; only enabling parallel bringup
>>> on machines with X2APIC which roughly correlates with "lots of CPUs"
>>> since that's where the benefit is.
>>
>> I think that this is the right approach, at least on the initial patch series.
>> KISS principle; do all the easy-but-important cases first, get it stable and working
>> and in later series/kernels the range can be expanded.... if it matters.
> 
> Agreed. I'll split it to do it only with X2APIC for the initial series, and then hold the CPUID 0x1 part back for the next phase.

This was an interesting find! I tested the latest branch 
parallel-6.2-rc6 and it works well. The numbers from Russ makes the 
patch series look so much better! :)

If we do it with x2apic only and not support non-x2apic CPUID 0x1 case, 
maybe we apply the following diff to part 1?

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index f53a060a899b..f6b89cf40076 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1564,7 +1564,7 @@ void __init native_smp_prepare_cpus(unsigned int 
max_cpus)
          * 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 ||
+       if (IS_ENABLED(CONFIG_X86_32) || !x2apic_mode ||
             (x2apic_mode && boot_cpu_data.cpuid_level < 0xb) ||
             cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
                 do_parallel_bringup = false;



For reusing timer calibration, calibrate_delay ends up being used in 
start_kernel, smp_callin, debug_calc_bogomips and check_cx686_slop. I 
think reusing timer calibration would be ok in the first 2 uses? but not 
really sure about the other 2. cx686 seems to be quite old so not sure 
if anyone will have it to test or will ever run 6.2 kernel on it :).  I 
guess if unsure, better to leave out initially and try and get part1 merged?

Thanks,
Usama
