Received-SPF: pass (google.com: domain of linux-kernel+bounces-48326-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <a139b1e0-7737-4c20-842c-64457dba17ca@intel.com>
Date: Thu, 1 Feb 2024 22:35:06 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/4] x86/kexec(): Reset TDX private memory on platforms
 with TDX erratum
Content-Language: en-US
To: Dave Hansen <dave.hansen@intel.com>, <linux-kernel@vger.kernel.org>
CC: <x86@kernel.org>, <kirill.shutemov@linux.intel.com>, <tglx@linutronix.de>,
	<bp@alien8.de>, <mingo@redhat.com>, <hpa@zytor.com>, <luto@kernel.org>,
	<peterz@infradead.org>, <thomas.lendacky@amd.com>, <chao.gao@intel.com>,
	<bhe@redhat.com>, <nik.borisov@suse.com>, <pbonzini@redhat.com>
References: <cover.1706698706.git.kai.huang@intel.com>
 <b52ed259e0d487b3a968b98da91cb4f55a28ae82.1706698706.git.kai.huang@intel.com>
 <12a927df-e437-40ff-ba4d-ceca5446c5b1@intel.com>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <12a927df-e437-40ff-ba4d-ceca5446c5b1@intel.com>
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?Qy9abXEyQkNiRERiSkZtV1ZmeWxnVjRLWldFQVQ3TTQzWU5XVjFIMnFNVE5q?=
 =?utf-8?B?MlBFTXVkL2xYTFhtOTlneTkwR0lHQTZ0bU9kemY5bURFRWVxL20vMkJaWXZY?=
 =?utf-8?B?QjhmbnpDT0ltak15WUxaaHl3ODVOblBmWlpGMlNPQWNzem9BRjJwbVZiUFlq?=
 =?utf-8?B?UzZud1pSY0NoUjFYc2ZZOWVyb3FRTEhSYThiU3F4YkpaRHlkK3JKbithRzc2?=
 =?utf-8?B?eFhtK01qT29Qb0dJaFIwc0Y4Ukd4czlISHEyMkZaeTZQaDB3aVRFZmtPRlhz?=
 =?utf-8?B?MGhhblJpY1A3TDU5d3BOYUtoQkJjVDZZZ3ZwSGVlZENGbkFDaGpKTFZaQzJa?=
 =?utf-8?B?d2w4bWUzZDRWVURYeitDZDJCTlZxUGdTd2pWWEM4VVZsVFFib25pZC9tYUQz?=
 =?utf-8?B?Tzd3SEhxakxNQ2NRY0xLT0ZCak1KVXdteUNBcDV4Y3VJMWUrV21CTjVGY0Z3?=
 =?utf-8?B?d0IrbkpxVURhcEhGcFBvTmVqWno5ek9NVi83Z1hUY2VaOXFSTTBPaUZ4dGl5?=
 =?utf-8?B?RXpOVldjNFZsTzhLWmRrckpOcWthZWp2SzFFdjF5OW1waE1paEd6Vk0yRnJI?=
 =?utf-8?B?VmpFcUNJZkJzMzFNUFZublEyUUJuVWdGUy9zMnVnd1lFdWRoZUJUcHF4c0oy?=
 =?utf-8?B?S2x2ajBFbGU5ekhFdldsYnkwTGZsUUJUV3ZhYmRtanNYdDJtdTZyV0ZVYWZ0?=
 =?utf-8?B?dzJ3eUlMVVNTQS83VFJnVUNNUVh3MDJlM3JPb2x5Y3BxQkxFUXhZUGxBbUZz?=
 =?utf-8?B?NnY4ZDVMdkd6ZzZpTElEenBnb08zSUxvUlhWcjVkVWtrb2N3TU5JZXg2aXEv?=
 =?utf-8?B?MklSRUZYQS9qdXhLRnlIckltOFNUTmR4cEJvZ3J2UmNqOFd1aHd5dnEwdUw3?=
 =?utf-8?B?S3AvK0R6YldLNy9NdzJpOEgwMFZZZHpWZXNDYnA5cHZiZ3NhcGFrTFFBeVAv?=
 =?utf-8?B?MCtJTmtXWmlpaUZWVVJmaGo2NVlJZmtHU2ZWaTNOa3ZmZkk4Y1VkbXd0ZzU1?=
 =?utf-8?B?ak5WMnlpOGUrUXNlQmY5YitkQzNOQmIzMFMvNUtJZWhsNmRKM0FQcHFxZ2Vq?=
 =?utf-8?B?OC9Lb3h5Z1ZKWWU4MU15ZG8rOUlhSHNWMyt2U0ZVMGFaZWhKZEtMdjBabWVq?=
 =?utf-8?B?OUZrQ3RTVzNoS1NNVnp0OE4xUGZjMzRSZG10K2VQN1JiNXJqSDQ0alhqdkl0?=
 =?utf-8?B?ajhDSVY5T1BidjlDTzRyczFBU1NDaCtRUUcvYjk1VzJiVkVkVVVuVjZKQklR?=
 =?utf-8?B?cTF0Wlp5SS9mWEVIbVlOcllsTERvNkh4QllNdWl4YlpZMFAwYmRrZkMyV1Fn?=
 =?utf-8?B?OWxFREZ6bEI1SkhvVk9RUmpQSjNQSDZ4dXJMejlHSGJxanJUNUtScUt1M25k?=
 =?utf-8?B?bmxhNDM4M210QzFTbncvWGYwRytPbTlBRlMwS3BjVDJsdlpyNStOTTRmeG1K?=
 =?utf-8?B?MXNNTzJqMHdmT3ZpaXJHb0MzYWZCL2F2QmY2Um5Zdkx3MXhoY1B3VDQvbDB2?=
 =?utf-8?B?ZHFoVzR4dTdSazNrMkVRU1lLVWoyZ1N0OUxtZ2hOc3Byb3JxakhRaWxHcCtD?=
 =?utf-8?B?eDRwNld5MVZBWC9nM2QzNnJvMGZjZ2tuS0s0czFRZ1dFbmtUY2xPcXZPVmFD?=
 =?utf-8?B?NFFEUlRSQ0RwaDFXUXZyaGdqdHc2a2liREs5MTlkUmhOM1EyT2dVbkpYN3Jl?=
 =?utf-8?B?MThhR1NwT2NjUHVkeVk3RlFQaTJYZmlrMlM0a0RKTkpiNG41akoyMWlwM3h4?=
 =?utf-8?B?ODFUMTUyYWYzcmxSMGhmQ251dHNuT0NNNjhReFBTTlQvWkVQOWNGcVorWHkz?=
 =?utf-8?B?WHc3MmRkWldRb3dHUnZvelBRV1FXMkpEY3NDNFFDSTJaUlZrMVlFSE5Dc3Bp?=
 =?utf-8?B?dGZFdkhqcmRucDQ3QjlKYlo0bEIwSldLdE9OQlZDVm1LeUlWZ21XSk9CZ3ZE?=
 =?utf-8?B?cWtrWjIzSUQwZmdVbUllV2QvTVdGYWNvZmRhdUJqa29XOEdBQS9rdTBhNDh6?=
 =?utf-8?B?WHZValBSNVdabFcwOGRMWUV6dUJFL3ZLZzc2REF2SUd3SnViaU51YWhpN1gz?=
 =?utf-8?B?T3drYys2cHpCZmFpMFFCcCtGbUtvMGF5ejlLYjl0b0c1azNVcWhyZWdRbGlk?=
 =?utf-8?Q?PQ+Ak8yb95hVYXtls0BfYvmOh?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d041bf95-3eca-4aa9-116c-08dc233303c1
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2024 14:35:19.7013
 (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: 2jmABsLCs1QOydbIV5zmF7rTlYJ02izd3KuQPp1K6yQwNrsCPdh0G+lu0Pc1j+EKyhV0f7bGQWUM+xC9l+M/sQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6379
X-OriginatorOrg: intel.com


On 1/02/2024 5:21 am, Dave Hansen wrote:
> On 1/31/24 03:31, Huang, Kai wrote:
>> --- a/arch/x86/kernel/machine_kexec_64.c
>> +++ b/arch/x86/kernel/machine_kexec_64.c
>> @@ -28,6 +28,7 @@
>>   #include <asm/setup.h>
>>   #include <asm/set_memory.h>
>>   #include <asm/cpu.h>
>> +#include <asm/tdx.h>
>>   
>>   #ifdef CONFIG_ACPI
>>   /*
>> @@ -298,9 +299,24 @@ void machine_kexec(struct kimage *image)
>>   	void *control_page;
>>   	int save_ftrace_enabled;
>>   
>> +	/*
>> +	 * For platforms with TDX "partial write machine check" erratum,
>> +	 * all TDX private pages need to be converted back to normal
>> +	 * before booting to the new kernel, otherwise the new kernel
>> +	 * may get unexpected machine check.
>> +	 *
>> +	 * But skip this when preserve_context is on.  The second kernel
>> +	 * shouldn't write to the first kernel's memory anyway.  Skipping
>> +	 * this also avoids killing TDX in the first kernel, which would
>> +	 * require more complicated handling.
>> +	 */
> 
> This is waaaaaaaaaaaaaaay too much information to stick in a generic
> function.  Just call tdx_reset_memory() and make the argument about

Hi Dave,

Thanks for review.

Agreed too much info here.  But I don't quite understand your second 
sentence here.  Can I have your suggestion again?

> 
>>   #ifdef CONFIG_KEXEC_JUMP
>>   	if (image->preserve_context)
>>   		save_processor_state();
>> +	else
>> +		tdx_reset_memory();
>> +#else
>> +	tdx_reset_memory();
>>   #endif
> 
> Wow, that's awfully hard to read.  I really wish folks' gag reflex would
> kick in when they see stuff like this to get them to spend an additional
> 15 seconds to turn this into:
> 
> 	if (IS_ENABLED(CONFIG_KEXEC_JUMP) && image->preserve_context)
> 		save_processor_state();
> 	else
> 		tdx_reset_memory();
> 

Yeah this is way better, but as Kirill pointed out we will have build 
error when both SUSPEND and HIBERNATION is off.

Maybe below?

#ifdef CONFIG_KEXEC_JUMP
	if (image->preserve_context)
		save_processor_state();
	else
#endif
	tdx_reset_memory();


>>   	save_ftrace_enabled = __ftrace_enabled_save();
>> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
>> index 9f1fed458a32..0537b1b76c2b 100644
>> --- a/arch/x86/virt/vmx/tdx/tdx.c
>> +++ b/arch/x86/virt/vmx/tdx/tdx.c
>> @@ -28,6 +28,7 @@
>>   #include <linux/acpi.h>
>>   #include <linux/suspend.h>
>>   #include <linux/acpi.h>
>> +#include <linux/reboot.h>
>>   #include <asm/page.h>
>>   #include <asm/special_insns.h>
>>   #include <asm/msr-index.h>
>> @@ -54,6 +55,8 @@ static DEFINE_MUTEX(tdx_module_lock);
>>   /* All TDX-usable memory regions.  Protected by mem_hotplug_lock. */
>>   static LIST_HEAD(tdx_memlist);
>>   
>> +static bool tdx_rebooting;
>> +
>>   typedef void (*sc_err_func_t)(u64 fn, u64 err, struct tdx_module_args *args);
>>   
>>   static inline void seamcall_err(u64 fn, u64 err, struct tdx_module_args *args)
>> @@ -1187,6 +1190,9 @@ static int __tdx_enable(void)
>>   {
>>   	int ret;
>>   
>> +	if (tdx_rebooting)
>> +		return -EAGAIN;
> 
> This whole 'tdx_rebooting' deserves to at least be in its own patch.
> Also -EAGAIN seems to be a really odd return code for a permanent failure.

Will move to a separate patch.

How about just -EINVAL?

> 
>>   	ret = init_tdx_module();
>>   	if (ret) {
>>   		pr_err("module initialization failed (%d)\n", ret);
>> @@ -1420,6 +1426,90 @@ static struct notifier_block tdx_memory_nb = {
>>   	.notifier_call = tdx_memory_notifier,
>>   };
>>   
>> +/*
>> + * Convert TDX private pages back to normal on platforms with
>> + * "partial write machine check" erratum.
>> + *
>> + * Called from machine_kexec() before booting to the new kernel.
>> + */
>> +void tdx_reset_memory(void)
>> +{
>> +	if (!boot_cpu_has(X86_FEATURE_TDX_HOST_PLATFORM))
>> +		return;
>> +
>> +	/*
>> +	 * Kernel read/write to TDX private memory doesn't
>> +	 * cause machine check on hardware w/o this erratum.
>> +	 */
>> +	if (!boot_cpu_has_bug(X86_BUG_TDX_PW_MCE))
>> +		return;
>> +
>> +	/* Called from kexec() when only rebooting cpu is alive */
>> +	WARN_ON_ONCE(num_online_cpus() != 1);
>> +
>> +	/*
>> +	 * tdx_reboot_notifier() waits until ongoing TDX module
>> +	 * initialization to finish, and module initialization is
>> +	 * rejected after that.  Therefore @tdx_module_status is
>> +	 * stable here and can be read w/o holding lock.
>> +	 */
>> +	if (tdx_module_status != TDX_MODULE_INITIALIZED)
>> +		return;
> 
> kexec() can't happen until after reboot notifiers are called.
> tdx_reboot_notifier() makes 'tdx_module_status' stable, so no lock is
> needed.
> 
> Right?

Yes.  I can replace the comment with your words.

> 
>> +	/*
>> +	 * Flush cache of all TDX private memory _before_ converting
>> +	 * them back to avoid silent memory corruption.
>> +	 */
>> +	native_wbinvd();
> 
> Since this is single-threaded, it also needs to mention where all the
> other CPU caches got flushed.

OK will point out.

> 
>> +	/*
>> +	 * Convert PAMTs back to normal.  All other cpus are already
>> +	 * dead and TDMRs/PAMTs are stable.
>> +	 *
>> +	 * Ideally it's better to cover all types of TDX private pages
>> +	 * here, but it's impractical:
>> +	 *
>> +	 *  - There's no existing infrastructure to tell whether a page
>> +	 *    is TDX private memory or not.
>> +	 *
>> +	 *  - Using SEAMCALL to query TDX module isn't feasible either:
>> +	 *    - VMX has been turned off by reaching here so SEAMCALL
>> +	 *      cannot be made;
>> +	 *    - Even SEAMCALL can be made the result from TDX module may
> 
> 		    ^ if     ^ a          ^,
> 

Thanks.  Will add.

>> +	 *      not be accurate (e.g., remote CPU can be stopped while
>> +	 *      the kernel is in the middle of reclaiming TDX private
>> +	 *      page and doing MOVDIR64B).
>> +	 *
>> +	 * One temporary solution could be just converting all memory
>> +	 * pages, but it's problematic too, because not all pages are
>> +	 * mapped as writable in direct mapping.  It can be done by
>> +	 * switching to the identical mapping for kexec() or a new page
>> +	 * table which maps all pages as writable, but the complexity is
>> +	 * overkill.
>> +	 *
>> +	 * Thus instead of doing something dramatic to convert all pages,
>> +	 * only convert PAMTs here.  Other kernel components which use
>> +	 * TDX need to do the conversion on their own by intercepting the
>> +	 * rebooting/shutdown notifier (KVM already does that).
>> +	 */
> 
> I'd leave the extended alternatives discussion in the changelog, not here.
> 
> Focus on what _this_ is doing and why it is imperfect:
> 
>   1. Just reset the PAMDs
>   2. This leaves A, B, and C undealt with
>   3. The risk of leaving those is ...
> 

Agreed. Will do.

> 
>> +	tdmrs_reset_pamt_all(&tdx_tdmr_list);
>> +}
>> +
>> +static int tdx_reboot_notifier(struct notifier_block *nb, unsigned long mode,
>> +			       void *unused)
>> +{
>> +	/* Wait ongoing TDX initialization to finish */
>> +	mutex_lock(&tdx_module_lock);
>> +	tdx_rebooting = true;
>> +	mutex_unlock(&tdx_module_lock);
>> +
>> +	return NOTIFY_OK;
>> +}
> 
> Why is 'tdx_rebooting' a new variable instead of a new state in
> tdx_module_status?
> 

I think we can but I believe using tdx_rebooting is more clear.

For instance, if we want to add a new state TDX_MODULE_ABORT for this 
case, when tdx_reboot_notifier() is called after module initialization, 
then the tdx_module_status (which is already TDX_MODULE_INITIALIZED) 
will be overwritten and we will lose the information that module 
initialization has been done successfully.

We may be able to avoid these using more complicated logic but I think 
using a separate tdx_rebooting is straightforward.