Hello Sean,
On 2/28/2025 4:32 PM, Sean Christopherson wrote:
> 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");
> }
>
Yes, i realized this is true and we need to return here if rc != -ENODEV.
So i will add a pre-patch to the series to fix this.
> 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;
> }
>
Yes, this is also true, we need to return an error here.
> 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?
Yes, we should be calling _sev_platform_init_locked() here instead of__sev_platform_init_locked()
to ensure that both implicit SNP and SEV INIT is done for these ioctls and followed by
__sev_firmware_shutdown() to do both SEV and SNP shutdown.
>
>> 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.
Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads.
For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX
and SEV (re)INIT after that to ensure that SEV is in UNINIT state before
DLFW_EX.
We still probably want to keep the deferred initialization for SEV in
__sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX
case.
Thanks,
Ashish
