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

[Edgecombe, Rick P]

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...

> 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.

>  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.

> 
> 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.

> 
> 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...

> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Reviewed-by: Tony Lindgren <tony.lindgren@xxxxxxxxxxxxxxx>
> Reviewed-by: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> Reviewed-by: Kai Huang <kai.huang@xxxxxxxxx>
> Reviewed-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
> ---
> v9:
>  - Use CPU0 as the primary CPU
> ---
>  arch/x86/include/asm/tdx_global_metadata.h  |  4 ++++
>  arch/x86/virt/vmx/tdx/seamldr.c             | 15 ++++++++++++++-
>  arch/x86/virt/vmx/tdx/tdx.c                 | 19 ++++++++++++++++++-
>  arch/x86/virt/vmx/tdx/tdx.h                 |  3 +++
>  arch/x86/virt/vmx/tdx/tdx_global_metadata.c | 13 +++++++++++++
>  5 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/tdx_global_metadata.h b/arch/x86/include/asm/tdx_global_metadata.h
> index 40689c8dc67e..41150d546589 100644
> --- a/arch/x86/include/asm/tdx_global_metadata.h
> +++ b/arch/x86/include/asm/tdx_global_metadata.h
> @@ -40,6 +40,10 @@ struct tdx_sys_info_td_conf {
>  	u64 cpuid_config_values[128][2];
>  };
>  
> +struct tdx_sys_info_handoff {
> +	u16 module_hv;
> +};
> +
>  struct tdx_sys_info {
>  	struct tdx_sys_info_version version;
>  	struct tdx_sys_info_features features;
> diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
> index 48fe71319fea..6114cab46196 100644
> --- a/arch/x86/virt/vmx/tdx/seamldr.c
> +++ b/arch/x86/virt/vmx/tdx/seamldr.c
> @@ -15,6 +15,7 @@
>  #include <asm/seamldr.h>
>  
>  #include "seamcall_internal.h"
> +#include "tdx.h"
>  
>  /* P-SEAMLDR SEAMCALL leaf function */
>  #define P_SEAMLDR_INFO			0x8000000000000000
> @@ -164,6 +165,7 @@ static int init_seamldr_params(struct seamldr_params *params, const u8 *data, u3
>   */
>  enum module_update_state {
>  	MODULE_UPDATE_START,
> +	MODULE_UPDATE_SHUTDOWN,
>  	MODULE_UPDATE_DONE,
>  };
>  
> @@ -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.

>  	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.

> +
> +	/*
> +	 * Use the module's handoff version as it is the highest the
> +	 * module can produce and most likely supported by newer modules.
> +	 */
> +	args.rcx = handoff.module_hv;
> +	return seamcall_prerr(TDH_SYS_SHUTDOWN, &args);
> +}
> +
>  static bool is_pamt_page(unsigned long phys)
>  {
>  	struct tdmr_info_list *tdmr_list = &tdx_tdmr_list;
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 76c5fb1e1ffe..f0c20dea0388 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -46,6 +46,7 @@
>  #define TDH_PHYMEM_PAGE_WBINVD		41
>  #define TDH_VP_WR			43
>  #define TDH_SYS_CONFIG			45
> +#define TDH_SYS_SHUTDOWN		52
>  #define TDH_SYS_DISABLE			69
>  
>  /*
> @@ -108,4 +109,6 @@ struct tdmr_info_list {
>  	int max_tdmrs;	/* How many 'tdmr_info's are allocated */
>  };
>  
> +int tdx_module_shutdown(void);
> +
>  #endif
> diff --git a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> index d54d4227990c..e793dec688ab 100644
> --- a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> +++ b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> @@ -100,6 +100,19 @@ static __init int get_tdx_sys_info_td_conf(struct tdx_sys_info_td_conf *sysinfo_
>  	return ret;
>  }
>  
> +static int get_tdx_sys_info_handoff(struct tdx_sys_info_handoff *sysinfo_handoff)
> +{
> +	int ret;
> +	u64 val;
> +
> +	ret = read_sys_metadata_field(0x8900000100000000, &val);
> +	if (ret)
> +		return ret;
> +
> +	sysinfo_handoff->module_hv = val;
> +	return 0;
> +}
> +
>  static __init int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
>  {
>  	int ret = 0;