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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <34890707-201a-44f9-afb3-b065ae71b246@amd.com>
Date: Mon, 5 May 2025 18:36:37 -0500
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Sean Christopherson <seanjc@...gle.com>,
 "Pratik R. Sampat" <prsampat@....com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org, kvm@...r.kernel.org,
 linux-kselftest@...r.kernel.org, pbonzini@...hat.com,
 thomas.lendacky@....com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, shuah@...nel.org, pgonda@...gle.com,
 nikunj@....com, pankaj.gupta@....com, michael.roth@....com, sraithal@....com
Subject: Re: [PATCH v8 00/10] Basic SEV-SNP Selftests

Hello Sean,

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)
> --
> 
> Ashish, what am I missing?
> 

As far as setting sev*_enabled is concerned, i believe they are more specific to SNP/SEV/SEV-ES being enabled in the system,
which is separate from SEV_INIT/SNP_INIT (SNP_INIT success indicates that RMP been initialized, SNP has to be already enabled via 
MSR_SYSCFG before SNP_INIT is called), though SEV_INIT/SNP_INIT may fail but SEV/SNP support will still be enabled on the
system.

Additionally as SEV_INIT/SNP_INIT during sev_platform_init() have failed, so any SEV/SEV-ES/SNP VM launch will fail
as the firmware will return invalid platform state as INITs have failed.

>From my understanding, sev*_enabled indicates the user support to enable/disable support for SEV/SEV-ES/SEV-SNP, 
as the sev*_enabled are the KVM module parameters, while sev*_supported indicates if platform has that support enabled.

And before the SEV/SNP init support was moved to KVM from CCP module, doing SEV/SNP INIT could fail but that still
had KVM detecting SEV/SNP support enabled, so this moving SEV/SNP init stuff to KVM module from CCP driver is
consistent with the previous behavior.

Thanks,
Ashish

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