Re: [PATCH v2 27/31] coco/tdx-host: Implement SPDM session setup

Nikolay Borisov
2026-04-02T11:29:48Z

On 27.03.26 г. 18:01 ч., Xu Yilun wrote:
> From: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
> Implementation for a most straightforward SPDM session setup, using all
> default session options. Retrieve device info data from TDX Module which
> contains the SPDM negotiation results.
>
> TDH.SPDM.CONNECT/DISCONNECT are TDX Module Extension introduced
> SEAMCALLs which can run for longer periods and interruptible. But there
> is resource constraints that limit how many SEAMCALLs of this kind can
> run simultaneously. The current situation is One SEAMCALL at a time.
> Otherwise TDX_OPERAND_BUSY is returned. To avoid "broken indefinite"
> retry, a tdx_ext_lock is used to guard these SEAMCALLs.
>
> Co-developed-by: Xu Yilun <yilun.xu@linux.intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
> arch/x86/include/asm/shared/tdx_errno.h | 2 +
> drivers/virt/coco/tdx-host/tdx-host.c | 301 +++++++++++++++++++++++-
> 2 files changed, 299 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/shared/tdx_errno.h b/arch/x86/include/asm/shared/tdx_errno.h
> index 8bf6765cf082..7db04fe30378 100644
> --- a/arch/x86/include/asm/shared/tdx_errno.h
> +++ b/arch/x86/include/asm/shared/tdx_errno.h
> @@ -29,6 +29,8 @@
> #define TDX_EPT_WALK_FAILED 0xC0000B0000000000ULL
> #define TDX_EPT_ENTRY_STATE_INCORRECT 0xC0000B0D00000000ULL
> #define TDX_METADATA_FIELD_NOT_READABLE 0xC0000C0200000000ULL
> +#define TDX_SPDM_SESSION_KEY_REQUIRE_REFRESH 0xC0000F4500000000ULL
> +#define TDX_SPDM_REQUEST 0xC0000F5700000000ULL
>
> /*
> * SW-defined error codes.
> diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> index 06f3d194e0a8..4d127b7c2591 100644
> --- a/drivers/virt/coco/tdx-host/tdx-host.c
> +++ b/drivers/virt/coco/tdx-host/tdx-host.c
> @@ -14,6 +14,7 @@
> #include <linux/pci-doe.h>
> #include <linux/pci-tsm.h>
> #include <linux/tsm.h>
> +#include <linux/vmalloc.h>
>
> #include <asm/cpu_device_id.h>
> #include <asm/tdx.h>
> @@ -32,8 +33,43 @@ MODULE_DEVICE_TABLE(x86cpu, tdx_host_ids);
> */
> static const struct tdx_sys_info *tdx_sysinfo;
>
> +#define TDISP_FUNC_ID GENMASK(15, 0)
> +#define TDISP_FUNC_ID_SEGMENT GENMASK(23, 16)
> +#define TDISP_FUNC_ID_SEG_VALID BIT(24)
> +
> +static inline u32 tdisp_func_id(struct pci_dev *pdev)
> +{
> + u32 func_id;
> +
> + func_id = FIELD_PREP(TDISP_FUNC_ID_SEGMENT, pci_domain_nr(pdev->bus));
> + if (func_id)
> + func_id |= TDISP_FUNC_ID_SEG_VALID;

This check implies pci_domain_nr returning 0 is considered invalid.
Other callers in the kernel seem to not care, they just use the domain
nr, so is this check spurious or intentional ?

> + func_id |= FIELD_PREP(TDISP_FUNC_ID,
> + PCI_DEVID(pdev->bus->number, pdev->devfn));
> +
> + return func_id;
> +}
> +
> +struct spdm_config_info_t {
> + u32 vmm_spdm_cap;
> +#define SPDM_CAP_HBEAT BIT(13)
> +#define SPDM_CAP_KEY_UPD BIT(14)

nit: move those defines above the struct definition, they just break the
reading flow as it is.

> + u8 spdm_session_policy;
> + u8 certificate_slot_mask;
> + u8 raw_bitstream_requested;
> +} __packed;
> +
> struct tdx_tsm_link {
> struct pci_tsm_pf0 pci;
> + u32 func_id;
> + struct page *in_msg;
> + struct page *out_msg;
> +
> + u64 spdm_id;
> + struct page *spdm_conf;
> + struct tdx_page_array *spdm_mt;
> + unsigned int dev_info_size;
> + void *dev_info_data;
> };
>
> static struct tdx_tsm_link *to_tdx_tsm_link(struct pci_tsm *tsm)

<snip>

> +
> +static void *tdx_dup_array_data(struct tdx_page_array *array,
> + unsigned int data_size)
> +{
> + unsigned int npages = (data_size + PAGE_SIZE - 1) / PAGE_SIZE;

nit: There's DIV_ROUND_UP

> + void *data, *dup_data;
> +
> + if (npages > array->nr_pages)
> + return NULL;
> +
> + data = vm_map_ram(array->pages, npages, -1);
> + if (!data)
> + return NULL;
> +
> + dup_data = kmemdup(data, data_size, GFP_KERNEL);
> + vm_unmap_ram(data, npages);
> +
> + return dup_data;
> +}
> +

<snip>

> +
> +DEFINE_FREE(tdx_spdm_session_teardown, struct tdx_tsm_link *,
> + if (!IS_ERR_OR_NULL(_T)) tdx_spdm_session_teardown(_T))
> +
> static int tdx_tsm_link_connect(struct pci_dev *pdev)
> {
> - return -ENXIO;
> + struct tdx_tsm_link *tlink = to_tdx_tsm_link(pdev->tsm);
> +
> + struct tdx_tsm_link *tlink_spdm __free(tdx_spdm_session_teardown) =
> + tdx_spdm_session_setup(tlink);

Is the free() really needed here, either the session is correctly setup
and tlink_spdm is returned. But if session_setup() files then what about
calling spdm_session_disconnect() on an unestablished session?

> + if (IS_ERR(tlink_spdm)) {
> + pci_err(pdev, "fail to setup spdm session\n");
> + return PTR_ERR(tlink_spdm);
> + }
> +
> + retain_and_null_ptr(tlink_spdm);
> +
> + return 0;
> }
>

<snip>