Re: [PATCH v2 07/21] coco/tdx-host: Expose P-SEAMLDR information via sysfs

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

On Tue, Sep 30, 2025 at 07:52:51PM -0700, Chao Gao wrote:
> TDX Module updates require userspace to select the appropriate module
> to load. Expose necessary information to facilitate this decision. Two
> values are needed:
> 
> - P-SEAMLDR version: for compatibility checks between TDX Module and
> 		     P-SEAMLDR
> - num_remaining_updates: indicates how many updates can be performed
> 
> Expose them as tdx-host device attributes.
> 
> Note that P-SEAMLDR sysfs nodes are hidden when INTEL_TDX_MODULE_UPDATE
> isn't enabled or when P-SEAMLDR isn't loaded by BIOS, both of which

I don't think we need to worry about whether P-SEAMLDR is loaded or not.
The tdx-host device exists only if TDX Module is loaded, and in turn
P-SEAMLDR is loaded.

> cause seamldr_get_info() to return NULL.
> 

[...]

> +static ssize_t seamldr_version_show(struct device *dev, struct device_attribute *attr,
> +				    char *buf)
> +{
> +	const struct seamldr_info *info = seamldr_get_info();
> +
> +	if (!info)
> +		return -ENXIO;
> +
> +	return sysfs_emit(buf, "%u.%u.%u\n", info->major_version,
> +					     info->minor_version,
> +					     info->update_version);
> +}
> +
> +static ssize_t num_remaining_updates_show(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	const struct seamldr_info *info = seamldr_get_info();
> +
> +	if (!info)
> +		return -ENXIO;
> +
> +	return sysfs_emit(buf, "%u\n", info->num_remaining_updates);
> +}
> +
> +/*
> + * Open-code DEVICE_ATTR_RO to specify a different 'show' function for
> + * P-SEAMLDR version as version_show() is used for the TDX Module version.
> + */
> +static struct device_attribute dev_attr_seamldr_version =
> +	__ATTR(version, 0444, seamldr_version_show, NULL);
> +static DEVICE_ATTR_RO(num_remaining_updates);
> +
> +static struct attribute *seamldr_attrs[] = {
> +	&dev_attr_seamldr_version.attr,
> +	&dev_attr_num_remaining_updates.attr,
> +	NULL,
> +};
> +
> +static umode_t seamldr_group_is_visible(struct kobject *kobj,
> +					struct attribute *attr, int n)
> +{
> +	return seamldr_get_info() ? attr->mode : 0;

I feel it is a little wierd here, need some explaination why use
seamldr_get_info() for visibility. At first glance, I get the impression
that we don't expose the attributes on 1st seamldr_get_info() failure,
and if 1st read success we expose the attributes, then we return read
failure on 2nd seamldr_get_info() failure. That's the motivation I'm
trying to make the logic simpler.

As you said, the purpose of using seamldr_get_info() here is for the 2
checks:

  1. If INTEL_TDX_MODULE_UPDATE is selected.
  2. If P-SEAMLOAD exists.

But P-SEAMLOAD must exist in tdx-host device context. The chain of
dependency is P-SEAMLOAD->TDX Module->tdx host device.

So the logic could be simplified as "if INTEL_TDX_MODULE_UPDATE
selected, expose seamldr sysfs". A common practice maybe:


#ifdef INTEL_TDX_MODULE_UPDATE
> +static struct attribute_group seamldr_group = {
> +	.name = "seamldr",
> +	.attrs = seamldr_attrs,
> +	.is_visible = seamldr_group_is_visible,

drop is_visible()

> +};
#endif
> +
> +static const struct attribute_group *tdx_host_groups[] = {
> +	&tdx_host_group,

#ifdef INTEL_TDX_MODULE_UPDATE
> +	&seamldr_group,
#endif

The #ifdef should be added for several places, which seems annoying but
may be fine for the first optional feature. Later, could solve this by
splitting the file into tdx-host/main.c tdx-host/update.c
tdx-host/connect.c ...

> +	NULL,
> +};