Re: [PATCH v3 1/7] crypto/ccp: hoist kernel part of SNP_PLATFORM_STATUS

[Tom Lendacky]

On 4/16/26 18:23, Sean Christopherson wrote:
> From: Tycho Andersen <tycho@xxxxxxxxxx>
> 
> ...to its own function. This way it can be used when the kernel needs
> access to the platform status regardless of the INIT state of the firmware.
> 
> No functional change intended.
> 
> Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Tycho Andersen (AMD) <tycho@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>

One minor comment below, but not a big deal.

Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>

> ---
>  drivers/crypto/ccp/sev-dev.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index aebf4dad545e..64fc402f58df 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -2367,7 +2367,8 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
>  	return ret;
>  }
>  
> -static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
> +static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
> +					int *error)
>  {
>  	struct sev_device *sev = psp_master->sev_data;
>  	struct sev_data_snp_addr buf;
> @@ -2375,9 +2376,6 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
>  	void *data;
>  	int ret;
>  
> -	if (!argp->data)
> -		return -EINVAL;
> -
>  	status_page = alloc_page(GFP_KERNEL_ACCOUNT);
>  	if (!status_page)
>  		return -ENOMEM;
> @@ -2400,7 +2398,7 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
>  	}
>  
>  	buf.address = __psp_pa(data);
> -	ret = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, &argp->error);
> +	ret = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
>  
>  	if (sev->snp_initialized) {
>  		/*
> @@ -2415,15 +2413,32 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
>  	if (ret)
>  		goto cleanup;
>  
> -	if (copy_to_user((void __user *)argp->data, data,
> -			 sizeof(struct sev_user_data_snp_status)))
> -		ret = -EFAULT;
> +	memcpy(status, data, sizeof(*status));
>  
>  cleanup:
>  	__free_pages(status_page, 0);
>  	return ret;
>  }
>  
> +static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
> +{
> +	struct sev_user_data_snp_status status;
> +	int ret;
> +
> +	if (!argp->data)
> +		return -EINVAL;
> +
> +	ret = __sev_do_snp_platform_status(&status, &argp->error);
> +	if (ret < 0)

Any non-zero value is an error, so this really should just be "if (ret)"

Thanks,
Tom

> +		return ret;
> +
> +	if (copy_to_user((void __user *)argp->data, &status,
> +			 sizeof(struct sev_user_data_snp_status)))
> +		ret = -EFAULT;
> +
> +	return ret;
> +}
> +
>  static int sev_ioctl_do_snp_commit(struct sev_issue_cmd *argp)
>  {
>  	struct sev_device *sev = psp_master->sev_data;