Re: [PATCH v9 10/23] coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates

[Dave Hansen <dave.hansen@intel.com>]

On 5/13/26 08:09, Chao Gao wrote:
> tl;dr: Select fw_upload for doing TDX module updates. The process of
> selecting among available update images is complicated and nuanced. Punt
> the selection policy out to userspace.

Shouldn't we also say that there is userspace out there to do this
today? Like it's not vaporware. Can we point to it?

> Long Version:
> 
> Linux kernel supports two primary firmware update mechanisms:
>   - request_firmware()
>   - firmware upload (or fw_upload)
> 
> The former is used by microcode updates, SEV firmware updates, etc. The
> latter is used by CXL and FPGA firmware updates.

Gah, another former/latter. Format it like this:

The kernel supports two primary firmware update mechanisms:
 1. request_firmware() - used by microcode, SEV firmware, hundreds of
			 other drivers
 2. 'struct fw_upload' - used by CXL, FPGA updates, dozens of others

Isn't that a billion times easier to parse?

> One key difference between them is: request_firmware() loads a named
> file from the filesystem where the filename is kernel-controlled, while
> fw_upload accepts firmware data directly from userspace.

One more thing to remove from your changelogs: this "is: " construct.
It's horribly awkward for a reader.

This, please:

	The key difference between is that request_firmware() loads a
	named file from the filesystem where the filename is kernel-
	controlled, while fw_upload accepts firmware data directly from
	userspace.

> Use fw_upload for TDX module updates as loading a named file isn't
> suitable for TDX (see below for more reasons). Specifically, register
> TDX faux device with fw_upload framework to expose sysfs interfaces
> and implement operations to process data blobs supplied by userspace.

This is just noise for the justification. We can't do it because it's
not suitable? That's not a reason.

Isn't this better?

	TDX module firmware update selection policy is too complex for
	the kernel. Leave it to userspace and use fw_upload.

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

How about just: The fw_upload cleanly supports both upgrades and
reversions to earlier module versions. TDX users are expected to need to
do both.

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

Not "Some".


x. A given module image can be compatible with several platforms. 1.5.2
   runs on <example 1> and <example 2>
y. Not all modules images are compatible with all platforms. 2.0.x runs
   <example 3> but not <example 1>.
z. A filesystem will have TDX module images for many platforms, the same
   as how /lib/firmware/intel-ucode/ has ucode for many processor
   models.

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

Again, I'd just give an actual example.

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

Here's a flow in userspace that I can imagine:

 1. Find all the available modules
 2. Filter out modules which are incompatible with this system
 3. Find the current running module version
 4. Decide which direction: upgrade or downgrade. Filter out modules
    which are not in the right direction
 5. Filter out modules which have a functionally too distant version
    (1.2.3=>1.2.4 is OK, but going to 1.2.999 is not)
 6. Optimize for fewest updates, or smallest updates. If allowed, go:
    1.2.3=>1.2.5, or 1.2.3=>1.2.4=>1.2.5?

Steps 4 and 6 are _pure_ policy.

> diff --git a/drivers/virt/coco/tdx-host/Kconfig b/drivers/virt/coco/tdx-host/Kconfig
> index d35d85ef91c0..ca600a39d97b 100644
> --- a/drivers/virt/coco/tdx-host/Kconfig
> +++ b/drivers/virt/coco/tdx-host/Kconfig
> @@ -1,6 +1,8 @@
>  config TDX_HOST_SERVICES
>  	tristate "TDX Host Services Driver"
>  	depends on INTEL_TDX_HOST
> +	select FW_LOADER
> +	select FW_UPLOAD
>  	default m
>  	help
>  	  Enable access to TDX host services like module update and

Doesn't this break the compile if FW_LOADER can't be selected? Or does
it error out at Kconfig time. I always forget.

> diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> index a540d658757b..c4c099cf3de1 100644
> --- a/drivers/virt/coco/tdx-host/tdx-host.c
> +++ b/drivers/virt/coco/tdx-host/tdx-host.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/device/faux.h>
> +#include <linux/firmware.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/sysfs.h>
> @@ -84,7 +85,7 @@ static struct attribute *seamldr_attrs[] = {
>  	NULL,
>  };
>  
> -static umode_t seamldr_group_visible(struct kobject *kobj, struct attribute *attr, int idx)
> +static bool supports_runtime_update(void)
>  {
>  	const struct tdx_sys_info *sysinfo = tdx_get_sysinfo();
>  
> @@ -99,7 +100,12 @@ static umode_t seamldr_group_visible(struct kobject *kobj, struct attribute *att
>  	if (boot_cpu_has_bug(X86_BUG_SEAMRET_INVD_VMCS))
>  		return 0;
>  
> -	return tdx_supports_runtime_update(sysinfo) ? attr->mode : 0;
> +	return tdx_supports_runtime_update(sysinfo);
> +}
> +
> +static umode_t seamldr_group_visible(struct kobject *kobj, struct attribute *attr, int idx)
> +{
> +	return supports_runtime_update() ? attr->mode : 0;
>  }
>  
>  static const struct attribute_group seamldr_group = {
> @@ -113,6 +119,81 @@ static const struct attribute_group *tdx_host_groups[] = {
>  	NULL,
>  };
>  
> +static enum fw_upload_err tdx_fw_prepare(struct fw_upload *fwl,
> +					 const u8 *data, u32 size)
> +{
> +	return FW_UPLOAD_ERR_NONE;
> +}
> +
> +static enum fw_upload_err tdx_fw_write(struct fw_upload *fwl, const u8 *data,
> +				       u32 offset, u32 size, u32 *written)
> +{
> +	int ret;
> +
> +	ret = seamldr_install_module(data, size);
> +	switch (ret) {
> +	case 0:
> +		*written = size;
> +		return FW_UPLOAD_ERR_NONE;
> +	default:
> +		return FW_UPLOAD_ERR_FW_INVALID;
> +	}
> +}

This is rather ugly for a single condition. Plus, it puts the error path
and the success path on the same footing. That's not great. How about:

	if (ret)
		return FW_UPLOAD_ERR_FW_INVALID;

	*written = size;
	return FW_UPLOAD_ERR_NONE;