Re: [PATCH v5 6/7] KVM: SVM: Add support to initialize SEV/SNP functionality in KVM

[Sean Christopherson]

On Fri, Feb 28, 2025, Ashish Kalra wrote:
> Hello Sean,
> 
> On 2/28/2025 12:31 PM, Sean Christopherson wrote:
> > On Tue, Feb 25, 2025, Ashish Kalra wrote:
> >> +	if (!sev_enabled)
> >> +		return;
> >> +
> >> +	/*
> >> +	 * Always perform SEV initialization at setup time to avoid
> >> +	 * complications when performing SEV initialization later
> >> +	 * (such as suspending active guests, etc.).
> > 
> > This is misleading and wildly incomplete.  *SEV* doesn't have complications, *SNP*
> > has complications.  And looking through sev_platform_init(), all of this code
> > is buggy.
> > 
> > The sev_platform_init() return code is completely disconnected from SNP setup.
> > It can return errors even if SNP setup succeeds, and can return success even if
> > SNP setup fails.
> > 
> > I also think it makes sense to require SNP to be initialized during KVM setup.
> 
> There are a few important considerations here: 
> 
> This is true that we require SNP to be initialized during KVM setup 
> and also as mentioned earlier we need SNP to be initialized (SNP_INIT_EX
> should be done) for SEV INIT to succeed if SNP host support is enabled.
> 
> So we essentially have to do SNP_INIT(_EX) for launching SEV/SEV-ES VMs when
> SNP host support is enabled. In other words, if SNP_INIT(_EX) is not issued or 
> fails then SEV/SEV-ES VMs can't be launched once SNP host support (SYSCFG.SNPEn) 
> is enabled as SEV INIT will fail in such a situation.

Doesn't that mean sev_platform_init() is broken and should error out if SNP
setup fails?  Because this doesn't match the above (or I'm misreading one or both).

	rc = __sev_snp_init_locked(&args->error);
	if (rc && rc != -ENODEV) {
		/*
		 * Don't abort the probe if SNP INIT failed,
		 * continue to initialize the legacy SEV firmware.
		 */
		dev_err(sev->dev, "SEV-SNP: failed to INIT, continue SEV INIT\n");
	}

And doesn't the min version check completely wreck everything?  I.e. if SNP *must*
be initialized if SYSCFG.SNPEn is set in order to utilize SEV/SEV-ES, then shouldn't
this be a fatal error too?

	if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
		dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
			SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
		return 0;
	}

And then aren't all of the bare calls to __sev_platform_init_locked() broken too?
E.g. if userspace calls sev_ioctl_do_pek_csr() without loading KVM, then SNP won't
be initialized and __sev_platform_init_locked() will fail, no?

> And the other consideration is that runtime setup of especially SEV-ES VMs will not
> work if/when first SEV-ES VM is launched, if SEV INIT has not been issued at 
> KVM setup time.
> 
> This is because qemu has a check for SEV INIT to have been done (via SEV platform
> status command) prior to launching SEV-ES VMs via KVM_SEV_INIT2 ioctl. 
>
> So effectively, __sev_guest_init() does not get invoked in case of launching 
> SEV_ES VMs, if sev_platform_init() has not been done to issue SEV INIT in 
> sev_hardware_setup().
> 
> In other words the deferred initialization only works for SEV VMs and not SEV-ES VMs.

In that case, I vote to kill off deferred initialization entirely, and commit to
enabling all of SEV+ when KVM loads (which we should have done from day one).
Assuming we can do that in a way that's compatible with the /dev/sev ioctls.