Re: [PATCH v4 19/24] x86/virt/tdx: Update tdx_sysinfo and check features post-update

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Thu, 2026-02-12 at 06:35 -0800, Chao Gao wrote:
> > tdx_sysinfo contains all metadata of the active TDX module, including
> > versions, supported features, and TDMR/TDCS/TDVPS information. These
> > values may change over updates. Blindly refreshing the entire tdx_sysinfo
> > could disrupt running software, as it may subtly rely on the previous state
> > unless proven otherwise.
> > 
> > Adopt a conservative approach, like microcode updates, by only refreshing
> > version information that does not affect functionality, while ignoring
> > all other changes. This is acceptable as new modules are required to
> > maintain backward compatibility.
> > 
> > Any updates to metadata beyond versions should be justified and reviewed on
> > a case-by-case basis.
> > 
> > Note that preallocating a tdx_sys_info buffer before updates is to avoid
> > having to handle -ENOMEM when updating tdx_sysinfo after a successful
> > update.
> > 
> > Signed-off-by: Chao Gao <chao.gao@intel.com>
> > Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> > ---
> > v3:
> >  - use 'old' instead of 'cur' as the local variable to represent the
> >    sysinfo of the previous module [Binbin]
> >  - combine if(ret) and WARN_ONCE(1, ...) to WARN_ONCE(ret, ...) [Binbin]
> >  - Improve the print log messages after detecting new features from updates.
> >    [Binbin]
> > 
> > v2:
> >  - don't add a separate function for version and feature checks. Do them
> >    directly in tdx_module_post_update()
> >  - add a comment about preallocating a tdx_sys_info buffer in
> >    seamldr_install_module().
> > ---
> >  arch/x86/virt/vmx/tdx/seamldr.c | 11 ++++++++-
> >  arch/x86/virt/vmx/tdx/tdx.c     | 43 +++++++++++++++++++++++++++++++++
> >  arch/x86/virt/vmx/tdx/tdx.h     |  3 +++
> >  3 files changed, 56 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
> > index 0ca802234695..3f37cc6c68ff 100644
> > --- a/arch/x86/virt/vmx/tdx/seamldr.c
> > +++ b/arch/x86/virt/vmx/tdx/seamldr.c
> > @@ -315,6 +315,15 @@ int seamldr_install_module(const u8 *data, u32 size)
> >  	if (WARN_ON_ONCE(!is_vmalloc_addr(data)))
> >  		return -EINVAL;
> >  
> > +	/*
> > +	 * Preallocating a tdx_sys_info buffer before updates is to avoid having to
> > +	 * handle -ENOMEM when updating tdx_sysinfo after a successful update.
> > +	 */
> > +	struct tdx_sys_info *sysinfo __free(kfree) = kzalloc(sizeof(*sysinfo),
> > +							     GFP_KERNEL);
> > +	if (!sysinfo)
> > +		return -ENOMEM;
> > +
> >  	struct seamldr_params *params __free(free_seamldr_params) =
> >  						init_seamldr_params(data, size);
> >  	if (IS_ERR(params))
> > @@ -332,6 +341,6 @@ int seamldr_install_module(const u8 *data, u32 size)
> >  	if (ret)
> >  		return ret;
> >  
> > -	return 0;
> > +	return tdx_module_post_update(sysinfo);
> >  }
> >  EXPORT_SYMBOL_FOR_MODULES(seamldr_install_module, "tdx-host");
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index a8adb2c97e2f..3f5edbc33a4f 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -1218,6 +1218,49 @@ int tdx_module_run_update(void)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Update tdx_sysinfo and check if any TDX module features changed after
> > + * updates
> > + */
> > +int tdx_module_post_update(struct tdx_sys_info *info)
> > +{
> > +	struct tdx_sys_info_version *old, *new;
> > +	int ret;
> > +
> > +	/* Shouldn't fail as the update has succeeded */
> > +	ret = get_tdx_sys_info(info);
> > +	if (WARN_ONCE(ret, "version retrieval failed after update, replace TDX Module\n"))
> > +		return ret;
> > +
> > +	old = &tdx_sysinfo.version;
> > +	new = &info->version;
> > +	pr_info("version %u.%u.%02u -> %u.%u.%02u\n", old->major_version,
> > +						      old->minor_version,
> > +						      old->update_version,
> > +						      new->major_version,
> > +						      new->minor_version,
> > +						      new->update_version);
> > +
> > +	/*
> > +	 * Blindly refreshing the entire tdx_sysinfo could disrupt running
> > +	 * software, as it may subtly rely on the previous state unless
> > +	 * proven otherwise.
> > +	 *
> > +	 * Only refresh version information (including handoff version)
> > +	 * that does not affect functionality, and ignore all other
> > +	 * changes.
> > +	 */
> > +	tdx_sysinfo.version	= info->version;
> > +	tdx_sysinfo.handoff	= info->handoff;
> > +
> > +	if (!memcmp(&tdx_sysinfo, info, sizeof(*info)))
> > +		return 0;

Ahh, late day reviewing this yesterday, I read this as memcpy(). This seems like
a good approach. I'd only wonder if this should either be a stronger warning, or
we can skip checking the TDX module is behaving. We rely on a lot already. But
feel free to disregard it.

Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

> > +
> > +	pr_info("TDX module features have changed after updates, but might not take effect.\n");
> > +	pr_info("Please consider updating your BIOS to install the TDX Module.\n");
> > +	return 0;
> > +}
> > +
> >  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 0887debfd139..d1807a476d3b 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.h
> > +++ b/arch/x86/virt/vmx/tdx/tdx.h
> > @@ -4,6 +4,8 @@
> >  
> >  #include <linux/bits.h>
> >  
> > +#include <asm/tdx_global_metadata.h>
> > +
> >  /*
> >   * This file contains both macros and data structures defined by the TDX
> >   * architecture and Linux defined software data structures and functions.
> > @@ -122,5 +124,6 @@ struct tdmr_info_list {
> >  
> >  int tdx_module_shutdown(void);
> >  int tdx_module_run_update(void);
> > +int tdx_module_post_update(struct tdx_sys_info *info);
> >  
> >  #endif