* Re: [PATCH v5 06/22] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Kiryl Shutsemau @ 2026-03-17 10:06 UTC (permalink / raw)
To: Chao Gao
Cc: x86, linux-coco, kvm, linux-kernel, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, nik.borisov, paulmck, pbonzini,
reinette.chatre, rick.p.edgecombe, sagis, seanjc, tony.lindgren,
vannapurve, vishal.l.verma, yilun.xu
In-Reply-To: <20260315135920.354657-7-chao.gao@intel.com>
On Sun, Mar 15, 2026 at 06:58:26AM -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. Register these attributes
> during device probe rather than creation, making it easier to hide them
> when P-SEAMLDR calls are unsafe (due to CPU erratum).
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
Few nitpicks below.
> ---
> v5:
> - fix typos [Binbin]
> - register seamldr_group during device probe
> v4:
> - Make seamldr attribute permission "0400" [Dave]
> - Don't include implementation details in OS ABI docs [Dave]
> - Tag tdx_host_group as static [Kai]
>
> v3:
> - use #ifdef rather than .is_visible() to control P-SEAMLDR sysfs
> visibility [Yilun]
> ---
> .../ABI/testing/sysfs-devices-faux-tdx-host | 23 +++++++
> drivers/virt/coco/tdx-host/tdx-host.c | 68 ++++++++++++++++++-
> 2 files changed, 90 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-faux-tdx-host b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
> index 2cf682b65acf..44b8356aed6b 100644
> --- a/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
> +++ b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
> @@ -4,3 +4,26 @@ Description: (RO) Report the version of the loaded TDX module. The TDX module
> version is formatted as x.y.z, where "x" is the major version,
> "y" is the minor version and "z" is the update version. Versions
> are used for bug reporting, TDX module updates etc.
> +
> +What: /sys/devices/faux/tdx_host/seamldr/version
> +Contact: linux-coco@lists.linux.dev
> +Description: (RO) Report the version of the loaded SEAM loader. The SEAM
> + loader version is formatted as x.y.z, where "x" is the major
> + version, "y" is the minor version and "z" is the update version.
> + Versions are used for bug reporting and compatibility checks.
> +
> +What: /sys/devices/faux/tdx_host/seamldr/num_remaining_updates
> +Contact: linux-coco@lists.linux.dev
> +Description: (RO) Report the number of remaining updates. TDX maintains a
> + log about each TDX module that has been loaded. This log has
> + a finite size, which limits the number of TDX module updates
> + that can be performed.
> +
> + After each successful update, the number reduces by one. Once it
> + reaches zero, further updates will fail until next reboot. The
> + number is always zero if the P-SEAMLDR doesn't support updates.
> +
> + See Intel® Trust Domain Extensions - SEAM Loader (SEAMLDR)
> + Interface Specification, Revision 343755-003, Chapter 3.3
> + "SEAMLDR_INFO" and Chapter 4.2 "SEAMLDR.INSTALL" for more
> + information.
Do you think revision and chapter numbers useful here?
> diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> index 0424933b2560..8d46e3c039ba 100644
> --- a/drivers/virt/coco/tdx-host/tdx-host.c
> +++ b/drivers/virt/coco/tdx-host/tdx-host.c
> @@ -11,6 +11,7 @@
> #include <linux/sysfs.h>
>
> #include <asm/cpu_device_id.h>
> +#include <asm/seamldr.h>
> #include <asm/tdx.h>
>
> static const struct x86_cpu_id tdx_host_ids[] = {
> @@ -42,6 +43,71 @@ static struct attribute *tdx_host_attrs[] = {
> };
> ATTRIBUTE_GROUPS(tdx_host);
>
> +static ssize_t seamldr_version_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct seamldr_info info;
> + int ret;
> +
> + ret = seamldr_get_info(&info);
> + if (ret)
> + return ret;
> +
> + return sysfs_emit(buf, "%u.%u.%02u\n", info.major_version,
> + info.minor_version,
> + info.update_version);
Maybe a comment why %02u used for update_version?
> +}
> +
> +static ssize_t num_remaining_updates_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct seamldr_info info;
> + int ret;
> +
> + ret = seamldr_get_info(&info);
> + if (ret)
> + return ret;
> +
> + return sysfs_emit(buf, "%u\n", info.num_remaining_updates);
> +}
> +
> +/*
> + * Open-code DEVICE_ATTR_ADMIN_RO to specify a different 'show' function
> + * for P-SEAMLDR version as version_show() is used for TDX module version.
> + *
> + * Admin-only readable as reading these attributes calls into P-SEAMLDR,
> + * which may have potential performance and system impact.
> + */
> +static struct device_attribute dev_attr_seamldr_version =
> + __ATTR(version, 0400, seamldr_version_show, NULL);
> +static DEVICE_ATTR_ADMIN_RO(num_remaining_updates);
> +
> +static struct attribute *seamldr_attrs[] = {
> + &dev_attr_seamldr_version.attr,
> + &dev_attr_num_remaining_updates.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group seamldr_group = {
> + .name = "seamldr",
> + .attrs = seamldr_attrs,
> +};
> +
> +static int seamldr_init(struct device *dev)
> +{
> + return devm_device_add_group(dev, &seamldr_group);
> +}
> +
> +static int tdx_host_probe(struct faux_device *fdev)
> +{
> + return seamldr_init(&fdev->dev);
> +}
> +
> +static const struct faux_device_ops tdx_host_ops = {
> + .probe = tdx_host_probe,
> +};
> +
> static struct faux_device *fdev;
>
> static int __init tdx_host_init(void)
> @@ -49,7 +115,7 @@ static int __init tdx_host_init(void)
> if (!x86_match_cpu(tdx_host_ids) || !tdx_get_sysinfo())
> return -ENODEV;
>
> - fdev = faux_device_create_with_groups(KBUILD_MODNAME, NULL, NULL, tdx_host_groups);
> + fdev = faux_device_create_with_groups(KBUILD_MODNAME, NULL, &tdx_host_ops, tdx_host_groups);
> if (!fdev)
> return -ENODEV;
>
> --
> 2.47.3
>
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v5 07/22] coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates
From: Kiryl Shutsemau @ 2026-03-17 10:20 UTC (permalink / raw)
To: Chao Gao
Cc: linux-kernel, linux-coco, kvm, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, nik.borisov, paulmck, pbonzini,
reinette.chatre, rick.p.edgecombe, sagis, seanjc, tony.lindgren,
vannapurve, vishal.l.verma, yilun.xu, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin
In-Reply-To: <20260315135920.354657-8-chao.gao@intel.com>
On Sun, Mar 15, 2026 at 06:58:27AM -0700, Chao Gao wrote:
> 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.
>
> 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.
>
> 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.
>
> Implementation notes:
> 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. An update completes synchronously within .write(), meaning
> .poll_complete() is only called after the update succeeds and so 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>
> Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
One minor thing below.
> ---
> v5:
> - remove a tail comment [Yan]
> - remove is_vmalloc_addr() check [Dave]
> - use devm_add_action_or_reset() for deinit [Yilun]
> - remove global tdx_fwl [Yilun]
> - clarify request_firmware() doesn't take filename from userspace
> [Rick]
>
> v4:
> - make tdx_fwl static [Kai]
> - don't support update canceling [Yilun]
> - explain why seamldr_init() doesn't return an error [Kai]
> - bail out if TDX module updates are not supported [Kai]
> - name the firmware "tdx_module" instead of "seamldr_upload" [Cedric]
>
> v3:
> - clear "cancel_request" in the "prepare" phase [Binbin]
> - Don't fail the whole tdx-host device if seamldr_init() met an error
> [Yilun]
> - Add kdoc for seamldr_install_module() and verify that the input
> buffer is vmalloc'd. [Yilun]
> ---
> arch/x86/include/asm/seamldr.h | 1 +
> arch/x86/include/asm/tdx.h | 6 ++
> arch/x86/virt/vmx/tdx/seamldr.c | 15 +++++
> drivers/virt/coco/tdx-host/Kconfig | 2 +
> drivers/virt/coco/tdx-host/tdx-host.c | 87 +++++++++++++++++++++++++++
> 5 files changed, 111 insertions(+)
>
> diff --git a/arch/x86/include/asm/seamldr.h b/arch/x86/include/asm/seamldr.h
> index c67e5bc910a9..ac6f80f7208b 100644
> --- a/arch/x86/include/asm/seamldr.h
> +++ b/arch/x86/include/asm/seamldr.h
> @@ -32,5 +32,6 @@ struct seamldr_info {
> static_assert(sizeof(struct seamldr_info) == 256);
>
> int seamldr_get_info(struct seamldr_info *seamldr_info);
> +int seamldr_install_module(const u8 *data, u32 size);
>
> #endif /* _ASM_X86_SEAMLDR_H */
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index cb2219302dfc..b3a7301e77c6 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -103,6 +103,12 @@ 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)
> +{
> + /* To be enabled when kernel is ready. */
> + return false;
> +}
> +
> 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 7c0cbab2c4c0..7114326d7569 100644
> --- a/arch/x86/virt/vmx/tdx/seamldr.c
> +++ b/arch/x86/virt/vmx/tdx/seamldr.c
> @@ -6,6 +6,7 @@
> */
> #define pr_fmt(fmt) "seamldr: " fmt
>
> +#include <linux/mm.h>
> #include <linux/spinlock.h>
>
> #include <asm/seamldr.h>
> @@ -39,3 +40,17 @@ int seamldr_get_info(struct seamldr_info *seamldr_info)
> return seamldr_call(P_SEAMLDR_INFO, &args);
> }
> EXPORT_SYMBOL_FOR_MODULES(seamldr_get_info, "tdx-host");
> +
> +/**
> + * seamldr_install_module - Install a new TDX module.
> + * @data: Pointer to the TDX module update blob.
> + * @size: Size of the TDX module update blob.
> + *
> + * Returns 0 on success, negative error code on failure.
> + */
> +int seamldr_install_module(const u8 *data, u32 size)
> +{
> + /* TODO: Update TDX module here */
> + return 0;
> +}
> +EXPORT_SYMBOL_FOR_MODULES(seamldr_install_module, "tdx-host");
> 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
> diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> index 8d46e3c039ba..1b93d20406c1 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>
> @@ -94,8 +95,94 @@ static const struct attribute_group seamldr_group = {
> .attrs = seamldr_attrs,
> };
>
> +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;
> +
> + /*
> + * tdx_fw_write() always processes all data on the first call with
> + * offset == 0. Since it never returns partial success (it either
> + * succeeds completely or fails), there is no subsequent call with
> + * non-zero offsets.
> + */
> + WARN_ON_ONCE(offset);
> + ret = seamldr_install_module(data, size);
> + switch (ret) {
> + case 0:
> + *written = size;
> + return FW_UPLOAD_ERR_NONE;
> + case -EBUSY:
> + return FW_UPLOAD_ERR_BUSY;
> + case -EIO:
> + return FW_UPLOAD_ERR_HW_ERROR;
> + case -ENOSPC:
> + return FW_UPLOAD_ERR_WEAROUT;
> + case -ENOMEM:
> + return FW_UPLOAD_ERR_RW_ERROR;
> + default:
> + return FW_UPLOAD_ERR_FW_INVALID;
> + }
> +}
> +
> +static enum fw_upload_err tdx_fw_poll_complete(struct fw_upload *fwl)
> +{
> + /*
> + * TDX module updates are completed in the previous phase
> + * (tdx_fw_write()). If any error occurred, the previous phase
> + * would return an error code to abort the update process. In
> + * other words, reaching this point means the update succeeded.
> + */
> + return FW_UPLOAD_ERR_NONE;
> +}
> +
> +/*
> + * TDX module updates cannot be cancelled. Provide a stub function since
> + * the firmware upload framework requires a .cancel operation.
> + */
> +static void tdx_fw_cancel(struct fw_upload *fwl)
> +{
> +}
> +
> +static const struct fw_upload_ops tdx_fw_ops = {
> + .prepare = tdx_fw_prepare,
> + .write = tdx_fw_write,
> + .poll_complete = tdx_fw_poll_complete,
> + .cancel = tdx_fw_cancel,
> +};
> +
> +static void seamldr_deinit(void *tdx_fwl)
> +{
> + firmware_upload_unregister(tdx_fwl);
> +}
> +
> static int seamldr_init(struct device *dev)
> {
> + const struct tdx_sys_info *tdx_sysinfo = tdx_get_sysinfo();
> + struct fw_upload *tdx_fwl;
> + int ret;
> +
> + if (WARN_ON_ONCE(!tdx_sysinfo))
> + return -EIO;
> +
> + if (!tdx_supports_runtime_update(tdx_sysinfo))
> + return 0;
Hm. Do we still want to register seamldr_group for this case?
Maybe move it up before the check?
> +
> + tdx_fwl = firmware_upload_register(THIS_MODULE, dev, "tdx_module",
> + &tdx_fw_ops, NULL);
> + if (IS_ERR(tdx_fwl))
> + return PTR_ERR(tdx_fwl);
> +
> + ret = devm_add_action_or_reset(dev, seamldr_deinit, tdx_fwl);
> + if (ret)
> + return ret;
> +
> return devm_device_add_group(dev, &seamldr_group);
> }
>
> --
> 2.47.3
>
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v5 08/22] x86/virt/seamldr: Allocate and populate a module update request
From: Kiryl Shutsemau @ 2026-03-17 10:59 UTC (permalink / raw)
To: Chao Gao
Cc: linux-kernel, linux-coco, kvm, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, nik.borisov, paulmck, pbonzini,
reinette.chatre, rick.p.edgecombe, sagis, seanjc, tony.lindgren,
vannapurve, vishal.l.verma, yilun.xu, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin
In-Reply-To: <20260315135920.354657-9-chao.gao@intel.com>
On Sun, Mar 15, 2026 at 06:58:28AM -0700, Chao Gao wrote:
> P-SEAMLDR uses the SEAMLDR_PARAMS structure to describe TDX module
> update requests. This structure contains physical addresses pointing to
> the module binary and its signature file (or sigstruct), along with an
> update scenario field.
>
> TDX modules are distributed in the tdx_blob format defined in
> blob_structure.txt from the "Intel TDX module Binaries Repository". A
> tdx_blob contains a header, sigstruct, and module binary. This is also the
> format supplied by the userspace to the kernel.
>
> Parse the tdx_blob format and populate a SEAMLDR_PARAMS structure
> accordingly. This structure will be passed to P-SEAMLDR to initiate the
> update.
>
> Note that the sigstruct_pa field in SEAMLDR_PARAMS has been extended to
> a 4-element array. The updated "SEAM Loader (SEAMLDR) Interface
> Specification" will be published separately. P-SEAMLDR compatibility
> validation (such as 4KB vs 16KB sigstruct support) is left to userspace,
> which must verify the P-SEAMLDR version meets the TDX module's minimum
> requirements.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
> ---
> v5:
> - use a macro for tdx_blob version (0x100) [Yan]
> - don't do alignment checking for the binary/sigstruct [Rick]
> - drop blob's sigstruct and validation checking
> - set seamldr_params.version to 1 when necessary
> - drop the link to blob_structure.txt which might be unstable [Kai]
>
> v4:
> - Remove checksum verification as it is optional
> - Convert comments to is_vmalloc_addr() checks [Kai]
> - Explain size/alignment checks in alloc_seamldr_params() [Kai]
>
> v3:
> - Print tdx_blob version in hex [Binbin]
> - Drop redundant sigstruct alignment check [Yilun]
> - Note buffers passed from firmware upload infrastructure are
> vmalloc()'d above alloc_seamldr_params()
> ---
> arch/x86/virt/vmx/tdx/seamldr.c | 141 ++++++++++++++++++++++++++++++++
> 1 file changed, 141 insertions(+)
>
> diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
> index 7114326d7569..20cb6c797ce5 100644
> --- a/arch/x86/virt/vmx/tdx/seamldr.c
> +++ b/arch/x86/virt/vmx/tdx/seamldr.c
> @@ -7,6 +7,7 @@
> #define pr_fmt(fmt) "seamldr: " fmt
>
> #include <linux/mm.h>
> +#include <linux/slab.h>
> #include <linux/spinlock.h>
>
> #include <asm/seamldr.h>
> @@ -16,6 +17,33 @@
> /* P-SEAMLDR SEAMCALL leaf function */
> #define P_SEAMLDR_INFO 0x8000000000000000
>
> +#define SEAMLDR_MAX_NR_MODULE_4KB_PAGES 496
> +#define SEAMLDR_MAX_NR_SIG_4KB_PAGES 4
> +
> +/*
> + * The seamldr_params "scenario" field specifies the operation mode:
> + * 0: Install TDX module from scratch (not used by kernel)
> + * 1: Update existing TDX module to a compatible version
> + */
> +#define SEAMLDR_SCENARIO_UPDATE 1
> +
> +/*
> + * This is called the "SEAMLDR_PARAMS" data structure and is defined
> + * in "SEAM Loader (SEAMLDR) Interface Specification".
> + *
> + * It describes the TDX module that will be installed.
> + */
> +struct seamldr_params {
> + u32 version;
> + u32 scenario;
> + u64 sigstruct_pa[SEAMLDR_MAX_NR_SIG_4KB_PAGES];
> + u8 reserved[80];
> + u64 num_module_pages;
> + u64 mod_pages_pa_list[SEAMLDR_MAX_NR_MODULE_4KB_PAGES];
> +} __packed;
> +
> +static_assert(sizeof(struct seamldr_params) == 4096);
> +
> /*
> * Serialize P-SEAMLDR calls since the hardware only allows a single CPU to
> * interact with P-SEAMLDR simultaneously. Use raw version as the calls can
> @@ -41,6 +69,114 @@ int seamldr_get_info(struct seamldr_info *seamldr_info)
> }
> EXPORT_SYMBOL_FOR_MODULES(seamldr_get_info, "tdx-host");
>
> +static void free_seamldr_params(struct seamldr_params *params)
> +{
> + free_page((unsigned long)params);
> +}
> +
> +static struct seamldr_params *alloc_seamldr_params(const void *module, unsigned int module_size,
> + const void *sig, unsigned int sig_size)
> +{
> + struct seamldr_params *params;
> + const u8 *ptr;
> + int i;
> +
> + if (module_size > SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K)
> + return ERR_PTR(-EINVAL);
> +
> + if (sig_size > SEAMLDR_MAX_NR_SIG_4KB_PAGES * SZ_4K)
> + return ERR_PTR(-EINVAL);
> +
> + params = (struct seamldr_params *)get_zeroed_page(GFP_KERNEL);
> + if (!params)
> + return ERR_PTR(-ENOMEM);
> +
> + /*
> + * Only use version 1 when required (sigstruct > 4KB) for backward
> + * compatibility with P-SEAMLDR that lacks version 1 support.
> + */
> + if (sig_size > SZ_4K)
> + params->version = 1;
> + else
> + params->version = 0;
> +
> + params->scenario = SEAMLDR_SCENARIO_UPDATE;
> +
> + ptr = sig;
> + for (i = 0; i < sig_size / SZ_4K; i++) {
> + /*
> + * Don't assume @sig is page-aligned although it is 4KB-aligned.
> + * Always add the in-page offset to get the physical address.
> + */
I don't follow this. If @sig is 4k aligned in VA, it is page aligned.
If you want to handle case when @sig is not 4k aligned, than this is
broken. You need to bump ptr to the next 4k boundary, not by 4k.
> + params->sigstruct_pa[i] = (vmalloc_to_pfn(ptr) << PAGE_SHIFT) +
> + ((unsigned long)ptr & ~PAGE_MASK);
> + ptr += SZ_4K;
> + }
> +
> + params->num_module_pages = module_size / SZ_4K;
> +
> + ptr = module;
> + for (i = 0; i < params->num_module_pages; i++) {
> + params->mod_pages_pa_list[i] = (vmalloc_to_pfn(ptr) << PAGE_SHIFT) +
> + ((unsigned long)ptr & ~PAGE_MASK);
> + ptr += SZ_4K;
Same here.
> + }
> +
> + return params;
> +}
> +
> +/*
> + * Intel TDX module blob. Its format is defined at:
> + * https://github.com/intel/tdx-module-binaries/blob/main/blob_structure.txt
> + *
> + * Note this structure differs from the reference above: the two variable-length
> + * fields "@sigstruct" and "@module" are represented as a single "@data" field
> + * here and split programmatically using the offset_of_module value.
> + */
> +struct tdx_blob {
> + u16 version;
> + u16 checksum;
> + u32 offset_of_module;
> + u8 signature[8];
> + u32 length;
> + u32 reserved0;
> + u64 reserved1[509];
> + u8 data[];
> +} __packed;
> +
> +/* Supported versions of the tdx_blob */
> +#define TDX_BLOB_VERSION_1 0x100
> +
> +static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
> +{
> + const struct tdx_blob *blob = (const void *)data;
> + int module_size, sig_size;
> + const void *sig, *module;
> +
> + /* Ensure the size is valid otherwise reading any field from the blob may overflow. */
> + if (size <= sizeof(struct tdx_blob) || size <= blob->offset_of_module)
> + return ERR_PTR(-EINVAL);
> +
> + if (blob->version != TDX_BLOB_VERSION_1) {
> + pr_err("unsupported blob version: %x\n", blob->version);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + /* Split the blob into a sigstruct and a module. */
> + sig = blob->data;
> + sig_size = blob->offset_of_module - sizeof(struct tdx_blob);
> + module = data + blob->offset_of_module;
> + module_size = size - blob->offset_of_module;
> +
> + if (sig_size <= 0 || module_size <= 0 || blob->length != size)
> + return ERR_PTR(-EINVAL);
Maybe add a comment somewhere that block->offset_of_module is relative
to start of struct tdx_blob, not blob->data and blob->length includes
length of struct tdx_blob.
It can be either way and it is better to give a reader a hint.
> +
> + return alloc_seamldr_params(module, module_size, sig, sig_size);
> +}
> +
> +DEFINE_FREE(free_seamldr_params, struct seamldr_params *,
> + if (!IS_ERR_OR_NULL(_T)) free_seamldr_params(_T))
> +
> /**
> * seamldr_install_module - Install a new TDX module.
> * @data: Pointer to the TDX module update blob.
> @@ -50,6 +186,11 @@ EXPORT_SYMBOL_FOR_MODULES(seamldr_get_info, "tdx-host");
> */
> int seamldr_install_module(const u8 *data, u32 size)
> {
> + struct seamldr_params *params __free(free_seamldr_params) =
> + init_seamldr_params(data, size);
> + if (IS_ERR(params))
> + return PTR_ERR(params);
> +
> /* TODO: Update TDX module here */
> return 0;
> }
> --
> 2.47.3
>
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v5 12/22] x86/virt/tdx: Reset software states during TDX module shutdown
From: Huang, Kai @ 2026-03-17 11:08 UTC (permalink / raw)
To: Gao, Chao
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
yilun.xu@linux.intel.com, dave.hansen@linux.intel.com,
bp@alien8.de, tony.lindgren@linux.intel.com, mingo@redhat.com,
Weiny, Ira, pbonzini@redhat.com, seanjc@google.com,
Verma, Vishal L, nik.borisov@suse.com, binbin.wu@linux.intel.com,
kas@kernel.org, sagis@google.com, Annapurve, Vishal,
hpa@zytor.com, Edgecombe, Rick P, linux-kernel@vger.kernel.org,
Chatre, Reinette, tglx@kernel.org, paulmck@kernel.org,
x86@kernel.org, Williams, Dan J
In-Reply-To: <abkQWsqR7BURMZxc@intel.com>
On Tue, 2026-03-17 at 16:27 +0800, Gao, Chao wrote:
> On Mon, Mar 16, 2026 at 05:06:49PM +0800, Huang, Kai wrote:
> >
> > > @@ -1179,6 +1179,7 @@ EXPORT_SYMBOL_FOR_KVM(tdx_enable);
> > > int tdx_module_shutdown(void)
> > > {
> > > struct tdx_module_args args = {};
> > > + int ret, cpu;
> > >
> > > /*
> > > * Shut down the TDX module and prepare handoff data for the next
> > > @@ -1188,7 +1189,22 @@ int tdx_module_shutdown(void)
> > > * modules as new modules likely have higher handoff version.
> > > */
> > > args.rcx = tdx_sysinfo.handoff.module_hv;
> > > - return seamcall_prerr(TDH_SYS_SHUTDOWN, &args);
> > > + ret = seamcall_prerr(TDH_SYS_SHUTDOWN, &args);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + tdx_module_status = TDX_MODULE_UNINITIALIZED;
> > > + sysinit_done = false;
> > > + sysinit_ret = 0;
> > > +
> > > + /*
> > > + * By reaching here CPUHP is disabled and all present CPUs
> > > + * are online. It's safe to just loop all online CPUs and
> > > + * reset the per-cpu flag.
> > > + */
> > > + for_each_online_cpu(cpu)
> > > + per_cpu(tdx_lp_initialized, cpu) = false;
> >
> > Since you have removed the requirement that P-SEAMLDR.INSTALL must be done
> > on all CPUs, and removed the relevant patch, the "all present CPUs are
> > online" part isn't correct anymore.
> >
> > And using for_each_online_cpu() isn't enough since this doesn't reset the
> > tdx_lp_initialized for offline CPUs.
> >
> > One way is to just use for_each_possible_cpu() here so tdx_lp_initialized
> > for all CPUs are reset. Since the "CPUHP is disabled" part is still correct
> > AFAICT (since stop_machine() disables CPUHP internally during the
> > operation), resetting tdx_lp_initialized for offline CPUs won't race with
> > CPUHP.
> >
> > And assuming this series will be applied after Sean's VMXON series, we will
>
> Yes.
>
> > have a TDX-specific CPUHP callback tdx_online_cpu() in TDX x86 core to do
> > tdx_cpu_enable(), which will then enable TDX again on the new-online CPU.
>
> Good point.
>
> Clearing tdx_lp_initialized for offlined CPUs makes sense, but I'd rather not
> justify this through "enabling TDX on new-online CPUs" since many details
> remain unclear. For example, there will be a SEAMCALL to disable TDX per-CPU.
Sorry which SEAMCALL are you referring to?
> It should be called when CPUs go offline so that those CPUs can be exempting
> from doing SEAMLDR.INSTALL during module updates. tdx_lp_initialized should
> have been cleared along with that "disable TDX per-CPU" SEAMCALL for offlined
> CPUs.
Hmm.. It sounds like we are missing some background here. Could you
elaborate what are the steps to update module on a subset of CPUs?
^ permalink raw reply
* Re: [PATCH net-next v3 0/2] dma-buf: heaps: system: add an option to allocate explicitly decrypted memory
From: Mostafa Saleh @ 2026-03-17 13:24 UTC (permalink / raw)
To: Jiri Pirko
Cc: dri-devel, linaro-mm-sig, iommu, linux-media, sumit.semwal,
benjamin.gaignard, Brian.Starkey, jstultz, tjmercier,
christian.koenig, m.szyprowski, robin.murphy, jgg, leon,
sean.anderson, ptesarik, catalin.marinas, aneesh.kumar,
suzuki.poulose, steven.price, thomas.lendacky, john.allen,
ashish.kalra, suravee.suthikulpanit, linux-coco
In-Reply-To: <20260305123641.164164-1-jiri@resnulli.us>
Hi Jiri,
On Thu, Mar 05, 2026 at 01:36:39PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
>
> Confidential computing (CoCo) VMs/guests, such as AMD SEV and Intel TDX,
> run with encrypted/protected memory which creates a challenge
> for devices that do not support DMA to it (no TDISP support).
>
> For kernel-only DMA operations, swiotlb bounce buffering provides a
> transparent solution by copying data through decrypted memory.
> However, the only way to get this memory into userspace is via the DMA
> API's dma_alloc_pages()/dma_mmap_pages() type interfaces which limits
> the use of the memory to a single DMA device, and is incompatible with
> pin_user_pages().
>
> These limitations are particularly problematic for the RDMA subsystem
> which makes heavy use of pin_user_pages() and expects flexible memory
> usage between many different DMA devices.
>
> This patch series enables userspace to explicitly request decrypted
> (shared) memory allocations from the dma-buf system heap.
> Userspace can mmap this memory and pass the dma-buf fd to other
> existing importers such as RDMA or DRM devices to access the
> memory. The DMA API is improved to allow the dma heap exporter to DMA
> map the shared memory to each importing device.
I have been looking into a similar problem with restricted-dma[1] and
the inability of the DMA API to recognize that a block of memory is
already decrypted.
However, in your case, adding a new attr “DMA_ATTR_CC_DECRYPTED” works
well as dma-buf owns the memory, and is both responsible for the
set_memory_decrypted() and passing the DMA attrs.
On the other hand, for restricted-dma, the memory decryption is deep
in the DMA direct memory allocation and the DMA API callers (for ex
virtio drivers) are clueless about it and can’t pass any attrs.
My proposal was specific to restricted-dma and won’t work for your case.
I am wondering if the kernel should have a more solid, unified method
for identifying already-decrypted memory instead. Perhaps we need a
way for the DMA API to natively recognize the encryption state of a
physical page (working alongside force_dma_unencrypted(dev)), rather
than relying on caller-provided attributes?
[1] https://lore.kernel.org/all/20260305170335.963568-1-smostafa@google.com/
Thanks,
Mostafa
>
> Jiri Pirko (2):
> dma-mapping: introduce DMA_ATTR_CC_DECRYPTED for pre-decrypted memory
> dma-buf: heaps: system: add system_cc_decrypted heap for explicitly
> decrypted memory
>
> drivers/dma-buf/heaps/system_heap.c | 103 ++++++++++++++++++++++++++--
> include/linux/dma-mapping.h | 6 ++
> include/trace/events/dma.h | 3 +-
> kernel/dma/direct.h | 14 +++-
> 4 files changed, 117 insertions(+), 9 deletions(-)
>
> --
> 2.51.1
>
^ permalink raw reply
* Re: [PATCH net-next v3 0/2] dma-buf: heaps: system: add an option to allocate explicitly decrypted memory
From: Jiri Pirko @ 2026-03-17 13:37 UTC (permalink / raw)
To: Mostafa Saleh
Cc: dri-devel, linaro-mm-sig, iommu, linux-media, sumit.semwal,
benjamin.gaignard, Brian.Starkey, jstultz, tjmercier,
christian.koenig, m.szyprowski, robin.murphy, jgg, leon,
sean.anderson, ptesarik, catalin.marinas, aneesh.kumar,
suzuki.poulose, steven.price, thomas.lendacky, john.allen,
ashish.kalra, suravee.suthikulpanit, linux-coco
In-Reply-To: <ablV_f_l7wD2m63E@google.com>
Tue, Mar 17, 2026 at 02:24:13PM +0100, smostafa@google.com wrote:
>Hi Jiri,
>
>On Thu, Mar 05, 2026 at 01:36:39PM +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>>
>> Confidential computing (CoCo) VMs/guests, such as AMD SEV and Intel TDX,
>> run with encrypted/protected memory which creates a challenge
>> for devices that do not support DMA to it (no TDISP support).
>>
>> For kernel-only DMA operations, swiotlb bounce buffering provides a
>> transparent solution by copying data through decrypted memory.
>> However, the only way to get this memory into userspace is via the DMA
>> API's dma_alloc_pages()/dma_mmap_pages() type interfaces which limits
>> the use of the memory to a single DMA device, and is incompatible with
>> pin_user_pages().
>>
>> These limitations are particularly problematic for the RDMA subsystem
>> which makes heavy use of pin_user_pages() and expects flexible memory
>> usage between many different DMA devices.
>>
>> This patch series enables userspace to explicitly request decrypted
>> (shared) memory allocations from the dma-buf system heap.
>> Userspace can mmap this memory and pass the dma-buf fd to other
>> existing importers such as RDMA or DRM devices to access the
>> memory. The DMA API is improved to allow the dma heap exporter to DMA
>> map the shared memory to each importing device.
>
>I have been looking into a similar problem with restricted-dma[1] and
>the inability of the DMA API to recognize that a block of memory is
>already decrypted.
>
>However, in your case, adding a new attr “DMA_ATTR_CC_DECRYPTED” works
>well as dma-buf owns the memory, and is both responsible for the
>set_memory_decrypted() and passing the DMA attrs.
>
>On the other hand, for restricted-dma, the memory decryption is deep
>in the DMA direct memory allocation and the DMA API callers (for ex
>virtio drivers) are clueless about it and can’t pass any attrs.
>My proposal was specific to restricted-dma and won’t work for your case.
>
>I am wondering if the kernel should have a more solid, unified method
>for identifying already-decrypted memory instead. Perhaps we need a
>way for the DMA API to natively recognize the encryption state of a
>physical page (working alongside force_dma_unencrypted(dev)), rather
>than relying on caller-provided attributes?
I actually had it originally implemented probably in the similar way you
suggest. I had a bit in page/folio struct to indicate the
"shared/decrypted" state. However I was told that adding such bit is
basically a no-go. Isn't that right?
>
>[1] https://lore.kernel.org/all/20260305170335.963568-1-smostafa@google.com/
>
>Thanks,
>Mostafa
>
>
>>
>> Jiri Pirko (2):
>> dma-mapping: introduce DMA_ATTR_CC_DECRYPTED for pre-decrypted memory
>> dma-buf: heaps: system: add system_cc_decrypted heap for explicitly
>> decrypted memory
>>
>> drivers/dma-buf/heaps/system_heap.c | 103 ++++++++++++++++++++++++++--
>> include/linux/dma-mapping.h | 6 ++
>> include/trace/events/dma.h | 3 +-
>> kernel/dma/direct.h | 14 +++-
>> 4 files changed, 117 insertions(+), 9 deletions(-)
>>
>> --
>> 2.51.1
>>
^ permalink raw reply
* Re: [PATCH v2 08/19] PCI/TSM: Add "evidence" support
From: Lukas Wunner @ 2026-03-17 14:13 UTC (permalink / raw)
To: Dan Williams
Cc: linux-coco, linux-pci, gregkh, aik, aneesh.kumar, yilun.xu,
bhelgaas, alistair23, jgg, Donald Hunter, Jakub Kicinski
In-Reply-To: <69b88bfe2662f_40c01002c@dwillia2-mobl4.notmuch>
On Mon, Mar 16, 2026 at 04:02:22PM -0700, Dan Williams wrote:
> Dan Williams wrote:
> > Lukas Wunner wrote:
> > > This doesn't look like it's ever been tested, so at the very least
> > > it should be marked RFC in the subject to convey that it's not yet
> > > in a cut-and-dried state.
> >
> > The 16MB limit has indeed not been tested, the test script in this set
> > was using smaller than 64K payloads to check out the interface.
>
> So 16MB works ok, slow, but works. A given attribute in this
> implementation never exceeds the limit
Famous last words.
If you look at netlink_dump(), it sizes the skb based on
nlk->max_recvmsg_len. If that's larger than 64k, you'll
try to fill as much as possible of that space with a single
netlink attribute. The computation of "available" in your
patch doesn't take the 65531 bytes limit for a netlink attribute
into account so it looks like you'll end up overflowing the length
of the netlink attribute.
Unfortunately nla_put() doesn't prevent such overflows, it does
all the size calculations with an int, not a u16.
Thanks,
Lukas
^ permalink raw reply
* Re: [PATCH v5 06/22] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Dave Hansen @ 2026-03-17 15:34 UTC (permalink / raw)
To: Kiryl Shutsemau, Chao Gao
Cc: x86, linux-coco, kvm, linux-kernel, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, nik.borisov, paulmck, pbonzini,
reinette.chatre, rick.p.edgecombe, sagis, seanjc, tony.lindgren,
vannapurve, vishal.l.verma, yilun.xu
In-Reply-To: <abkmceqUtMTnLI0V@thinkstation>
On 3/17/26 03:06, Kiryl Shutsemau wrote:
>> + return sysfs_emit(buf, "%u.%u.%02u\n", info.major_version,
>> + info.minor_version,
>> + info.update_version);
> Maybe a comment why %02u used for update_version?
Could we just stick this in a:
#define TDX_VERSION_FMT "%u.%u.%02u"
and document it there, once and for all?
Users can do:
return sysfs_emit(buf, TDX_VERSION_FMT "\n", ...
There are going to be a couple of these, I expect.
^ permalink raw reply
* Re: [PATCH net-next v3 0/2] dma-buf: heaps: system: add an option to allocate explicitly decrypted memory
From: Mostafa Saleh @ 2026-03-17 15:40 UTC (permalink / raw)
To: Jiri Pirko
Cc: dri-devel, linaro-mm-sig, iommu, linux-media, sumit.semwal,
benjamin.gaignard, Brian.Starkey, jstultz, tjmercier,
christian.koenig, m.szyprowski, robin.murphy, jgg, leon,
sean.anderson, ptesarik, catalin.marinas, aneesh.kumar,
suzuki.poulose, steven.price, thomas.lendacky, john.allen,
ashish.kalra, suravee.suthikulpanit, linux-coco
In-Reply-To: <xdy5anped2koy47cuxbbqocyypisl7lagwvpuokpzpggohk2dp@yilc5ihictph>
On Tue, Mar 17, 2026 at 02:37:02PM +0100, Jiri Pirko wrote:
> Tue, Mar 17, 2026 at 02:24:13PM +0100, smostafa@google.com wrote:
> >Hi Jiri,
> >
> >On Thu, Mar 05, 2026 at 01:36:39PM +0100, Jiri Pirko wrote:
> >> From: Jiri Pirko <jiri@nvidia.com>
> >>
> >> Confidential computing (CoCo) VMs/guests, such as AMD SEV and Intel TDX,
> >> run with encrypted/protected memory which creates a challenge
> >> for devices that do not support DMA to it (no TDISP support).
> >>
> >> For kernel-only DMA operations, swiotlb bounce buffering provides a
> >> transparent solution by copying data through decrypted memory.
> >> However, the only way to get this memory into userspace is via the DMA
> >> API's dma_alloc_pages()/dma_mmap_pages() type interfaces which limits
> >> the use of the memory to a single DMA device, and is incompatible with
> >> pin_user_pages().
> >>
> >> These limitations are particularly problematic for the RDMA subsystem
> >> which makes heavy use of pin_user_pages() and expects flexible memory
> >> usage between many different DMA devices.
> >>
> >> This patch series enables userspace to explicitly request decrypted
> >> (shared) memory allocations from the dma-buf system heap.
> >> Userspace can mmap this memory and pass the dma-buf fd to other
> >> existing importers such as RDMA or DRM devices to access the
> >> memory. The DMA API is improved to allow the dma heap exporter to DMA
> >> map the shared memory to each importing device.
> >
> >I have been looking into a similar problem with restricted-dma[1] and
> >the inability of the DMA API to recognize that a block of memory is
> >already decrypted.
> >
> >However, in your case, adding a new attr “DMA_ATTR_CC_DECRYPTED” works
> >well as dma-buf owns the memory, and is both responsible for the
> >set_memory_decrypted() and passing the DMA attrs.
> >
> >On the other hand, for restricted-dma, the memory decryption is deep
> >in the DMA direct memory allocation and the DMA API callers (for ex
> >virtio drivers) are clueless about it and can’t pass any attrs.
> >My proposal was specific to restricted-dma and won’t work for your case.
> >
> >I am wondering if the kernel should have a more solid, unified method
> >for identifying already-decrypted memory instead. Perhaps we need a
> >way for the DMA API to natively recognize the encryption state of a
> >physical page (working alongside force_dma_unencrypted(dev)), rather
> >than relying on caller-provided attributes?
>
> I actually had it originally implemented probably in the similar way you
> suggest. I had a bit in page/folio struct to indicate the
> "shared/decrypted" state. However I was told that adding such bit is
> basically a no-go. Isn't that right?
>
Yes, I believe it’s discouraged to add new fields to the struct page.
But I see the memory encryption API is spilling in different places
and I am not sure if that’s a good enough justification for that or
maybe we just need to re-architect it.
For the restricted-dma stuff, we don’t actually care about the
address, a device can either handle encryption or not, so relying on
force_dma_unencrypted(struct device *) which is implemented by the
architecture is enough, and we just need to integrate that so it
can be used from SWIOTLB and DMA-direct (and other places)
consistently. (although that might not be a simple as it sounds)
I am not sure in the dma-buf case if that would be enough, but
another way to have this per page and to avoid encoding this in
struct page, is to push this problem to the arch code and it can
rely on things as the page table (I believe ARM CCA have a bit
for that)
Anyway, I think there should be some boundaries in the kernel that
defines that instead of each subsystem having its assumptions,
especially memory encryption/decryption problems that can easily
cause security issues.
Thanks,
Mostafa
>
> >
> >[1] https://lore.kernel.org/all/20260305170335.963568-1-smostafa@google.com/
> >
> >Thanks,
> >Mostafa
> >
> >
> >>
> >> Jiri Pirko (2):
> >> dma-mapping: introduce DMA_ATTR_CC_DECRYPTED for pre-decrypted memory
> >> dma-buf: heaps: system: add system_cc_decrypted heap for explicitly
> >> decrypted memory
> >>
> >> drivers/dma-buf/heaps/system_heap.c | 103 ++++++++++++++++++++++++++--
> >> include/linux/dma-mapping.h | 6 ++
> >> include/trace/events/dma.h | 3 +-
> >> kernel/dma/direct.h | 14 +++-
> >> 4 files changed, 117 insertions(+), 9 deletions(-)
> >>
> >> --
> >> 2.51.1
> >>
^ permalink raw reply
* SVSM Development Call March 18, 2026
From: Jörg Rödel @ 2026-03-17 17:06 UTC (permalink / raw)
To: coconut-svsm, linux-coco
Hi,
Here is the call for agenda items for this weeks SVSM development call. Please
send any agenda items you have in mind as a reply to this email or raise them
in the meeting.
We will use the LF Zoom instance. Details of the meeting can be found in our
governance repository at:
https://github.com/coconut-svsm/governance
The link to the COCONUT-SVSM calendar is:
https://zoom-lfx.platform.linuxfoundation.org/meetings/coconut-svsm?view=week
The meeting will be recorded and the recording eventually published.
Regards,
Jörg
^ permalink raw reply
* Re: [PATCH v2 08/19] PCI/TSM: Add "evidence" support
From: Lukas Wunner @ 2026-03-17 18:14 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Dan Williams, linux-coco, linux-pci, gregkh, aik, aneesh.kumar,
yilun.xu, bhelgaas, alistair23, jgg, Donald Hunter
In-Reply-To: <20260314111245.76d18d73@kernel.org>
On Sat, Mar 14, 2026 at 11:12:45AM -0700, Jakub Kicinski wrote:
> On Mon, 2 Mar 2026 16:01:56 -0800 Dan Williams wrote:
> > The implementation adheres to the guideline from:
> > Documentation/userspace-api/netlink/genetlink-legacy.rst
> >
> > New Netlink families should never respond to a DO operation with
> > multiple replies, with ``NLM_F_MULTI`` set. Use a filtered dump
> > instead.
>
> My understanding of F_MULTI is that deserializer is supposed to
> continue deserializing into current object.
So is the "should" above meant to be understood in the RFC 2119 way,
i.e. as a mere recommendation?
The problem we're facing is that nlattr::nla_len is u16, so the maximum
size is 65531 bytes (65535 minus header). That's insufficient for
transmitting blobs that are several megabytes in size.
The obvious solution is to split the blobs into smaller chunks and
transmit each chunk in an attribute of the same type. The application
then concatenates them together to reconstruct the blob. For particularly
large blobs, it may even be necessary to split across multiple messages
by way of NLM_F_MULTI.
Apart from the attribute size limitation, there's the problem that copying
large blobs in memory is inefficient. Ideally we'd want zero-copy.
The solution I came up with is to attach the blob's pages as fragments
to the skb. Conceptually the fragments succeed the linear buffer of the
skb, so by putting the nlattr header into the linear buffer and attaching
the blob as fragments, the receiver consumes the netlink message in a
natural way. This patch introduces an nla_put_blob() helper which was
pretty straightforward:
https://github.com/l1k/linux/commit/af9b939fc30b
This patch is taking advantage of the helper:
https://github.com/l1k/linux/commit/009663bd172e
The only change I had to make is amending nlmsg_end() to take the
fragments into account when calculating the nlmsg_len.
The patch does achieve zero-copy on the sender's end. It may also
achieve zero-copy on the receiver's end if the receiver is in the
kernel. However it does *not* achieve zero-copy if the receiver is
in user space. That's because:
simple_copy_to_iter()
copy_to_iter()
_copy_to_iter()
copy_to_user_iter()
raw_copy_to_user()
... will just stupidly copy the data into the user space buffer.
It might be possible to achieve zero-copy in user space via io_uring.
At this point perhaps your conclusion is that netlink isn't the right
protocol for this job. It's great for transmitting sets of small items,
some of which may be optional, but it's obviously not well-suited for
large items.
Jason Gunthorpe was quite insistent that we use netlink and you know
how consensus-oriented kernel development is. Indeed sysfs has turned
out not to be ideal because the protocol that we're dealing with
(SPDM - DMTF DSP0274) allows many degrees of freedom and making
them available through sysfs quickly becomes unwieldy.
E.g. when installing a certificate onto a device, the protocol allows
specifying additional parameters (a keypair ID and a certificate model)
together with the certificate chain that shall be installed. That doesn't
square well with the "one value per file" sysfs model. User space would
have to write the keypair ID and certificate model to separate attributes,
then write the certificate chain to a third attribute. So the kernel would
need some kind of state machine to keep track of which sysfs attributes
have been written. It gets quite ugly.
As another example, the SPDM protocol allows retrieving measurements
from the device. The measurements are indexed by an 8-bit number.
To expose them via sysfs, the kernel would have to retrieve all of them
on device enumeration so that it knows which indices are populated
and need to be exposed in sysfs. That would incur a delay on device
enumeration and thus lead to slower boot times.
If netlink is at all the right protocol for the job, I'm wondering if an
extension for larger attributes would be entertained. Basically a
variation of struct nlattr, but with a 24-bit or 32-bit size and
maybe a list of fragment numbers. The latter would be useful to have
*multiple* zero-copy attributes because the patches linked above only
allow for a single zero-copy attribute per nlmsg.
Thanks,
Lukas
^ permalink raw reply
* Re: [PATCH v2 08/19] PCI/TSM: Add "evidence" support
From: Lukas Wunner @ 2026-03-17 18:24 UTC (permalink / raw)
To: Dan Williams
Cc: linux-coco, linux-pci, gregkh, aik, aneesh.kumar, yilun.xu,
bhelgaas, alistair23, jgg, Donald Hunter, Jakub Kicinski
In-Reply-To: <20260303000207.1836586-9-dan.j.williams@intel.com>
On Mon, Mar 02, 2026 at 04:01:56PM -0800, Dan Williams wrote:
> + type: const
> + name: max-nonce-size
> + value: 256
[...]
> +#define PCI_TSM_MAX_OBJECT_SIZE 16777216
> +#define PCI_TSM_MAX_NONCE_SIZE 256
> +#define PCI_TSM_MAX_OBJ_TYPE 4
Where is the maximum nonce size of 256 bytes coming from?
Such definitions should always be accompanied by a spec reference,
not pulled out of thin air.
SPDM nonces are 32 bytes, I assume that's what we're dealing with here?
This patch:
https://github.com/l1k/linux/commit/bca645e08ee9
... contains the following definition:
#define SPDM_NONCE_SZ 32 /* SPDM 1.0.0 table 20 */
Though it's defined in a private header in lib/spdm/spdm.h. If there's
a need outside of the SPDM library, its visibility can be broadened
of course.
Thanks,
Lukas
^ permalink raw reply
* Re: [PATCH 3/4] x86/virt/tdx: Add SEAMCALL wrapper for TDH.SYS.DISABLE
From: Edgecombe, Rick P @ 2026-03-17 21:55 UTC (permalink / raw)
To: kas@kernel.org
Cc: pbonzini@redhat.com, Hansen, Dave, seanjc@google.com,
bp@alien8.de, ackerleytng@google.com, hpa@zytor.com,
linux-kernel@vger.kernel.org, mingo@redhat.com, x86@kernel.org,
tglx@kernel.org, kvm@vger.kernel.org, linux-coco@lists.linux.dev,
Huang, Kai, Verma, Vishal L, Gao, Chao
In-Reply-To: <abkdNQnvgJWmaKWk@thinkstation>
On Tue, 2026-03-17 at 09:47 +0000, Kiryl Shutsemau wrote:
> We debated checking the feature bit before allowing kexec, but decided it was
> > simpler to just blindly call and ignore the errors. The reasoning was that this
> > is already a somewhat exotic scenario being addressed, and future modules will
> > have the feature. So maintaining a check for the feature bit only helps a little
> > bit, for a short time. And then only if the user would rather have kexec blocked
> > than attempt it. Do you think it is worth it?
>
> No, I see very limited reason to support stale TDX modules. Users are
> expected to keep the module up-to-date, so skipping enumeration should
> be okay. But it deserves explanation in the commit message or a comment.
Ok.
>
> > >
> > > Silently ignore any other errors?
> >
> > Do you think it's worth a warn? There are a couple other considerations.
> > - Kai brought up offline that we should handle TDX_SYS_BUSY here too.
> > - Previous kexec patches had trouble solving races around tdx enabling. So we
> > have to handle the seamcall failures.
> >
> > So we have to exclude a few different errors in different ways. And then the
> > warn worthy error codes either don't impact anything, or the new kernel will
> > fail to initialize the TDX module and give notice there.
>
> The delayed error is harder to debug. It can be useful to leave a
> breadcrumbs.
Ok, we can parse the errors.
>
> Also, do we want to make try_init_module_global() return failure after
> tdx_sys_disable()? I guess, TDH_SYS_LP_INIT will fail anyway, so it
> shouldn't matter.
Yea, a side effect of TDH.SYS.DISABLE is that it blocks other seamcalls while it
is executing. I guess the scenario here is TDX init racing with kexec.
But in general if TDX is disabled while any TDX stuff is running, the seamcalls
will be surprised. This is not fully related to TDH.SYS.DISABLE, because VMXOFF
will also cause similar SEAMCALL failures. Each SEAMCALL path would need to
handle the rug pull. And probably we need to balance harmless noise against the
code it takes to be quieter.
try_init_module_global() is different in that it's kernel side code that gets
confused, but I'm not sure how it could be handled in a non-racy way either.
So... I'd think to leave it. Maybe what we really need is a big block comment
about TDX enable/disable lifecycle quirks.
^ permalink raw reply
* Re: [PATCH v5 06/22] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Chao Gao @ 2026-03-18 6:54 UTC (permalink / raw)
To: Kiryl Shutsemau
Cc: x86, linux-coco, kvm, linux-kernel, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, nik.borisov, paulmck, pbonzini,
reinette.chatre, rick.p.edgecombe, sagis, seanjc, tony.lindgren,
vannapurve, vishal.l.verma, yilun.xu
In-Reply-To: <abkmceqUtMTnLI0V@thinkstation>
>> +
>> +What: /sys/devices/faux/tdx_host/seamldr/version
>> +Contact: linux-coco@lists.linux.dev
>> +Description: (RO) Report the version of the loaded SEAM loader. The SEAM
>> + loader version is formatted as x.y.z, where "x" is the major
>> + version, "y" is the minor version and "z" is the update version.
>> + Versions are used for bug reporting and compatibility checks.
>> +
>> +What: /sys/devices/faux/tdx_host/seamldr/num_remaining_updates
>> +Contact: linux-coco@lists.linux.dev
>> +Description: (RO) Report the number of remaining updates. TDX maintains a
>> + log about each TDX module that has been loaded. This log has
>> + a finite size, which limits the number of TDX module updates
>> + that can be performed.
>> +
>> + After each successful update, the number reduces by one. Once it
>> + reaches zero, further updates will fail until next reboot. The
>> + number is always zero if the P-SEAMLDR doesn't support updates.
>> +
>> + See Intel® Trust Domain Extensions - SEAM Loader (SEAMLDR)
>> + Interface Specification, Revision 343755-003, Chapter 3.3
>> + "SEAMLDR_INFO" and Chapter 4.2 "SEAMLDR.INSTALL" for more
>> + information.
>
>Do you think revision and chapter numbers useful here?
I think it's nice-to-have. Without specific references, people would need to
read the entire 26-page spec. But I can drop them if they make it too verbose.
^ permalink raw reply
* Re: [PATCH v5 04/22] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs
From: Xiaoyao Li @ 2026-03-18 7:13 UTC (permalink / raw)
To: Chao Gao, linux-kernel, linux-coco, kvm
Cc: binbin.wu, dan.j.williams, dave.hansen, ira.weiny, kai.huang, kas,
nik.borisov, paulmck, pbonzini, reinette.chatre, rick.p.edgecombe,
sagis, seanjc, tony.lindgren, vannapurve, vishal.l.verma,
yilun.xu, Farrah Chen, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86, H. Peter Anvin
In-Reply-To: <20260315135920.354657-5-chao.gao@intel.com>
On 3/15/2026 9:58 PM, Chao Gao wrote:
> The TDX architecture uses the "SEAMCALL" instruction to communicate with
> SEAM mode software. Right now, the only SEAM mode software that the kernel
> communicates with is the TDX module. But, there is actually another
> component that runs in SEAM mode but it is separate from the TDX module:
> the persistent SEAM loader or "P-SEAMLDR". Right now, the only component
> that communicates with it is the BIOS which loads the TDX module itself at
> boot. But, to support updating the TDX module, the kernel now needs to be
> able to talk to it.
>
> P-SEAMLDR SEAMCALLs differ from TDX module SEAMCALLs in areas such as
> concurrency requirements. Add a P-SEAMLDR wrapper to handle these
> differences and prepare for implementing concrete functions.
>
> Note that unlike P-SEAMLDR, there is also a non-persistent SEAM loader
> ("NP-SEAMLDR"). This is an authenticated code module (ACM) that is not
> callable at runtime. Only BIOS launches it to load P-SEAMLDR at boot;
> the kernel does not need to interact with it for runtime update.
>
> For details of P-SEAMLDR SEAMCALLs, see Intel® Trust Domain CPU
> Architectural Extensions, Revision 343754-002, Chapter 2.3 "INSTRUCTION
> SET REFERENCE".
SDM started to contain SEAMCALL definitions. How about just dropping
this paragraph to avoid people from reading the old doc?
...
> +static __maybe_unused int seamldr_call(u64 fn, struct tdx_module_args *args)
> +{
> + guard(raw_spinlock)(&seamldr_lock);
> + return seamcall_prerr(fn, args);
How about adding the reason of why choosing seamcall_prerr() instead of
seamcall_prerr_ret() in the changelog?
^ permalink raw reply
* Re: [PATCH v2 08/19] PCI/TSM: Add "evidence" support
From: Dan Williams @ 2026-03-18 7:22 UTC (permalink / raw)
To: Lukas Wunner, Dan Williams
Cc: linux-coco, linux-pci, gregkh, aik, aneesh.kumar, yilun.xu,
bhelgaas, alistair23, jgg, Donald Hunter, Jakub Kicinski
In-Reply-To: <ablhfHEIsz5IPW14@wunner.de>
Lukas Wunner wrote:
> On Mon, Mar 16, 2026 at 04:02:22PM -0700, Dan Williams wrote:
> > Dan Williams wrote:
> > > Lukas Wunner wrote:
> > > > This doesn't look like it's ever been tested, so at the very least
> > > > it should be marked RFC in the subject to convey that it's not yet
> > > > in a cut-and-dried state.
> > >
> > > The 16MB limit has indeed not been tested, the test script in this set
> > > was using smaller than 64K payloads to check out the interface.
> >
> > So 16MB works ok, slow, but works. A given attribute in this
> > implementation never exceeds the limit
>
> Famous last words.
I am not convinced you have found the "gotcha" you think you have...
> If you look at netlink_dump(), it sizes the skb based on
> nlk->max_recvmsg_len. If that's larger than 64k, you'll
> try to fill as much as possible of that space with a single
> netlink attribute. The computation of "available" in your
> patch doesn't take the 65531 bytes limit for a netlink attribute
> into account so it looks like you'll end up overflowing the length
> of the netlink attribute.
The @len to nla_put() should not overflow because it is based on the
available tailroom in the skb minus netlink overhead. The "inventive"
hack that Jakub is reacting to is that this scheme requires a receiver
that assumes repeating the attribute in the receive stream must be
handled as concatenation.
> Unfortunately nla_put() doesn't prevent such overflows, it does
> all the size calculations with an int, not a u16.
Are we looking at the same capacity calculation?
len = min(available - overhead, object_len - ctx->offset);
^ permalink raw reply
* Re: [PATCH v2 08/19] PCI/TSM: Add "evidence" support
From: Dan Williams @ 2026-03-18 7:41 UTC (permalink / raw)
To: Lukas Wunner, Dan Williams
Cc: linux-coco, linux-pci, gregkh, aik, aneesh.kumar, yilun.xu,
bhelgaas, alistair23, jgg, Donald Hunter, Jakub Kicinski
In-Reply-To: <abmcWIgaAyw66h5J@wunner.de>
Lukas Wunner wrote:
> On Mon, Mar 02, 2026 at 04:01:56PM -0800, Dan Williams wrote:
> > + type: const
> > + name: max-nonce-size
> > + value: 256
> [...]
> > +#define PCI_TSM_MAX_OBJECT_SIZE 16777216
> > +#define PCI_TSM_MAX_NONCE_SIZE 256
> > +#define PCI_TSM_MAX_OBJ_TYPE 4
>
> Where is the maximum nonce size of 256 bytes coming from?
I took it from Aneesh's off-list RFC, and meant to circle back with him.
Yes, it should come with a spec reference.
I am having trouble finding a clear reference for ARM CCA that clarifies
that measurement recollection takes the SPDM standard nonce as an input.
Perhaps the document I have "DEN0137 1.1-alp8" is out of date? TDX and
SEV-TIO do reference SPDM for the nonce size.
> Such definitions should always be accompanied by a spec reference,
> not pulled out of thin air.
Yes.
> SPDM nonces are 32 bytes, I assume that's what we're dealing with here?
>
> This patch:
> https://github.com/l1k/linux/commit/bca645e08ee9
>
> ... contains the following definition:
> #define SPDM_NONCE_SZ 32 /* SPDM 1.0.0 table 20 */
>
> Though it's defined in a private header in lib/spdm/spdm.h. If there's
> a need outside of the SPDM library, its visibility can be broadened
> of course.
Of course. Again this points to a need to pull this proposal out
separate from the rest.
The ARM CCA spec does reference the EAT nonce which is 64-bytes. So it
may be the case that PCI_TSM_MAX_NONCE_SIZE != SPDM_NONCE_SZ depending
on what evidence can be collected over this interface, but I am not
finding any spec references for 256, Aneesh?
^ permalink raw reply
* Re: [PATCH v5 05/22] x86/virt/seamldr: Retrieve P-SEAMLDR information
From: Xiaoyao Li @ 2026-03-18 7:53 UTC (permalink / raw)
To: Chao Gao, linux-kernel, linux-coco, kvm
Cc: binbin.wu, dan.j.williams, dave.hansen, ira.weiny, kai.huang, kas,
nik.borisov, paulmck, pbonzini, reinette.chatre, rick.p.edgecombe,
sagis, seanjc, tony.lindgren, vannapurve, vishal.l.verma,
yilun.xu, Farrah Chen, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86, H. Peter Anvin
In-Reply-To: <20260315135920.354657-6-chao.gao@intel.com>
On 3/15/2026 9:58 PM, Chao Gao wrote:
> P-SEAMLDR returns its information such as version number, in response to
> the SEAMLDR.INFO SEAMCALL.
>
> This information is useful for userspace. For example, the admin can decide
> which TDX module versions are compatible with the P-SEAMLDR according to
> the P-SEAMLDR version.
>
> Retrieve P-SEAMLDR information in preparation for exposing P-SEAMLDR
This patch only implements the function but nowhere calls it. Please
adjust the wording and the PATCH subject.
...
> +
> +/*
> + * This is called the "SEAMLDR_INFO" data structure and is defined
I'm curious why you changed "This called" to "This is called" in this
v5. In v4[1] you just used what Dave provided to v3[2]
[1] https://lore.kernel.org/kvm/20260212143606.534586-6-chao.gao@intel.com/
[2]
https://lore.kernel.org/all/b2e2fd5e-8aff-4eda-a648-9ae9f8234d25@intel.com/
^ permalink raw reply
* Re: [PATCH v2 08/19] PCI/TSM: Add "evidence" support
From: Dan Williams @ 2026-03-18 7:56 UTC (permalink / raw)
To: Lukas Wunner, Jakub Kicinski
Cc: Dan Williams, linux-coco, linux-pci, gregkh, aik, aneesh.kumar,
yilun.xu, bhelgaas, alistair23, jgg, Donald Hunter
In-Reply-To: <abmaG0jC7b05Lytz@wunner.de>
Lukas Wunner wrote:
[..]
> At this point perhaps your conclusion is that netlink isn't the right
> protocol for this job. It's great for transmitting sets of small items,
> some of which may be optional, but it's obviously not well-suited for
> large items.
Right, and sysfs is not well suited for transaction in/out semantics.
> Jason Gunthorpe was quite insistent that we use netlink and you know
Jason can of course correct me, but the insistence was less that netlink
was the right tool for the job, and more that sysfs was the wrong tool
for the job.
Netlink appears to be the least worst option.
> how consensus-oriented kernel development is. Indeed sysfs has turned
> out not to be ideal because the protocol that we're dealing with
> (SPDM - DMTF DSP0274) allows many degrees of freedom and making
> them available through sysfs quickly becomes unwieldy.
[..]
> If netlink is at all the right protocol for the job, I'm wondering if an
> extension for larger attributes would be entertained.
It is still not clear to me that allowing larger attributes are part of
the solution space. They seem to be a premature performance optimization
once userspace is prepared to reassemble a large blob over multiple
messages.
^ permalink raw reply
* Re: [PATCH v5 06/22] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Xiaoyao Li @ 2026-03-18 8:20 UTC (permalink / raw)
To: Chao Gao, Kiryl Shutsemau
Cc: x86, linux-coco, kvm, linux-kernel, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, nik.borisov, paulmck, pbonzini,
reinette.chatre, rick.p.edgecombe, sagis, seanjc, tony.lindgren,
vannapurve, vishal.l.verma, yilun.xu
In-Reply-To: <abpMHxwhYqbmrX8s@intel.com>
On 3/18/2026 2:54 PM, Chao Gao wrote:
>>> +
>>> +What: /sys/devices/faux/tdx_host/seamldr/version
>>> +Contact: linux-coco@lists.linux.dev
>>> +Description: (RO) Report the version of the loaded SEAM loader. The SEAM
>>> + loader version is formatted as x.y.z, where "x" is the major
>>> + version, "y" is the minor version and "z" is the update version.
>>> + Versions are used for bug reporting and compatibility checks.
>>> +
>>> +What: /sys/devices/faux/tdx_host/seamldr/num_remaining_updates
>>> +Contact: linux-coco@lists.linux.dev
>>> +Description: (RO) Report the number of remaining updates. TDX maintains a
>>> + log about each TDX module that has been loaded. This log has
>>> + a finite size, which limits the number of TDX module updates
>>> + that can be performed.
>>> +
>>> + After each successful update, the number reduces by one. Once it
>>> + reaches zero, further updates will fail until next reboot. The
>>> + number is always zero if the P-SEAMLDR doesn't support updates.
>>> +
>>> + See Intel® Trust Domain Extensions - SEAM Loader (SEAMLDR)
>>> + Interface Specification, Revision 343755-003, Chapter 3.3
>>> + "SEAMLDR_INFO" and Chapter 4.2 "SEAMLDR.INSTALL" for more
>>> + information.
>>
>> Do you think revision and chapter numbers useful here?
>
> I think it's nice-to-have. Without specific references, people would need to
> read the entire 26-page spec. But I can drop them if they make it too verbose.
I have some concerns about the "revision". The good thing is that it can
tell what the attribute was built against while the bad thing is that it
might not be easy for people to find an old revision years later.
^ permalink raw reply
* Re: [PATCH v5 07/22] coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates
From: Chao Gao @ 2026-03-18 8:28 UTC (permalink / raw)
To: Kiryl Shutsemau
Cc: linux-kernel, linux-coco, kvm, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, nik.borisov, paulmck, pbonzini,
reinette.chatre, rick.p.edgecombe, sagis, seanjc, tony.lindgren,
vannapurve, vishal.l.verma, yilun.xu, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin
In-Reply-To: <abko0v2bYxNJtegT@thinkstation>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
>
>Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
Thanks a lot for your reviews.
<snip>
>> static int seamldr_init(struct device *dev)
>> {
>> + const struct tdx_sys_info *tdx_sysinfo = tdx_get_sysinfo();
>> + struct fw_upload *tdx_fwl;
>> + int ret;
>> +
>> + if (WARN_ON_ONCE(!tdx_sysinfo))
>> + return -EIO;
>> +
>> + if (!tdx_supports_runtime_update(tdx_sysinfo))
>> + return 0;
>
>Hm. Do we still want to register seamldr_group for this case?
seamldr_group currently serves only module updates, so registering it when
updates aren't supported seems unnecessary.
>
>Maybe move it up before the check?
If new use cases emerge and need seamldr version etc, we can do the
changes.
FWIW, seamldr_group visibility does matter in one case: it must be
hidden on CPUs with an erratum (see
https://lore.kernel.org/kvm/20260315135920.354657-19-chao.gao@intel.com/)
^ permalink raw reply
* Re: [PATCH v5 08/22] x86/virt/seamldr: Allocate and populate a module update request
From: Chao Gao @ 2026-03-18 8:50 UTC (permalink / raw)
To: Kiryl Shutsemau
Cc: linux-kernel, linux-coco, kvm, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, nik.borisov, paulmck, pbonzini,
reinette.chatre, rick.p.edgecombe, sagis, seanjc, tony.lindgren,
vannapurve, vishal.l.verma, yilun.xu, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin
In-Reply-To: <abkvOYQIi2RvEYh8@thinkstation>
>> + ptr = sig;
>> + for (i = 0; i < sig_size / SZ_4K; i++) {
>> + /*
>> + * Don't assume @sig is page-aligned although it is 4KB-aligned.
>> + * Always add the in-page offset to get the physical address.
>> + */
>
>I don't follow this. If @sig is 4k aligned in VA, it is page aligned.
Dan's concern was that PAGE_SIZE is not guaranteed to be 4096.
I agree that PAGE_SIZE is 4K on x86 today. But to address that concern, I saw
two options:
1. Add WARN_ON_ONCE(PAGE_SIZE != SZ_4K), or
2. Handle it as in the code above.
I didn't find existing code using option 1 in x86, so I chose option 2.
>
>If you want to handle case when @sig is not 4k aligned, than this is
>broken. You need to bump ptr to the next 4k boundary, not by 4k.
@sig is 4KB aligned.
<snip>
>> +static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
>> +{
>> + const struct tdx_blob *blob = (const void *)data;
>> + int module_size, sig_size;
>> + const void *sig, *module;
>> +
>> + /* Ensure the size is valid otherwise reading any field from the blob may overflow. */
>> + if (size <= sizeof(struct tdx_blob) || size <= blob->offset_of_module)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (blob->version != TDX_BLOB_VERSION_1) {
>> + pr_err("unsupported blob version: %x\n", blob->version);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + /* Split the blob into a sigstruct and a module. */
>> + sig = blob->data;
>> + sig_size = blob->offset_of_module - sizeof(struct tdx_blob);
>> + module = data + blob->offset_of_module;
>> + module_size = size - blob->offset_of_module;
>> +
>> + if (sig_size <= 0 || module_size <= 0 || blob->length != size)
>> + return ERR_PTR(-EINVAL);
>
>Maybe add a comment somewhere that block->offset_of_module is relative
>to start of struct tdx_blob, not blob->data and blob->length includes
>length of struct tdx_blob.
>
>It can be either way and it is better to give a reader a hint.
Sure. Will do.
^ permalink raw reply
* Re: [PATCH v5 05/22] x86/virt/seamldr: Retrieve P-SEAMLDR information
From: Chao Gao @ 2026-03-18 8:57 UTC (permalink / raw)
To: Xiaoyao Li
Cc: linux-kernel, linux-coco, kvm, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, kas, nik.borisov, paulmck,
pbonzini, reinette.chatre, rick.p.edgecombe, sagis, seanjc,
tony.lindgren, vannapurve, vishal.l.verma, yilun.xu, Farrah Chen,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin
In-Reply-To: <0f5fe0ba-1699-4762-90b0-60fd8bb9c869@intel.com>
On Wed, Mar 18, 2026 at 03:53:26PM +0800, Xiaoyao Li wrote:
>On 3/15/2026 9:58 PM, Chao Gao wrote:
>> P-SEAMLDR returns its information such as version number, in response to
>> the SEAMLDR.INFO SEAMCALL.
>>
>> This information is useful for userspace. For example, the admin can decide
>> which TDX module versions are compatible with the P-SEAMLDR according to
>> the P-SEAMLDR version.
>>
>> Retrieve P-SEAMLDR information in preparation for exposing P-SEAMLDR
>
>This patch only implements the function but nowhere calls it. Please adjust
>the wording and the PATCH subject.
How about:
Add a helper to retrieve ...?
>
>...
>
>> +
>> +/*
>> + * This is called the "SEAMLDR_INFO" data structure and is defined
>
>I'm curious why you changed "This called" to "This is called" in this v5. In
>v4[1] you just used what Dave provided to v3[2]
I ran the patches through Copilot before posting, and it flagged "This called"
as a typo, so I fixed it.
>
>[1] https://lore.kernel.org/kvm/20260212143606.534586-6-chao.gao@intel.com/
>[2]
>https://lore.kernel.org/all/b2e2fd5e-8aff-4eda-a648-9ae9f8234d25@intel.com/
^ permalink raw reply
* Re: [PATCH v5 06/22] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Chao Gao @ 2026-03-18 9:10 UTC (permalink / raw)
To: Xiaoyao Li
Cc: Kiryl Shutsemau, x86, linux-coco, kvm, linux-kernel, binbin.wu,
dan.j.williams, dave.hansen, ira.weiny, kai.huang, nik.borisov,
paulmck, pbonzini, reinette.chatre, rick.p.edgecombe, sagis,
seanjc, tony.lindgren, vannapurve, vishal.l.verma, yilun.xu
In-Reply-To: <3614e6fb-bdad-4e59-9769-5dc9869e03bf@intel.com>
On Wed, Mar 18, 2026 at 04:20:54PM +0800, Xiaoyao Li wrote:
>On 3/18/2026 2:54 PM, Chao Gao wrote:
>> > > +
>> > > +What: /sys/devices/faux/tdx_host/seamldr/version
>> > > +Contact: linux-coco@lists.linux.dev
>> > > +Description: (RO) Report the version of the loaded SEAM loader. The SEAM
>> > > + loader version is formatted as x.y.z, where "x" is the major
>> > > + version, "y" is the minor version and "z" is the update version.
>> > > + Versions are used for bug reporting and compatibility checks.
>> > > +
>> > > +What: /sys/devices/faux/tdx_host/seamldr/num_remaining_updates
>> > > +Contact: linux-coco@lists.linux.dev
>> > > +Description: (RO) Report the number of remaining updates. TDX maintains a
>> > > + log about each TDX module that has been loaded. This log has
>> > > + a finite size, which limits the number of TDX module updates
>> > > + that can be performed.
>> > > +
>> > > + After each successful update, the number reduces by one. Once it
>> > > + reaches zero, further updates will fail until next reboot. The
>> > > + number is always zero if the P-SEAMLDR doesn't support updates.
>> > > +
>> > > + See Intel® Trust Domain Extensions - SEAM Loader (SEAMLDR)
>> > > + Interface Specification, Revision 343755-003, Chapter 3.3
>> > > + "SEAMLDR_INFO" and Chapter 4.2 "SEAMLDR.INSTALL" for more
>> > > + information.
>> >
>> > Do you think revision and chapter numbers useful here?
>>
>> I think it's nice-to-have. Without specific references, people would need to
>> read the entire 26-page spec. But I can drop them if they make it too verbose.
>
>I have some concerns about the "revision". The good thing is that it can tell
>what the attribute was built against while the bad thing is that it might not
>be easy for people to find an old revision years later.
I'm okay with dropping the revision number.
The intent is not to require readers to find the exact same revision. If they
can find that exact revision, great. If not, the chapter numbers may differ
in newer revisions.
Kirill, do you mean dropping the numbers but keeping the chapter titles:
See Intel® Trust Domain Extensions - SEAM Loader (SEAMLDR)
Interface Specification, Chapter "SEAMLDR_INFO" and Chapter
"SEAMLDR.INSTALL" for more information.
This keeps a targeted reference so readers do not need to scan the entire spec.
^ permalink raw reply
* Re: [PATCH v5 06/22] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Kiryl Shutsemau @ 2026-03-18 9:28 UTC (permalink / raw)
To: Chao Gao
Cc: Xiaoyao Li, x86, linux-coco, kvm, linux-kernel, binbin.wu,
dan.j.williams, dave.hansen, ira.weiny, kai.huang, nik.borisov,
paulmck, pbonzini, reinette.chatre, rick.p.edgecombe, sagis,
seanjc, tony.lindgren, vannapurve, vishal.l.verma, yilun.xu
In-Reply-To: <abpr/Sg3QXfDd1Q+@intel.com>
On Wed, Mar 18, 2026 at 05:10:21PM +0800, Chao Gao wrote:
> On Wed, Mar 18, 2026 at 04:20:54PM +0800, Xiaoyao Li wrote:
> >On 3/18/2026 2:54 PM, Chao Gao wrote:
> >> > > +
> >> > > +What: /sys/devices/faux/tdx_host/seamldr/version
> >> > > +Contact: linux-coco@lists.linux.dev
> >> > > +Description: (RO) Report the version of the loaded SEAM loader. The SEAM
> >> > > + loader version is formatted as x.y.z, where "x" is the major
> >> > > + version, "y" is the minor version and "z" is the update version.
> >> > > + Versions are used for bug reporting and compatibility checks.
> >> > > +
> >> > > +What: /sys/devices/faux/tdx_host/seamldr/num_remaining_updates
> >> > > +Contact: linux-coco@lists.linux.dev
> >> > > +Description: (RO) Report the number of remaining updates. TDX maintains a
> >> > > + log about each TDX module that has been loaded. This log has
> >> > > + a finite size, which limits the number of TDX module updates
> >> > > + that can be performed.
> >> > > +
> >> > > + After each successful update, the number reduces by one. Once it
> >> > > + reaches zero, further updates will fail until next reboot. The
> >> > > + number is always zero if the P-SEAMLDR doesn't support updates.
> >> > > +
> >> > > + See Intel® Trust Domain Extensions - SEAM Loader (SEAMLDR)
> >> > > + Interface Specification, Revision 343755-003, Chapter 3.3
> >> > > + "SEAMLDR_INFO" and Chapter 4.2 "SEAMLDR.INSTALL" for more
> >> > > + information.
> >> >
> >> > Do you think revision and chapter numbers useful here?
> >>
> >> I think it's nice-to-have. Without specific references, people would need to
> >> read the entire 26-page spec. But I can drop them if they make it too verbose.
> >
> >I have some concerns about the "revision". The good thing is that it can tell
> >what the attribute was built against while the bad thing is that it might not
> >be easy for people to find an old revision years later.
>
> I'm okay with dropping the revision number.
>
> The intent is not to require readers to find the exact same revision. If they
> can find that exact revision, great. If not, the chapter numbers may differ
> in newer revisions.
>
> Kirill, do you mean dropping the numbers but keeping the chapter titles:
>
> See Intel® Trust Domain Extensions - SEAM Loader (SEAMLDR)
> Interface Specification, Chapter "SEAMLDR_INFO" and Chapter
> "SEAMLDR.INSTALL" for more information.
>
> This keeps a targeted reference so readers do not need to scan the entire spec.
Yeah, looks good to me.
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox