[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <699aacca-b82a-78df-3a74-4b5b82de92c2@suse.com>
Date: Wed, 28 Sep 2022 12:17:08 +0200
From: Juergen Gross <jgross@...e.com>
To: Borislav Petkov <bp@...en8.de>
Cc: xen-devel@...ts.xenproject.org, x86@...nel.org,
linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v3 03/10] x86/mtrr: replace use_intel() with a local flag
On 12.09.22 11:10, Juergen Gross wrote:
> 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.
Hmm, this might be a little bit risky.
It can be done, but then X86_FEATURE_MTRR could be set even for cpus
NOT supporting it (the 32-bit special cases AMD, CENTAUR, CYRIX).
So we have the following alternatives:
- do the switch to X86_FEATURE_MTRR risking code breakage for later
code changes querying X86_FEATURE_MTRR and assuming the MTRR MSRs
being available
- keep the current bool
- replace the bool with mtrr_if != NULL
- add a new synthetic feature, e.g. X86_FEATURE_MTRR_ENABLED (which in
fact would be just a replacement of the current bool)
My preference would be the replacement with mtrr_if != NULL.
Juergen
Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3099 bytes)
Download attachment "OpenPGP_signature" of type "application/pgp-signature" (496 bytes)
Powered by blists - more mailing lists