Re: [PATCH 3/4] x86/kexec(): Reset TDX private memory on platforms with TDX erratum

["Huang, Kai" <kai.huang@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.