Re: [PATCH v2 5/5] KVM: selftests: teach sev_*_test about revoking VM types

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The patch updates SEV-ES and SEV-SNP support checks to use `KVM_CAP_VM_TYPES` instead of `CPUID` to handle cases where VM types are revoked (e.g., due to ASID exhaustion), but misses applying this logic to basic SEV, leading to test crashes.
--

--- Patch [5]: [PATCH v2 5/5] KVM: selftests: teach sev_*_test about revoking VM types ---
commit 2ec0ef2342e44224fa7c2705ca4877c7bb8e8605
Author: Tycho Andersen (AMD) <[email protected]>

KVM: selftests: teach sev_*_test about revoking VM types

Updates KVM selftests to use the VM type bit instead of CPUID to
determine SEV-ES and SEV-SNP support. This accommodates environments
where kernel and firmware configurations revoke VM types.

> diff --git a/tools/testing/selftests/kvm/x86/sev_init2_tests.c b/tools/testing/selftests/kvm/x86/sev_init2_tests.c
> index b238615196ade..97bd036b4f1c5 100644
> --- a/tools/testing/selftests/kvm/x86/sev_init2_tests.c
> +++ b/tools/testing/selftests/kvm/x86/sev_init2_tests.c
> @@ -133,9 +133,8 @@ int main(int argc, char *argv[])
>  	have_sev = kvm_cpu_has(X86_FEATURE_SEV);
>  	TEST_ASSERT(have_sev == !!(kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SEV_VM)),
>  		    "sev: KVM_CAP_VM_TYPES (%x) does not match cpuid (checking %x)",
>  		    kvm_check_cap(KVM_CAP_VM_TYPES), 1 << KVM_X86_SEV_VM);
>  
>  	TEST_REQUIRE(kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SEV_VM));

[Severity: Medium]
This isn't a bug introduced by this patch, but does the same logic need to
be applied to basic SEV here? 

If base SEV ASIDs are exhausted, the kernel revokes KVM_X86_SEV_VM support,
but CPUID still advertises X86_FEATURE_SEV. Since this code asserts that
CPUID perfectly matches KVM_CAP_VM_TYPES, won't this cause a test crash when
they inevitably diverge?

> -	have_sev_es = kvm_cpu_has(X86_FEATURE_SEV_ES);
> +	have_sev_es = kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SEV_ES_VM);

[ ... ]

> diff --git a/tools/testing/selftests/kvm/x86/sev_migrate_tests.c b/tools/testing/selftests/kvm/x86/sev_migrate_tests.c
> index 0a6dfba3905b6..3f2c3b00e3bc4 100644
> --- a/tools/testing/selftests/kvm/x86/sev_migrate_tests.c
> +++ b/tools/testing/selftests/kvm/x86/sev_migrate_tests.c
> @@ -374,7 +374,7 @@ int main(int argc, char *argv[])
>  
>  	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV));

[Severity: Medium]
This is a pre-existing issue, but should this TEST_REQUIRE check
KVM_CAP_VM_TYPES instead of CPUID? 

If the SEV VM type has been revoked, CPUID will still report support, which
might lead to crashes upon VM creation when the migration tests execute.

> -	have_sev_es = kvm_cpu_has(X86_FEATURE_SEV_ES);
> +	have_sev_es = kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SEV_ES_VM);
>  
>  	if (kvm_has_cap(KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM)) {
>  		test_sev_migrate_from(/* es= */ false);

[ ... ]

> diff --git a/tools/testing/selftests/kvm/x86/sev_smoke_test.c b/tools/testing/selftests/kvm/x86/sev_smoke_test.c
> index 8bd37a476f159..f3c39335ff39a 100644
> --- a/tools/testing/selftests/kvm/x86/sev_smoke_test.c
> +++ b/tools/testing/selftests/kvm/x86/sev_smoke_test.c
> @@ -247,8 +247,8 @@ int main(int argc, char *argv[])
>  {
>  	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SEV));
>  
>  	test_sev_smoke(guest_sev_code, KVM_X86_SEV_VM, 0);

[Severity: Medium]
This is a pre-existing issue, but does gating execution on CPUID rather
than KVM_CAP_VM_TYPES cause an issue here? 

If KVM lacks SEV support despite CPUID, won't it crash when calling
test_sev_smoke() for base SEV?

> -	if (kvm_cpu_has(X86_FEATURE_SEV_ES))
> +	if (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SEV_ES_VM))
>  		test_sev_smoke(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5