Re: [PATCH v3 10/12] KVM: selftests: Test VMware hypercall exits

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Low] KVM: selftests: native_hypercall_insn() rejects Zhaoxin VMX CPUs
- [Low] KVM: selftests: vmware_hypercall_test open-codes
  vcpu_run_complete_io()
- [Low] KVM: selftests: exit_reasons_known[] omits
  KVM_EXIT_STRING(VMWARE)
- [Low] KVM: selftests: main() in vmware_hypercall_test duplicates
  (false, true) case
--

commit 54cae967f2b7981d769102ff002d20cd884b4d2d
Author: Zack Rusin <zack.rusin@broadcom.com>

KVM: selftests: Test VMware hypercall exits

This commit adds vmware_hypercall_test to cover KVM_VMWARE_ENABLE_HYPERCALL
and KVM_EXIT_VMWARE, and enables VMware hypercall exits in xen_vmcall_test.

> diff --git a/tools/testing/selftests/kvm/x86/vmware_hypercall_test.c b/tools/testing/selftests/kvm/x86/vmware_hypercall_test.c
> new file mode 100644
> index 0000000000000..d882d3661022b
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/vmware_hypercall_test.c
> @@ -0,0 +1,388 @@
[ ... ]
> +static const u8 *native_hypercall_insn(void)
> +{
> +	if (host_cpu_is_intel)
> +		return vmx_vmcall;
> +	if (host_cpu_is_amd_compatible)
> +		return svm_vmmcall;
> +
> +	TEST_FAIL("VMware hypercall test requires an Intel or AMD-compatible CPU");
> +}

[Severity: Low]
Should native_hypercall_insn() check host_cpu_is_intel_compatible instead
of host_cpu_is_intel?  Zhaoxin CPUs implement VMX and use VMCALL (as
handled in fix_hypercall_test.c and X86_HYPERCALL()), so checking only
host_cpu_is_intel causes the test to hit TEST_FAIL() on Zhaoxin hosts.

[ ... ]

> +static void run_to_vmware_exit(struct vmware_test *test, bool cpl3,
> +			       bool compat)
> +{
> +	test->vcpu->run->kvm_valid_regs = KVM_SYNC_X86_REGS;
> +	vcpu_run(test->vcpu);
> +	TEST_ASSERT_KVM_EXIT_REASON(test->vcpu, KVM_EXIT_VMWARE);
> +	TEST_ASSERT(test->vcpu->run->kvm_valid_regs & KVM_SYNC_X86_REGS,
> +		    "KVM did not return synchronized GPRs");
> +	assert_hypercall_state(test, cpl3, compat);
> +}

[Severity: Low]
Does exit_reasons_known[] in tools/testing/selftests/kvm/lib/kvm_util.c
need a KVM_EXIT_STRING(VMWARE) entry?  Without it, exit_reason_str()
returns "Unknown" if TEST_ASSERT_KVM_EXIT_REASON() fails on an expected or
unexpected KVM_EXIT_VMWARE exit.

[ ... ]

> +static void test_synchronized_output(bool compat)
> +{
[ ... ]
> +	run->s.regs.regs = expected;
> +	run->kvm_dirty_regs = KVM_SYNC_X86_REGS;
> +	run->immediate_exit = 1;
> +
> +	errno = 0;
> +	ret = __vcpu_run(test.vcpu);
> +	run->immediate_exit = 0;
> +	TEST_ASSERT(ret == -1 && errno == EINTR,
> +		    "Completion with immediate_exit failed: ret=%d errno=%d",
> +		    ret, errno);
[ ... ]
> +}
> +
> +static void test_valid_exception(u32 vector, u32 error_code, u64 address,
> +				 bool dirty_regs)
> +{
[ ... ]
> +	run->immediate_exit = 1;
> +
> +	errno = 0;
> +	ret = __vcpu_run(test.vcpu);
> +	run->immediate_exit = 0;
> +	TEST_ASSERT(ret == -1 && errno == EINTR,
> +		    "Exception completion failed: ret=%d errno=%d", ret, errno);

[Severity: Low]
Can test_synchronized_output() and test_valid_exception() use
vcpu_run_complete_io(test.vcpu) instead of open-coding the immediate_exit
sequence?

[ ... ]

> +int main(int argc, char *argv[])
> +{
[ ... ]
> +	test_synchronized_output(false);
> +	test_synchronized_output(true);
> +
> +	test = create_test(true, false, ARG_RAX);
> +	run_to_vmware_exit(&test, true, false);
> +	destroy_test(&test);
> +
> +	test = create_test(false, true, ARG_RAX);
> +	run_to_vmware_exit(&test, false, true);
> +	destroy_test(&test);

[Severity: Low]
Was the second create_test() / run_to_vmware_exit() block meant to pass
(true, true) instead of (false, true)?  test_synchronized_output(true)
already exercises (cpl3=false, compat=true), whereas (cpl3=true,
compat=true) is not tested.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794374897.git.zack.rusin@broadcom.com?part=10