On 06/02/2023 08:05, David Woodhouse wrote:
On Sun, 2023-02-05 at 22:13 +0000, Usama Arif wrote:
On 04/02/2023 22:31, David Woodhouse wrote:
On 4 February 2023 18:18:55 GMT, Arjan van de Ven <arjan@xxxxxxxxxxxxxxx> 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?
Using x2apic_mode would also disable parallel boot when the CPU *does*
have X2APIC support but the kernel just isn't using it at the moment. I
think boot_cpu_has(X86_FEATURE_X2APIC) is the better criterion?
x2apic_mode is set to 0 only in the case when nox2apic is specified in
the kernel cmdline or if x2apic_setup fails. As 0xB leaf gets the
"x2apic id" and not the "apic id", I thought it would be better to not
use the x2apic id if the user doesnt want to use x2apic (cmdline), or
the kernel fails to set it up.
Another thing I noticed from the Intel Architecture Manual CPUID—CPU
Identification section:
"CPUID leaf 1FH is a preferred superset to leaf 0BH. Intel recommends
first checking for the existence of Leaf 1FH before using leaf 0BH."
So I think we should switch from 0BH to using the 1FH leaf EDX register.
I was thinking I'd tweak the 'no_parallel_bringup' command line
argument into something that also allows us to *enable* it without
X2APIC being supported.
But I've also been pondering the fact that this is all only for 64-bit
anyway. It's not like we're doing it for the zoo of ancient i586 and
even i486 machines where the APICs were hooked up with blue wire and
duct tape.
Maybe "64-bit only" is good enough, with a command line opt-out. And
maybe a printk pointing out the existence of that command line option
before the bringup, just in case?
I think that makes sense, the patch only has a significant impact when
the core count is high, and x2apic was made to support higher CPU count.
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?
It already explicitly allows the calibration to be reused from another
CPU in the same *core*, but no further. I think we'd need to be sure
about the correctness of extending that further, and which of the mess
of constant/invariant/we-really-mean-it-this-time TSC flags need to be
set for that to be OK.
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?
I doubt cx686 advertises constant TSC; the early ones didn't even *have* a TSC.
Does it even support SMP?
Just checked the wiki page, and it says core count is 1 :)
