Re: [PATCH kernel v3 4/4] crypto/ccp: Implement SEV-TIO PCIe IDE (phase1)

[Tom Lendacky <thomas.lendacky@amd.com>]

On 12/1/25 20:44, 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.
> 
> Detect the PSP support (reported via FEATURE_INFO + SNP_PLATFORM_STATUS)
> and enable SEV-TIO in the SNP_INIT_EX call if the hardware supports TIO.
> 
> 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@amd.com>

Just some minor comments below. After those are addressed:

For the ccp related changes in the whole series:

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
> Changes:
> v2:
> * moved declarations from sev-dev-tio.h to sev-dev.h
> * removed include "sev-dev-tio.h" from sev-dev.c to fight errors when TSM is disabled
> * converted /** to /* as these are part of any external API and trigger unwanted kerneldoc warnings
> * got rid of ifdefs
> * "select PCI_TSM" moved under CRYPTO_DEV_SP_PSP
> * open coded SNP_SEV_TIO_SUPPORTED
> * renamed tio_present to tio_supp to match the flag name
> * merged "crypto: ccp: Enable SEV-TIO feature in the PSP when supported" to this one
> ---
>  drivers/crypto/ccp/Kconfig       |   1 +
>  drivers/crypto/ccp/Makefile      |   4 +
>  drivers/crypto/ccp/sev-dev-tio.h | 123 +++
>  drivers/crypto/ccp/sev-dev.h     |   9 +
>  include/linux/psp-sev.h          |  11 +-
>  drivers/crypto/ccp/sev-dev-tio.c | 864 ++++++++++++++++++++
>  drivers/crypto/ccp/sev-dev-tsm.c | 405 +++++++++
>  drivers/crypto/ccp/sev-dev.c     |  51 +-
>  8 files changed, 1465 insertions(+), 3 deletions(-)
> 

> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 9e0c16b36f9c..d6095d1467b3 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -75,6 +75,10 @@ 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");
>  
> +static bool sev_tio_enabled = IS_ENABLED(CONFIG_PCI_TSM);
> +module_param_named(tio, sev_tio_enabled, bool, 0444);
> +MODULE_PARM_DESC(tio, "Enables TIO in SNP_INIT_EX");

Hmmm... I thought you said you wanted to hide the module parameter if
CONFIG_PCI_TSM isn't enabled. Either way, it's fine.

> +
>  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 +255,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;
> @@ -1434,6 +1438,19 @@ 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);
> +
> +		bool tio_supp = !!(sev->snp_feat_info_0.ebx & SNP_SEV_TIO_SUPPORTED);

Please put the variable definition at the top of the "if" block instead
of in the middle of the code.
> +
> +		data.tio_en = tio_supp && sev_tio_enabled && amd_iommu_sev_tio_supported();

Don't you still want to take CONFIG_PCI_TSM into account?

	data.tio_en = IS_ENABLED(CONFIG_PCI_TSM) && tio_supp && sev_tio_enabled && amd_iommu_sev_tio_supported();

or
	if (IS_ENABLED(CONFIG_PCI_TSM)
		data.tio_en = tio_supp && sev_tio_enabled && amd_iommu_sev_tio_supported();

But if you change back to #ifdef the module parameter, then you won't
need the IS_ENABLED() check here because sev_tio_enabled will be set
based on CONFIG_PCI_TSM and will be false and not changeable if
CONFIG_PCI_TSM is not y.
> +
> +		/*
> +		 * When psp_init_on_probe is disabled, the userspace calling
> +		 * SEV ioctl can inadvertently shut down SNP and SEV-TIO causing
> +		 * unexpected state loss.
> +		 */

After this is merged, lets see if sev_move_to_init_state() can be
cleaned up to avoid this situation.

Thanks,
Tom

> +		if (data.tio_en && !psp_init_on_probe)
> +			dev_warn(sev->dev, "SEV-TIO as incompatible with psp_init_on_probe=0\n");
> +
>  		cmd = SEV_CMD_SNP_INIT_EX;
>  	} else {
>  		cmd = SEV_CMD_SNP_INIT;
> @@ -1471,7 +1488,8 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
>  
>  	snp_hv_fixed_pages_state_update(sev, HV_FIXED);
>  	sev->snp_initialized = true;
> -	dev_dbg(sev->dev, "SEV-SNP firmware initialized\n");
> +	dev_dbg(sev->dev, "SEV-SNP firmware initialized, SEV-TIO is %s\n",
> +		data.tio_en ? "enabled" : "disabled");
>  
>  	dev_info(sev->dev, "SEV-SNP API:%d.%d build:%d\n", sev->api_major,
>  		 sev->api_minor, sev->build);
> @@ -1479,6 +1497,23 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
>  	atomic_notifier_chain_register(&panic_notifier_list,
>  				       &snp_panic_notifier);
>  
> +	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);
> +		}
> +	}
> +
>  	sev_es_tmr_size = SNP_TMR_SIZE;
>  
>  	return 0;
> @@ -2758,8 +2793,20 @@ static void __sev_firmware_shutdown(struct sev_device *sev, bool panic)
>  
>  static void sev_firmware_shutdown(struct sev_device *sev)
>  {
> +	/*
> +	 * 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);
> +
>  	mutex_lock(&sev_cmd_mutex);
> +
>  	__sev_firmware_shutdown(sev, false);
> +
> +	kfree(sev->tio_status);
> +	sev->tio_status = NULL;
> +
>  	mutex_unlock(&sev_cmd_mutex);
>  }
>