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.
> + 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?
...
> +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?
> +
> + 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...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
