Re: [PATCH v4 2/5] crypto: ccp: Add support for SNP_FEATURE_INFO command

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

Hello Tom,

On 6/3/2025 10:21 AM, Tom Lendacky wrote:
> On 5/19/25 18:56, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> The FEATURE_INFO command provides host and guests a programmatic means
> 
> s/provides host and guests/provides hypervisors/
> 
>> to learn about the supported features of the currently loaded firmware.
>> FEATURE_INFO command leverages the same mechanism as the CPUID instruction.
>> Instead of using the CPUID instruction to retrieve Fn8000_0024,
>> software can use FEATURE_INFO.
>>
>> The hypervisor may provide Fn8000_0024 values to the guest via the CPUID
>> page in SNP_LAUNCH_UPDATE. As with all CPUID output recorded in that page,
>> the hypervisor can filter Fn8000_0024. The firmware will examine
>> Fn8000_0024 and apply its CPUID policy.
> 
> This paragraph has nothing to do with this patch, please remove it.

Ok.

> 
>>
>> Switch to using SNP platform status instead of SEV platform status if
>> SNP is enabled and cache SNP platform status and feature information
>> from CPUID 0x8000_0024, sub-function 0, in the sev_device structure.
> 
> Since the SEV platform status and SNP platform status differ, I think this
> patch should be split into two separate patches.
> 
> The first first patch would cache the current SEV platform status return
> structure and eliminate the separate state field (as state is unique
> between SEV and SNP).

Eliminate the state field ? 

But isn't the sev_device->state field used also as driver's internal state information
and not directly mapped to platform status, except the initial state is the platform state 
on module load, so why to remove the field altogether ?

> The api_major/api_minor/build can probably remain,
> since the same value *should* be reported for both SNP and SEV platform
> status command.
> 

Ok. 

> The second patch would cache the SNP platform status and feature info
> data, with this status data being used for the api_major/api_minor/build.
> 

Ok. 

>>
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> ---
>>  drivers/crypto/ccp/sev-dev.c | 81 ++++++++++++++++++++++++++++++++++++
>>  drivers/crypto/ccp/sev-dev.h |  3 ++
>>  include/linux/psp-sev.h      | 29 +++++++++++++
>>  3 files changed, 113 insertions(+)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 3451bada884e..b642f1183b8b 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -233,6 +233,7 @@ static int sev_cmd_buffer_len(int cmd)
>>  	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
>>  	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
>>  	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);
>>  	default:				return 0;
>>  	}
>>  
>> @@ -1073,6 +1074,69 @@ static void snp_set_hsave_pa(void *arg)
>>  	wrmsrq(MSR_VM_HSAVE_PA, 0);
>>  }
>>  
>> +static int snp_get_platform_data(struct sev_user_data_status *status, int *error)
>> +{
>> +	struct sev_data_snp_feature_info snp_feat_info;
>> +	struct sev_device *sev = psp_master->sev_data;
>> +	struct snp_feature_info *feat_info;
>> +	struct sev_data_snp_addr buf;
>> +	struct page *page;
>> +	int rc;
>> +
>> +	/*
>> +	 * The output buffer must be firmware page if SEV-SNP is
>> +	 * initialized.
>> +	 */
> 
> This comment should be expanded and say that this function is intended to
> be called when SNP is not initialized or you make this work for both
> situations.
>

Ok, i believe i will simply make it work for the case when SNP is not initialized, as that
is the way it is being called. 
 
>> +	if (sev->snp_initialized)
>> +		return -EINVAL;
>> +
>> +	buf.address = __psp_pa(&sev->snp_plat_status);
>> +	rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
>> +
> 
> Remove blank line.
> 
>> +	if (rc) {
>> +		dev_err(sev->dev, "SNP PLATFORM_STATUS command failed, ret = %d, error = %#x\n",
>> +			rc, *error);
>> +		return rc;
>> +	}
>> +
>> +	status->api_major = sev->snp_plat_status.api_major;
>> +	status->api_minor = sev->snp_plat_status.api_minor;
>> +	status->build = sev->snp_plat_status.build_id;
>> +	status->state = sev->snp_plat_status.state;
> 
> This may need to be moved based on how the patches lay out.
> 
>> +
>> +	/*
>> +	 * Do feature discovery of the currently loaded firmware,
>> +	 * and cache feature information from CPUID 0x8000_0024,
>> +	 * sub-function 0.
>> +	 */
>> +	if (sev->snp_plat_status.feature_info) {
>> +		/*
>> +		 * Use dynamically allocated structure for the SNP_FEATURE_INFO
>> +		 * command to handle any alignment and page boundary check
>> +		 * requirements.
>> +		 */
>> +		page = alloc_page(GFP_KERNEL);
>> +		if (!page)
>> +			return -ENOMEM;
> 
> Add a blank line.
> 
>> +		feat_info = page_address(page);
>> +		snp_feat_info.length = sizeof(snp_feat_info);
>> +		snp_feat_info.ecx_in = 0;
>> +		snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
>> +
>> +		rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, error);
>> +
> 
> Remove blank line.
> 
>> +		if (!rc)
>> +			sev->feat_info = *feat_info;
>> +		else
>> +			dev_err(sev->dev, "SNP FEATURE_INFO command failed, ret = %d, error = %#x\n",
>> +				rc, *error);
>> +
>> +		__free_page(page);
>> +	}
>> +
>> +	return rc;
>> +}
>> +
>>  static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
>>  {
>>  	struct sev_data_range_list *range_list = arg;
>> @@ -1597,6 +1661,23 @@ static int sev_get_api_version(void)
>>  	struct sev_user_data_status status;
>>  	int error = 0, ret;
>>  
>> +	/*
>> +	 * Use SNP platform status if SNP is enabled and cache
>> +	 * SNP platform status and SNP feature information.
>> +	 */
>> +	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) {
>> +		ret = snp_get_platform_data(&status, &error);
>> +		if (ret) {
>> +			dev_err(sev->dev,
>> +				"SEV-SNP: failed to get status. Error: %#x\n", error);
>> +			return 1;
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * Fallback to SEV platform status if SNP is not enabled
>> +	 * or SNP platform status fails.
>> +	 */
> 
> I think this comment is incorrect, aren't you calling this on success of
> snp_get_platform_data() and returning on error?

Yes. 

> 
> You want both platform status outputs cached. So the above behavior is
> correct, I believe, that we error out on SNP platform status failure.
> 

Why do we want to cache *both* SEV and SNP platform status, as of now only api_major & minor, build and state
fields are used from SEV platform status --- is this just for future use cases ?

Thanks,
Ashish

> Thanks,
> Tom
> 
>>  	ret = sev_platform_status(&status, &error);
>>  	if (ret) {
>>  		dev_err(sev->dev,
>> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
>> index 3e4e5574e88a..1c1a51e52d2b 100644
>> --- a/drivers/crypto/ccp/sev-dev.h
>> +++ b/drivers/crypto/ccp/sev-dev.h
>> @@ -57,6 +57,9 @@ struct sev_device {
>>  	bool cmd_buf_backup_active;
>>  
>>  	bool snp_initialized;
>> +
>> +	struct sev_user_data_snp_status snp_plat_status;
>> +	struct snp_feature_info feat_info;
>>  };
>>  
>>  int sev_dev_init(struct psp_device *psp);
>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>> index 0b3a36bdaa90..0149d4a6aceb 100644
>> --- a/include/linux/psp-sev.h
>> +++ b/include/linux/psp-sev.h
>> @@ -107,6 +107,7 @@ enum sev_cmd {
>>  	SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX = 0x0CA,
>>  	SEV_CMD_SNP_COMMIT		= 0x0CB,
>>  	SEV_CMD_SNP_VLEK_LOAD		= 0x0CD,
>> +	SEV_CMD_SNP_FEATURE_INFO	= 0x0CE,
>>  
>>  	SEV_CMD_MAX,
>>  };
>> @@ -812,6 +813,34 @@ struct sev_data_snp_commit {
>>  	u32 len;
>>  } __packed;
>>  
>> +/**
>> + * struct sev_data_snp_feature_info - SEV_SNP_FEATURE_INFO structure
>> + *
>> + * @length: len of the command buffer read by the PSP
>> + * @ecx_in: subfunction index
>> + * @feature_info_paddr : SPA of the FEATURE_INFO structure
>> + */
>> +struct sev_data_snp_feature_info {
>> +	u32 length;
>> +	u32 ecx_in;
>> +	u64 feature_info_paddr;
>> +} __packed;
>> +
>> +/**
>> + * struct feature_info - FEATURE_INFO structure
>> + *
>> + * @eax: output of SNP_FEATURE_INFO command
>> + * @ebx: output of SNP_FEATURE_INFO command
>> + * @ecx: output of SNP_FEATURE_INFO command
>> + * #edx: output of SNP_FEATURE_INFO command
>> + */
>> +struct snp_feature_info {
>> +	u32 eax;
>> +	u32 ebx;
>> +	u32 ecx;
>> +	u32 edx;
>> +} __packed;
>> +
>>  #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>  
>>  /**