Message-ID: <f6810eb0-3834-e5b4-6c1f-9422dfff5560@amd.com>
Date: Tue, 5 Nov 2024 15:58:43 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Dionna Glaze <dionnaglaze@...gle.com>, x86@...nel.org,
 linux-kernel@...r.kernel.org, Ashish Kalra <ashish.kalra@....com>,
 John Allen <john.allen@....com>, Herbert Xu <herbert@...dor.apana.org.au>,
 "David S. Miller" <davem@...emloft.net>, Michael Roth
 <michael.roth@....com>, Borislav Petkov <bp@...en8.de>
Cc: linux-crypto@...r.kernel.org
Subject: Re: [PATCH v4 4/6] crypto: ccp: Add DOWNLOAD_FIRMWARE_EX support

On 11/4/24 19:05, Dionna Glaze wrote:
> The DOWNLOAD_FIRMWARE_EX command requires cache flushing and introduces
> new error codes that could be returned to user space.
> 
> The function is in sev-dev.c rather than sev-fw.c to avoid exposing the
> __sev_do_cmd_locked function in the sev-dev.h header.

Please say "why" this is being added.

> 
> Signed-off-by: Dionna Glaze <dionnaglaze@...gle.com>
> ---
>  drivers/crypto/ccp/sev-dev.c | 66 +++++++++++++++++++++++++++++++-----
>  include/linux/psp-sev.h      | 26 ++++++++++++++
>  include/uapi/linux/psp-sev.h |  5 +++
>  3 files changed, 89 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 9265b6d534bbe..32f7b6147905e 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd)
>  	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
>  	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
>  	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
> +	case SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX:	return sizeof(struct sev_data_download_firmware_ex);
>  	default:				return 0;
>  	}
>  
> @@ -1597,14 +1598,7 @@ static int sev_update_firmware(struct device *dev)
>  		return -1;
>  	}
>  
> -	/*
> -	 * SEV FW expects the physical address given to it to be 32
> -	 * byte aligned. Memory allocated has structure placed at the
> -	 * beginning followed by the firmware being passed to the SEV
> -	 * FW. Allocate enough memory for data structure + alignment
> -	 * padding + SEV FW.
> -	 */
> -	data_size = ALIGN(sizeof(struct sev_data_download_firmware), 32);
> +	data_size = ALIGN(sizeof(struct sev_data_download_firmware), SEV_FW_ALIGNMENT);
>  
>  	order = get_order(firmware->size + data_size);
>  	p = alloc_pages(GFP_KERNEL, order);
> @@ -1645,6 +1639,62 @@ static int sev_update_firmware(struct device *dev)
>  	return ret;
>  }
>  
> +int sev_snp_download_firmware_ex(struct sev_device *sev, const u8 *data, u32 size, int *error)
> +{
> +	struct sev_data_download_firmware_ex *data_ex;
> +	int ret, order;
> +	struct page *p;
> +	u64 data_size;
> +	void *fw_dest;
> +
> +	data_size = ALIGN(sizeof(struct sev_data_download_firmware_ex),
> +			  SEV_FW_ALIGNMENT);

This can be a single line.

> +
> +	order = get_order(size + data_size);
> +	p = alloc_pages(GFP_KERNEL, order);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Copy firmware data to a kernel allocated contiguous
> +	 * memory region.
> +	 */
> +	data_ex = page_address(p);
> +	fw_dest = page_address(p) + data_size;
> +	memset(data_ex, 0, data_size);
> +	memcpy(fw_dest, data, size);
> +
> +	data_ex->address = __psp_pa(fw_dest);
> +	data_ex->len = size;
> +	data_ex->cmdlen = sizeof(struct sev_data_download_firmware_ex);
> +
> +	/*
> +	 * SNP_COMMIT should be issued explicitly to commit the updated
> +	 * firmware after guest context pages have been updated.
> +	 */
> +	ret = sev_do_cmd(SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX, data_ex, error);
> +

Remove blank line.

> +	if (ret)
> +		goto free_err;
> +
> +	__free_pages(p, order);

If you remove this...

> +
> +	/* Need to do a DF_FLUSH after live firmware update */
> +	wbinvd_on_all_cpus();
> +	ret = __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, error);
> +	if (ret) {
> +		dev_dbg(sev->dev, "DF_FLUSH error %d\n", *error);
> +		goto fw_err;

and remove this...

> +	}
> +
> +	return 0;

... you can remove this return 0 statement and the fw_err label.

> +
> +free_err:
> +	__free_pages(p, order);
> +fw_err:
> +	return ret;
> +}
> +
>  static int __sev_snp_shutdown_locked(int *error, bool panic)
>  {
>  	struct psp_device *psp = psp_master;
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 903ddfea85850..6a08c56cd9771 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -16,6 +16,15 @@
>  
>  #define SEV_FW_BLOB_MAX_SIZE	0x4000	/* 16KB */
>  
> +/*
> + * SEV FW expects the physical address given to it to be 32
> + * byte aligned. Memory allocated has structure placed at the
> + * beginning followed by the firmware being passed to the SEV
> + * FW. Allocate enough memory for data structure + alignment
> + * padding + SEV FW.
> + */
> +#define SEV_FW_ALIGNMENT       32

Does this need to be in include/linux/psp-sev.h or can it go in
drivers/crypto/ccp/sev-dev.h ?

> +
>  /**
>   * SEV platform state
>   */
> @@ -185,6 +194,23 @@ struct sev_data_download_firmware {
>  	u32 len;				/* In */
>  } __packed;
>  
> +/**
> + * struct sev_data_download_firmware_ex - DOWNLOAD_FIRMWARE_EX command parameters
> + *
> + * @length: length of this command buffer
> + * @address: physical address of firmware image
> + * @len: len of the firmware image
> + * @commit: automatically commit the newly installed image
> + */
> +struct sev_data_download_firmware_ex {
> +	u32 cmdlen;				/* In */

This doesn't match the name above in the comment.

> +	u32 reserved;				/* In */
> +	u64 address;				/* In */
> +	u32 len;				/* In */
> +	u32 commit:1;				/* In */
> +	u32 reserved2:31;			/* In */

These names typically match what is in the spec, so I would expect to
see length, fw_paddr (or fw_address), and fw_len.

> +} __packed;
> +
>  /**
>   * struct sev_data_get_id - GET_ID command parameters
>   *
> diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
> index 832c15d9155bd..936464d4f282a 100644
> --- a/include/uapi/linux/psp-sev.h
> +++ b/include/uapi/linux/psp-sev.h
> @@ -80,6 +80,11 @@ typedef enum {
>  	SEV_RET_INVALID_PAGE_OWNER,
>  	SEV_RET_INVALID_PAGE_AEAD_OFLOW,
>  	SEV_RET_RMP_INIT_REQUIRED,
> +	SEV_RET_BAD_SVN,
> +	SEV_RET_BAD_VERSION,
> +	SEV_RET_SHUTDOWN_REQUIRED,
> +	SEV_RET_UPDATE_FAILED,
> +	SEV_RET_RESTORE_REQUIRED,

Hmmm... yeah, as Alexey mentioned, these return values are not correct.

SEV_RET_INVALID_PAGE_SIZE should follow SEV_RET_SECURE_DATA_INVALID

so "SEV_RET_INVALID_KEY = 0x27" should be moved after
SEV_RET_RMP_INIT_REQUIRED

and SEV_RET_RMP_INIT_REQUIRED should be = 0x20, since RB_MODE_EXITED is
0x1f.

And then you might as well include RMP_INIT_FAILED in the list.

The list should look like:

	...
	SEV_RET_SECURE_DATA_INVALID,
	SEV_RET_INVALID_PAGE_SIZE,
	SEV_RET_INVALID_PAGE_STATE,
	SEV_RET_INVALID_MDATA_ENTRY,
	SEV_RET_INVALID_PAGE_OWNER,
	SEV_RET_AEAD_OFLOW,
	SEV_RET_RB_MODE_EXITED,
	SEV_RET_RMP_INIT_REQUIRED,
	SEV_RET_BAD_SVN,
	SEV_RET_BAD_VERSION,
	SEV_RET_SHUTDOWN_REQUIRED,
	SEV_RET_UPDATE_FAILED,
	SEV_RET_RESTORE_REQUIRED,
	SEV_RET_RMP_INIT_FAILED,
	SEV_RET_INVALID_KEY,
	SEV_RET_MAX,

Of course this is already upstream in uapi, so I don't know how it is
supposed to be handled.

Thanks,
Tom

>  	SEV_RET_MAX,
>  } sev_ret_code;
>  

Powered by blists - more mailing lists