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

["Pratik R. Sampat" <prsampat@amd.com>]

On 5/5/2025 6:15 PM, Sean Christopherson wrote:
> 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)
> --
> 

I agree with this approach. One thing maybe to consider further is to also call
into SEV_platform_status() to check for init so that SEV/SEV-ES is not
penalized and disabled for SNP's failures. Another approach could be to break
up the SEV and SNP init setup so that we can spare a couple of platform calls
in the process?

> 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.
Ack!

Pratik