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

[dan.j.williams]

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@xxxxxxx>
> 
> Just some minor comments below. After those are addressed:
> 
> For the ccp related changes in the whole series:
> 
> Acked-by: Tom Lendacky <thomas.lendacky@xxxxxxx>

Thanks, Tom. Given Alexey is likely sleeping and this needs to start
soaking in linux-next today to have a chance, I will take a stab at
these fixups.

> > 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 think it makes sense to hide options that have no effect.

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

Yeah, I like that side effect.

-- >8 --