Message-ID: <fd5bdddc-fd22-4373-a8ff-3933c63cbacc@amd.com>
Date: Mon, 1 Dec 2025 09:23:17 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Alexey Kardashevskiy <aik@....com>, linux-kernel@...r.kernel.org
Cc: linux-crypto@...r.kernel.org, John Allen <john.allen@....com>,
 Herbert Xu <herbert@...dor.apana.org.au>,
 "David S. Miller" <davem@...emloft.net>, Ashish Kalra
 <ashish.kalra@....com>, Joerg Roedel <joro@...tes.org>,
 Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
 Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
 "Borislav Petkov (AMD)" <bp@...en8.de>, Kim Phillips <kim.phillips@....com>,
 Jerry Snitselaar <jsnitsel@...hat.com>, Vasant Hegde <vasant.hegde@....com>,
 Jason Gunthorpe <jgg@...pe.ca>, Gao Shiyuan <gaoshiyuan@...du.com>,
 Sean Christopherson <seanjc@...gle.com>, Nikunj A Dadhania <nikunj@....com>,
 Michael Roth <michael.roth@....com>, Amit Shah <amit.shah@....com>,
 Peter Gonda <pgonda@...gle.com>, iommu@...ts.linux.dev
Subject: Re: [PATCH kernel v2 5/5] crypto/ccp: Implement SEV-TIO PCIe IDE
 (phase1)

On 11/21/25 02:06, Alexey Kardashevskiy wrote:
> Implement the SEV-TIO (Trusted I/O) firmware interface for PCIe TDISP
> (Trust Domain In-Socket Protocol). This enables secure communication
> between trusted domains and PCIe devices through the PSP (Platform
> Security Processor).
> 
> The implementation includes:
> - Device Security Manager (DSM) operations for establishing secure links
> - SPDM (Security Protocol and Data Model) over DOE (Data Object Exchange)
> - IDE (Integrity Data Encryption) stream management for secure PCIe
> 
> This module bridges the SEV firmware stack with the generic PCIe TSM
> framework.
> 
> This is phase1 as described in Documentation/driver-api/pci/tsm.rst.
> 
> On AMD SEV, the AMD PSP firmware acts as TSM (manages the security/trust).
> The CCP driver provides the interface to it and registers in the TSM
> subsystem.
> 
> Implement SEV TIO PSP command wrappers in sev-dev-tio.c and store
> the data in the SEV-TIO-specific structs.
> 
> Implement TSM hooks and IDE setup in sev-dev-tsm.c.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@....com>
> ---
> Changes:
> v2:
> * address bunch of comments from v1 (almost all)
> ---
>  drivers/crypto/ccp/Kconfig       |   1 +
>  drivers/crypto/ccp/Makefile      |   8 +
>  drivers/crypto/ccp/sev-dev-tio.h | 142 ++++
>  drivers/crypto/ccp/sev-dev.h     |   7 +
>  include/linux/psp-sev.h          |   7 +
>  drivers/crypto/ccp/sev-dev-tio.c | 863 ++++++++++++++++++++
>  drivers/crypto/ccp/sev-dev-tsm.c | 405 +++++++++
>  drivers/crypto/ccp/sev-dev.c     |  48 +-
>  8 files changed, 1480 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
> index f394e45e11ab..3e737d3e21c8 100644
> --- a/drivers/crypto/ccp/Kconfig
> +++ b/drivers/crypto/ccp/Kconfig
> @@ -25,6 +25,7 @@ config CRYPTO_DEV_CCP_CRYPTO
>  	default m
>  	depends on CRYPTO_DEV_CCP_DD
>  	depends on CRYPTO_DEV_SP_CCP
> +	select PCI_TSM

This shouldn't be here. This is CCP related, not SEV related. This
should be moved under CRYPTO_DEV_SP_PSP.

>  	select CRYPTO_HASH
>  	select CRYPTO_SKCIPHER
>  	select CRYPTO_AUTHENC
> diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile
> index a9626b30044a..839df68b70ff 100644
> --- a/drivers/crypto/ccp/Makefile
> +++ b/drivers/crypto/ccp/Makefile
> @@ -16,6 +16,14 @@ ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += psp-dev.o \
>                                     hsti.o \
>                                     sfs.o
>  
> +ifeq ($(CONFIG_CRYPTO_DEV_SP_PSP)$(CONFIG_PCI_TSM),yy)
> +ccp-y += sev-dev-tsm.o sev-dev-tio.o
> +endif
> +
> +ifeq ($(CONFIG_CRYPTO_DEV_SP_PSP)$(CONFIG_PCI_TSM),my)
> +ccp-m += sev-dev-tsm.o sev-dev-tio.o
> +endif
> +

Would it be clearer / cleaner to do:

ifeq ($(CONFIG_PCI_TSM),y)
ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += sev-dev-tsm.o sev-dev-tio.o
endif

>  obj-$(CONFIG_CRYPTO_DEV_CCP_CRYPTO) += ccp-crypto.o
>  ccp-crypto-objs := ccp-crypto-main.o \
>  		   ccp-crypto-aes.o \
> diff --git a/drivers/crypto/ccp/sev-dev-tio.h b/drivers/crypto/ccp/sev-dev-tio.h
> new file mode 100644
> index 000000000000..7c42351210ef
> --- /dev/null
> +++ b/drivers/crypto/ccp/sev-dev-tio.h
> @@ -0,0 +1,142 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __PSP_SEV_TIO_H__
> +#define __PSP_SEV_TIO_H__
> +
> +#include <linux/pci-tsm.h>
> +#include <linux/tsm.h>
> +#include <linux/pci-ide.h>
> +#include <uapi/linux/psp-sev.h>
> +
> +#if defined(CONFIG_CRYPTO_DEV_SP_PSP)

Since the TSM related files are included based on CONFIG_PCI_TSM,
shouldn't this use CONFIG_PCI_TSM?> +
> +struct sla_addr_t {
> +	union {
> +		u64 sla;
> +		struct {
> +			u64 page_type:1;
> +			u64 page_size:1;
> +			u64 reserved1:10;
> +			u64 pfn:40;
> +			u64 reserved2:12;

	u64 page_type	:1,
	    page_size	:1,
	    reserved1	:10,
	    pfn		:40,
	    reserved2	:12;

This makes it easier to understand. Please do this everywhere you define
bitfields.

> +		};
> +	};
> +} __packed;
> +
> +#define SEV_TIO_MAX_COMMAND_LENGTH	128
> +
> +/* SPDM control structure for DOE */
> +struct tsm_spdm {
> +	unsigned long req_len;
> +	void *req;
> +	unsigned long rsp_len;
> +	void *rsp;
> +};
> +
> +/* Describes TIO device */
> +struct tsm_dsm_tio {
> +	u8 cert_slot;
> +	struct sla_addr_t dev_ctx;
> +	struct sla_addr_t req;
> +	struct sla_addr_t resp;
> +	struct sla_addr_t scratch;
> +	struct sla_addr_t output;
> +	size_t output_len;
> +	size_t scratch_len;
> +	struct tsm_spdm spdm;
> +	struct sla_buffer_hdr *reqbuf; /* vmap'ed @req for DOE */
> +	struct sla_buffer_hdr *respbuf; /* vmap'ed @resp for DOE */
> +
> +	int cmd;
> +	int psp_ret;
> +	u8 cmd_data[SEV_TIO_MAX_COMMAND_LENGTH];
> +	void *data_pg; /* Data page for DEV_STATUS/TDI_STATUS/TDI_INFO/ASID_FENCE */
> +
> +#define TIO_IDE_MAX_TC	8
> +	struct pci_ide *ide[TIO_IDE_MAX_TC];
> +};
> +
> +/* Describes TSM structure for PF0 pointed by pci_dev->tsm */
> +struct tio_dsm {
> +	struct pci_tsm_pf0 tsm;
> +	struct tsm_dsm_tio data;
> +	struct sev_device *sev;
> +};
> +
> +/* Data object IDs */
> +#define SPDM_DOBJ_ID_NONE		0
> +#define SPDM_DOBJ_ID_REQ		1
> +#define SPDM_DOBJ_ID_RESP		2
> +
> +struct spdm_dobj_hdr {
> +	u32 id;     /* Data object type identifier */
> +	u32 length; /* Length of the data object, INCLUDING THIS HEADER */
> +	union {
> +		u16 ver; /* Version of the data object structure */
> +		struct {
> +			u8 minor;
> +			u8 major;
> +		} version;
> +	};
> +} __packed;
> +
> +/**
> + * struct sev_tio_status - TIO_STATUS command's info_paddr buffer
> + *
> + * @length: Length of this structure in bytes
> + * @tio_en: Indicates that SNP_INIT_EX initialized the RMP for SEV-TIO
> + * @tio_init_done: Indicates TIO_INIT has been invoked
> + * @spdm_req_size_min: Minimum SPDM request buffer size in bytes
> + * @spdm_req_size_max: Maximum SPDM request buffer size in bytes
> + * @spdm_scratch_size_min: Minimum SPDM scratch buffer size in bytes
> + * @spdm_scratch_size_max: Maximum SPDM scratch buffer size in bytes
> + * @spdm_out_size_min: Minimum SPDM output buffer size in bytes
> + * @spdm_out_size_max: Maximum for the SPDM output buffer size in bytes
> + * @spdm_rsp_size_min: Minimum SPDM response buffer size in bytes
> + * @spdm_rsp_size_max: Maximum SPDM response buffer size in bytes
> + * @devctx_size: Size of a device context buffer in bytes
> + * @tdictx_size: Size of a TDI context buffer in bytes
> + * @tio_crypto_alg: TIO crypto algorithms supported
> + */
> +struct sev_tio_status {
> +	u32 length;
> +	union {
> +		u32 flags;
> +		struct {
> +			u32 tio_en:1;
> +			u32 tio_init_done:1;
> +		};
> +	};
> +	u32 spdm_req_size_min;
> +	u32 spdm_req_size_max;
> +	u32 spdm_scratch_size_min;
> +	u32 spdm_scratch_size_max;
> +	u32 spdm_out_size_min;
> +	u32 spdm_out_size_max;
> +	u32 spdm_rsp_size_min;
> +	u32 spdm_rsp_size_max;
> +	u32 devctx_size;
> +	u32 tdictx_size;
> +	u32 tio_crypto_alg;
> +	u8 reserved[12];
> +} __packed;
> +
> +int sev_tio_init_locked(void *tio_status_page);
> +int sev_tio_continue(struct tsm_dsm_tio *dev_data);
> +
> +int sev_tio_dev_create(struct tsm_dsm_tio *dev_data, u16 device_id, u16 root_port_id,
> +		       u8 segment_id);
> +int sev_tio_dev_connect(struct tsm_dsm_tio *dev_data, u8 tc_mask, u8 ids[8], u8 cert_slot);
> +int sev_tio_dev_disconnect(struct tsm_dsm_tio *dev_data, bool force);
> +int sev_tio_dev_reclaim(struct tsm_dsm_tio *dev_data);
> +
> +#endif	/* CONFIG_CRYPTO_DEV_SP_PSP */
> +
> +#if defined(CONFIG_PCI_TSM)
> +void sev_tsm_init_locked(struct sev_device *sev, void *tio_status_page);
> +void sev_tsm_uninit(struct sev_device *sev);
> +int sev_tio_cmd_buffer_len(int cmd);
> +#else
> +static inline int sev_tio_cmd_buffer_len(int cmd) { return 0; }
> +#endif
> +
> +#endif	/* __PSP_SEV_TIO_H__ */
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> index b9029506383f..dced4a8e9f01 100644
> --- a/drivers/crypto/ccp/sev-dev.h
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -34,6 +34,8 @@ struct sev_misc_dev {
>  	struct miscdevice misc;
>  };
>  
> +struct sev_tio_status;
> +
>  struct sev_device {
>  	struct device *dev;
>  	struct psp_device *psp;
> @@ -61,6 +63,11 @@ struct sev_device {
>  
>  	struct sev_user_data_snp_status snp_plat_status;
>  	struct snp_feature_info snp_feat_info_0;
> +
> +#if defined(CONFIG_PCI_TSM)
> +	struct tsm_dev *tsmdev;
> +	struct sev_tio_status *tio_status;
> +#endif

Do you need the #if here? Can this just be part of the sev_device struct
and just always NULL if CONFIG_PCI_TSM isn't set?

Is "struct tsm_dev" not defined if CONFIG_PCI_TSM isn't 'y'? I would
think it would be simpler for all to have that defined no matter what.
>  };
>  
>  int sev_dev_init(struct psp_device *psp);
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index c0c817ca3615..cce864dbf281 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -109,6 +109,13 @@ enum sev_cmd {
>  	SEV_CMD_SNP_VLEK_LOAD		= 0x0CD,
>  	SEV_CMD_SNP_FEATURE_INFO	= 0x0CE,
>  
> +	/* SEV-TIO commands */
> +	SEV_CMD_TIO_STATUS		= 0x0D0,
> +	SEV_CMD_TIO_INIT		= 0x0D1,
> +	SEV_CMD_TIO_DEV_CREATE		= 0x0D2,
> +	SEV_CMD_TIO_DEV_RECLAIM		= 0x0D3,
> +	SEV_CMD_TIO_DEV_CONNECT		= 0x0D4,
> +	SEV_CMD_TIO_DEV_DISCONNECT	= 0x0D5,
>  	SEV_CMD_MAX,
>  };
>  
> diff --git a/drivers/crypto/ccp/sev-dev-tio.c b/drivers/crypto/ccp/sev-dev-tio.c
> new file mode 100644
> index 000000000000..f7b2a515aae7
> --- /dev/null
> +++ b/drivers/crypto/ccp/sev-dev-tio.c

...

> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 2f1c9614d359..365867f381e9 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -38,6 +38,7 @@
>  
>  #include "psp-dev.h"
>  #include "sev-dev.h"
> +#include "sev-dev-tio.h"
>  
>  #define DEVICE_NAME		"sev"
>  #define SEV_FW_FILE		"amd/sev.fw"
> @@ -75,6 +76,12 @@ static bool psp_init_on_probe = true;
>  module_param(psp_init_on_probe, bool, 0444);
>  MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
>  
> +#if defined(CONFIG_PCI_TSM)

Not sure the module parameter should be hidden in this case. But if you
do want it hidden, why not move the #if down one line so that
sev_tio_enabled is always defined. And then...

> +static bool sev_tio_enabled = true;

static bool sev_tio_enabled = IS_ENABLED(CONFIG_PCI_TSM)

will give the proper default.
> +module_param_named(tio, sev_tio_enabled, bool, 0444);
> +MODULE_PARM_DESC(tio, "Enables TIO in SNP_INIT_EX");
> +#endif
> +
>  MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
>  MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
>  MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */
> @@ -251,7 +258,7 @@ static int sev_cmd_buffer_len(int cmd)
>  	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
>  	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct sev_data_snp_feature_info);
>  	case SEV_CMD_SNP_VLEK_LOAD:		return sizeof(struct sev_user_data_snp_vlek_load);
> -	default:				return 0;
> +	default:				return sev_tio_cmd_buffer_len(cmd);
>  	}
>  
>  	return 0;
> @@ -1439,8 +1446,14 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
>  		data.init_rmp = 1;
>  		data.list_paddr_en = 1;
>  		data.list_paddr = __psp_pa(snp_range_list);
> +
> +#if defined(CONFIG_PCI_TSM)
>  		data.tio_en = sev_tio_present(sev) &&
> +			sev_tio_enabled && psp_init_on_probe &&

Why add the psp_init_on_probe check here? Why is it not compatible?
psp_init_on_probe is for SEV and SEV-ES, not SNP.

Instead of the #if, please use IS_ENABLED(CONFIG_PCI_TSM) so that the
#ifdefs can be eliminated from the code.

Having all these checks in sev_tio_supported() (comment from earlier
patch) will simplify things.
>  			amd_iommu_sev_tio_supported();
> +		if (sev_tio_present(sev) && !psp_init_on_probe)
> +			dev_warn(sev->dev, "SEV-TIO as incompatible with psp_init_on_probe=0\n");
> +#endif
>  		cmd = SEV_CMD_SNP_INIT_EX;
>  	} else {
>  		cmd = SEV_CMD_SNP_INIT;
> @@ -1487,6 +1500,24 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
>  	atomic_notifier_chain_register(&panic_notifier_list,
>  				       &snp_panic_notifier);
>  
> +#if defined(CONFIG_PCI_TSM)

Does this have to be here? If the functions are properly #ifdef'd in the
header file, then you won't have a compile issue and tio_en would only
be set if CONFIG_PCI_TSM is y.

> +	if (data.tio_en) {
> +		/*
> +		 * This executes with the sev_cmd_mutex held so down the stack
> +		 * snp_reclaim_pages(locked=false) might be needed (which is extremely
> +		 * unlikely) but will cause a deadlock.
> +		 * Instead of exporting __snp_alloc_firmware_pages(), allocate a page
> +		 * for this one call here.
> +		 */
> +		void *tio_status = page_address(__snp_alloc_firmware_pages(
> +			GFP_KERNEL_ACCOUNT | __GFP_ZERO, 0, true));
> +
> +		if (tio_status) {
> +			sev_tsm_init_locked(sev, tio_status);
> +			__snp_free_firmware_pages(virt_to_page(tio_status), 0, true);
> +		}
> +	}
> +#endif

Add a blank line here.

>  	sev_es_tmr_size = SNP_TMR_SIZE;
>  
>  	return 0;
> @@ -2766,7 +2797,22 @@ static void __sev_firmware_shutdown(struct sev_device *sev, bool panic)
>  
>  static void sev_firmware_shutdown(struct sev_device *sev)
>  {
> +#if defined(CONFIG_PCI_TSM)

Ditto on the #if here. Shouldn't the function be properly ifdef'd in the
header file which would not require this?

> +	/*
> +	 * Calling without sev_cmd_mutex held as TSM will likely try disconnecting
> +	 * IDE and this ends up calling sev_do_cmd() which locks sev_cmd_mutex.
> +	 */
> +	if (sev->tio_status)
> +		sev_tsm_uninit(sev);

Should this be part of __sev_firmware_shutdown() or
__sev_snp_shutdown_locked()?

> +#endif
> +
>  	mutex_lock(&sev_cmd_mutex);
> +
> +#if defined(CONFIG_PCI_TSM)

Ditto here. Without CONFIG_PCI_TSM, sev->tio_status will be NULL
already, so kfree() will just return.

> +	kfree(sev->tio_status);
> +	sev->tio_status = NULL;

Wouldn't it be safer to do this after the call to
__sev_firmware_shutdown() in case that function some day needs to use
that structure?

Thanks,
Tom

> +#endif
> +
>  	__sev_firmware_shutdown(sev, false);
>  	mutex_unlock(&sev_cmd_mutex);
>  }

Powered by blists - more mailing lists