Re: [PATCH v2 1/2] crypto: ccp - Initialize PSP when reading psp data file failed

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

On 8/16/22 14:32, Jacky Li wrote:
> Currently the OS fails the PSP initialization when the file specified at
> 'init_ex_path' does not exist or has invalid content. However the SEV
> spec just requires users to allocate 32KB of 0xFF in the file, which can
> be taken care of by the OS easily.
> 
> To improve the robustness during the PSP init, leverage the retry
> mechanism and continue the init process:
> 
> Before the first INIT_EX call, if the content is invalid or missing,
> continue the process by feeding those contents into PSP instead of
> aborting. PSP will then override it with 32KB 0xFF and return
> SEV_RET_SECURE_DATA_INVALID status code. In the second INIT_EX call,
> this 32KB 0xFF content will then be fed and PSP will write the valid
> data to the file.
> 
> In order to do this, sev_read_init_ex_file should only be called once
> for the first INIT_EX call. Calling it again for the second INIT_EX call
> will cause the invalid file content overwriting the valid 32KB 0xFF data
> provided by PSP in the first INIT_EX call.
> 
> Co-developed-by: Peter Gonda <pgonda@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Signed-off-by: Jacky Li <jackyli@google.com>
> Reported-by: Alper Gun <alpergun@google.com>
> ---
> Changelog since v1:
> - Add the message to indicate the possible file creation.
> - Return 0 when the file does not exist in sev_read_init_ex_file().
> - Move sev_read_init_ex_file() before the first call to INIT_EX.
> - Rephrase the last paragraph of the commit message.
> 
>   .../virt/kvm/x86/amd-memory-encryption.rst    |  5 ++-
>   drivers/crypto/ccp/sev-dev.c                  | 36 +++++++++++--------
>   2 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/virt/kvm/x86/amd-memory-encryption.rst b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> index 2d307811978c..935aaeb97fe6 100644
> --- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> +++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> @@ -89,9 +89,8 @@ context. In a typical workflow, this command should be the first command issued.
>   
>   The firmware can be initialized either by using its own non-volatile storage or
>   the OS can manage the NV storage for the firmware using the module parameter
> -``init_ex_path``. The file specified by ``init_ex_path`` must exist. To create
> -a new NV storage file allocate the file with 32KB bytes of 0xFF as required by
> -the SEV spec.
> +``init_ex_path``. If the file specified by ``init_ex_path`` does not exist or
> +is invalid, the OS will create or override the file with output from PSP.
>   
>   Returns: 0 on success, -negative on error
>   
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 9f588c9728f8..fb7ca45a2f0d 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -211,18 +211,24 @@ static int sev_read_init_ex_file(void)
>   	if (IS_ERR(fp)) {
>   		int ret = PTR_ERR(fp);
>   
> -		dev_err(sev->dev,
> -			"SEV: could not open %s for read, error %d\n",
> -			init_ex_path, ret);
> +		if (ret == -ENOENT) {
> +			dev_info(sev->dev,
> +				"SEV: %s does not exist and will be created later.\n",
> +				init_ex_path);
> +			ret = 0;
> +		} else {
> +			dev_err(sev->dev,
> +				"SEV: could not open %s for read, error %d\n",
> +				init_ex_path, ret);
> +		}
>   		return ret;
>   	}
>   
>   	nread = kernel_read(fp, sev_init_ex_buffer, NV_LENGTH, NULL);
>   	if (nread != NV_LENGTH) {
> -		dev_err(sev->dev,
> -			"SEV: failed to read %u bytes to non volatile memory area, ret %ld\n",
> +		dev_info(sev->dev,
> +			"SEV: could not read %u bytes to non volatile memory area, ret %ld\n",
>   			NV_LENGTH, nread);
> -		return -EIO;
>   	}
>   
>   	dev_dbg(sev->dev, "SEV: read %ld bytes from NV file\n", nread);
> @@ -410,17 +416,12 @@ static int __sev_init_locked(int *error)
>   static int __sev_init_ex_locked(int *error)
>   {
>   	struct sev_data_init_ex data;
> -	int ret;
>   
>   	memset(&data, 0, sizeof(data));
>   	data.length = sizeof(data);
>   	data.nv_address = __psp_pa(sev_init_ex_buffer);
>   	data.nv_len = NV_LENGTH;
>   
> -	ret = sev_read_init_ex_file();
> -	if (ret)
> -		return ret;
> -
>   	if (sev_es_tmr) {
>   		/*
>   		 * Do not include the encryption mask on the physical
> @@ -439,7 +440,7 @@ static int __sev_platform_init_locked(int *error)
>   {
>   	struct psp_device *psp = psp_master;
>   	struct sev_device *sev;
> -	int rc, psp_ret = -1;
> +	int rc = 0, psp_ret = -1;

This change doesn't look necessary, but not worth having you submit a v3.

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

>   	int (*init_function)(int *error);
>   
>   	if (!psp || !psp->sev_data)
> @@ -450,8 +451,15 @@ static int __sev_platform_init_locked(int *error)
>   	if (sev->state == SEV_STATE_INIT)
>   		return 0;
>   
> -	init_function = sev_init_ex_buffer ? __sev_init_ex_locked :
> -			__sev_init_locked;
> +	if (sev_init_ex_buffer) {
> +		init_function = __sev_init_ex_locked;
> +		rc = sev_read_init_ex_file();
> +		if (rc)
> +			return rc;
> +	} else {
> +		init_function = __sev_init_locked;
> +	}
> +
>   	rc = init_function(&psp_ret);
>   	if (rc && psp_ret == SEV_RET_SECURE_DATA_INVALID) {
>   		/*