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

[Nikolay Borisov]

On 27.03.26 г. 18:01 ч., Xu Yilun wrote:
> From: Zhenzhong Duan <zhenzhong.duan@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@xxxxxxxxx>
> ---
>   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>