Re: [PATCH Part2 v6 12/49] crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP

[Borislav Petkov]

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