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.
