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

[Alexey Kardashevskiy <aik@amd.com>]

On 3/12/25 01:52, Tom Lendacky wrote:
> 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>

Dan did it right (thanks Dan!).


> 
>> ---
>> 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.

I did but you did not and I do not care that much :)

> 
>> +
>>   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.


Ah true. I thought sev_tio_enabled=IS_ENABLED(CONFIG_PCI_TSM) does it but missed that sev_tio_enabled is exported as a parameter so not a constant at compile time.


>> +
>> +		/*
>> +		 * 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.

Do we want to keep psp_init_on_probe, why? Thanks,


> 
> 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);
>>   }
>>   
> 

-- 
Alexey