Re: [PATCH v3 2/5] x86/cpu: Initialize boot CPU cpuinfo defaults early

[bot+bpf-ci@kernel.org]

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

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index c3dce89ba5904..66695addbdf33 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1812,16 +1812,15 @@ static void init_cpu_info(struct cpuinfo_x86 *c)
>   * Do minimum CPU detection early.
>   * Fields really needed: vendor, cpuid_level, family, model, mask,
>   * cache alignment.
> - * The others are not touched to avoid unwanted side effects.
> + * The others are reset to their defaults here and only filled in later,
> + * by identify_cpu().
                         ^^

Does this comment match what the code does? The comment says fields
outside the "really needed" list are only reset here and filled in later
by identify_cpu(), but early_identify_cpu() itself fills in most of those
fields before it returns.

After init_cpu_info() resets the fields, early_identify_cpu() continues
to call:

  cpuid_scan_cpu(c);         /* re-populates c->cpuid */
  cpu_detect(c);             /* x86_clflush_size, x86_cache_alignment */
  get_cpu_cap(c);            /* x86_capability[], extended_cpuid_level */
  get_cpu_address_sizes(c);  /* x86_phys_bits, x86_virt_bits,
                                x86_cache_alignment */

Of the twelve fields init_cpu_info() resets, only three (x86_cache_size,
x86_model_id and, under CONFIG_X86_VMX_FEATURE_NAMES, vmx_capability) are
left for identify_cpu(); the capability array, the CPUID table,
extended_cpuid_level and the address sizes are all re-established in this
function.

The sentence also contradicts the line immediately above it, which already
lists "cache alignment" among the fields early detection does produce.

This matters more than a normal comment nit because the very next paragraph
is a WARNING about what may and may not be added to this function: a reader
who believes the non-essential fields are untouched until identify_cpu()
would draw the wrong conclusion about what state is valid at this point in
boot.

Could the comment say that the architectural defaults for the remaining
fields are established here, and that only the cache size, model string
(and VMX capabilities) are filled in later by identify_cpu()?

>   *
>   * WARNING: this function is only called on the boot CPU.  Don't add code
>   * here that is supposed to run on all CPUs.
>   */
>  static void __init early_identify_cpu(struct cpuinfo_x86 *c)
>  {
> -	memset(&c->x86_capability, 0, sizeof(c->x86_capability));
> -	memset(&c->cpuid, 0, sizeof(c->cpuid));
> -	c->extended_cpuid_level = 0;
> +	init_cpu_info(c);
>
>  	if (!cpuid_feature())
>  		identify_cpu_without_cpuid(c);


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35285131592