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

Tom Lendacky
2026-04-17T13:48:28Z

On 4/16/26 18:23, Sean Christopherson wrote:
> From: Tycho Andersen <tycho@kernel.org>
>
> ...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@gondor.apana.org.au>
> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

One minor comment below, but not a big deal.

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

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