Re: [PATCH v8 00/10] Basic SEV-SNP Selftests

[Sean Christopherson <seanjc@google.com>]

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.