Re: [Patch v3 5/7] crypto: ccp - Allow SNP platform data to be queried after SNP INIT

["Pratik R. Sampat" <prsampat@amd.com>]

On 10/6/26 9:45 AM, Tom Lendacky wrote:
> On 10/5/26 11:15, Pratik R. Sampat wrote:
>> In preparation for refreshing the cached SNP platform status and feature
>> information after a successful firmware live update from
>> DOWNLOAD_FIRMWARE_EX, allow snp_get_platform_data() to be called while
>> the SNP firmware is in the INIT state.
>>
>> Once SNP is initialized the firmware requires the output page of both
>> commands to be firmware-owned. sev->snp_plat_status cannot satisfy that
>> as it is embedded in struct sev_device, so use
>> __sev_do_snp_platform_status(), which stages the output through a
>> dedicated page. Allocate the SNP_FEATURE_INFO output page with
>> __snp_alloc_firmware_pages(), which transitions the page to
>> firmware-owned when SNP is initialized, and release it with
>> __snp_free_firmware_pages(), which reclaims it before freeing.
>>
>> Typically this was only ever exercised during initialization. However
>> DOWNLOAD_FIRMWARE_EX can change that. Live firmware update can race with
>> IOCTLs such as SEV_FACTORY_RESET which could cause operations performed
>> on incorrect cached platform states. Avoid this race by wrapping the
>> function under the sev_cmd_mutex.
>>
>> Co-developed-by: Tycho Andersen (AMD) <tycho@kernel.org>
>> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
>> Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
> 
> Just a minor comment below.
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> 
>> ---
>>  drivers/crypto/ccp/sev-dev.c | 27 ++++++++++++---------------
>>  1 file changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 1ed9e61a95cc..c40401c861d3 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -131,6 +131,8 @@ static void __sev_firmware_shutdown(struct sev_device *sev, bool panic);
>>  
>>  static int snp_shutdown_on_panic(struct notifier_block *nb,
>>  				 unsigned long reason, void *arg);
>> +static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
>> +					int *error);
>>  
>>  static struct notifier_block snp_panic_notifier = {
>>  	.notifier_call = snp_shutdown_on_panic,
>> @@ -1261,19 +1263,12 @@ static int snp_get_platform_data(struct sev_device *sev, int *error)
>>  {
>>  	struct sev_data_snp_feature_info snp_feat_info;
>>  	struct snp_feature_info *feat_info;
>> -	struct sev_data_snp_addr buf;
>>  	struct page *page;
>>  	int rc;
>>  
>> -	/*
>> -	 * This function is expected to be called before SNP is
>> -	 * initialized.
>> -	 */
>> -	if (sev->snp_initialized)
>> -		return -EINVAL;
>> +	lockdep_assert_held(&sev_cmd_mutex);
>>  
>> -	buf.address = __psp_pa(&sev->snp_plat_status);
>> -	rc = sev_do_cmd(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
>> +	rc = __sev_do_snp_platform_status(&sev->snp_plat_status, error);
>>  	if (rc) {
>>  		dev_err(sev->dev, "SNP PLATFORM_STATUS command failed, ret = %d, error = %#x\n",
>>  			rc, *error);
>> @@ -1297,23 +1292,23 @@ static int snp_get_platform_data(struct sev_device *sev, int *error)
>>  	 * command to ensure structure is 8-byte aligned, and does not
>>  	 * cross a page boundary.
>>  	 */
>> -	page = alloc_page(GFP_KERNEL);
>> -	if (!page)
>> +	page = __snp_alloc_firmware_pages(GFP_KERNEL, 0, true);
>> +	feat_info =  page ? page_address(page) : NULL;
>> +	if (!feat_info)
>>  		return -ENOMEM;
>>  
>> -	feat_info = page_address(page);
> 
> This is a bit of churn when all you have to do is replace alloc_page()
> with __snp_alloc_firmware_pages(), right?
> 

I'll clean that right up!

Thanks,
--Pratik

> Thanks,
> Tom
> 
>>  	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(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, error);
>> +	rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, error);
>>  	if (!rc)
>>  		sev->snp_feat_info_0 = *feat_info;
>>  	else
>>  		dev_err(sev->dev, "SNP FEATURE_INFO command failed, ret = %d, error = %#x\n",
>>  			rc, *error);
>>  
>> -	__free_page(page);
>> +	__snp_free_firmware_pages(page, 0, true);
>>  
>>  	return rc;
>>  }
>> @@ -2097,6 +2092,8 @@ static int sev_get_api_version(void)
>>  	struct sev_user_data_status status;
>>  	int error = 0, ret;
>>  
>> +	guard(mutex)(&sev_cmd_mutex);
>> +
>>  	/*
>>  	 * Cache SNP platform status and SNP feature information
>>  	 * if SNP is available.
>> @@ -2107,7 +2104,7 @@ static int sev_get_api_version(void)
>>  			return 1;
>>  	}
>>  
>> -	ret = sev_platform_status(&status, &error);
>> +	ret = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &status, &error);
>>  	if (ret) {
>>  		dev_err(sev->dev,
>>  			"SEV: failed to get status. Error: %#x\n", error);
>