There was an early firmware issue on Genoa which supported only SNP_INIT
or SEV_INIT, but this issue is resolved now.
Now, the main constraints are that SNP_INIT is always required before
SEV_INIT in case we want to launch SNP guests. In other words, if only
SEV_INIT is done on a platform which supports SNP we won't be able to
launch SNP guests after that.
So once we have RMP table setup (in BIOS) we will always do an SNP_INIT
and SEV_INIT will be ideally done only (on demand) when an SEV guest is
launched.
Thanks,
Ashish
On 1/5/2023 5:37 PM, Kalra, Ashish wrote:
> Hello Jarkko,
>
> On 12/31/2022 8:27 AM, Jarkko Sakkinen wrote:
>> On Wed, Dec 14, 2022 at 01:40:29PM -0600, Michael Roth wrote:
>>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>> {
>>> struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>> @@ -260,13 +279,23 @@ static int sev_guest_init(struct kvm *kvm,
>>> struct kvm_sev_cmd *argp)
>>> return ret;
>>> sev->active = true;
>>> - sev->es_active = argp->id == KVM_SEV_ES_INIT;
>>> + sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id ==
>>> KVM_SEV_SNP_INIT);
>>> + sev->snp_active = argp->id == KVM_SEV_SNP_INIT;
>>> asid = sev_asid_new(sev);
>>> if (asid < 0)
>>> goto e_no_asid;
>>> sev->asid = asid;
>>> - ret = sev_platform_init(&argp->error);
>>> + if (sev->snp_active) {
>>> + ret = verify_snp_init_flags(kvm, argp);
>>> + if (ret)
>>> + goto e_free;
>>> +
>>> + ret = sev_snp_init(&argp->error, false);
>>> + } else {
>>> + ret = sev_platform_init(&argp->error);
>>> + }
>>
>> Couldn't sev_snp_init() and sev_platform_init() be called unconditionally
>> in order?
>>
>> Since there is a hardware constraint that SNP init needs to always happen
>> before platform init, shouldn't SNP init happen as part of
>> __sev_platform_init_locked() instead?
>>
>
> On Genoa there is currently an issue that if we do an SNP_INIT before an
> SEV_INIT and then attempt to launch a SEV guest that may fail, so we
> need to keep SNP INIT and SEV INIT separate.
>
> We need to provide a way to run (existing) SEV guests on a system that
> supports SNP without doing an SNP_INIT at all.
>
> This is done using psp_init_on_probe parameter of the CCP module to
> avoid doing either SNP/SEV firmware initialization during module load
> and then defer the firmware initialization till someone launches a guest
> of one flavor or the other.
>
> And then sev_guest_init() does either SNP or SEV firmware init depending
> on the type of the guest being launched.
>
>> I found these call sites for __sev_platform_init_locked(), none of which
>> follow the correct call order:
>>
>> * sev_guest_init()
>
> As explained above, this call site is important for deferring the
> firmware initialization to an actual guest launch.
>
>> * sev_ioctl_do_pek_csr
>> * sev_ioctl_do_pdh_export()
>> * sev_ioctl_do_pek_import()
>> * sev_ioctl_do_pek_pdh_gen()
>> * sev_pci_init()
>>
>> For me it looks like a bit flakky API use to have sev_snp_init() as an
>> API
>> call.
>>
>> I would suggest to make SNP init internal to the ccp driver and take care
>> of the correct orchestration over there.
>>
>
> Due to Genoa issue, we may still need SNP init and SEV init to be
> invoked separately outside the CCP driver.
>
>> Also, how it currently works in this patch set, if the firmware did not
>> load correctly, SNP init halts the whole system. The version check needs
>> to be in all call paths.
>>
>
> Yes, i agree with that.
>
> Thanks,
> Ashish
On Mon, Jan 23, 2023 at 04:49:14PM -0600, Kalra, Ashish wrote:
> There was an early firmware issue on Genoa which supported only SNP_INIT or
> SEV_INIT, but this issue is resolved now.
>
> Now, the main constraints are that SNP_INIT is always required before
> SEV_INIT in case we want to launch SNP guests. In other words, if only
> SEV_INIT is done on a platform which supports SNP we won't be able to launch
> SNP guests after that.
>
> So once we have RMP table setup (in BIOS) we will always do an SNP_INIT and
> SEV_INIT will be ideally done only (on demand) when an SEV guest is
> launched.
OK, thanks for the clarification!
BR, Jarkko