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

["Kalra, Ashish" <ashish.kalra@amd.com>]

On 12/2/2025 4:30 PM, Alexey Kardashevskiy wrote:
> 
> 
> 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,
> 

I think we still need to support "psp_init_on_probe" to support deferred SEV initialization and continue supporting SEV INIT_EX.

Thanks,
Ashish

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