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

[Kalra, Ashish]

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