Re: [PATCH v2 08/21] coco/tdx-host: Implement FW_UPLOAD sysfs ABI for TDX Module updates

[Xu Yilun <yilun.xu@linux.intel.com>]

On Tue, Sep 30, 2025 at 07:52:52PM -0700, Chao Gao wrote:
> The firmware upload framework provides a standard mechanism for firmware
> updates by allowing device drivers to expose sysfs interfaces for
> user-initiated updates.
> 
> Register with this framework to expose sysfs interfaces for TDX Module
> updates and implement operations to process data blobs supplied by
> userspace.
> 
> Note that:
> 1. P-SEAMLDR processes the entire update at once rather than
>    chunk-by-chunk, so .write() is called only once per update; so the
>    offset should be always 0.
> 2. TDX Module Updates complete synchronously within .write(), meaning
>    .poll_complete() is only called after successful updates and therefore
>    always returns success
> 
> Why fw_upload instead of request_firmware()?
> ============================================
> The explicit file selection capabilities of fw_upload is preferred over
> the implicit file selection of request_firmware() for the following
> reasons:
> 
> a. Intel distributes all versions of the TDX Module, allowing admins to
> load any version rather than always defaulting to the latest. This
> flexibility is necessary because future extensions may require reverting to
> a previous version to clear fatal errors.
> 
> b. Some module version series are platform-specific. For example, the 1.5.x
> series is for certain platform generations, while the 2.0.x series is
> intended for others.
> 
> c. The update policy for TDX Module updates is non-linear at times. The
> latest TDX Module may not be compatible. For example, TDX Module 1.5.x
> may be updated to 1.5.y but not to 1.5.y+1. This policy is documented
> separately in a file released along with each TDX Module release.
> 
> So, the default policy of "request_firmware()" of "always load latest", is
> not suitable for TDX. Userspace needs to deploy a more sophisticated policy
> check (e.g., latest may not be compatible), and there is potential
> operator choice to consider.
> 
> Just have userspace pick rather than add kernel mechanism to change the
> default policy of request_firmware().
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
> ---
>  arch/x86/Kconfig                      |   2 +
>  arch/x86/include/asm/seamldr.h        |   2 +
>  arch/x86/include/asm/tdx.h            |   5 ++
>  arch/x86/virt/vmx/tdx/seamldr.c       |   7 ++
>  drivers/virt/coco/tdx-host/tdx-host.c | 122 +++++++++++++++++++++++++-
>  5 files changed, 137 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 6b47383d2958..2bf4bb3dfe71 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1908,6 +1908,8 @@ config INTEL_TDX_HOST
>  config INTEL_TDX_MODULE_UPDATE
>  	bool "Intel TDX module runtime update"
>  	depends on TDX_HOST_SERVICES
> +	select FW_LOADER
> +	select FW_UPLOAD
>  	help
>  	  This enables the kernel to support TDX module runtime update. This
>  	  allows the admin to update the TDX module to the same or any newer
> diff --git a/arch/x86/include/asm/seamldr.h b/arch/x86/include/asm/seamldr.h
> index d1e9f6e16e8d..692bde5e9bb4 100644
> --- a/arch/x86/include/asm/seamldr.h
> +++ b/arch/x86/include/asm/seamldr.h
> @@ -20,8 +20,10 @@ struct seamldr_info {
>  
>  #ifdef CONFIG_INTEL_TDX_MODULE_UPDATE
>  const struct seamldr_info *seamldr_get_info(void);
> +int seamldr_install_module(const u8 *data, u32 size);
>  #else
>  static inline const struct seamldr_info *seamldr_get_info(void) { return NULL; }
> +static inline int seamldr_install_module(const u8 *data, u32 size) { return -EOPNOTSUPP; }
>  #endif
>  
>  #endif
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 7ad026618a23..2422904079a3 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -107,6 +107,11 @@ int tdx_enable(void);
>  const char *tdx_dump_mce_info(struct mce *m);
>  const struct tdx_sys_info *tdx_get_sysinfo(void);
>  
> +static inline bool tdx_supports_runtime_update(const struct tdx_sys_info *sysinfo)
> +{
> +	return false; /* To be enabled when kernel is ready */
> +}
> +
>  int tdx_guest_keyid_alloc(void);
>  u32 tdx_get_nr_guest_keyids(void);
>  void tdx_guest_keyid_free(unsigned int keyid);
> diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
> index 08c2e3fe6071..69c059194c61 100644
> --- a/arch/x86/virt/vmx/tdx/seamldr.c
> +++ b/arch/x86/virt/vmx/tdx/seamldr.c
> @@ -69,3 +69,10 @@ const struct seamldr_info *seamldr_get_info(void)
>  	return seamldr_call(P_SEAMLDR_INFO, &args) ? NULL : &seamldr_info;
>  }
>  EXPORT_SYMBOL_GPL_FOR_MODULES(seamldr_get_info, "tdx-host");
> +
> +int seamldr_install_module(const u8 *data, u32 size)
> +{
> +	/* TODO: Update TDX Module here */
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL_FOR_MODULES(seamldr_install_module, "tdx-host");
> diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> index 42570c5b221b..418e90797689 100644
> --- a/drivers/virt/coco/tdx-host/tdx-host.c
> +++ b/drivers/virt/coco/tdx-host/tdx-host.c
> @@ -10,6 +10,7 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/sysfs.h>
>  #include <linux/device/faux.h>
> +#include <linux/firmware.h>
>  #include <asm/cpu_device_id.h>
>  #include <asm/seamldr.h>
>  #include <asm/tdx.h>
> @@ -21,6 +22,13 @@ static const struct x86_cpu_id tdx_host_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(x86cpu, tdx_host_ids);
>  
> +struct tdx_fw_upload_status {
> +	bool cancel_request;
> +};
> +
> +struct fw_upload *tdx_fwl;
> +static struct tdx_fw_upload_status tdx_fw_upload_status;
> +
>  static struct faux_device *fdev;

Make the fdev declaration right before tdx_host_init(), try best to keep
the update stuff in one bluk.

[...]

> +static int seamldr_init(struct device *dev)
> +{
> +	const struct seamldr_info *seamldr_info = seamldr_get_info();
> +	const struct tdx_sys_info *tdx_sysinfo = tdx_get_sysinfo();
> +	int ret;
> +
> +	if (!tdx_sysinfo || !seamldr_info)
> +		return -ENXIO;
> +
> +	if (!tdx_supports_runtime_update(tdx_sysinfo)) {
> +		pr_info("Current TDX Module cannot be updated. Consider BIOS updates\n");
> +		return -EOPNOTSUPP;

I don't think we fail out the whole tdx-host here. We should skip the
optional feature if it is not supported to allow other features work.
E.g. the TDX Module version, the P-SEAMLOAD version, TDX Connect.

> +	}
> +
> +	if (!seamldr_info->num_remaining_updates) {
> +		pr_info("P-SEAMLDR doesn't support TDX Module updates\n");
> +		return -EOPNOTSUPP;
> +	}

Ditto. And keeping num_remaining_updates sysfs node visible and returning 0
is valuable, it clearly tells why update is impossible and aligns with
the situation when the user keeps on updating and exhausts the available
updates.

> +
> +	tdx_fwl = firmware_upload_register(THIS_MODULE, dev, "seamldr_upload",
> +					   &tdx_fw_ops, &tdx_fw_upload_status);
> +	ret = PTR_ERR_OR_ZERO(tdx_fwl);
> +	if (ret)
> +		pr_err("failed to register module uploader %d\n", ret);
> +
> +	return ret;
> +}