On Mon, May 05, 2025, Pratik R. Sampat wrote:
> Hi Sean,
>
> On 5/2/25 4:50 PM, Sean Christopherson wrote:
> > On Wed, 05 Mar 2025 16:59:50 -0600, Pratik R. Sampat wrote:
> >> This patch series extends the sev_init2 and the sev_smoke test to
> >> exercise the SEV-SNP VM launch workflow.
> >>
> >> Primarily, it introduces the architectural defines, its support in the
> >> SEV library and extends the tests to interact with the SEV-SNP ioctl()
> >> wrappers.
> >>
> >> [...]
> >
> > Applied 2-9 to kvm-x86 selftests. AIUI, the KVM side of things should
> > already
> > be fixed. If KVM isn't fixed, I want to take that discussion/patch to a
> > separate thread.
> >
>
> Thanks for pulling these patches in.
>
> For 1 - Ashish's commit now returns failure for this case [1].
> Although, it appears that the return code isn't checked within
> sev_platform_init()[2], so it shouldn't change existing behavior. In the
> kselftest case, if platform init fails, the selftest will also fail — just as
> it does currently too.
Argh, now I remember the issue. But _sev_platform_init_locked() returns '0' if
psp_init_on_probe is true, and I don't see how deferring __sev_snp_init_locked()
will magically make it succeed the second time around.
So shouldn't the KVM code be this?
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index e0f446922a6e..dd04f979357d 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3038,6 +3038,14 @@ void __init sev_hardware_setup(void)
sev_snp_supported = sev_snp_enabled &&
cc_platform_has(CC_ATTR_HOST_SEV_SNP);
out:
+ if (sev_enabled) {
+ init_args.probe = true;
+ if (sev_platform_init(&init_args))
+ sev_supported = sev_es_supported = sev_snp_supported =
false;
+ else
+ sev_snp_supported &= sev_is_snp_initialized();
+ }
+
if (boot_cpu_has(X86_FEATURE_SEV))
pr_info("SEV %s (ASIDs %u - %u)\n",
sev_supported ? min_sev_asid <= max_sev_asid ?
"enabled" :
@@ -3067,12 +3075,6 @@ void __init sev_hardware_setup(void)
if (!sev_enabled)
return;
-
- /*
- * Do both SNP and SEV initialization at KVM module load.
- */
- init_args.probe = true;
- sev_platform_init(&init_args);
}
void sev_hardware_unsetup(void)
--
Ashish, what am I missing?
> Regardless of what we decide on what the right behavior is, fail vs skip (I
> don't mind the former) we can certainly do that over new patches rebased over
> the new series.
FAIL, for sure. Unless someone else pipes up with a good reason why they need
to defer INIT_EX, that's Google's problem to solve.
