On Fri, Oct 29, 2021 at 11:56 AM Tom Lendacky <thomas.lendacky@xxxxxxx> wrote: > > On 10/29/21 12:26 PM, Peter Gonda wrote: > > On Fri, Oct 29, 2021 at 8:45 AM Tom Lendacky <thomas.lendacky@xxxxxxx> wrote: > >> > >> On 10/28/21 12:57 PM, Peter Gonda wrote: > >>> From: David Rientjes <rientjes@xxxxxxxxxx> > >>> > >>> Add new module parameter to allow users to use SEV_INIT_EX instead of > >>> SEV_INIT. This helps users who lock their SPI bus to use the PSP for SEV > >>> functionality. The 'init_ex_path' parameter defaults to NULL which means > >>> the kernel will use SEV_INIT, if a path is specified SEV_INIT_EX will be > >>> used with the data found at the path. On certain PSP commands this > >>> file is written to as the PSP updates the NV memory region. Depending on > >>> file system initialization this file open may fail during module init > >>> but the CCP driver for SEV already has sufficient retries for platform > >>> initialization. During normal operation of PSP system and SEV commands > >>> if the PSP has not been initialized it is at run time. > >> > >> IIUC, it looks as though the file has to exist before the very first use, > >> otherwise the initialization will fail. Did you consider checking for the > >> presence of the file first and, if not there, just using a memory area of > >> all f's (as documented in the SEV API)? Then on successful INIT, the > >> memory would be written and the file created. > > > > Thats a great idea. I'll add that functionality. > > It all depends on how you want to treat the absence of the file. I can see > use cases for both creating automatically or failing (e.g. there was a > typo in the path). > > > > >> > >> Either way, probably worth adding to the commit message. And if you stay > >> with having to pre-allocate the file, it's worth adding to the SEV > >> documentation what is required to be done to initialize the file. > >> > >> Although, INIT_EX is probably worth adding to the SEV documentation in > >> general. > > > > Is this (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.kernel.org%2Fdoc%2FDocumentation%2Fvirt%2Fkvm%2Famd-memory-encryption.rst&data=04%7C01%7CThomas.Lendacky%40amd.com%7Cd9723c44ea764cc7540e08d99b013bbb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637711251880475091%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=WJtwak6F%2Bn7PC8R1ohsC%2FNKGi7oDWFTi72NJDteyQoM%3D&reserved=0) > > the documentation you were referring too? If so I can add another > > patch before this one to document INIT, then add the INIT_EX > > documentation here. Or were you thinking something else? > > Yes, that's the documentation I was referring to. I think you can just add > something to the KVM_SEV_INIT entry that talks about the new module > parameter and a bit about the file and how that file will be used. If you > decide to stay with a pre-created file, some details about how to create > the file would then be appropriate. > I've updated the doc and the commit message. I went with the do not auto create option since it keeps the patch simpler, fixes typos as you've said, and its pretty easy for users to create with a dd command or something. > Thanks, > Tom > > > > >> > >>> > >>> Signed-off-by: David Rientjes <rientjes@xxxxxxxxxx> > >>> Co-developed-by: Peter Gonda <pgonda@xxxxxxxxxx> > >>> Signed-off-by: Peter Gonda <pgonda@xxxxxxxxxx> > >>> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx> > >>> Cc: Brijesh Singh <brijesh.singh@xxxxxxx> > >>> Cc: Marc Orr <marcorr@xxxxxxxxxx> > >>> Cc: Joerg Roedel <jroedel@xxxxxxx> > >>> Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> > >>> Cc: David Rientjes <rientjes@xxxxxxxxxx> > >>> Cc: John Allen <john.allen@xxxxxxx> > >>> Cc: "David S. Miller" <davem@xxxxxxxxxxxxx> > >>> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx> ( > >>> Cc: linux-crypto@xxxxxxxxxxxxxxx > >>> Cc: linux-kernel@xxxxxxxxxxxxxxx > >>> --- > >>> drivers/crypto/ccp/sev-dev.c | 186 ++++++++++++++++++++++++++++++++--- > >>> include/linux/psp-sev.h | 21 ++++ > >>> 2 files changed, 192 insertions(+), 15 deletions(-) > >>> > >>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > >>> index b568ae734857..c8718b4cbc93 100644 > >>> --- a/drivers/crypto/ccp/sev-dev.c > >>> +++ b/drivers/crypto/ccp/sev-dev.c > >>> @@ -22,6 +22,7 @@ > >>> #include <linux/firmware.h> > >>> #include <linux/gfp.h> > >>> #include <linux/cpufeature.h> > >>> +#include <linux/fs.h> > >>> > >>> #include <asm/smp.h> > >>> > >>> @@ -43,6 +44,10 @@ static int psp_probe_timeout = 5; > >>> module_param(psp_probe_timeout, int, 0644); > >>> MODULE_PARM_DESC(psp_probe_timeout, " default timeout value, in seconds, during PSP device probe"); > >>> > >>> +static char *init_ex_path; > >>> +module_param(init_ex_path, charp, 0660); > >>> +MODULE_PARM_DESC(init_ex_path, " Path for INIT_EX data; if set try INIT_EX"); > >>> + > >>> MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */ > >>> MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */ > >>> MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */ > >>> @@ -58,6 +63,14 @@ static int psp_timeout; > >>> #define SEV_ES_TMR_SIZE (1024 * 1024) > >>> static void *sev_es_tmr; > >>> > >>> +/* INIT_EX NV Storage: > >>> + * The NV Storage is a 32Kb area and must be 4Kb page aligned. Use the page > >>> + * allocator to allocate the memory, which will return aligned memory for the > >>> + * specified allocation order. > >>> + */ > >>> +#define NV_LENGTH (32 << 10) > >> > >> Just me, but I think '32 * 1024' would be a bit clearer. > > > > SGTM and more consistent with SEV_ES_TMR_SIZE. > > > >> > >>> +static void *sev_init_ex_nv_address; > >>> + > >>> static inline bool sev_version_greater_or_equal(u8 maj, u8 min) > >>> { > >>> struct sev_device *sev = psp_master->sev_data; > >>> @@ -135,6 +148,7 @@ static int sev_cmd_buffer_len(int cmd) > >>> case SEV_CMD_GET_ID: return sizeof(struct sev_data_get_id); > >>> case SEV_CMD_ATTESTATION_REPORT: return sizeof(struct sev_data_attestation_report); > >>> case SEV_CMD_SEND_CANCEL: return sizeof(struct sev_data_send_cancel); > >>> + case SEV_CMD_INIT_EX: return sizeof(struct sev_data_init_ex); > >> > >> Maybe move this to just under the SEV_CMD_INIT: case statement? > >> > >>> default: return 0; > >>> } > >>> > >>> @@ -156,6 +170,89 @@ static void *sev_fw_alloc(unsigned long len) > >>> return page_address(page); > >>> } > >>> > >>> +static int sev_read_nv_memory(void) > >>> +{ > >>> + struct file *fp; > >>> + ssize_t nread; > >>> + > >>> + if (!sev_init_ex_nv_address) > >>> + return -EOPNOTSUPP; > >>> + > >>> + fp = filp_open(init_ex_path, O_RDONLY, 0); > >>> + if (IS_ERR(fp)) { > >>> + dev_err(psp_master->dev, "sev could not open file for read\n"); > >>> + return PTR_ERR(fp); > >>> + } > >>> + > >>> + nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, 0); > >>> + dev_dbg(psp_master->dev, "sev NV read %d bytes\n", nread); > >>> + filp_close(fp, NULL); > >> > >> Add a blank line here. > >> > >>> + return 0; > >>> +} > >>> + > >>> +static int sev_write_nv_memory(void) > >>> +{ > >>> + struct sev_device *sev = psp_master->sev_data; > >>> + struct file *fp; > >>> + loff_t offset = 0; > >>> + int ret; > >>> + > >>> + if (!sev_init_ex_nv_address) > >>> + return -EOPNOTSUPP; > >>> + > >>> + fp = filp_open(init_ex_path, O_CREAT | O_WRONLY, 0600); > >>> + if (IS_ERR(fp)) { > >>> + dev_err(sev->dev, "sev NV data could not be created\n"); > >>> + return PTR_ERR(fp); > >>> + } > >> > >> Add a blank line here. > >> > >>> + ret = kernel_write(fp, sev_init_ex_nv_address, NV_LENGTH, &offset); > >>> + vfs_fsync(fp, 0); > >>> + filp_close(fp, NULL); > >>> + > >>> + if (ret != NV_LENGTH) { > >>> + dev_err(sev->dev, > >>> + "failed to write %d bytes to non volatile memory area, ret=%lu\n", > >>> + NV_LENGTH, ret); > >>> + if (ret >= 0) > >>> + return -EIO; > >>> + return ret; > >>> + } > >>> + > >>> + dev_dbg(sev->dev, "wrote to non volatile memory area\n"); > >> > >> Add a blank line here. > > > > Added all these blank lines. > > > >> > >>> + return 0; > >>> +} > >>> + > >>> +static void sev_write_nv_memory_if_required(int cmd_id) > >>> +{ > >>> + struct sev_device *sev = psp_master->sev_data; > >>> + int ret; > >>> + > >>> + if (!sev_init_ex_nv_address) > >>> + return; > >>> + > >>> + /* > >>> + * Only a few platform commands modify the SPI/NV area, > >>> + * but none of the non-platform commands do. Only INIT, > >> > >> maybe say INIT(_EX)? > > > > Done. > > > >> > >>> + * PLATFORM_RESET, PEK_GEN, PEK_CERT_IMPORT, and > >>> + * PDH_GEN do. > >> > >> Does SHUTDOWN modify the SPI/NV area? Otherwise a separate comment about > >> why it is included below. > > > > Double checked the FW doc looks like it does not. Richard can you > > confirm? If it doesn't I'll remove this case. > > > > > > >>> + */ > >>> + switch (cmd_id) { > >>> + case SEV_CMD_FACTORY_RESET: > >>> + case SEV_CMD_INIT_EX: > >>> + case SEV_CMD_PDH_GEN: > >>> + case SEV_CMD_PEK_CERT_IMPORT: > >>> + case SEV_CMD_PEK_GEN: > >>> + case SEV_CMD_SHUTDOWN: > >>> + break; > >>> + default: > >>> + return; > >>> + }; > >>> + > >>> + ret = sev_write_nv_memory(); > >>> + if (ret) > >>> + dev_err(sev->dev, "sev NV write failed %d\n", ret); > >> > >> You already have error messages in the sev_write_nv_memory() function, > >> this one probably isn't needed. > > > > Removed. > > > >> > >>> +} > >>> + > >>> static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) > >>> { > >>> struct psp_device *psp = psp_master; > >>> @@ -225,6 +322,8 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) > >>> dev_dbg(sev->dev, "sev command %#x failed (%#010x)\n", > >>> cmd, reg & PSP_CMDRESP_ERR_MASK); > >>> ret = -EIO; > >>> + } else { > >>> + sev_write_nv_memory_if_required(cmd); > >>> } > >>> > >>> print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data, > >>> @@ -251,22 +350,42 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret) > >>> return rc; > >>> } > >>> > >>> -static int __sev_platform_init_locked(int *error) > >>> +static int __sev_init_locked(int *error) > >>> { > >>> - struct psp_device *psp = psp_master; > >>> struct sev_data_init data; > >>> - struct sev_device *sev; > >>> - int rc = 0; > >>> > >>> - if (!psp || !psp->sev_data) > >>> - return -ENODEV; > >>> + memset(&data, 0, sizeof(data)); > >>> + if (sev_es_tmr) { > >>> + u64 tmr_pa; > >>> > >>> - sev = psp->sev_data; > >>> + /* > >>> + * Do not include the encryption mask on the physical > >>> + * address of the TMR (firmware should clear it anyway). > >>> + */ > >>> + tmr_pa = __pa(sev_es_tmr); > >>> > >>> - if (sev->state == SEV_STATE_INIT) > >>> - return 0; > >>> + data.flags |= SEV_INIT_FLAGS_SEV_ES; > >>> + data.tmr_address = tmr_pa; > >>> + data.tmr_len = SEV_ES_TMR_SIZE; > >>> + } > >>> + > >>> + return __sev_do_cmd_locked(SEV_CMD_INIT, &data, 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_nv_address); > >>> + data.nv_len = NV_LENGTH; > >>> + > >>> + ret = sev_read_nv_memory(); > >>> + if (ret) > >>> + return ret; > >>> + > >>> if (sev_es_tmr) { > >>> u64 tmr_pa; > >>> > >>> @@ -276,12 +395,30 @@ static int __sev_platform_init_locked(int *error) > >>> */ > >>> tmr_pa = __pa(sev_es_tmr); > >>> > >>> - data.flags |= SEV_INIT_FLAGS_SEV_ES; > >> > >> Inadvertant deletion? > > > > Oops only testing with SEV guests. Will test with ES too. Fixed. > > > >> > >>> data.tmr_address = tmr_pa; > >>> data.tmr_len = SEV_ES_TMR_SIZE; > >>> } > >> > >> Add a blank line here. > >> > >>> + return __sev_do_cmd_locked(SEV_CMD_INIT_EX, &data, error); > >>> +} > >>> + > >>> +static int __sev_platform_init_locked(int *error) > >>> +{ > >>> + struct psp_device *psp = psp_master; > >>> + struct sev_device *sev; > >>> + int rc; > >>> + int (*init_function)(int *error) = sev_init_ex_nv_address ? > >>> + __sev_init_ex_locked : > >>> + __sev_init_locked; > >> > >> This seems a bit much in the declaration. How about moving the assignment > >> down to just before the first call? > > > > Done. > > > >> > >>> + > >>> + if (!psp || !psp->sev_data) > >>> + return -ENODEV; > >>> + > >>> + sev = psp->sev_data; > >>> > >>> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error); > >>> + if (sev->state == SEV_STATE_INIT) > >>> + return 0; > >>> + > >>> + rc = init_function(error); > >>> if (rc && *error == SEV_RET_SECURE_DATA_INVALID) { > >>> /* > >>> * INIT command returned an integrity check failure > >>> @@ -290,8 +427,8 @@ static int __sev_platform_init_locked(int *error) > >>> * failed and persistent state has been erased. > >>> * Retrying INIT command here should succeed. > >>> */ > >>> - dev_dbg(sev->dev, "SEV: retrying INIT command"); > >>> - rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error); > >>> + dev_err(sev->dev, "SEV: retrying INIT command"); > >> > >> Maybe dev_notice() instead of dev_err()? > > > > Done., > > > >> > >>> + rc = init_function(error); > >>> } > >>> > >>> if (rc) > >>> @@ -307,7 +444,7 @@ static int __sev_platform_init_locked(int *error) > >>> > >>> dev_dbg(sev->dev, "SEV firmware initialized\n"); > >>> > >>> - return rc; > >>> + return 0; > >>> } > >>> > >>> int sev_platform_init(int *error) > >>> @@ -987,7 +1124,7 @@ static int sev_misc_init(struct sev_device *sev) > >>> > >>> init_waitqueue_head(&sev->int_queue); > >>> sev->misc = misc_dev; > >>> - dev_dbg(dev, "registered SEV device\n"); > >>> + dev_err(dev, "registered SEV device\n"); > >> > >> Not sure this is a necessary change... but, if you don't want this as a > >> dev_dbg() then it should be a dev_info(), because it is not an error. > > > > Oops this was my debugging. Reverted. > > > >> > >>> > >>> return 0; > >>> } > >>> @@ -1061,6 +1198,12 @@ static void sev_firmware_shutdown(struct sev_device *sev) > >>> get_order(SEV_ES_TMR_SIZE)); > >>> sev_es_tmr = NULL; > >>> } > >>> + > >>> + if (sev_init_ex_nv_address) { > >>> + free_pages((unsigned long)sev_init_ex_nv_address, > >>> + get_order(NV_LENGTH)); > >>> + sev_init_ex_nv_address = NULL; > >>> + } > >>> } > >>> > >>> void sev_dev_destroy(struct psp_device *psp) > >>> @@ -1105,6 +1248,19 @@ void sev_pci_init(void) > >>> sev_update_firmware(sev->dev) == 0) > >>> sev_get_api_version(); > >>> > >>> + /* If an init_ex_path is provided rely on INIT_EX for PSP initialization > >>> + * instead of INIT. > >>> + */ > >>> + if (init_ex_path) { > >>> + sev_init_ex_nv_address = sev_fw_alloc(NV_LENGTH); > >>> + if (!sev_init_ex_nv_address) { > >>> + dev_warn( > >> > >> Shouldn't this be a dev_err(), since you are erroring out? > >> > >>> + sev->dev, > >> > >> Move this up to the previous line, i.e.: dev_err(sev->dev, > >> > >>> + "SEV: INIT_EX NV storage allocation failed, INIT-EX support unavailable\n"); > >> > >> Since you're erroring out, probably enough to just have the first part of > >> that message. But if not: > >> > >> s/INIT-EX/INIT_EX/ > > > > Fixed this log line. > > > >> > >> Thanks, > >> Tom > >> > >>> + goto err; > >>> + } > >>> + } > >>> + > >>> /* Obtain the TMR memory area for SEV-ES use */ > >>> sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE); > >>> if (!sev_es_tmr) > >>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h > >>> index d48a7192e881..1595088c428b 100644 > >>> --- a/include/linux/psp-sev.h > >>> +++ b/include/linux/psp-sev.h > >>> @@ -52,6 +52,7 @@ enum sev_cmd { > >>> SEV_CMD_DF_FLUSH = 0x00A, > >>> SEV_CMD_DOWNLOAD_FIRMWARE = 0x00B, > >>> SEV_CMD_GET_ID = 0x00C, > >>> + SEV_CMD_INIT_EX = 0x00D, > >>> > >>> /* Guest commands */ > >>> SEV_CMD_DECOMMISSION = 0x020, > >>> @@ -102,6 +103,26 @@ struct sev_data_init { > >>> u32 tmr_len; /* In */ > >>> } __packed; > >>> > >>> +/** > >>> + * struct sev_data_init_ex - INIT_EX command parameters > >>> + * > >>> + * @length: len of the command buffer read by the PSP > >>> + * @flags: processing flags > >>> + * @tmr_address: system physical address used for SEV-ES > >>> + * @tmr_len: len of tmr_address > >>> + * @nv_address: system physical address used for PSP NV storage > >>> + * @nv_len: len of nv_address > >>> + */ > >>> +struct sev_data_init_ex { > >>> + u32 length; /* In */ > >>> + u32 flags; /* In */ > >>> + u64 tmr_address; /* In */ > >>> + u32 tmr_len; /* In */ > >>> + u32 reserved; /* In */ > >>> + u64 nv_address; /* In/Out */ > >>> + u32 nv_len; /* In */ > >>> +} __packed; > >>> + > >>> #define SEV_INIT_FLAGS_SEV_ES 0x01 > >>> > >>> /** > >>>
