Messages in this thread | | | Date | Thu, 5 Jan 2023 17:37:20 -0600 | Subject | Re: [PATCH RFC v7 37/64] KVM: SVM: Add KVM_SNP_INIT command | From | "Kalra, Ashish" <> |
| |
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
| |