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

[Pratik R. Sampat]

Hi Tom,

Thanks for the review.

On 9/10/26 4:09 PM, Tom Lendacky wrote:
> On 9/10/26 12:02, 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, and mark/reclaim the SNP_FEATURE_INFO page around the
>> command.
>>
>> Co-developed-by: Tycho Andersen (AMD) <tycho@xxxxxxxxxx>
>> Signed-off-by: Tycho Andersen (AMD) <tycho@xxxxxxxxxx>
>> Signed-off-by: Pratik R. Sampat <prsampat@xxxxxxx>
>> ---
>>  drivers/crypto/ccp/sev-dev.c | 44 ++++++++++++++++++++++++++++--------
>>  1 file changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 1ed9e61a95cc..e891d6d1c6f0 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;
>> -
>> -	buf.address = __psp_pa(&sev->snp_plat_status);
>> -	rc = sev_do_cmd(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
>> +	mutex_lock(&sev_cmd_mutex);
>> +	rc = __sev_do_snp_platform_status(&sev->snp_plat_status, error);
>> +	mutex_unlock(&sev_cmd_mutex);
>>  	if (rc) {
>>  		dev_err(sev->dev, "SNP PLATFORM_STATUS command failed, ret = %d, error = %#x\n",
>>  			rc, *error);
>> @@ -1302,17 +1297,46 @@ static int snp_get_platform_data(struct sev_device *sev, int *error)
>>  		return -ENOMEM;
>>  
>>  	feat_info = page_address(page);
>> +
>> +	/* If SNP is initialized, transition to use a firmware-owned page */
>> +	if (sev->snp_initialized) {
>> +		if (rmp_mark_pages_firmware(__pa(feat_info), 1, false)) {
>> +			*error = SEV_RET_NO_FW_CALL;
>> +			rc = -EFAULT;
>> +			goto free_page;
>> +		}
>> +	}
>> +
> 
> You could change to use snp_alloc_firmware_page() which will allocate
> the page and place it in the proper state for you.
> 
>>  	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);
>> +
>> +	/*
>> +	 * The feature_info page will be in reclaim state on success, or left
>> +	 * in firmware state on failure. Transition the pages back to
>> +	 * Hypervisor-owned state.
>> +	 *
>> +	 * snp_reclaim_pages() has already pinned the page via snp_leak_pages()
>> +	 * if it could not do so, which keeps it away from the allocator. The
>> +	 * reference taken here is dropped either way.
>> +	 */
>> +	if (sev->snp_initialized) {
>> +		if (snp_reclaim_pages(__pa(feat_info), 1, false)) {
>> +			*error = SEV_RET_NO_FW_CALL;
>> +			rc = -EFAULT;
>> +			goto free_page;
>> +		}
>> +	}
> 
> And then snp_free_firmware_page() here (after you copy the data -
> similar to what you did in snp_verify_mitigation()).
> 

Sure, I'll use snp_alloc_firmware_page + snp_free_firmware_page() and make
things cleaner.

> (I think the same changes apply to __sev_do_snp_platform_status(), but
> don't worry about it for this series)
> 

Right. The only difference I believe would be to use
__snp_alloc_firmware_pages() and __snp_free_firmware_pages() with locked=true
since that is always called with the sev_cmd_mutex held.

I could send that as a separate clean up patch.

Thanks!
--Pratik

> Thanks,
> Tom
> 
>> +
>>  	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:
>>  	__free_page(page);
>>  
>>  	return rc;
>