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

["Huang, Kai" <kai.huang@intel.com>]

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