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
