Re: [PATCH v9 14/23] x86/virt/seamldr: Shut down the current TDX module

[Chao Gao <chao.gao@intel.com>]

On Tue, May 19, 2026 at 11:00:54AM +0800, Edgecombe, Rick P wrote:
>On Wed, 2026-05-13 at 08:09 -0700, Chao Gao wrote:
>> The first step of TDX module updates is shutting down the current TDX
>> module. This step also packs state information that needs to be
>> preserved across updates as handoff data, 
>> 
>
>kinda reads like handoff data is an existing term, but its the first reference
>in this series.
>
>Maybe packs state information that needs to be preserved across updates, called
>"handoff data". This handoff data is consumed...

Sure. Will do.

>
>> which will be consumed by the
>> updated module. The handoff data is stored internally in the SEAM range
>> and is hidden from the kernel.
>> 
>> To ensure a successful update, the new module must be able to consume
>> the handoff data generated by the old module.
>> 
>
>Is it too obvious thing to state? Above you already say it's needed.

Ok. Let me drop it.

>
>>  Since handoff data layout
>> may change between modules, the handoff data is versioned. Each module
>> has a native handoff version and provides backward support for several
>> older versions.
>> 
>> The complete handoff versioning protocol is complex as it supports both
>> module upgrades and downgrades. See details in Intel® Trust Domain
>> Extensions (Intel® TDX) Module Base Architecture Specification, Chapter
>> "Handoff Versioning".
>> 
>> Ideally, the kernel needs to retrieve the handoff versions supported by
>> the current module and the new module and select a version supported by
>> both. But, since this implementation chooses to only support module
>> upgrades, simply request the current module to generate handoff data
>> using its highest supported version, expecting that the new module will
>> likely support it.
>
>Hmm, "likely"? Is this trying to justify the kernel's policy? Dunno, stands out
>as weird to me. Like "this will mostly work". Sounds incomplete, rather than a
>reason of "this policy is the optimal initial implementation" or something like
>that.

how about:

... But since this implementation only supports module upgrades, simply request
handoff data from the current module using its highest supported version.
That is sufficient for this upgrade-only implementation.

>
>> 
>> Retrieve the module's handoff version from TDX global metadata and add an
>> update step to shut down the module.
>> 
>
>This is small patch with both things, but it's almost two changes.
>
>>  Module shutdown has global effect, so
>> it only needs to run on one CPU.
>
>I wouldn't think having some global effect would necessarily exclude having to
>run on multiple CPUs. Or at least I don't follow. Is it a TDX arch thing? I
>guess it's ok.

Yes. This comes from the TDX architecture. I will just say in the changelog
that module shutdown only needs to run on one CPU.

>
>> 
>> Note that the handoff information isn't cached in tdx_sysinfo. It is used
>> only for module shutdown, and is present only when the TDX module supports
>> updates. Caching it in get_tdx_sys_info() would require extra update-support
>> guards and refreshing the cached value across module updates.
>
>Instead of being a "note", could this be just an imperative: Don't cache the
>handoff information in tdx_sysinfo...

Sure. Will do.

>
>> @@ -214,8 +216,16 @@ static void init_state(struct update_ctrl *ctrl)
>>  static int do_seamldr_install_module(void *seamldr_params)
>>  {
>>  	enum module_update_state newstate, curstate = MODULE_UPDATE_START;
>> +	int cpu = smp_processor_id();
>> +	bool primary;
>>  	int ret = 0;
>>  
>> +	/*
>> +	 * Use CPU 0 to execute update steps that must run exactly once.
>> +	 * Note CPU 0 is always online.
>> +	 */
>> +	primary = cpu == 0;
>> +
>
>Where does the term 'primary' come from?
>I'm guessing that the global steps must
>each be run on the same CPU? Is that right? And we just pick the cpu that we
>know much be online? Or can the global steps be run on different CPUs? Or they
>*have* to be run on cpu 0? It might be worth some comments explaining, depending
>on the answers to those questions.

"primary" is just my name for the CPU that runs the global steps. There is
nothing special about CPU 0 beyond the fact that it is guaranteed to be
online, so it is a convenient choice.

I can rename it to something like 'is_primary_cpu' or 'is_global_step_cpu'
for clarity.

how about:

/*
 * Some steps must be run on exactly one CPU. Pick CPU 0 to execute those
 * steps because CPU 0 is always online.
 */

>
>>  	do {
>>  		newstate = READ_ONCE(update_ctrl.state);
>>  
>> @@ -226,7 +236,10 @@ static int do_seamldr_install_module(void *seamldr_params)
>>  
>>  		curstate = newstate;
>>  		switch (curstate) {
>> -		/* TODO: add the update steps. */
>> +		case MODULE_UPDATE_SHUTDOWN:
>> +			if (primary)
>> +				ret = tdx_module_shutdown();
>> +			break;
>>  		default:
>>  			break;
>>  		}
>> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
>> index 1621695d7561..da3c1e857b26 100644
>> --- a/arch/x86/virt/vmx/tdx/tdx.c
>> +++ b/arch/x86/virt/vmx/tdx/tdx.c
>> @@ -321,7 +321,7 @@ static __init int build_tdx_memlist(struct list_head *tmb_list)
>>  	return ret;
>>  }
>>  
>> -static __init int read_sys_metadata_field(u64 field_id, u64 *data)
>> +static int read_sys_metadata_field(u64 field_id, u64 *data)
>>  {
>>  	struct tdx_module_args args = {};
>>  	int ret;
>> @@ -1267,6 +1267,23 @@ static __init int tdx_enable(void)
>>  }
>>  subsys_initcall(tdx_enable);
>>  
>> +int tdx_module_shutdown(void)
>> +{
>> +	struct tdx_sys_info_handoff handoff = {};
>> +	struct tdx_module_args args = {};
>> +	int ret;
>> +
>> +	ret = get_tdx_sys_info_handoff(&handoff);
>> +	WARN_ON_ONCE(ret);
>
>Take or leave it:
>
>  Why not just WARN_ON_ONCE(get_tdx_sys_info_handoff(&handoff));
>  And we can drop the ret var. Save 2 LOC.

Dave had a different preference here:

https://lore.kernel.org/kvm/8b9d7fa7-6534-48e7-a4fa-c21260b1c762@intel.com/