Re: [PATCH v2 7/9] crypto: ccp: Add new SEV/SNP platform initialization API

[Alexey Kardashevskiy]

On 17/12/24 10:59, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@xxxxxxx>
>
> Add new SNP platform initialization API to allow separate SEV and SNP
> initialization.
>
> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> ---
>   drivers/crypto/ccp/sev-dev.c | 15 +++++++++++++++
>   include/linux/psp-sev.h      | 17 +++++++++++++++++
>   2 files changed, 32 insertions(+)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 001e7a401a6d..53c438b2b712 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1375,6 +1375,21 @@ int sev_platform_init(struct sev_platform_init_args *args)
>   }
>   EXPORT_SYMBOL_GPL(sev_platform_init);
>   
> +int sev_snp_platform_init(struct sev_platform_init_args *args)
> +{
> +	int rc;
> +
> +	if (!psp_master || !psp_master->sev_data)
> +		return -ENODEV;
> +
> +	mutex_lock(&sev_cmd_mutex);


I'm told that in 2024 we should use guard(mutex)(&sev_cmd_mutex) and 
drop explicit mutex_unlock(). I'm not a huge fan but there is a point :)


> +	rc = __sev_snp_init_locked(&args->error);
> +	mutex_unlock(&sev_cmd_mutex);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(sev_snp_platform_init);
> +
>   static int __sev_platform_shutdown_locked(int *error)
>   {
>   	struct psp_device *psp = psp_master;
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 335b29b31457..e50643aef8a9 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -828,6 +828,21 @@ struct sev_data_snp_commit {
>    */
>   int sev_platform_init(struct sev_platform_init_args *args);
>   
> +/**
> + * sev_snp_platform_init - perform SNP INIT command
> + *
> + * @args: struct sev_platform_init_args to pass in arguments
> + *
> + * Returns:
> + * 0 if the SEV successfully processed the command
> + * -%ENODEV    if the SNP support is not enabled
> + * -%ENOMEM    if the SNP range list allocation failed
> + * -%E2BIG     if the HV_Fixed list is too big
> + * -%ETIMEDOUT if the SEV command timed out
> + * -%EIO       if the SEV returned a non-zero return code

The only caller ignores these, may be drop the returning value and print 
the errors inside sev_snp_platform_init() (if whatever 
__sev_snp_init_locked() already prints is not enough)?

Also, looks like 5/9 6/9 7/9 can be squashed into one patch, they touch 
the same files, equally do nothing until later patches, pretty straight 
forward. Thanks,


> + */
> +int sev_snp_platform_init(struct sev_platform_init_args *args);
> +
>   /**
>    * sev_platform_status - perform SEV PLATFORM_STATUS command
>    *
> @@ -955,6 +970,8 @@ sev_platform_status(struct sev_user_data_status *status, int *error) { return -E
>   
>   static inline int sev_platform_init(struct sev_platform_init_args *args) { return -ENODEV; }
>   
> +static inline int sev_snp_platform_init(struct sev_platform_init_args *args) { return -ENODEV; }
> +
>   static inline int
>   sev_guest_deactivate(struct sev_data_deactivate *data, int *error) { return -ENODEV; }
>   

--
Alexey