lkml.org 
[lkml]   [2023]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH RFC v7 37/64] KVM: SVM: Add KVM_SNP_INIT command
From
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

\
 
 \ /
  Last update: 2023-03-26 23:29    [W:0.148 / U:0.472 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site