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.
