Re: [PATCH 10/11] KVM: selftests: Extend nested x2APIC test to validate disabling x2APIC virt

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] selftests: vmread return value overwrites control variable
- [High] selftests: nested_x2apic_test fails unconditionally on SVM
- [High] selftests: unchecked secondary execution controls in l1_vmx_code_part2
--

commit 654f28b9879b51044371cba2ce7892e66b1a94e7
Author: Sean Christopherson <seanjc@google.com>

KVM: selftests: Extend nested x2APIC test to validate disabling x2APIC virt

This commit extends the nested x2APIC test to further validate disabling
x2APIC virtualization. It splits the guest code and intercept checks to
validate interactions with secondary execution controls and TPR shadows.

> diff --git a/tools/testing/selftests/kvm/x86/nested_x2apic_test.c b/tools/testing/selftests/kvm/x86/nested_x2apic_test.c
> index 3b59ba3e33420..e94d4e77256bd 100644
> --- a/tools/testing/selftests/kvm/x86/nested_x2apic_test.c
> +++ b/tools/testing/selftests/kvm/x86/nested_x2apic_test.c
[ ... ]
> +static void l1_vmx_code_part2(void)
>  {
> -	x2apic_enable();
> +	u64 control;
>  
> -	if (this_cpu_has(X86_FEATURE_SVM))
> -		l1_svm_code(test_data);
> -	else
> -		l1_vmx_code(test_data);
> +	control = vmreadz(CPU_BASED_VM_EXEC_CONTROL);
> +	control &= ~CPU_BASED_TPR_SHADOW;
> +	GUEST_ASSERT_EQ(vmwrite(CPU_BASED_VM_EXEC_CONTROL, control), 0);
> +
> +	control = vmread(SECONDARY_VM_EXEC_CONTROL, &control);

[Severity: High]
Does this assignment overwrite the control variable with the return value of
vmread()? Since vmread() returns 0 on success, control gets assigned 0
instead of retaining the VMCS value populated by reference, which then causes
a regression by corrupting the subsequent vmwrite() when it clears all
secondary execution controls.

> +	control &= ~(SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
> +			SECONDARY_EXEC_APIC_REGISTER_VIRT |
> +			SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> +	GUEST_ASSERT_EQ(vmwrite(SECONDARY_VM_EXEC_CONTROL, control), 0);

[Severity: High]
Are we missing a check for CPU_BASED_ACTIVATE_SECONDARY_CONTROLS before
accessing the secondary execution controls here?

In l1_vmx_code(), we check CPU_BASED_ACTIVATE_SECONDARY_CONTROLS before
modifying them. Since we don't verify it here, won't this cause a test
regression on incompatible CPUs when vmread() and vmwrite() return error
status codes?

> +
> +	GUEST_ASSERT(!vmresume());
> +	GUEST_ASSERT_EQ(vmreadz(VM_EXIT_REASON), EXIT_REASON_CPUID);
> +	GUEST_ASSERT_EQ(vmwrite(GUEST_RIP,
> +			vmreadz(GUEST_RIP) + vmreadz(VM_EXIT_INSTRUCTION_LEN)), 0);
> +}
[ ... ]
> +static void l1_guest_code(void *test_data)
> +{
> +	x2apic_enable();
> +
> +	if (this_cpu_has(X86_FEATURE_SVM))
> +		l1_svm_code(test_data);
> +	else
> +		l1_vmx_code(test_data);
> +
> +	GUEST_ASSERT_EQ(x2apic_read_reg(APIC_TASKPRI), 0);

[Severity: High]
Will this assertion unconditionally fail on SVM hardware, causing a test
regression?

Since TPR shadow behavior is only implemented for VMX, L2's write of 0xf0
directly modifies L1's TPR on SVM. When l1_guest_code() expects L1's TPR to
be 0, it appears this will fail. Is this the intended behavior for SVM tests?

> +	x2apic_write_reg(APIC_TASKPRI, 0xf0);
> +
> +	l1_test_x2apic_intercepts();
> +
> +	if (this_cpu_has(X86_FEATURE_VMX))
> +		l1_vmx_code_part2();
> +
> +	l1_test_x2apic_intercepts();
> +
>  	GUEST_DONE();
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260926053253.195597-1-pbonzini@redhat.com?part=10