| Message ID | 20241105010558.1266699-5-dionnaglaze@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support SEV firmware hotloading | expand |
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@google.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; >
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); + + 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); + + if (ret) + goto free_err; + + __free_pages(p, order); + + /* 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; + } + + return 0; + +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 + /** * 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 */ + u32 reserved; /* In */ + u64 address; /* In */ + u32 len; /* In */ + u32 commit:1; /* In */ + u32 reserved2:31; /* In */ +} __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, SEV_RET_MAX, } sev_ret_code;
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. Signed-off-by: Dionna Glaze <dionnaglaze@google.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(-)