Re: [PATCH kernel v2 5/5] crypto/ccp: Implement SEV-TIO PCIe IDE (phase1)
From: Alexey Kardashevskiy
Date: Mon Dec 01 2025 - 21:05:00 EST
On 2/12/25 02:23, Tom Lendacky wrote:
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@xxxxxxx>
---
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;
okay for formatting but...
This makes it easier to understand. Please do this everywhere you define
bitfields.
...I really want to keep the union here (do not care in other places though) for easier comparison of a whole structure.
+ };
+ };
+} __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
The module parameter won't do anything, why keep it exposed, only because of ifdefs?
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.
If psp_init_on_probe is not set, then systemd (or modprobe?) loads kvm_amd and at that point SEV init is delayed but SNP init is not so SEV-TIO gets enabled.
Then, there is some systemd service in my test Ubuntu which:
1) runs QEMU to discover something, with SEV enabled, that trigger SEV_PDH_CERT_EXPORT
2) the kernel ioctl handler has to initialize SEV
3) sev_move_to_init_state() returns shutdown_required=true (it does not distinguish SEV and SNP)
4) the SEV_PDH_CERT_EXPORT handler shuts down both SEV and SNP (which includes SEV-TIO).
The right thing to do is just not use psp_init_on_probe as it is really a debugging knob. But people are going to use it while DOWNLOAD_EX (which we need this psp_init_on_probe thing for) and SEV-TIO are still in their infancy. It took me half a day to sort this all in my head, hence the check.
I will remove it from the above but leave the warning below and add the comment:
/*
* When psp_init_on_probe is disabled, the userspace calling SEV ioctl
* can inadvertently shut down SNP and SEV-TIO during initialization,
* causing unexpected state loss.
*/
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.
I am open coding sev_tio_supported(), and ditching 4/5, seems pointless as hardly anyone will want to enable just TIO in the PSP without the host os support for it, right?
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()?
As the comment above explains, the sev_cmd_mutex is a problem. I'd have to have to modes of tsm::disconnect - one triggered by the user and called without sev_cmd_mutex, and the other one coming from here with sev_cmd_mutex.
+#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.
The ifdef is here because there was one in sev-dev.h, I am dropping it now.
+ 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?
Uff. Right, __sev_firmware_shutdown() shuts down both SNP and SEV, I'll move that down.
Thanks,
Thanks for the review!
Tom
+#endif
+
__sev_firmware_shutdown(sev, false);
mutex_unlock(&sev_cmd_mutex);
}
--
Alexey