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

[Tom Lendacky <thomas.lendacky@amd.com>]

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?

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