Re: [PATCH] x86/cpu/intel: Fix MTRR verification for TME enabled platforms

From: Huang, Kai
Date: Mon Sep 25 2023 - 19:07:06 EST


On Sat, 2023-09-23 at 07:37 +0000, Li, Xin3 wrote:
> Hi Jeremy,
>
> > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > index be4045628fd3..34c54432bf00 100644
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -184,6 +184,90 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
> > return false;
> > }
> >
> > +#define MSR_IA32_TME_ACTIVATE 0x982
>
> I know you're just moving the definitions, however we usually define MSRs
> and their bits in arch/x86/include/asm/msr-index.h.
>
> > +
> > +/* Helpers to access TME_ACTIVATE MSR */
> > +#define TME_ACTIVATE_LOCKED(x) (x & 0x1)
> > +#define TME_ACTIVATE_ENABLED(x) (x & 0x2)
>
> What about:
>
> #define TME_ACTIVATE_LOCKED(x) (x & BIT(0))
> #define TME_ACTIVATE_ENABLED(x) (x & BIT(1))

(Also + Kirill who is the author of detect_tme())

By moving these bit definition to msr-index.h, IMHO we should just define the
macros for the bit positions, but remove the (x) part. This is consistent with
all other existing definitions.

Something like:

#define TME_ACTIVATE_LOCKED BIT(0)
...


>
> > +
> > +#define TME_ACTIVATE_POLICY(x) ((x >> 4) & 0xf) /* Bits 7:4 */
>
> And:
>
> /* Bits 7:4 are TME activate policy bits */
> #define TME_ACTIVATE_POLICY_OFFSET 4
> #define TME_ACTIVATE_POLICY_MASK 0xf
> #define TME_ACTIVATE_POLICY(x) \
> ((x >> TME_ACTIVATE_POLICY_OFFSET) & TME_ACTIVATE_POLICY_MASK)
>
> > +#define TME_ACTIVATE_KEYID_BITS(x) ((x >> 32) & 0xf) /* Bits 35:32 */
> > +
> > +#define TME_ACTIVATE_CRYPTO_ALGS(x) ((x >> 48) & 0xffff) /* Bits
> > 63:48 */
>
> ditto
>
> > @@ -335,6 +419,9 @@ static void early_init_intel(struct cpuinfo_x86 *c)
> > */
> > if (detect_extended_topology_early(c) < 0)
> > detect_ht_early(c);
> > +
>
> Please add a comment here explaining why detect_tme() needs to be called
> in early_init_intel().

Also this works because init_intel() also calls early_init_intel(). I noticed
before that people don't like this. So maybe it's worth adding some extra words
saying this, in case people may want to stop calling early_init_intel() from
init_intel() in the future, but I am not sure whether this is worth pointing
out.

Or, should we change to call detect_tme() from bsp_init_intel() instead of
early_init_intel(), and still keep calling detect_tme() from init_intel()?

This also avoids calling detect_tme() twice for BSP IIUC, because AFAICT
early_init_intel() is called twice for BSP.

>
> > + if (cpu_has(c, X86_FEATURE_TME))
> > + detect_tme(c);
> > }
> >
> > static void bsp_init_intel(struct cpuinfo_x86 *c)
>
> Thanks!
> Xin