Re: [RFC][PATCH 11/34] x86/cpu/intel: Prepare MKTME for "address configuration" infrastructure

[Huang, Kai]

On 27/02/2024 1:14 am, Kirill A. Shutemov wrote:
> On Fri, Feb 23, 2024 at 08:22:16AM -0800, Dave Hansen wrote:
> > On 2/23/24 03:33, Kirill A. Shutemov wrote:
> > > On Thu, Feb 22, 2024 at 10:39:41AM -0800, Dave Hansen wrote:
> > > > From: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> > > >
> > > > Intel also does memory encryption and also fiddles with the physical
> > > > address bits.  This is currently called for *each* CPU, but practically
> > > > only done on the boot CPU because of 'mktme_status'.
> > > >
> > > > Move it from the "each CPU" ->c_init() function to ->c_bsp_init() where
> > > > the whole thing only gets called once ever.  This also necessitates moving
> > > > detect_tme() and its entourage around in the file.
> > > The state machine around mktme_state doesn't make sense if we only call it
> > > on boot CPU, so detect_tme() can be drastically simplified. I can do it on
> > > top of the patchset.
> >
> > That would be great.  Looking at it again, the (tme_activate !=
> > tme_activate_cpu0) block is total cruft now.  It probably just needs to
> > get moved to secondary CPU startup.
>
> I have never saw the check to be useful. I think it can be just dropped.
>
> The patch below makes detect_tme() only enumerate TME and MKTME. And
> report number of keyid bits. Kernel doesn't care about anything else.
>
> Any comments?
>
>  From 1080535093d21f025d46fd610de5ad788591f4b5 Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
> Date: Mon, 26 Feb 2024 14:01:01 +0200
> Subject: [PATCH] x86/cpu/intel: Simplify detect_tme()
>
> The detect_tme() function is now only called by the boot CPU. The logic
> for cross-checking TME configuration between CPUs is no longer used. It
> has never identified a real problem and can be safely removed.
>
> The kernel does not use MKTME and is not concerned with MKTME policy or
> encryption algorithms. There is no need to check them.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> ---
>   arch/x86/kernel/cpu/intel.c | 44 ++-----------------------------------
>   1 file changed, 2 insertions(+), 42 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 4192aa4576f4..60918b49344c 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -329,55 +329,20 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>   #define TME_ACTIVATE_CRYPTO_ALGS(x)	((x >> 48) & 0xffff)	/* Bits 63:48 */
>   #define TME_ACTIVATE_CRYPTO_AES_XTS_128	1
>   
> -/* Values for mktme_status (SW only construct) */
> -#define MKTME_ENABLED			0
> -#define MKTME_DISABLED			1
> -#define MKTME_UNINITIALIZED		2
> -static int mktme_status = MKTME_UNINITIALIZED;
> -
>   static int detect_tme(struct cpuinfo_x86 *c)
>   {
> -	u64 tme_activate, tme_policy, tme_crypto_algs;
> -	int keyid_bits = 0, nr_keyids = 0;
> -	static u64 tme_activate_cpu0 = 0;
> +	int keyid_bits, nr_keyids;
> +	u64 tme_activate;
>   
>   	rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
>   
> -	if (mktme_status != MKTME_UNINITIALIZED) {
> -		if (tme_activate != tme_activate_cpu0) {
> -			/* Broken BIOS? */
> -			pr_err_once("x86/tme: configuration is inconsistent between CPUs\n");
> -			pr_err_once("x86/tme: MKTME is not usable\n");
> -			mktme_status = MKTME_DISABLED;
> -
> -			/* Proceed. We may need to exclude bits from x86_phys_bits. */
> -		}
> -	} else {
> -		tme_activate_cpu0 = tme_activate;
> -	}
> -
>   	if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
>   		pr_info_once("x86/tme: not enabled by BIOS\n");

Since now detect_tme() is only called on BSP, seems we can change to 
pr_info(), which is used ...
> -		mktme_status = MKTME_DISABLED;
>   		return 0;
>   	}
>   
> -	if (mktme_status != MKTME_UNINITIALIZED)
> -		goto detect_keyid_bits;
> -
>   	pr_info("x86/tme: enabled by BIOS\n");

.. right here anyway.

>   
> -	tme_policy = TME_ACTIVATE_POLICY(tme_activate);
> -	if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS_128)
> -		pr_warn("x86/tme: Unknown policy is active: %#llx\n", tme_policy);
> -
> -	tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
> -	if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128)) {
> -		pr_err("x86/mktme: No known encryption algorithm is supported: %#llx\n",
> -				tme_crypto_algs);
> -		mktme_status = MKTME_DISABLED;
> -	}
> -detect_keyid_bits:
>   	keyid_bits = TME_ACTIVATE_KEYID_BITS(tme_activate);
>   	nr_keyids = (1UL << keyid_bits) - 1;
>   	if (nr_keyids) {
> @@ -387,11 +352,6 @@ static int detect_tme(struct cpuinfo_x86 *c)
>   		pr_info_once("x86/mktme: disabled by BIOS\n");
>   	}
>   
> -	if (mktme_status == MKTME_UNINITIALIZED) {
> -		/* MKTME is usable */
> -		mktme_status = MKTME_ENABLED;
> -	}
> -
>   	return keyid_bits;

Other than that the code change LGTM.

Reviewed-by: Kai Huang <kai.huang@xxxxxxxxx>