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