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

[Juergen Gross <jgross@xxxxxxxx>]

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature