Re: [PATCH 6/6] KVM: selftests: Test Hyper-V extended hypercall exit to userspace

[David Matlack <dmatlack@google.com>]

On Fri, Nov 04, 2022 at 09:57:04PM -0700, Vipin Sharma wrote:
> Hyper-V extended hypercalls by default exit to userspace. Verify
> userspace gets the call, update the result and then guest verifies
> result it received.
> 
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> ---
>  tools/testing/selftests/kvm/.gitignore        |  1 +
>  tools/testing/selftests/kvm/Makefile          |  1 +
>  .../kvm/x86_64/hyperv_extended_hcalls.c       | 90 +++++++++++++++++++
>  3 files changed, 92 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/x86_64/hyperv_extended_hcalls.c
> 
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index 2f0d705db9db..ffe06dd1cc6e 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -24,6 +24,7 @@
>  /x86_64/kvm_pv_test
>  /x86_64/hyperv_clock
>  /x86_64/hyperv_cpuid
> +/x86_64/hyperv_extended_hcalls

nit: Any reason not to name this hyperv_extended_hypercalls? It's not
too long and as a non-Hyper-V developer it's easier to read.

>  /x86_64/hyperv_features
>  /x86_64/hyperv_svm_test
>  /x86_64/max_vcpuid_cap_test
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 0172eb6cb6ee..366345099363 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -85,6 +85,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/emulator_error_test
>  TEST_GEN_PROGS_x86_64 += x86_64/fix_hypercall_test
>  TEST_GEN_PROGS_x86_64 += x86_64/hyperv_clock
>  TEST_GEN_PROGS_x86_64 += x86_64/hyperv_cpuid
> +TEST_GEN_PROGS_x86_64 += x86_64/hyperv_extended_hcalls
>  TEST_GEN_PROGS_x86_64 += x86_64/hyperv_features
>  TEST_GEN_PROGS_x86_64 += x86_64/hyperv_svm_test
>  TEST_GEN_PROGS_x86_64 += x86_64/kvm_clock_test
> diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_extended_hcalls.c b/tools/testing/selftests/kvm/x86_64/hyperv_extended_hcalls.c
> new file mode 100644
> index 000000000000..d378877235d4
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/hyperv_extended_hcalls.c
> @@ -0,0 +1,90 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Test Hyper-V extended hypercalls

It would probably be worth adding a note in this comment that the
negative tests for extended hypercalls live in hyperv_features.c, that
way someone doesn't accidentally go down the rabbit hole of adding
negative tests here in the future.

> + *
> + * Copyright 2020 Google LLC

2022 :)

> + * Author: Vipin Sharma <vipinsh@google.com>
> + */
> +
> +#include "kvm_util.h"
> +#include "processor.h"
> +#include "hyperv.h"
> +
> +/* Any value is fine */
> +#define EXT_CAPABILITIES 0xbull
> +
> +static void guest_code(vm_vaddr_t pgs_gpa, vm_vaddr_t output_pg_gva)
> +{
> +	uint64_t res, vector;
> +	uint64_t *output_gva;
> +
> +	wrmsr(HV_X64_MSR_GUEST_OS_ID, hv_linux_guest_id());
> +	wrmsr(HV_X64_MSR_HYPERCALL, pgs_gpa);
> +
> +	output_gva = (uint64_t *)output_pg_gva;
> +
> +	vector = hypercall(HV_EXT_CALL_QUERY_CAPABILITIES, pgs_gpa,
> +			   pgs_gpa + 4096, &res);
> +
> +	GUEST_ASSERT_1(!vector, vector);
> +	GUEST_ASSERT_2(res == HV_STATUS_SUCCESS, res, HV_STATUS_SUCCESS);

GUEST_ASSERT_EQ(res, HV_STATUS_SUCCESS);

> +
> +	/* TLFS states output will be a uint64_t value */
> +	GUEST_ASSERT_2(*output_gva == EXT_CAPABILITIES, *output_gva,
> +		       EXT_CAPABILITIES);

GUEST_ASSERT_EQ(*output_gva, EXT_CAPABILITIES);

> +
> +	GUEST_DONE();
> +}
> +
> +static void guest_extended_hcall_test(void)
> +{
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_run *run;
> +	struct kvm_vm *vm;
> +	struct ucall uc;
> +	vm_vaddr_t hcall_page;
> +	uint64_t *outval;
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +	run = vcpu->run;
> +	vcpu_enable_cap(vcpu, KVM_CAP_HYPERV_ENFORCE_CPUID, 1);
> +	vcpu_set_hv_cpuid(vcpu);

Check if KVM offers HV_ENABLE_EXTENDED_HYPERCALLS in CPUID, and skip the
test if not.

> +
> +	/* Hypercall input/output */
> +	hcall_page = vm_vaddr_alloc_pages(vm, 2);
> +	memset(addr_gva2hva(vm, hcall_page), 0x0, 2 * getpagesize());

s/getpagesize()/vm->page_size/

> +	vcpu_args_set(vcpu, 2, addr_gva2gpa(vm, hcall_page), hcall_page + 4096);

s/4096/vm->page_size/

And to avoid hard-coding 4096 in guest_code(), you could pass in the GPA
of the ouput page as another argument.

> +
> +	vcpu_run(vcpu);
> +
> +	TEST_ASSERT((run->exit_reason == KVM_EXIT_HYPERV),
> +		    "unexpected exit reason: %u (%s)", run->exit_reason,
> +		    exit_reason_str(run->exit_reason));
> +
> +	outval = addr_gpa2hva(vm, run->hyperv.u.hcall.params[1]);
> +	*outval = EXT_CAPABILITIES;
> +	run->hyperv.u.hcall.result = HV_STATUS_SUCCESS;
> +
> +	vcpu_run(vcpu);
> +
> +	TEST_ASSERT((run->exit_reason == KVM_EXIT_IO),
> +		    "unexpected exit reason: %u (%s)", run->exit_reason,
> +		    exit_reason_str(run->exit_reason));

Optional: Asserting a specific exit reason is a pretty common pattern in
the x86 selftests. It'd be nice to create a common macro for it. e.g.

	ASSERT_EXIT_REASON(vcpu, KVM_EXIT_IO);

> +
> +	switch (get_ucall(vcpu, &uc)) {
> +	case UCALL_ABORT:
> +		REPORT_GUEST_ASSERT_2(uc, "arg1 = %ld, arg2 = %ld");
> +		break;
> +	case UCALL_DONE:
> +		break;
> +	default:
> +		TEST_FAIL("Unhandled ucall: %ld", uc.cmd);
> +	}
> +
> +	kvm_vm_free(vm);
> +}
> +
> +int main(void)
> +{
> +	guest_extended_hcall_test();

Why not just put all this in main()?

> +}

return 0?

> -- 
> 2.38.1.273.g43a17bfeac-goog
>