Re: [PATCH v7 2/3] x86/cacheinfo: Delete global num_cache_leaves

[Borislav Petkov]

On Fri, Sep 13, 2024 at 01:31:54AM -0700, Ricardo Neri wrote:
> diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> index 392d09c936d6..182cacd772b8 100644
> --- a/arch/x86/kernel/cpu/cacheinfo.c
> +++ b/arch/x86/kernel/cpu/cacheinfo.c
> @@ -178,7 +178,16 @@ struct _cpuid4_info_regs {
>  	struct amd_northbridge *nb;
>  };
>  
> -static unsigned short num_cache_leaves;
> +static inline unsigned int get_num_cache_leaves(unsigned int cpu)
> +{
> +	return get_cpu_cacheinfo(cpu)->num_leaves;
> +}

There already is

#define cache_leaves(cpu)       (ci_cacheinfo(cpu)->num_leaves)

And there's also get_cpu_cacheinfo().

And now you're adding more silly wrappers. Yuck.

Can we pls use *one* of those things and work with it everywhere?

> @@ -742,19 +753,19 @@ void init_intel_cacheinfo(struct cpuinfo_x86 *c)
>  	unsigned int l2_id = 0, l3_id = 0, num_threads_sharing, index_msb;
>  
>  	if (c->cpuid_level > 3) {
> -		static int is_initialized;
> -
> -		if (is_initialized == 0) {
> -			/* Init num_cache_leaves from boot CPU */
> -			num_cache_leaves = find_num_cache_leaves(c);
> -			is_initialized++;
> -		}
> +		/*
> +		 * There should be at least one leaf. A non-zero value means
> +		 * that the number of leaves has been initialized.
> +		 */
> +		if (!get_num_cache_leaves(c->cpu_index))
> +			set_num_cache_leaves(c->cpu_index,
> +					     find_num_cache_leaves(c));

Ugly linebreak.

>  
>  		/*
>  		 * Whenever possible use cpuid(4), deterministic cache
>  		 * parameters cpuid leaf to find the cache details
>  		 */
> -		for (i = 0; i < num_cache_leaves; i++) {
> +		for (i = 0; i < get_num_cache_leaves(c->cpu_index); i++) {
>  			struct _cpuid4_info_regs this_leaf = {};
>  			int retval;
>  
> @@ -790,14 +801,14 @@ void init_intel_cacheinfo(struct cpuinfo_x86 *c)
>  	 * Don't use cpuid2 if cpuid4 is supported. For P4, we use cpuid2 for
>  	 * trace cache
>  	 */
> -	if ((num_cache_leaves == 0 || c->x86 == 15) && c->cpuid_level > 1) {
> +	if ((!get_num_cache_leaves(c->cpu_index) || c->x86 == 15) && c->cpuid_level > 1) {
>  		/* supports eax=2  call */
>  		int j, n;
>  		unsigned int regs[4];
>  		unsigned char *dp = (unsigned char *)regs;
>  		int only_trace = 0;
>  
> -		if (num_cache_leaves != 0 && c->x86 == 15)
> +		if (get_num_cache_leaves(c->cpu_index) && c->x86 == 15)
>  			only_trace = 1;
>  
>  		/* Number of times to iterate */
> @@ -993,12 +1004,9 @@ int init_cache_level(unsigned int cpu)
>  {
>  	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>  
> -	if (!num_cache_leaves)
> -		return -ENOENT;

Why not

	if (!cache_leaves(cpu))
		return -ENOENT;

?

>  	if (!this_cpu_ci)
>  		return -EINVAL;
>  	this_cpu_ci->num_levels = 3;
> -	this_cpu_ci->num_leaves = num_cache_leaves;
>  	return 0;
>  }
>  
> -- 
> 2.34.1
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette