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

[Kalra, Ashish]

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