Re: [PATCH v9 2/3] x86/cpu: Check if feature string is non-zero

[Maciej Wieczor-Retman]

On 2026-03-10 at 15:35:51 -0700, Sohil Mehta wrote:
>On 3/10/2026 11:03 AM, Maciej Wieczor-Retman wrote:
>> From: Maciej Wieczor-Retman <maciej.wieczor-retman@xxxxxxxxx>
>> 
>> In filter_cpuid_features, x86_cap_flags[] is read, but it's not verified
>
>filter_cpuid_features()

Sure, I'll change it

>
>> whether the string is non-zero which could lead to unwanted output.
>> 
>> In two more places there are open coded paths that try to retrieve a
>> feature string, and if there isn't one, the feature word and bit are
>> returned instead. 
>
>How about wording the next sentence as:
>
>Add a common helper to fix filter_cpuid_features() as well as clean up
>the open coded cases.

Yes, that sounds okay.

>> While correcting filter_cpuid_features() with a helper
>> it's trivial to also clean up these open coded cases.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@xxxxxxxxx>
>> ---
>
>> diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
>> index ad235dda1ded..93e8ad2786bf 100644
>> --- a/arch/x86/include/asm/cpu.h
>> +++ b/arch/x86/include/asm/cpu.h
>> @@ -9,6 +9,8 @@
>>  #include <linux/percpu.h>
>>  #include <asm/ibt.h>
>>  
>> +#define X86_CAP_BUF_SIZE			16
>> +
>>  #ifndef CONFIG_SMP
>>  #define cpu_physical_id(cpu)			boot_cpu_physical_apicid
>>  #define cpu_acpi_id(cpu)			0
>> @@ -67,4 +69,6 @@ int intel_microcode_sanity_check(void *mc, bool print_err, int hdr_type);
>>  
>>  extern struct cpumask cpus_stop_mask;
>>  
>> +const char *x86_cap_name(unsigned int bit, char *buf);
>> +
>
>These declarations - X86_CAP_BUF_SIZE and x86_cap_name() are better
>suited to asm/cpufeature.h instead of asm/cpu.h.
>
>
>Also, it would make more sense to have the #define closer to the
>function declaration. Maybe, right above it?

Sure, I was wondering what the best place would be. asm/cpufeature.h does make
more sense.

>
>>  #endif /* _ASM_X86_CPU_H */
>
>...
>
>>  
>> +/*
>> + * Return the feature "name" if available, otherwise return the
>> + * X86_FEATURE_* numerals to make it easier to identify the feature.
>> + */
>
>Should we add a sentence here to say that all callers must pass a buffer
>of size X86_CAP_BUF_SIZE.

Yes, perhaps it's a good idea.

>
>> +const char *x86_cap_name(unsigned int bit, char *buf)
>> +{
>> +	unsigned int word = bit >> 5;
>> +	const char *name = NULL;
>> +
>> +	if (likely(word < NCAPINTS))
>> +		name = x86_cap_flags[bit];
>> +	else if (likely(word < NCAPINTS + NBUGINTS))
>> +		name = x86_bug_flags[bit - 32 * NCAPINTS];
>> +
>
>Can we get rid of the two likely() annotations here? Is x86_cap_name()
>called from any performance critical path?

As far as I can tell not really but it does get called during boot time a bunch
of times. But is it an issue to sprinkle these likely() calls in non-critical
performance paths?

>
>> +	if (name)
>> +		return name;
>> +
>> +	snprintf(buf, X86_CAP_BUF_SIZE, "%u:%u", word, bit & 31);
>> +	return buf;
>> +}
>> +
>>  /*
>>   * This does the hard work of actually picking apart the CPU stuff...
>>   */

-- 
Kind regards
Maciej Wieczór-Retman