Re: [PATCH v1 5/5] x86/cpu: Don't reset boot CPU cpuinfo in identify_cpu()

[Ihor Solodrai]

On 7/30/26 2:26 PM, Borislav Petkov wrote:
> On Fri, Jul 03, 2026 at 05:20:46PM -0700, Ihor Solodrai wrote:
>> identify_cpu() used to rebuild c->x86_capability from scratch:
>> init_cpu_info() memsets the capability array to zero and
>> identify_cpu() then re-reads CPUID. On the boot CPU identify_cpu()
>> runs after local_irq_enable() and before alternatives are patched. An
>> interrupt delivered in the window between the memset() and
>> get_cpu_cap() therefore will observe cleared x86_capability [1].
>>
>> However the boot CPU's capabilities have already been scanned, with
>> interrupts off, by early_identify_cpu(). So move the init_cpu_info()
>> call out of identify_cpu() into its callers, and skip it for the boot
>> CPU on 64-bit.
>>
>> Secondary CPUs call init_cpu_info() then identify_cpu() from
>> identify_secondary_cpu(), with interrupts off, exactly as
>> before. 32-bit still resets in identify_boot_cpu(): it relies on the
>> reset for the no-CPUID cpuid_level default.
> 
> Ok, this commit message is all over the place. Please structure it something
> like this:
> 
> 1. Prepare the context for the explanation briefly.
> 
> 2. Explain the problem at hand.
> 
> 3. "It happens because of <...>"
> 
> 4. "Fix it by doing X"
> 
> 5. "(Potentially do Y)."
> 
> And some of those above are optional depending on the issue being
> explained.
> 
> Also, do not talk about what your patches do - that should (hopefully) be
> visible from the diff itself. Rather, talk about *why* you're doing what
> you're doing.
> 
> In the patches where you do only mechanical movement, add
> 
> "No functional changes."
> 
> to the commit message to denote that.

Thank you for the review and the feedback. I'll respin.

> 
> 
>> [1] https://lore.kernel.org/bpf/20260610175651.647515-1-ihor.solodrai@xxxxxxxxx/
>>
>> Signed-off-by: Ihor Solodrai <ihor.solodrai@xxxxxxxxx>
>> ---
>>  arch/x86/kernel/cpu/common.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> index 8f093c6c7ccc..cd3d7c37c174 100644
>> --- a/arch/x86/kernel/cpu/common.c
>> +++ b/arch/x86/kernel/cpu/common.c
>> @@ -1986,8 +1986,6 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>>  
>>  	c->loops_per_jiffy = loops_per_jiffy;
>>  
>> -	init_cpu_info(c);
>> -
>>  	if (!cpuid_feature())
>>  		identify_cpu_without_cpuid(c);
>>  
>> @@ -2156,6 +2154,13 @@ static void identify_cpu_32(struct cpuinfo_x86 *c)
>>  
>>  static __init void identify_boot_cpu(void)
>>  {
>> +	/*
>> +	 * The boot CPU's capabilities were already scanned in early_identify_cpu().
>> +	 * However 32-bit still needs to init_cpu_info() here for the no-CPUID
>> +	 * cpuid_level default.
>> +	 */
>> +	if (IS_ENABLED(CONFIG_X86_32))
>> +		init_cpu_info(&boot_cpu_data);
>>  	identify_cpu(&boot_cpu_data);
>>  	if (HAS_KERNEL_IBT && cpu_feature_enabled(X86_FEATURE_IBT))
>>  		pr_info("CET detected: Indirect Branch Tracking enabled\n");
>> @@ -2178,6 +2183,7 @@ void identify_secondary_cpu(unsigned int cpu)
>>  		*c = boot_cpu_data;
>>  	c->cpu_index = cpu;
>>  
>> +	init_cpu_info(c);
>>  	identify_cpu(c);
>>  	identify_cpu_32(c);
> 
> Also, when you look at the final version, do you see how identify_cpu() and
> identify_cpu_32() are soo close together so that you can basically move the
> body of identify_cpu_32() to the end of identify_cpu() and then you don't need
> that helper either.

I actually did notice that, but I guess I fixated on the idea that
32bit part should be encapsulated. I'll fold identify_cpu_32() into
identify_cpu() in the next revision.

Thanks!

> 
> Other than that, the flows do look better. 32-bit still needs dealing with the
> no-CPUID case but that's for another day. I can't really find it in me to
> care, actually, 32-bit needs to go away and we'll simplify a looot of code. Oh
> well.
> 
> Thx.
>