| Message ID | 87a0481526e66ddd5f6192cbb43a50708aee2883.1655761627.git.ashish.kalra@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) | expand |
Hello Boris, On 10/1/2022 12:33 PM, Borislav Petkov wrote: > On Mon, Jun 20, 2022 at 11:04:29PM +0000, Ashish Kalra wrote: >> +static int __sev_snp_init_locked(int *error) >> +{ >> + struct psp_device *psp = psp_master; >> + struct sev_device *sev; >> + int rc = 0; >> + >> + if (!psp || !psp->sev_data) >> + return -ENODEV; >> + >> + sev = psp->sev_data; >> + >> + if (sev->snp_inited) > > snp_inited? That's silly. > > snp_initialized > > pls. Yes. > >> + return 0; >> + >> + /* >> + * The SNP_INIT requires the MSR_VM_HSAVE_PA must be set to 0h > > /* Clear MSR_VM_HSAVE_PA on all cores before SNP_INIT */ > >> + * across all cores. >> + */ >> + on_each_cpu(snp_set_hsave_pa, NULL, 1); >> + >> + /* Issue the SNP_INIT firmware command. */ > > Useless comment. > >> + rc = __sev_do_cmd_locked(SEV_CMD_SNP_INIT, NULL, error); >> + if (rc) >> + return rc; >> + >> + /* Prepare for first SNP guest launch after INIT */ >> + wbinvd_on_all_cpus(); > > Can you put a wbinvd() in snp_set_hsave_pa() instead and save yourself > the second IPI? > > Or is that order of the commands: > > 1. clear MSR IPI > 2. SNP_INIT > 3. WBINVD IPI > 4. ... > > mandatory? > Yes, we need to do: wbinvd_on_all_cpus(); SNP_DF_FLUSH Need to ensure all the caches are clear before launching the first guest and this has to be a combination of WBINVD and SNP_DF_FLUSH command. > ... > >> +static int __sev_snp_shutdown_locked(int *error) >> +{ >> + struct sev_device *sev = psp_master->sev_data; >> + int ret; >> + >> + if (!sev->snp_inited) >> + return 0; >> + >> + /* SHUTDOWN requires the DF_FLUSH */ >> + wbinvd_on_all_cpus(); >> + __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, NULL); > > Why isn't this retval checked? From the SNP FW ABI specs, for the SNP_SHUTDOWN command: Firmware checks for every encryption capable ASID that the ASID is not in use by a guest and a DF_FLUSH is not required. If a DF_FLUSH is required, the firmware returns DFFLUSH_REQUIRED. Considering that SNP_SHUTDOWN command will check if DF_FLUSH was required and if so, and not invoked before that command, returns an error indicating that DFFLUSH is required. This way, we can cleverly avoid taking the error code path for DF_FLUSH command here and instead let the SNP_SHUTDOWN command failure below indicate if DF_FLUSH command failed. This also ensures that we always invoke SNP_SHUTDOWN command, irrespective of SNP_DF_FLUSH command failure as SNP_DF_FLUSH may actually not be required by the SHUTDOWN command. > >> + >> + ret = __sev_do_cmd_locked(SEV_CMD_SNP_SHUTDOWN, NULL, error); >> + if (ret) { >> + dev_err(sev->dev, "SEV-SNP firmware shutdown failed\n"); >> + return ret; >> + } >> + >> + sev->snp_inited = false; >> + dev_dbg(sev->dev, "SEV-SNP firmware shutdown\n"); >> + >> + return ret; >> +} > > ... > >> void sev_dev_destroy(struct psp_device *psp) >> @@ -1287,6 +1385,26 @@ void sev_pci_init(void) >> } >> } >> >> + /* >> + * If boot CPU supports the SNP, then first attempt to initialize > > s/the SNP/SNP/g > >> + * the SNP firmware. >> + */ >> + if (cpu_feature_enabled(X86_FEATURE_SEV_SNP)) { >> + if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) { >> + dev_err(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n", >> + SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR); >> + } else { >> + rc = sev_snp_init(&error); >> + if (rc) { >> + /* >> + * If we failed to INIT SNP then don't abort the probe. > > Who's "we"? > >> + * Continue to initialize the legacy SEV firmware. >> + */ >> + dev_err(sev->dev, "SEV-SNP: failed to INIT error %#x\n", error); >> + } >> + } >> + } >> + >> /* 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 01ba9dc46ca3..ef4d42e8c96e 100644 >> --- a/include/linux/psp-sev.h >> +++ b/include/linux/psp-sev.h >> @@ -769,6 +769,20 @@ struct sev_data_snp_init_ex { >> */ >> int sev_platform_init(int *error); >> >> +/** >> + * sev_snp_init - perform SEV SNP_INIT command >> + * >> + * @error: SEV command return code >> + * >> + * Returns: >> + * 0 if the SEV successfully processed the command >> + * -%ENODEV if the SEV device is not available >> + * -%ENOTSUPP if the SEV does not support SEV >> + * -%ETIMEDOUT if the SEV command timed out >> + * -%EIO if the SEV returned a non-zero return code > > Something's weird with those args. I think it should be > > %-ENODEV > > and so on... > Yes, off course %-<errno> Thanks, Ashish
Some more follow up regarding avoiding the second IPI: >> >>> + rc = __sev_do_cmd_locked(SEV_CMD_SNP_INIT, NULL, error); >>> + if (rc) >>> + return rc; >>> + >>> + /* Prepare for first SNP guest launch after INIT */ >>> + wbinvd_on_all_cpus(); >> >> Can you put a wbinvd() in snp_set_hsave_pa() instead and save yourself >> the second IPI? >> >> Or is that order of the commands: >> >> 1. clear MSR IPI >> 2. SNP_INIT >> 3. WBINVD IPI >> 4. ... >> >> mandatory? >> > > Yes, we need to do: > > wbinvd_on_all_cpus(); > SNP_DF_FLUSH > > Need to ensure all the caches are clear before launching the first guest > and this has to be a combination of WBINVD and SNP_DF_FLUSH command. > I had related discussions with the HW architect: SNP firmware will fail ACTIVATE if DFFLUSH isn't called, and DFFLUSH requires the WBINVD on all cores. By requiring WBIDVD on all cores, we're a) requiring the caches to be flushed, and b) forcing the hypervisor to exit all guests at least once since SEV/SNP has been enabled, since the WBINVDs must be done in host mode. The order is: VM_HSAVE_PA IPI SNP_INIT WBIVND (IPI) DF_FLUSH so that means we can't combine the IPIs. Also, this is not a performance critical path, so should we really be so concerned about it? Thanks, Ashish
Another follow up: >>> int sev_platform_init(int *error); >>> +/** >>> + * sev_snp_init - perform SEV SNP_INIT command >>> + * >>> + * @error: SEV command return code >>> + * >>> + * Returns: >>> + * 0 if the SEV successfully processed the command >>> + * -%ENODEV if the SEV device is not available >>> + * -%ENOTSUPP if the SEV does not support SEV >>> + * -%ETIMEDOUT if the SEV command timed out >>> + * -%EIO if the SEV returned a non-zero return code >> >> Something's weird with those args. I think it should be >> >> %-ENODEV >> >> and so on... >> > > Yes, off course %-<errno> > I see that other drivers are also using the same convention: include/linux/regset.h: .. /** * user_regset_set_fn - type of @set function in &struct user_regset * @target: thread being examined * @regset: regset being examined * @pos: offset into the regset data to access, in bytes * @count: amount of data to copy, in bytes * @kbuf: if not %NULL, a kernel-space pointer to copy from * @ubuf: if @kbuf is %NULL, a user-space pointer to copy from * * Store register values. Return %0 on success; -%EIO or -%ENODEV * are usual failure returns. The @pos and @count values are in ... include/linux/psp-tee.h: .. /** * psp_tee_process_cmd() - Process command in Trusted Execution Environment * @cmd_id: TEE command ID (&enum tee_cmd_id) * @buf: Command buffer for TEE processing. On success, is updated * with the response * @len: Length of command buffer in bytes * @status: On success, holds the TEE command execution status * * This function submits a command to the Trusted OS for processing in the * TEE environment and waits for a response or until the command times out. * * Returns: * 0 if TEE successfully processed the command * -%ENODEV if PSP device not available * -%EINVAL if invalid input * -%ETIMEDOUT if TEE command timed out * -%EBUSY if PSP device is not responsive */ ... Thanks, Ashish
On Fri, Oct 14, 2022 at 04:31:42PM -0500, Kalra, Ashish wrote: > so that means we can't combine the IPIs. Ok, thanks for checking. > Also, this is not a performance critical path, so should we really be so > concerned about it? Not that concerned but we're always trying to be as optimal as possible. Thx.
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index 9cb3265f3bef..f1173221d0b9 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -33,6 +33,10 @@ #define SEV_FW_FILE "amd/sev.fw" #define SEV_FW_NAME_SIZE 64 +/* Minimum firmware version required for the SEV-SNP support */ +#define SNP_MIN_API_MAJOR 1 +#define SNP_MIN_API_MINOR 51 + static DEFINE_MUTEX(sev_cmd_mutex); static struct sev_misc_dev *misc_dev; @@ -775,6 +779,98 @@ static int sev_update_firmware(struct device *dev) return ret; } +static void snp_set_hsave_pa(void *arg) +{ + wrmsrl(MSR_VM_HSAVE_PA, 0); +} + +static int __sev_snp_init_locked(int *error) +{ + struct psp_device *psp = psp_master; + struct sev_device *sev; + int rc = 0; + + if (!psp || !psp->sev_data) + return -ENODEV; + + sev = psp->sev_data; + + if (sev->snp_inited) + return 0; + + /* + * The SNP_INIT requires the MSR_VM_HSAVE_PA must be set to 0h + * across all cores. + */ + on_each_cpu(snp_set_hsave_pa, NULL, 1); + + /* Issue the SNP_INIT firmware command. */ + rc = __sev_do_cmd_locked(SEV_CMD_SNP_INIT, NULL, error); + if (rc) + return rc; + + /* Prepare for first SNP guest launch after INIT */ + wbinvd_on_all_cpus(); + rc = __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, error); + if (rc) + return rc; + + sev->snp_inited = true; + dev_dbg(sev->dev, "SEV-SNP firmware initialized\n"); + + return rc; +} + +int sev_snp_init(int *error) +{ + int rc; + + if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) + return -ENODEV; + + mutex_lock(&sev_cmd_mutex); + rc = __sev_snp_init_locked(error); + mutex_unlock(&sev_cmd_mutex); + + return rc; +} +EXPORT_SYMBOL_GPL(sev_snp_init); + +static int __sev_snp_shutdown_locked(int *error) +{ + struct sev_device *sev = psp_master->sev_data; + int ret; + + if (!sev->snp_inited) + return 0; + + /* SHUTDOWN requires the DF_FLUSH */ + wbinvd_on_all_cpus(); + __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, NULL); + + ret = __sev_do_cmd_locked(SEV_CMD_SNP_SHUTDOWN, NULL, error); + if (ret) { + dev_err(sev->dev, "SEV-SNP firmware shutdown failed\n"); + return ret; + } + + sev->snp_inited = false; + dev_dbg(sev->dev, "SEV-SNP firmware shutdown\n"); + + return ret; +} + +static int sev_snp_shutdown(int *error) +{ + int rc; + + mutex_lock(&sev_cmd_mutex); + rc = __sev_snp_shutdown_locked(NULL); + mutex_unlock(&sev_cmd_mutex); + + return rc; +} + static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable) { struct sev_device *sev = psp_master->sev_data; @@ -1231,6 +1327,8 @@ static void sev_firmware_shutdown(struct sev_device *sev) get_order(NV_LENGTH)); sev_init_ex_buffer = NULL; } + + sev_snp_shutdown(NULL); } void sev_dev_destroy(struct psp_device *psp) @@ -1287,6 +1385,26 @@ void sev_pci_init(void) } } + /* + * If boot CPU supports the SNP, then first attempt to initialize + * the SNP firmware. + */ + if (cpu_feature_enabled(X86_FEATURE_SEV_SNP)) { + if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) { + dev_err(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n", + SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR); + } else { + rc = sev_snp_init(&error); + if (rc) { + /* + * If we failed to INIT SNP then don't abort the probe. + * Continue to initialize the legacy SEV firmware. + */ + dev_err(sev->dev, "SEV-SNP: failed to INIT error %#x\n", error); + } + } + } + /* Obtain the TMR memory area for SEV-ES use */ sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE); if (!sev_es_tmr) @@ -1302,6 +1420,9 @@ void sev_pci_init(void) dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n", error, rc); + dev_info(sev->dev, "SEV%s API:%d.%d build:%d\n", sev->snp_inited ? + "-SNP" : "", sev->api_major, sev->api_minor, sev->build); + return; err: diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h index 666c21eb81ab..186ad20cbd24 100644 --- a/drivers/crypto/ccp/sev-dev.h +++ b/drivers/crypto/ccp/sev-dev.h @@ -52,6 +52,8 @@ struct sev_device { u8 build; void *cmd_buf; + + bool snp_inited; }; int sev_dev_init(struct psp_device *psp); diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h index 01ba9dc46ca3..ef4d42e8c96e 100644 --- a/include/linux/psp-sev.h +++ b/include/linux/psp-sev.h @@ -769,6 +769,20 @@ struct sev_data_snp_init_ex { */ int sev_platform_init(int *error); +/** + * sev_snp_init - perform SEV SNP_INIT command + * + * @error: SEV command return code + * + * Returns: + * 0 if the SEV successfully processed the command + * -%ENODEV if the SEV device is not available + * -%ENOTSUPP if the SEV does not support SEV + * -%ETIMEDOUT if the SEV command timed out + * -%EIO if the SEV returned a non-zero return code + */ +int sev_snp_init(int *error); + /** * sev_platform_status - perform SEV PLATFORM_STATUS command * @@ -876,6 +890,8 @@ sev_platform_status(struct sev_user_data_status *status, int *error) { return -E static inline int sev_platform_init(int *error) { return -ENODEV; } +static inline int sev_snp_init(int *error) { return -ENODEV; } + static inline int sev_guest_deactivate(struct sev_data_deactivate *data, int *error) { return -ENODEV; }