Received-SPF: pass (google.com: domain of linux-kernel+bounces-84070-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <b32cdda4-5cca-4608-b403-30ab6ce668de@intel.com>
Date: Wed, 28 Feb 2024 10:48:19 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC][PATCH 11/34] x86/cpu/intel: Prepare MKTME for "address
 configuration" infrastructure
Content-Language: en-US
To: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>, Dave Hansen
	<dave.hansen@intel.com>
CC: Dave Hansen <dave.hansen@linux.intel.com>, <linux-kernel@vger.kernel.org>,
	<pbonzini@redhat.com>, <tglx@linutronix.de>, <x86@kernel.org>, <bp@alien8.de>
References: <20240222183926.517AFCD2@davehans-spike.ostc.intel.com>
 <20240222183941.7CB634A5@davehans-spike.ostc.intel.com>
 <z52bbgqxtr7wpa3yqqgbwurb6vx6i7gpddae2rrbxkjasuhnuk@7zsudlb4tw6s>
 <b01f92b3-65ea-474b-8dc9-f2e1ca6ae0af@intel.com>
 <wtdmrkjfvlf4b5mkpqw537u6xuxkdajix2knbo5ivanjzzpvvg@qqlw7gaetujj>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <wtdmrkjfvlf4b5mkpqw537u6xuxkdajix2knbo5ivanjzzpvvg@qqlw7gaetujj>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Ri90NkUwYmloakxOY3lyQzFocVptZWU5VUFtWkpNdEFGMjFFZ25MaDl0UThW?=
 =?utf-8?B?dmNMeGZGZ3Z1bTlSaHA4WklBTmFCNEpob2VONzJmNUNSOG9xQUs2dEZPZ0Vx?=
 =?utf-8?B?eHVRMmtFV2tVUzV0SzRYQ0pYZ3pTbzk0cDZYaHk2U3Fid1hIRmdZdE9sRWFU?=
 =?utf-8?B?SzZwMEcveHU0dGlMVDg2eDJxbUQxenVOUmZ3bTlpa2theUpzdThIT25XSUlx?=
 =?utf-8?B?R05yRnkxSnNoazFQVm1MaWdUV3pCaDk2eEx1REY4MkFPTzYzVlBOVCsxdUw1?=
 =?utf-8?B?Ukk5b25pdVlTWDVCeXBkYnBDVmFLT1dpWllJdmRlRnlpdWViS0htbTB1cTN2?=
 =?utf-8?B?T2w5c092Y01vUUg0ZEczMXorYXFsMmh6Zk9XMUJ2aHFFbWlIWElyT3VQQUVj?=
 =?utf-8?B?SkhBU2JCZkFZYUFCMlpPaFhXeHlibDRnTnU4OHJCMGZuSFcxU1dnS3dzdnZP?=
 =?utf-8?B?ZmdHS2J4ZWVEdmVENWFQOWpqOUMrQS9NT2RRTUxZcHFHRjBwT0wwRkJ1Q1Ba?=
 =?utf-8?B?VEFDdmxqS0ZNb1VkTGxIMWxNR3NZREprMm5qcGF2VE1tWE1YZHZZcVZKa29X?=
 =?utf-8?B?YmIyc1hQSjhYMzl6NkhIbW1HR21zZXlwdTEwWkM5Y0xiNk9SNlhPR1JHbHhH?=
 =?utf-8?B?MlpSWXlJeEYxdEdSc3M5SkZrako1TUVHT3ZyTVlPS2VlUDlUR0FtOWtJVHB0?=
 =?utf-8?B?TUF0TG5xRHFzS1ZpQWhTa0ZkQlQ2d1FCTVhPUjZlRi9JWmlLV0hRWkU1MkFO?=
 =?utf-8?B?bytGRnBpVFNwaHA2SlViTDJKZG5BcVROV3pKN2JndGN0b1gyaHlpMUpvenAr?=
 =?utf-8?B?aENzcGtycXBBU0hrYXBwMzV3NXA2ZHdPNUh0VTV0QmRkY2tpd21DUSttVlI4?=
 =?utf-8?B?WnZtVk1IdHF4Nlh6bU1kNG5PeFdjYk1CdmVpS1pCMTJ1NDYzNi9ENDJHaE5W?=
 =?utf-8?B?NEZjQjVmQjJtRy9GVW9yWmdsQW1WRzZWNlRBeXJvNERqWC9YWGNTL0tNYlVt?=
 =?utf-8?B?MTJObGhjRFVMc051dWVBaENZK0FNRERQQlhaMjA2U0pWSFJxTnpiSnpsVVNa?=
 =?utf-8?B?UkJoakg3YlhYUWcxMHdCb05vYnlhZFVWb3dvek1BdzY0alFvKyt4VEs1RzFR?=
 =?utf-8?B?OHlHdndKZ2VoOTl1TmJMTXlhbStxKzVYT1pKdnk0RjNxRVVjWG8vVG10WFRz?=
 =?utf-8?B?TnpWekcrLytNUm92OUVmc1BYV2VyVFNkOWRMNFpSc25UeFQ2anBwb1lwZm8x?=
 =?utf-8?B?RkxFQkQwL2NBUXIxb3NFVFNNYjUrMmRpdHNyeGV0Yms3aFBZUmJpK21FSWVl?=
 =?utf-8?B?cG5jSHhMSXRhQm5VazR6UUdMNGZ0a0RGVkNKRzM1Z2NjWDAwZ1lhUko1UnlN?=
 =?utf-8?B?VEZMZTJ0aThlQTdiNmE0UE1ZNkJtanhjV2JBNXY5dCtFNUlLeVhidW1xd2Uv?=
 =?utf-8?B?NW44OFRvUTAzUmlLQ2ZmNVllQVRFVGdOSXNZWjZEQU9NeUIrUmpzb1JGMGxu?=
 =?utf-8?B?ZCtlRHNwa3JTTk9WSndFWElYZ0w2cHpGVnhyMWd5RkZlSW84U0RXOEtEKzln?=
 =?utf-8?B?MHVhc0ZtK3k2YzcwL1pOQWNVK2F4anQ1ZmREZldIZzVIdEJ5YTBkR1FlSDRH?=
 =?utf-8?B?eFgvYjRmTUR2NHp6VjM3cFU2QUxRK0RlRDJmYUNoUytZRUJIQzFGckRJMzla?=
 =?utf-8?B?VjBBend4WjBCNzA1QjRIb0hVQXBwVmRQbU9TeklsaDkwUEV6d0U2dFh6N1Zm?=
 =?utf-8?B?ODZMcjlzVU45MTROays2ekh5MW9QVmRyVVF2ZkFnZ0YvYm5hdWhZZGNhQnh2?=
 =?utf-8?B?cFMzQ2N2MFVuL2hYSnhGWGN6WlJXZlZocGlaMVZyUWlPRmFlQUY0aVZ3S1Zq?=
 =?utf-8?B?SHRuV2Q4ODYyRDJiSEtxVEoyYWZpakxsd3J3d1IvRzVwS3ZVUXp1Z0RtVU5m?=
 =?utf-8?B?dnNQaEFYQnNIR01Nd1NpZW93YkJ6aktuL2NGNWJFd3ZEMnlPSy9sOUc4ZDcw?=
 =?utf-8?B?eXpDVEJZenlVM0wzOXA2cWFEMExQRHZFQkNTNVBRVTBOdFBWWVNrRXFHbHlH?=
 =?utf-8?B?TUtsTlIrbGdVNGJQRUwrRW1WdEtqRm9uUHpxQmVkSFlvbXRHaEQ0WUdZRnFD?=
 =?utf-8?Q?b/yBpv/TIP4BN9xVmfeS6fwKn?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0e2d59bc-2578-4014-20dd-08dc37ddd51f
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Feb 2024 21:48:28.4167
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: UGWe05dE6wZWAaLWrX0SB1w8BDaud+3zDsFFBdtdF+XxiXfj/57KqTDmfHyCwGIr6O+80yd+5lNCvmsOQws7kw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5278
X-OriginatorOrg: intel.com


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@linux.intel.com>
>>>>
>>>> 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@linux.intel.com>
> 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@linux.intel.com>
> ---
>   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@intel.com>