Re: [PATCH v3 03/10] x86/mtrr: replace use_intel() with a local flag

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3248 bytes --]

On 11.09.22 12:16, Borislav Petkov wrote:
> On Thu, Sep 08, 2022 at 10:49:07AM +0200, Juergen Gross wrote:
>> diff --git a/arch/x86/include/asm/cacheinfo.h b/arch/x86/include/asm/cacheinfo.h
>> index 86b2e0dcc4bf..1aeafa9888f7 100644
>> --- a/arch/x86/include/asm/cacheinfo.h
>> +++ b/arch/x86/include/asm/cacheinfo.h
>> @@ -2,6 +2,11 @@
>>   #ifndef _ASM_X86_CACHEINFO_H
>>   #define _ASM_X86_CACHEINFO_H
>>   
>> +/* Kernel controls MTRR and/or PAT MSRs. */
>> +extern unsigned int cache_generic;
> 
> So this should be called something more descriptive like
> 
> 	memory_caching_types

In the end this variable doesn't specify which caching types are available,
but the ways to select/control the caching types.

So what about "memory_caching_select" or "memory_caching_control" instead?

> or so to denote that this is a bitfield of supported memory caching
> technologies. The code then would read as
> 
> 	if (memory_caching_types & CACHE_MTRR)
> 
> The name's still not optimal tho - needs more brooding over.
> 
>> +#define CACHE_GENERIC_MTRR 0x01
>> +#define CACHE_GENERIC_PAT  0x02
> 
> And those should be CACHE_{MTRR,PAT}.

Fine with me.

>>   void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu);
>>   void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu);
>>   
>> diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
>> index 66556833d7af..3b05d3ade7a6 100644
>> --- a/arch/x86/kernel/cpu/cacheinfo.c
>> +++ b/arch/x86/kernel/cpu/cacheinfo.c
>> @@ -35,6 +35,9 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_llc_shared_map);
>>   /* Shared L2 cache maps */
>>   DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_l2c_shared_map);
>>   
>> +/* Kernel controls MTRR and/or PAT MSRs. */
>> +unsigned int cache_generic;
> 
> This should either be __ro_after_init and initialized to 0 or you need
> accessors...

Okay.

> 
>>   u32 num_var_ranges;
>> -static bool __mtrr_enabled;
>> -
>> -static bool mtrr_enabled(void)
>> -{
>> -	return __mtrr_enabled;
>> -}
>> +static bool mtrr_enabled;
> 
> Hmm, I don't like this. There's way too many boolean flags in the mtrr
> code. There's mtrr_state.enabled too. ;-\
> 
> Can we set (or clear) X86_FEATURE_MTRR to denote MTRR enablement status
> and get rid of one more boolean flag?

I'll have a look.

> 
> ...
> 
>>   void __init mtrr_bp_init(void)
>>   {
>> +	bool use_generic = false;
>>   	u32 phys_addr;
>>   
>>   	init_ifs();
>> @@ -694,6 +691,7 @@ void __init mtrr_bp_init(void)
>>   
>>   	if (boot_cpu_has(X86_FEATURE_MTRR)) {
>>   		mtrr_if = &generic_mtrr_ops;
>> +		use_generic = true;
>>   		size_or_mask = SIZE_OR_MASK_BITS(36);
>>   		size_and_mask = 0x00f00000;
>>   		phys_addr = 36;
>> @@ -755,15 +753,18 @@ void __init mtrr_bp_init(void)
>>   	}
>>   
>>   	if (mtrr_if) {
>> -		__mtrr_enabled = true;
>> -		set_num_var_ranges();
>> +		mtrr_enabled = true;
>> +		set_num_var_ranges(use_generic);
> 
> You don't need use_generic either:
> 
> 		set_num_var_ranges(mtrr_if == generic_mtrr_ops);
> 
> (The reason being I wanna get rid of that nasty minefield of boolean
> vars all round that code).

Fine with me.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]