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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3733 bytes --]

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@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?

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?

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? 

> 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?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]