[V2 PATCH 0/8] Execute hypercalls from guests according to cpu type

[V2 PATCH 0/8] Execute hypercalls from guests according to cpu type

[Vishal Annapurve]

This series is posted in context of the discussion at:
https://lore.kernel.org/lkml/Ywa9T+jKUpaHLu%2Fl@google.com/

Changes in v2:
* Addressed comments from Andrew and David
  * Common function with constructor attribute used to setup initial state
  * Changes are split in more logical granules as per feedback

Major changes:
1) Move common startup logic to a single function in kvm_util.c
2) Introduce following APIs:
	kvm_selftest_arch_init: to perform arch specific common startup.
	kvm_arch_post_vm_elf_load: to update the guest memory state to convey
		common information to guests.
3) For x86, capture cpu type at startup and pass on the cpu type to
guest after guest elf is loaded.
4) Execute hypercall instruction from within guest VMs according to the
cpu type. This will help prevent an extra kvm exit during hypercall
execution.

Vishal Annapurve (8):
  KVM: selftests: move common startup logic to kvm_util.c
  KVM: selftests: Add arch specific initialization
  KVM: selftests: Add arch specific post vm load setup
  KVM: selftests: x86: Precompute the result for is_{intel,amd}_cpu()
  KVM: selftests: x86: delete svm_vmcall_test
  KVM: selftests: x86: Execute cpu specific hypercall from nested guests
  Kvm: selftests: x86: Execute cpu specific vmcall instruction
  KVM: selftests: x86: xen: Execute cpu specific vmcall instruction

 tools/testing/selftests/kvm/.gitignore        |  1 -
 .../selftests/kvm/aarch64/arch_timer.c        |  3 -
 .../selftests/kvm/aarch64/hypercalls.c        |  2 -
 .../testing/selftests/kvm/aarch64/vgic_irq.c  |  3 -
 .../selftests/kvm/include/kvm_util_base.h     |  9 +++
 .../selftests/kvm/include/x86_64/processor.h  | 10 +++
 .../selftests/kvm/include/x86_64/vmx.h        |  9 ---
 .../selftests/kvm/lib/aarch64/processor.c     | 22 +++---
 tools/testing/selftests/kvm/lib/elf.c         |  2 +
 tools/testing/selftests/kvm/lib/kvm_util.c    |  8 ++
 .../selftests/kvm/lib/riscv/processor.c       |  8 ++
 .../selftests/kvm/lib/s390x/processor.c       |  8 ++
 .../selftests/kvm/lib/x86_64/perf_test_util.c |  2 +-
 .../selftests/kvm/lib/x86_64/processor.c      | 38 +++++++++-
 .../testing/selftests/kvm/memslot_perf_test.c |  3 -
 tools/testing/selftests/kvm/rseq_test.c       |  3 -
 tools/testing/selftests/kvm/s390x/memop.c     |  2 -
 tools/testing/selftests/kvm/s390x/resets.c    |  2 -
 .../selftests/kvm/s390x/sync_regs_test.c      |  3 -
 .../selftests/kvm/set_memory_region_test.c    |  3 -
 .../kvm/x86_64/cr4_cpuid_sync_test.c          |  3 -
 .../kvm/x86_64/emulator_error_test.c          |  3 -
 .../selftests/kvm/x86_64/hyperv_cpuid.c       |  3 -
 .../selftests/kvm/x86_64/platform_info_test.c |  3 -
 .../kvm/x86_64/pmu_event_filter_test.c        |  3 -
 .../selftests/kvm/x86_64/set_sregs_test.c     |  3 -
 tools/testing/selftests/kvm/x86_64/smm_test.c |  2 +-
 .../testing/selftests/kvm/x86_64/state_test.c |  8 +-
 .../kvm/x86_64/svm_nested_soft_inject_test.c  |  3 -
 .../selftests/kvm/x86_64/svm_vmcall_test.c    | 74 -------------------
 .../selftests/kvm/x86_64/sync_regs_test.c     |  3 -
 .../selftests/kvm/x86_64/userspace_io_test.c  |  3 -
 .../kvm/x86_64/userspace_msr_exit_test.c      |  3 -
 .../kvm/x86_64/vmx_apic_access_test.c         |  2 +-
 .../selftests/kvm/x86_64/vmx_dirty_log_test.c |  2 +-
 .../kvm/x86_64/vmx_nested_tsc_scaling_test.c  |  2 +-
 .../kvm/x86_64/vmx_preemption_timer_test.c    |  2 +-
 .../kvm/x86_64/vmx_tsc_adjust_test.c          |  2 +-
 .../selftests/kvm/x86_64/xen_shinfo_test.c    | 64 ++++++----------
 .../selftests/kvm/x86_64/xen_vmcall_test.c    | 14 +++-
 40 files changed, 138 insertions(+), 205 deletions(-)
 delete mode 100644 tools/testing/selftests/kvm/x86_64/svm_vmcall_test.c

-- 
2.37.2.789.g6183377224-goog

[V2 PATCH 1/8] KVM: selftests: move common startup logic to kvm_util.c

[Vishal Annapurve]

Consolidate common startup logic in one place by implementing a single
setup function with __attribute((constructor)) for all selftests within
kvm_util.c.

This allows moving logic like:
        /* Tell stdout not to buffer its content */
        setbuf(stdout, NULL);
to a single file for all selftests.

This will also allow any required setup at entry in future to be done in
common main function.

More context is discussed at:
https://lore.kernel.org/lkml/Ywa9T+jKUpaHLu%2Fl@google.com/

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vishal Annapurve <vannapurve@google.com>
---
 tools/testing/selftests/kvm/aarch64/arch_timer.c            | 3 ---
 tools/testing/selftests/kvm/aarch64/hypercalls.c            | 2 --
 tools/testing/selftests/kvm/aarch64/vgic_irq.c              | 3 ---
 tools/testing/selftests/kvm/lib/kvm_util.c                  | 6 ++++++
 tools/testing/selftests/kvm/memslot_perf_test.c             | 3 ---
 tools/testing/selftests/kvm/rseq_test.c                     | 3 ---
 tools/testing/selftests/kvm/s390x/memop.c                   | 2 --
 tools/testing/selftests/kvm/s390x/resets.c                  | 2 --
 tools/testing/selftests/kvm/s390x/sync_regs_test.c          | 3 ---
 tools/testing/selftests/kvm/set_memory_region_test.c        | 3 ---
 tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c    | 3 ---
 tools/testing/selftests/kvm/x86_64/emulator_error_test.c    | 3 ---
 tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c           | 3 ---
 tools/testing/selftests/kvm/x86_64/platform_info_test.c     | 3 ---
 tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c  | 3 ---
 tools/testing/selftests/kvm/x86_64/set_sregs_test.c         | 3 ---
 .../selftests/kvm/x86_64/svm_nested_soft_inject_test.c      | 3 ---
 tools/testing/selftests/kvm/x86_64/sync_regs_test.c         | 3 ---
 tools/testing/selftests/kvm/x86_64/userspace_io_test.c      | 3 ---
 .../testing/selftests/kvm/x86_64/userspace_msr_exit_test.c  | 3 ---
 20 files changed, 6 insertions(+), 54 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
index 574eb73f0e90..07836bd2672b 100644
--- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
+++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
@@ -462,9 +462,6 @@ int main(int argc, char *argv[])
 {
 	struct kvm_vm *vm;
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	if (!parse_args(argc, argv))
 		exit(KSFT_SKIP);
 
diff --git a/tools/testing/selftests/kvm/aarch64/hypercalls.c b/tools/testing/selftests/kvm/aarch64/hypercalls.c
index a39da3fe4952..6463fd118429 100644
--- a/tools/testing/selftests/kvm/aarch64/hypercalls.c
+++ b/tools/testing/selftests/kvm/aarch64/hypercalls.c
@@ -306,8 +306,6 @@ static void test_run(void)
 
 int main(void)
 {
-	setbuf(stdout, NULL);
-
 	test_run();
 	return 0;
 }
diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
index 17417220a083..3f204f2e93bf 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
@@ -818,9 +818,6 @@ int main(int argc, char **argv)
 	int opt;
 	bool eoi_split = false;
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	while ((opt = getopt(argc, argv, "hn:e:l:")) != -1) {
 		switch (opt) {
 		case 'n':
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 9889fe0d8919..3c83838999f5 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1979,3 +1979,9 @@ void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data,
 		break;
 	}
 }
+
+void __attribute((constructor)) kvm_selftest_init(void)
+{
+	/* Tell stdout not to buffer its content. */
+	setbuf(stdout, NULL);
+}
diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index 44995446d942..f7ba77ff45c9 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -1007,9 +1007,6 @@ int main(int argc, char *argv[])
 	struct test_result rbestslottime;
 	int tctr;
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	if (!parse_args(argc, argv, &targs))
 		return -1;
 
diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c
index fac248a43666..1cc459822c27 100644
--- a/tools/testing/selftests/kvm/rseq_test.c
+++ b/tools/testing/selftests/kvm/rseq_test.c
@@ -205,9 +205,6 @@ int main(int argc, char *argv[])
 	struct kvm_vcpu *vcpu;
 	u32 cpu, rseq_cpu;
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	r = sched_getaffinity(0, sizeof(possible_mask), &possible_mask);
 	TEST_ASSERT(!r, "sched_getaffinity failed, errno = %d (%s)", errno,
 		    strerror(errno));
diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
index 9113696d5178..3fd81e58f40c 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -760,8 +760,6 @@ int main(int argc, char *argv[])
 
 	TEST_REQUIRE(kvm_has_cap(KVM_CAP_S390_MEM_OP));
 
-	setbuf(stdout, NULL);	/* Tell stdout not to buffer its content */
-
 	ksft_print_header();
 
 	ksft_set_plan(ARRAY_SIZE(testlist));
diff --git a/tools/testing/selftests/kvm/s390x/resets.c b/tools/testing/selftests/kvm/s390x/resets.c
index 19486084eb30..e41e2cb8ffa9 100644
--- a/tools/testing/selftests/kvm/s390x/resets.c
+++ b/tools/testing/selftests/kvm/s390x/resets.c
@@ -296,8 +296,6 @@ int main(int argc, char *argv[])
 	bool has_s390_vcpu_resets = kvm_check_cap(KVM_CAP_S390_VCPU_RESETS);
 	int idx;
 
-	setbuf(stdout, NULL);	/* Tell stdout not to buffer its content */
-
 	ksft_print_header();
 	ksft_set_plan(ARRAY_SIZE(testlist));
 
diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
index 3fdb6e2598eb..2ddde41c44ba 100644
--- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
@@ -231,9 +231,6 @@ int main(int argc, char *argv[])
 
 	TEST_REQUIRE(kvm_has_cap(KVM_CAP_SYNC_REGS));
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	ksft_print_header();
 
 	ksft_set_plan(ARRAY_SIZE(testlist));
diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
index 0d55f508d595..614141d6e53d 100644
--- a/tools/testing/selftests/kvm/set_memory_region_test.c
+++ b/tools/testing/selftests/kvm/set_memory_region_test.c
@@ -392,9 +392,6 @@ int main(int argc, char *argv[])
 	int i, loops;
 #endif
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 #ifdef __x86_64__
 	/*
 	 * FIXME: the zero-memslot test fails on aarch64 and s390x because
diff --git a/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c b/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
index 4208487652f8..1027a671c7d3 100644
--- a/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
+++ b/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
@@ -57,9 +57,6 @@ int main(int argc, char *argv[])
 
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XSAVE));
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
 	run = vcpu->run;
 
diff --git a/tools/testing/selftests/kvm/x86_64/emulator_error_test.c b/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
index 236e11755ba6..3334adcfd591 100644
--- a/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
+++ b/tools/testing/selftests/kvm/x86_64/emulator_error_test.c
@@ -156,9 +156,6 @@ int main(int argc, char *argv[])
 	uint64_t *hva;
 	int rc;
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	TEST_REQUIRE(kvm_has_cap(KVM_CAP_SMALLER_MAXPHYADDR));
 
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c b/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
index e804eb08dff9..5c27efbf405e 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
@@ -134,9 +134,6 @@ int main(int argc, char *argv[])
 	const struct kvm_cpuid2 *hv_cpuid_entries;
 	struct kvm_vcpu *vcpu;
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	TEST_REQUIRE(kvm_has_cap(KVM_CAP_HYPERV_CPUID));
 
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
diff --git a/tools/testing/selftests/kvm/x86_64/platform_info_test.c b/tools/testing/selftests/kvm/x86_64/platform_info_test.c
index 76417c7d687b..310a104d94f0 100644
--- a/tools/testing/selftests/kvm/x86_64/platform_info_test.c
+++ b/tools/testing/selftests/kvm/x86_64/platform_info_test.c
@@ -72,9 +72,6 @@ int main(int argc, char *argv[])
 	struct kvm_vm *vm;
 	uint64_t msr_platform_info;
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	TEST_REQUIRE(kvm_has_cap(KVM_CAP_MSR_PLATFORM_INFO));
 
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
diff --git a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
index ea4e259a1e2e..a6ffa245c897 100644
--- a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
+++ b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
@@ -447,9 +447,6 @@ int main(int argc, char *argv[])
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	TEST_REQUIRE(kvm_has_cap(KVM_CAP_PMU_EVENT_FILTER));
 
 	TEST_REQUIRE(use_intel_pmu() || use_amd_pmu());
diff --git a/tools/testing/selftests/kvm/x86_64/set_sregs_test.c b/tools/testing/selftests/kvm/x86_64/set_sregs_test.c
index 2bb08bf2125d..a284fcef6ed7 100644
--- a/tools/testing/selftests/kvm/x86_64/set_sregs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/set_sregs_test.c
@@ -82,9 +82,6 @@ int main(int argc, char *argv[])
 	uint64_t cr4;
 	int rc;
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	/*
 	 * Create a dummy VM, specifically to avoid doing KVM_SET_CPUID2, and
 	 * use it to verify all supported CR4 bits can be set prior to defining
diff --git a/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
index e637d7736012..e497ace629c1 100644
--- a/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
+++ b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
@@ -194,9 +194,6 @@ static void run_test(bool is_nmi)
 
 int main(int argc, char *argv[])
 {
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SVM));
 
 	TEST_ASSERT(kvm_cpu_has(X86_FEATURE_NRIPS),
diff --git a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
index 9b6db0b0b13e..d2f9b5bdfab2 100644
--- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
@@ -90,9 +90,6 @@ int main(int argc, char *argv[])
 	struct kvm_vcpu_events events;
 	int rv, cap;
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
 	TEST_REQUIRE((cap & TEST_SYNC_FIELDS) == TEST_SYNC_FIELDS);
 	TEST_REQUIRE(!(cap & INVALID_SYNC_FIELD));
diff --git a/tools/testing/selftests/kvm/x86_64/userspace_io_test.c b/tools/testing/selftests/kvm/x86_64/userspace_io_test.c
index 7316521428f8..91076c9787b4 100644
--- a/tools/testing/selftests/kvm/x86_64/userspace_io_test.c
+++ b/tools/testing/selftests/kvm/x86_64/userspace_io_test.c
@@ -56,9 +56,6 @@ int main(int argc, char *argv[])
 	struct kvm_vm *vm;
 	struct ucall uc;
 
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
 	run = vcpu->run;
 
diff --git a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
index a4f06370a245..8ef5c8b25e95 100644
--- a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
+++ b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
@@ -735,9 +735,6 @@ static void test_msr_permission_bitmap(void)
 
 int main(int argc, char *argv[])
 {
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
 	test_msr_filter_allow();
 
 	test_msr_filter_deny();
-- 
2.37.2.789.g6183377224-goog

Re: [V2 PATCH 1/8] KVM: selftests: move common startup logic to kvm_util.c

[Andrew Jones]

On Thu, Sep 15, 2022 at 12:04:41AM +0000, Vishal Annapurve wrote:
> Consolidate common startup logic in one place by implementing a single
> setup function with __attribute((constructor)) for all selftests within
> kvm_util.c.
> 
> This allows moving logic like:
>         /* Tell stdout not to buffer its content */
>         setbuf(stdout, NULL);
> to a single file for all selftests.
> 
> This will also allow any required setup at entry in future to be done in
> common main function.
> 
> More context is discussed at:
> https://lore.kernel.org/lkml/Ywa9T+jKUpaHLu%2Fl@google.com/
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> ---

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

[V2 PATCH 2/8] KVM: selftests: Add arch specific initialization

[Vishal Annapurve]

Introduce arch specific API: kvm_selftest_arch_init to allow each arch to
handle initialization before running any selftest logic.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vishal Annapurve <vannapurve@google.com>
---
 .../selftests/kvm/include/kvm_util_base.h      |  5 +++++
 .../selftests/kvm/lib/aarch64/processor.c      | 18 +++++++++---------
 tools/testing/selftests/kvm/lib/kvm_util.c     |  2 ++
 .../selftests/kvm/lib/riscv/processor.c        |  4 ++++
 .../selftests/kvm/lib/s390x/processor.c        |  4 ++++
 .../selftests/kvm/lib/x86_64/processor.c       |  4 ++++
 6 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 24fde97f6121..98edbbda9f97 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -834,4 +834,9 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm)
 	return __vm_enable_cap(vm, KVM_CAP_VM_DISABLE_NX_HUGE_PAGES, 0);
 }
 
+/*
+ * API to execute architecture specific setup before executing selftest logic.
+ */
+void kvm_selftest_arch_init(void);
+
 #endif /* SELFTEST_KVM_UTIL_BASE_H */
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index 6f5551368944..2281d6c5d02f 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -495,15 +495,6 @@ void aarch64_get_supported_page_sizes(uint32_t ipa,
 	close(kvm_fd);
 }
 
-/*
- * arm64 doesn't have a true default mode, so start by computing the
- * available IPA space and page sizes early.
- */
-void __attribute__((constructor)) init_guest_modes(void)
-{
-       guest_modes_append_default();
-}
-
 void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
 	       uint64_t arg2, uint64_t arg3, uint64_t arg4, uint64_t arg5,
 	       uint64_t arg6, struct arm_smccc_res *res)
@@ -528,3 +519,12 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
 		       [arg4] "r"(arg4), [arg5] "r"(arg5), [arg6] "r"(arg6)
 		     : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7");
 }
+
+/*
+ * arm64 doesn't have a true default mode, so start by computing the
+ * available IPA space and page sizes early.
+ */
+void kvm_selftest_arch_init(void)
+{
+	guest_modes_append_default();
+}
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 3c83838999f5..dafe4471a6c7 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1984,4 +1984,6 @@ void __attribute((constructor)) kvm_selftest_init(void)
 {
 	/* Tell stdout not to buffer its content. */
 	setbuf(stdout, NULL);
+
+	kvm_selftest_arch_init();
 }
diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c
index 604478151212..26660dd2ba78 100644
--- a/tools/testing/selftests/kvm/lib/riscv/processor.c
+++ b/tools/testing/selftests/kvm/lib/riscv/processor.c
@@ -362,3 +362,7 @@ void vcpu_args_set(struct kvm_vcpu *vcpu, unsigned int num, ...)
 void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
 {
 }
+
+void kvm_selftest_arch_init(void)
+{
+}
diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
index 89d7340d9cbd..8654ec74009a 100644
--- a/tools/testing/selftests/kvm/lib/s390x/processor.c
+++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
@@ -218,3 +218,7 @@ void vcpu_arch_dump(FILE *stream, struct kvm_vcpu *vcpu, uint8_t indent)
 void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
 {
 }
+
+void kvm_selftest_arch_init(void)
+{
+}
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 2e6e61bbe81b..20bf125f9363 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1311,3 +1311,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
 
 	return val == 'Y';
 }
+
+void kvm_selftest_arch_init(void)
+{
+}
-- 
2.37.2.789.g6183377224-goog

Re: [V2 PATCH 2/8] KVM: selftests: Add arch specific initialization

[Andrew Jones]

On Thu, Sep 15, 2022 at 12:04:42AM +0000, Vishal Annapurve wrote:
> Introduce arch specific API: kvm_selftest_arch_init to allow each arch to
> handle initialization before running any selftest logic.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> ---
>  .../selftests/kvm/include/kvm_util_base.h      |  5 +++++
>  .../selftests/kvm/lib/aarch64/processor.c      | 18 +++++++++---------
>  tools/testing/selftests/kvm/lib/kvm_util.c     |  2 ++
>  .../selftests/kvm/lib/riscv/processor.c        |  4 ++++
>  .../selftests/kvm/lib/s390x/processor.c        |  4 ++++
>  .../selftests/kvm/lib/x86_64/processor.c       |  4 ++++
>  6 files changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 24fde97f6121..98edbbda9f97 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -834,4 +834,9 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm)
>  	return __vm_enable_cap(vm, KVM_CAP_VM_DISABLE_NX_HUGE_PAGES, 0);
>  }
>  
> +/*
> + * API to execute architecture specific setup before executing selftest logic.
> + */
> +void kvm_selftest_arch_init(void);
> +
>  #endif /* SELFTEST_KVM_UTIL_BASE_H */
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index 6f5551368944..2281d6c5d02f 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -495,15 +495,6 @@ void aarch64_get_supported_page_sizes(uint32_t ipa,
>  	close(kvm_fd);
>  }
>  
> -/*
> - * arm64 doesn't have a true default mode, so start by computing the
> - * available IPA space and page sizes early.
> - */
> -void __attribute__((constructor)) init_guest_modes(void)
> -{
> -       guest_modes_append_default();
> -}
> -
>  void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
>  	       uint64_t arg2, uint64_t arg3, uint64_t arg4, uint64_t arg5,
>  	       uint64_t arg6, struct arm_smccc_res *res)
> @@ -528,3 +519,12 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
>  		       [arg4] "r"(arg4), [arg5] "r"(arg5), [arg6] "r"(arg6)
>  		     : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7");
>  }
> +
> +/*
> + * arm64 doesn't have a true default mode, so start by computing the
> + * available IPA space and page sizes early.
> + */

It'd be better to move this comment inside the function above the
guest_modes_append_default call.

> +void kvm_selftest_arch_init(void)
> +{
> +	guest_modes_append_default();
> +}
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 3c83838999f5..dafe4471a6c7 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -1984,4 +1984,6 @@ void __attribute((constructor)) kvm_selftest_init(void)
>  {
>  	/* Tell stdout not to buffer its content. */
>  	setbuf(stdout, NULL);
> +
> +	kvm_selftest_arch_init();
>  }
> diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c
> index 604478151212..26660dd2ba78 100644
> --- a/tools/testing/selftests/kvm/lib/riscv/processor.c
> +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c
> @@ -362,3 +362,7 @@ void vcpu_args_set(struct kvm_vcpu *vcpu, unsigned int num, ...)
>  void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
>  {
>  }
> +
> +void kvm_selftest_arch_init(void)
> +{
> +}
> diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
> index 89d7340d9cbd..8654ec74009a 100644
> --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
> +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
> @@ -218,3 +218,7 @@ void vcpu_arch_dump(FILE *stream, struct kvm_vcpu *vcpu, uint8_t indent)
>  void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
>  {
>  }
> +
> +void kvm_selftest_arch_init(void)
> +{
> +}
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 2e6e61bbe81b..20bf125f9363 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1311,3 +1311,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
>  
>  	return val == 'Y';
>  }
> +
> +void kvm_selftest_arch_init(void)
> +{
> +}
> -- 
> 2.37.2.789.g6183377224-goog
>

Otherwise,

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Re: [V2 PATCH 2/8] KVM: selftests: Add arch specific initialization

[Vishal Annapurve]

On Thu, Sep 15, 2022 at 2:44 AM Andrew Jones <andrew.jones@linux.dev> wrote:
>
> On Thu, Sep 15, 2022 at 12:04:42AM +0000, Vishal Annapurve wrote:
> > Introduce arch specific API: kvm_selftest_arch_init to allow each arch to
> > handle initialization before running any selftest logic.
> >
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> > ---
> >  .../selftests/kvm/include/kvm_util_base.h      |  5 +++++
> >  .../selftests/kvm/lib/aarch64/processor.c      | 18 +++++++++---------
> >  tools/testing/selftests/kvm/lib/kvm_util.c     |  2 ++
> >  .../selftests/kvm/lib/riscv/processor.c        |  4 ++++
> >  .../selftests/kvm/lib/s390x/processor.c        |  4 ++++
> >  .../selftests/kvm/lib/x86_64/processor.c       |  4 ++++
> >  6 files changed, 28 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > index 24fde97f6121..98edbbda9f97 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > @@ -834,4 +834,9 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm)
> >       return __vm_enable_cap(vm, KVM_CAP_VM_DISABLE_NX_HUGE_PAGES, 0);
> >  }
> >
> > +/*
> > + * API to execute architecture specific setup before executing selftest logic.
> > + */
> > +void kvm_selftest_arch_init(void);
> > +
> >  #endif /* SELFTEST_KVM_UTIL_BASE_H */
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > index 6f5551368944..2281d6c5d02f 100644
> > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > @@ -495,15 +495,6 @@ void aarch64_get_supported_page_sizes(uint32_t ipa,
> >       close(kvm_fd);
> >  }
> >
> > -/*
> > - * arm64 doesn't have a true default mode, so start by computing the
> > - * available IPA space and page sizes early.
> > - */
> > -void __attribute__((constructor)) init_guest_modes(void)
> > -{
> > -       guest_modes_append_default();
> > -}
> > -
> >  void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
> >              uint64_t arg2, uint64_t arg3, uint64_t arg4, uint64_t arg5,
> >              uint64_t arg6, struct arm_smccc_res *res)
> > @@ -528,3 +519,12 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
> >                      [arg4] "r"(arg4), [arg5] "r"(arg5), [arg6] "r"(arg6)
> >                    : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7");
> >  }
> > +
> > +/*
> > + * arm64 doesn't have a true default mode, so start by computing the
> > + * available IPA space and page sizes early.
> > + */
>
> It'd be better to move this comment inside the function above the
> guest_modes_append_default call.
>

Ack, will fix this in the next series.
> > +void kvm_selftest_arch_init(void)
> > +{
> > +     guest_modes_append_default();
> > +}
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 3c83838999f5..dafe4471a6c7 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -1984,4 +1984,6 @@ void __attribute((constructor)) kvm_selftest_init(void)
> >  {
> >       /* Tell stdout not to buffer its content. */
> >       setbuf(stdout, NULL);
> > +
> > +     kvm_selftest_arch_init();
> >  }
> > diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c
> > index 604478151212..26660dd2ba78 100644
> > --- a/tools/testing/selftests/kvm/lib/riscv/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c
> > @@ -362,3 +362,7 @@ void vcpu_args_set(struct kvm_vcpu *vcpu, unsigned int num, ...)
> >  void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
> >  {
> >  }
> > +
> > +void kvm_selftest_arch_init(void)
> > +{
> > +}
> > diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
> > index 89d7340d9cbd..8654ec74009a 100644
> > --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
> > @@ -218,3 +218,7 @@ void vcpu_arch_dump(FILE *stream, struct kvm_vcpu *vcpu, uint8_t indent)
> >  void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
> >  {
> >  }
> > +
> > +void kvm_selftest_arch_init(void)
> > +{
> > +}
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > index 2e6e61bbe81b..20bf125f9363 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > @@ -1311,3 +1311,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
> >
> >       return val == 'Y';
> >  }
> > +
> > +void kvm_selftest_arch_init(void)
> > +{
> > +}
> > --
> > 2.37.2.789.g6183377224-goog
> >
>
> Otherwise,
>
> Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Re: [V2 PATCH 2/8] KVM: selftests: Add arch specific initialization

[David Matlack]

On Thu, Sep 15, 2022 at 12:04:42AM +0000, Vishal Annapurve wrote:
> Introduce arch specific API: kvm_selftest_arch_init to allow each arch to
> handle initialization before running any selftest logic.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> ---
>  .../selftests/kvm/include/kvm_util_base.h      |  5 +++++
>  .../selftests/kvm/lib/aarch64/processor.c      | 18 +++++++++---------
>  tools/testing/selftests/kvm/lib/kvm_util.c     |  2 ++
>  .../selftests/kvm/lib/riscv/processor.c        |  4 ++++
>  .../selftests/kvm/lib/s390x/processor.c        |  4 ++++
>  .../selftests/kvm/lib/x86_64/processor.c       |  4 ++++
>  6 files changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 24fde97f6121..98edbbda9f97 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -834,4 +834,9 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm)
>  	return __vm_enable_cap(vm, KVM_CAP_VM_DISABLE_NX_HUGE_PAGES, 0);
>  }
>  
> +/*
> + * API to execute architecture specific setup before executing selftest logic.

nit: s/before executing selftest logic/before main()/

("selftest logic" is vague)

> + */
> +void kvm_selftest_arch_init(void);
> +
>  #endif /* SELFTEST_KVM_UTIL_BASE_H */
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index 6f5551368944..2281d6c5d02f 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -495,15 +495,6 @@ void aarch64_get_supported_page_sizes(uint32_t ipa,
>  	close(kvm_fd);
>  }
>  
> -/*
> - * arm64 doesn't have a true default mode, so start by computing the
> - * available IPA space and page sizes early.
> - */
> -void __attribute__((constructor)) init_guest_modes(void)
> -{
> -       guest_modes_append_default();
> -}
> -
>  void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
>  	       uint64_t arg2, uint64_t arg3, uint64_t arg4, uint64_t arg5,
>  	       uint64_t arg6, struct arm_smccc_res *res)
> @@ -528,3 +519,12 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
>  		       [arg4] "r"(arg4), [arg5] "r"(arg5), [arg6] "r"(arg6)
>  		     : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7");
>  }
> +
> +/*
> + * arm64 doesn't have a true default mode, so start by computing the
> + * available IPA space and page sizes early.
> + */
> +void kvm_selftest_arch_init(void)
> +{
> +	guest_modes_append_default();
> +}
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 3c83838999f5..dafe4471a6c7 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -1984,4 +1984,6 @@ void __attribute((constructor)) kvm_selftest_init(void)
>  {
>  	/* Tell stdout not to buffer its content. */
>  	setbuf(stdout, NULL);
> +
> +	kvm_selftest_arch_init();
>  }

Suggest defining a default no-op implementation of
kvm_selftest_arch_init() using __weak since most architectures do not
actually need an implementation.

> diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c
> index 604478151212..26660dd2ba78 100644
> --- a/tools/testing/selftests/kvm/lib/riscv/processor.c
> +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c
> @@ -362,3 +362,7 @@ void vcpu_args_set(struct kvm_vcpu *vcpu, unsigned int num, ...)
>  void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
>  {
>  }
> +
> +void kvm_selftest_arch_init(void)
> +{
> +}
> diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
> index 89d7340d9cbd..8654ec74009a 100644
> --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
> +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
> @@ -218,3 +218,7 @@ void vcpu_arch_dump(FILE *stream, struct kvm_vcpu *vcpu, uint8_t indent)
>  void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
>  {
>  }
> +
> +void kvm_selftest_arch_init(void)
> +{
> +}
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 2e6e61bbe81b..20bf125f9363 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1311,3 +1311,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
>  
>  	return val == 'Y';
>  }
> +
> +void kvm_selftest_arch_init(void)
> +{
> +}
> -- 
> 2.37.2.789.g6183377224-goog
> 

Re: [V2 PATCH 2/8] KVM: selftests: Add arch specific initialization

[Vishal Annapurve]

On Wed, Sep 21, 2022 at 1:51 PM David Matlack <dmatlack@google.com> wrote:
>
> On Thu, Sep 15, 2022 at 12:04:42AM +0000, Vishal Annapurve wrote:
> > Introduce arch specific API: kvm_selftest_arch_init to allow each arch to
> > handle initialization before running any selftest logic.
> >
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> > ---
> >  .../selftests/kvm/include/kvm_util_base.h      |  5 +++++
> >  .../selftests/kvm/lib/aarch64/processor.c      | 18 +++++++++---------
> >  tools/testing/selftests/kvm/lib/kvm_util.c     |  2 ++
> >  .../selftests/kvm/lib/riscv/processor.c        |  4 ++++
> >  .../selftests/kvm/lib/s390x/processor.c        |  4 ++++
> >  .../selftests/kvm/lib/x86_64/processor.c       |  4 ++++
> >  6 files changed, 28 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > index 24fde97f6121..98edbbda9f97 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > @@ -834,4 +834,9 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm)
> >       return __vm_enable_cap(vm, KVM_CAP_VM_DISABLE_NX_HUGE_PAGES, 0);
> >  }
> >
> > +/*
> > + * API to execute architecture specific setup before executing selftest logic.
>
> nit: s/before executing selftest logic/before main()/
>

Ack, will fix this in the next series.

> ("selftest logic" is vague)
>
> > + */
> > +void kvm_selftest_arch_init(void);
> > +
> >  #endif /* SELFTEST_KVM_UTIL_BASE_H */
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > index 6f5551368944..2281d6c5d02f 100644
> > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > @@ -495,15 +495,6 @@ void aarch64_get_supported_page_sizes(uint32_t ipa,
> >       close(kvm_fd);
> >  }
> >
> > -/*
> > - * arm64 doesn't have a true default mode, so start by computing the
> > - * available IPA space and page sizes early.
> > - */
> > -void __attribute__((constructor)) init_guest_modes(void)
> > -{
> > -       guest_modes_append_default();
> > -}
> > -
> >  void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
> >              uint64_t arg2, uint64_t arg3, uint64_t arg4, uint64_t arg5,
> >              uint64_t arg6, struct arm_smccc_res *res)
> > @@ -528,3 +519,12 @@ void smccc_hvc(uint32_t function_id, uint64_t arg0, uint64_t arg1,
> >                      [arg4] "r"(arg4), [arg5] "r"(arg5), [arg6] "r"(arg6)
> >                    : "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7");
> >  }
> > +
> > +/*
> > + * arm64 doesn't have a true default mode, so start by computing the
> > + * available IPA space and page sizes early.
> > + */
> > +void kvm_selftest_arch_init(void)
> > +{
> > +     guest_modes_append_default();
> > +}
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 3c83838999f5..dafe4471a6c7 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -1984,4 +1984,6 @@ void __attribute((constructor)) kvm_selftest_init(void)
> >  {
> >       /* Tell stdout not to buffer its content. */
> >       setbuf(stdout, NULL);
> > +
> > +     kvm_selftest_arch_init();
> >  }
>
> Suggest defining a default no-op implementation of
> kvm_selftest_arch_init() using __weak since most architectures do not
> actually need an implementation.
>

Ack, will update this in the next series.

> > ...
> >

[V2 PATCH 3/8] KVM: selftests: Add arch specific post vm load setup

[Vishal Annapurve]

Add arch specific API kvm_selftest_post_vm_elf_load to possibly communicate
information to VM that is already known to selftest VMM logic.

This API will be used in followup commit to convey cpu vendor type to the
guest vm.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vishal Annapurve <vannapurve@google.com>
---
 tools/testing/selftests/kvm/include/kvm_util_base.h | 4 ++++
 tools/testing/selftests/kvm/lib/aarch64/processor.c | 4 ++++
 tools/testing/selftests/kvm/lib/elf.c               | 2 ++
 tools/testing/selftests/kvm/lib/riscv/processor.c   | 4 ++++
 tools/testing/selftests/kvm/lib/s390x/processor.c   | 4 ++++
 tools/testing/selftests/kvm/lib/x86_64/processor.c  | 4 ++++
 6 files changed, 22 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 98edbbda9f97..73cfee3ebd76 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -839,4 +839,8 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm)
  */
 void kvm_selftest_arch_init(void);
 
+/*
+ * API to execute architecture specific setup after loading the vm elf.
+ */
+void kvm_arch_post_vm_elf_load(struct kvm_vm *vm);
 #endif /* SELFTEST_KVM_UTIL_BASE_H */
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index 2281d6c5d02f..12627c560f66 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -528,3 +528,7 @@ void kvm_selftest_arch_init(void)
 {
 	guest_modes_append_default();
 }
+
+void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
+{
+}
diff --git a/tools/testing/selftests/kvm/lib/elf.c b/tools/testing/selftests/kvm/lib/elf.c
index 9f54c098d9d0..b8963a7146ce 100644
--- a/tools/testing/selftests/kvm/lib/elf.c
+++ b/tools/testing/selftests/kvm/lib/elf.c
@@ -189,4 +189,6 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
 				phdr.p_filesz);
 		}
 	}
+
+	kvm_arch_post_vm_elf_load(vm);
 }
diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c
index 26660dd2ba78..4491c0d4be45 100644
--- a/tools/testing/selftests/kvm/lib/riscv/processor.c
+++ b/tools/testing/selftests/kvm/lib/riscv/processor.c
@@ -366,3 +366,7 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
 void kvm_selftest_arch_init(void)
 {
 }
+
+void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
+{
+}
diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
index 8654ec74009a..332501b3693f 100644
--- a/tools/testing/selftests/kvm/lib/s390x/processor.c
+++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
@@ -222,3 +222,7 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
 void kvm_selftest_arch_init(void)
 {
 }
+
+void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
+{
+}
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 20bf125f9363..25ae972f5c71 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1315,3 +1315,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
 void kvm_selftest_arch_init(void)
 {
 }
+
+void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
+{
+}
-- 
2.37.2.789.g6183377224-goog

Re: [V2 PATCH 3/8] KVM: selftests: Add arch specific post vm load setup

[David Matlack]

On Thu, Sep 15, 2022 at 12:04:43AM +0000, Vishal Annapurve wrote:
> Add arch specific API kvm_selftest_post_vm_elf_load to possibly communicate
> information to VM that is already known to selftest VMM logic.
> 
> This API will be used in followup commit to convey cpu vendor type to the
> guest vm.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> ---
>  tools/testing/selftests/kvm/include/kvm_util_base.h | 4 ++++
>  tools/testing/selftests/kvm/lib/aarch64/processor.c | 4 ++++
>  tools/testing/selftests/kvm/lib/elf.c               | 2 ++
>  tools/testing/selftests/kvm/lib/riscv/processor.c   | 4 ++++
>  tools/testing/selftests/kvm/lib/s390x/processor.c   | 4 ++++
>  tools/testing/selftests/kvm/lib/x86_64/processor.c  | 4 ++++
>  6 files changed, 22 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 98edbbda9f97..73cfee3ebd76 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -839,4 +839,8 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm)
>   */
>  void kvm_selftest_arch_init(void);
>  
> +/*
> + * API to execute architecture specific setup after loading the vm elf.

It's not a "vm elf" per-se, it's "loading the elf into the VM". How
about:

/*
 * API to execute arch-specific logic after loading the selftest ELF image
 * into the VM.
 */

> + */
> +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm);
>  #endif /* SELFTEST_KVM_UTIL_BASE_H */
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index 2281d6c5d02f..12627c560f66 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -528,3 +528,7 @@ void kvm_selftest_arch_init(void)
>  {
>  	guest_modes_append_default();
>  }
> +
> +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
> +{
> +}
> diff --git a/tools/testing/selftests/kvm/lib/elf.c b/tools/testing/selftests/kvm/lib/elf.c
> index 9f54c098d9d0..b8963a7146ce 100644
> --- a/tools/testing/selftests/kvm/lib/elf.c
> +++ b/tools/testing/selftests/kvm/lib/elf.c
> @@ -189,4 +189,6 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
>  				phdr.p_filesz);
>  		}
>  	}
> +
> +	kvm_arch_post_vm_elf_load(vm);
>  }

Same suggestion here as the previous patch: Use __weak to define a
default no-op implementation of kvm_arch_post_vm_elf_load().

> diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c
> index 26660dd2ba78..4491c0d4be45 100644
> --- a/tools/testing/selftests/kvm/lib/riscv/processor.c
> +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c
> @@ -366,3 +366,7 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
>  void kvm_selftest_arch_init(void)
>  {
>  }
> +
> +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
> +{
> +}
> diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
> index 8654ec74009a..332501b3693f 100644
> --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
> +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
> @@ -222,3 +222,7 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
>  void kvm_selftest_arch_init(void)
>  {
>  }
> +
> +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
> +{
> +}
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 20bf125f9363..25ae972f5c71 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1315,3 +1315,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
>  void kvm_selftest_arch_init(void)
>  {
>  }
> +
> +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
> +{
> +}
> -- 
> 2.37.2.789.g6183377224-goog
> 

Re: [V2 PATCH 3/8] KVM: selftests: Add arch specific post vm load setup

[Vishal Annapurve]

On Wed, Sep 21, 2022 at 1:54 PM David Matlack <dmatlack@google.com> wrote:
>
> On Thu, Sep 15, 2022 at 12:04:43AM +0000, Vishal Annapurve wrote:
> > Add arch specific API kvm_selftest_post_vm_elf_load to possibly communicate
> > information to VM that is already known to selftest VMM logic.
> >
> > This API will be used in followup commit to convey cpu vendor type to the
> > guest vm.
> >
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> > ---
> >  tools/testing/selftests/kvm/include/kvm_util_base.h | 4 ++++
> >  tools/testing/selftests/kvm/lib/aarch64/processor.c | 4 ++++
> >  tools/testing/selftests/kvm/lib/elf.c               | 2 ++
> >  tools/testing/selftests/kvm/lib/riscv/processor.c   | 4 ++++
> >  tools/testing/selftests/kvm/lib/s390x/processor.c   | 4 ++++
> >  tools/testing/selftests/kvm/lib/x86_64/processor.c  | 4 ++++
> >  6 files changed, 22 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > index 98edbbda9f97..73cfee3ebd76 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > @@ -839,4 +839,8 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm)
> >   */
> >  void kvm_selftest_arch_init(void);
> >
> > +/*
> > + * API to execute architecture specific setup after loading the vm elf.
>
> It's not a "vm elf" per-se, it's "loading the elf into the VM". How
> about:
>
> /*
>  * API to execute arch-specific logic after loading the selftest ELF image
>  * into the VM.
>  */
>

Ack. Will update this in the next series.

> > + */
> > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm);
> >  #endif /* SELFTEST_KVM_UTIL_BASE_H */
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > index 2281d6c5d02f..12627c560f66 100644
> > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > @@ -528,3 +528,7 @@ void kvm_selftest_arch_init(void)
> >  {
> >       guest_modes_append_default();
> >  }
> > +
> > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
> > +{
> > +}
> > diff --git a/tools/testing/selftests/kvm/lib/elf.c b/tools/testing/selftests/kvm/lib/elf.c
> > index 9f54c098d9d0..b8963a7146ce 100644
> > --- a/tools/testing/selftests/kvm/lib/elf.c
> > +++ b/tools/testing/selftests/kvm/lib/elf.c
> > @@ -189,4 +189,6 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
> >                               phdr.p_filesz);
> >               }
> >       }
> > +
> > +     kvm_arch_post_vm_elf_load(vm);
> >  }
>
> Same suggestion here as the previous patch: Use __weak to define a
> default no-op implementation of kvm_arch_post_vm_elf_load().
>
> > diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c
> > index 26660dd2ba78..4491c0d4be45 100644
> > --- a/tools/testing/selftests/kvm/lib/riscv/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c
> > @@ -366,3 +366,7 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
> >  void kvm_selftest_arch_init(void)
> >  {
> >  }
> > +
> > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
> > +{
> > +}
> > diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
> > index 8654ec74009a..332501b3693f 100644
> > --- a/tools/testing/selftests/kvm/lib/s390x/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
> > @@ -222,3 +222,7 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
> >  void kvm_selftest_arch_init(void)
> >  {
> >  }
> > +
> > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
> > +{
> > +}
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > index 20bf125f9363..25ae972f5c71 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > @@ -1315,3 +1315,7 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
> >  void kvm_selftest_arch_init(void)
> >  {
> >  }
> > +
> > +void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
> > +{
> > +}
> > --
> > 2.37.2.789.g6183377224-goog
> >

Re: [V2 PATCH 3/8] KVM: selftests: Add arch specific post vm load setup

[Sean Christopherson]

On Mon, Sep 26, 2022, Vishal Annapurve wrote:
> On Wed, Sep 21, 2022 at 1:54 PM David Matlack <dmatlack@google.com> wrote:
> >
> > On Thu, Sep 15, 2022 at 12:04:43AM +0000, Vishal Annapurve wrote:
> > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > index 98edbbda9f97..73cfee3ebd76 100644
> > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > @@ -839,4 +839,8 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm)
> > >   */
> > >  void kvm_selftest_arch_init(void);
> > >
> > > +/*
> > > + * API to execute architecture specific setup after loading the vm elf.
> >
> > It's not a "vm elf" per-se, it's "loading the elf into the VM". How
> > about:
> >
> > /*
> >  * API to execute arch-specific logic after loading the selftest ELF image
> >  * into the VM.
> >  */
> >
> 
> Ack. Will update this in the next series.

Even better, call it from __vm_create() and name it something like
kvm_arch_vm_post_create().  Like David said, while the hook has a dependency on
being called after loading the ELF image, the action that arch code is expected
to take doesn't have anything to do with loading the ELF image.

And then instead of introducing an arch hook with no implementation, the patch that
adds the hook can instead use it to replace the x86-64 #ifdef in __vm_create().

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index dafe4471a6c7..593dfadb662e 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -298,9 +298,8 @@ struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint32_t nr_runnable_vcpus,
 
        kvm_vm_elf_load(vm, program_invocation_name);
 
-#ifdef __x86_64__
-       vm_create_irqchip(vm);
-#endif
+       kvm_arch_vm_post_create(vm);
+
        return vm;
 }
 
 

Re: [V2 PATCH 3/8] KVM: selftests: Add arch specific post vm load setup

[Vishal Annapurve]

On Mon, Oct 3, 2022 at 8:42 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Sep 26, 2022, Vishal Annapurve wrote:
> > On Wed, Sep 21, 2022 at 1:54 PM David Matlack <dmatlack@google.com> wrote:
> > >
> > > On Thu, Sep 15, 2022 at 12:04:43AM +0000, Vishal Annapurve wrote:
> > > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > > index 98edbbda9f97..73cfee3ebd76 100644
> > > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > > @@ -839,4 +839,8 @@ static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm)
> > > >   */
> > > >  void kvm_selftest_arch_init(void);
> > > >
> > > > +/*
> > > > + * API to execute architecture specific setup after loading the vm elf.
> > >
> > > It's not a "vm elf" per-se, it's "loading the elf into the VM". How
> > > about:
> > >
> > > /*
> > >  * API to execute arch-specific logic after loading the selftest ELF image
> > >  * into the VM.
> > >  */
> > >
> >
> > Ack. Will update this in the next series.
>
> Even better, call it from __vm_create() and name it something like
> kvm_arch_vm_post_create().  Like David said, while the hook has a dependency on
> being called after loading the ELF image, the action that arch code is expected
> to take doesn't have anything to do with loading the ELF image.
>
> And then instead of introducing an arch hook with no implementation, the patch that
> adds the hook can instead use it to replace the x86-64 #ifdef in __vm_create().
>

Today upstream kernel selftests don't have scenarios where
kvm_vm_elf_load can get called directly outside ___vm_create but there
are selftests that are up for review [1], [2] that may call
kvm_vm_elf_load directly. Above suggestion will not work in this
scenario, is it suitable to assume that all the callers of
kvm_vm_elf_load will eventually execute kvm_arch_vm_post_create?

[1] https://lore.kernel.org/lkml/20220810152033.946942-12-pgonda@google.com/
[2] https://lore.kernel.org/lkml/20220819174659.2427983-1-vannapurve@google.com/T/#u

> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index dafe4471a6c7..593dfadb662e 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -298,9 +298,8 @@ struct kvm_vm *__vm_create(enum vm_guest_mode mode, uint32_t nr_runnable_vcpus,
>
>         kvm_vm_elf_load(vm, program_invocation_name);
>
> -#ifdef __x86_64__
> -       vm_create_irqchip(vm);
> -#endif
> +       kvm_arch_vm_post_create(vm);
> +
>         return vm;
>  }
>
>

Re: [V2 PATCH 3/8] KVM: selftests: Add arch specific post vm load setup

[Sean Christopherson]

On Mon, Oct 03, 2022, Vishal Annapurve wrote:
> On Mon, Oct 3, 2022 at 8:42 AM Sean Christopherson <seanjc@google.com> wrote:
> > Even better, call it from __vm_create() and name it something like
> > kvm_arch_vm_post_create().  Like David said, while the hook has a dependency on
> > being called after loading the ELF image, the action that arch code is expected
> > to take doesn't have anything to do with loading the ELF image.
> >
> > And then instead of introducing an arch hook with no implementation, the patch that
> > adds the hook can instead use it to replace the x86-64 #ifdef in __vm_create().
> >
> 
> Today upstream kernel selftests don't have scenarios where
> kvm_vm_elf_load can get called directly outside ___vm_create but there
> are selftests that are up for review [1], [2] that may call
> kvm_vm_elf_load directly. Above suggestion will not work in this
> scenario, is it suitable to assume that all the callers of
> kvm_vm_elf_load will eventually execute kvm_arch_vm_post_create?

No, but that's irrelevant.  And actually, in any reasonable hypothetical situation
I can think of, it's actually undesirable to always call kvm_arch_vm_post_create()
after kvm_vm_elf_load().

Hypothetically, if there were a use case where kvm_vm_elf_load() is called multiple
times, then stuffing the "Intel vs. "AMD" flag should only be done for the binary
that actually defines that flag.  The flag is defined by the library's processor.c,
and so the hook should be tied to the library's loading of its binary, i.e. to the
creation of the VM.

If a test were loading multiple binaries, and the test wanted to tweak things
specific to a binary after loading said binary, then the test can and should do
that without needed a library arch hook.

If the arch hook was to take action specific to loading _any_ binary, than yes, a
hook in kvm_vm_elf_load() would be in order, but this hook is about taking action
when creating a VM, not to loading a binary.

But this is all very, very hypothetical.  I can't think of a scenario where
loading multiple binaries would be less complex than solving whatever hypothetical
problem makes it difficult to link everything into a single binary.

And if a test manually loads a binary _and_ wants to actually run the guest after
doing vm_create_barebones() or ____vm_create(), then the test is doing it wrong,
as those low level APIs are provided _only_ for cases where a test doesn't need
to run vCPUs.

Re: [V2 PATCH 3/8] KVM: selftests: Add arch specific post vm load setup

[Vishal Annapurve]

On Wed, Oct 5, 2022 at 1:40 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Oct 03, 2022, Vishal Annapurve wrote:
> > On Mon, Oct 3, 2022 at 8:42 AM Sean Christopherson <seanjc@google.com> wrote:
> > > Even better, call it from __vm_create() and name it something like
> > > kvm_arch_vm_post_create().  Like David said, while the hook has a dependency on
> > > being called after loading the ELF image, the action that arch code is expected
> > > to take doesn't have anything to do with loading the ELF image.
> > >
> > > And then instead of introducing an arch hook with no implementation, the patch that
> > > adds the hook can instead use it to replace the x86-64 #ifdef in __vm_create().
> > >
> >
> > Today upstream kernel selftests don't have scenarios where
> > kvm_vm_elf_load can get called directly outside ___vm_create but there
> > are selftests that are up for review [1], [2] that may call
> > kvm_vm_elf_load directly. Above suggestion will not work in this
> > scenario, is it suitable to assume that all the callers of
> > kvm_vm_elf_load will eventually execute kvm_arch_vm_post_create?
>
> No, but that's irrelevant.  And actually, in any reasonable hypothetical situation
> I can think of, it's actually undesirable to always call kvm_arch_vm_post_create()
> after kvm_vm_elf_load().
>
> Hypothetically, if there were a use case where kvm_vm_elf_load() is called multiple
> times, then stuffing the "Intel vs. "AMD" flag should only be done for the binary
> that actually defines that flag.  The flag is defined by the library's processor.c,
> and so the hook should be tied to the library's loading of its binary, i.e. to the
> creation of the VM.
>
> If a test were loading multiple binaries, and the test wanted to tweak things
> specific to a binary after loading said binary, then the test can and should do
> that without needed a library arch hook.
>
> If the arch hook was to take action specific to loading _any_ binary, than yes, a
> hook in kvm_vm_elf_load() would be in order, but this hook is about taking action
> when creating a VM, not to loading a binary.
>
> But this is all very, very hypothetical.  I can't think of a scenario where
> loading multiple binaries would be less complex than solving whatever hypothetical
> problem makes it difficult to link everything into a single binary.
>
> And if a test manually loads a binary _and_ wants to actually run the guest after
> doing vm_create_barebones() or ____vm_create(), then the test is doing it wrong,
> as those low level APIs are provided _only_ for cases where a test doesn't need
> to run vCPUs.

Ack, will update this patch as per the feedback here.

[V2 PATCH 4/8] KVM: selftests: x86: Precompute the result for is_{intel,amd}_cpu()

[Vishal Annapurve]

Cache the vendor CPU type in a global variable so that multiple calls
to is_intel_cpu() do not need to re-execute CPUID.

Add cpu vendor check in kvm_hypercall() so that it executes correct
vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids
exit to KVM which anyway tries to patch the instruction according to
the cpu type.

As part of this change, sync the global variable is_cpu_amd into the
guest so the guest can determine which hypercall instruction to use
without needing to re-execute CPUID for every hypercall.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vishal Annapurve <vannapurve@google.com>
---
 .../testing/selftests/kvm/lib/x86_64/processor.c  | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 25ae972f5c71..c0ae938772f6 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -19,6 +19,7 @@
 #define MAX_NR_CPUID_ENTRIES 100
 
 vm_vaddr_t exception_handlers;
+static bool is_cpu_amd;
 
 static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent)
 {
@@ -1019,7 +1020,7 @@ static bool cpu_vendor_string_is(const char *vendor)
 
 bool is_intel_cpu(void)
 {
-	return cpu_vendor_string_is("GenuineIntel");
+	return (is_cpu_amd == false);
 }
 
 /*
@@ -1027,7 +1028,7 @@ bool is_intel_cpu(void)
  */
 bool is_amd_cpu(void)
 {
-	return cpu_vendor_string_is("AuthenticAMD");
+	return (is_cpu_amd == true);
 }
 
 void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits)
@@ -1182,9 +1183,15 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
 {
 	uint64_t r;
 
-	asm volatile("vmcall"
+	if (is_amd_cpu())
+		asm volatile("vmmcall"
 		     : "=a"(r)
 		     : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3));
+	else
+		asm volatile("vmcall"
+		     : "=a"(r)
+		     : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3));
+
 	return r;
 }
 
@@ -1314,8 +1321,10 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
 
 void kvm_selftest_arch_init(void)
 {
+	is_cpu_amd = cpu_vendor_string_is("AuthenticAMD");
 }
 
 void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
 {
+	sync_global_to_guest(vm, is_cpu_amd);
 }
-- 
2.37.2.789.g6183377224-goog

Re: [V2 PATCH 4/8] KVM: selftests: x86: Precompute the result for is_{intel,amd}_cpu()

[David Matlack]

On Thu, Sep 15, 2022 at 12:04:44AM +0000, Vishal Annapurve wrote:
> Cache the vendor CPU type in a global variable so that multiple calls
> to is_intel_cpu() do not need to re-execute CPUID.
> 
> Add cpu vendor check in kvm_hypercall() so that it executes correct
> vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids
> exit to KVM which anyway tries to patch the instruction according to
> the cpu type.

The commit shortlog makes no mention (nor even implies) that this commit
adds AMD support to kvm_hypercall(). Please break this commit up into 2.
One to precompute the result of is_{intel,amd}_cpu() and one to add AMD
support to kvm_hypercall().

If you really want to keep this as one commit (I don't know what the
benefit would be), please change the shortlog and commit message to
focus on the kvm_hypercall() change, as that is the real goal of this
commit. The precomputation is arguably and implementation detail. e.g.

  KVM: selftest: Add support for AMD to kvm_hypercall()

  Make it possible to use kvm_hypercall() on AMD by checking if running
  on an AMD CPU and, if so, using vmmcall instead of vmcall. In order to
  avoid executing CPUID in the guest on every call t kvm_hypercall()
  (which would be slow), pre-compute the result of is_{intel,amd}_cpu()
  as part of kvm_selftest_arch_init() and sync it into the guest
  after loading the ELF image.

But again, it'd be cleaner just to split it up. Caching the result of
is_{intel,amd}_cpu() is useful in its own right, independent of the
kvm_hypercall() change.

> 
> As part of this change, sync the global variable is_cpu_amd into the
> guest so the guest can determine which hypercall instruction to use
> without needing to re-execute CPUID for every hypercall.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> ---
>  .../testing/selftests/kvm/lib/x86_64/processor.c  | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 25ae972f5c71..c0ae938772f6 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -19,6 +19,7 @@
>  #define MAX_NR_CPUID_ENTRIES 100
>  
>  vm_vaddr_t exception_handlers;
> +static bool is_cpu_amd;
>  
>  static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent)
>  {
> @@ -1019,7 +1020,7 @@ static bool cpu_vendor_string_is(const char *vendor)
>  
>  bool is_intel_cpu(void)
>  {
> -	return cpu_vendor_string_is("GenuineIntel");
> +	return (is_cpu_amd == false);
>  }
>  
>  /*
> @@ -1027,7 +1028,7 @@ bool is_intel_cpu(void)
>   */
>  bool is_amd_cpu(void)
>  {
> -	return cpu_vendor_string_is("AuthenticAMD");
> +	return (is_cpu_amd == true);
>  }
>  
>  void kvm_get_cpu_address_width(unsigned int *pa_bits, unsigned int *va_bits)
> @@ -1182,9 +1183,15 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
>  {
>  	uint64_t r;
>  
> -	asm volatile("vmcall"
> +	if (is_amd_cpu())
> +		asm volatile("vmmcall"
>  		     : "=a"(r)
>  		     : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3));
> +	else
> +		asm volatile("vmcall"
> +		     : "=a"(r)
> +		     : "a"(nr), "b"(a0), "c"(a1), "d"(a2), "S"(a3));
> +
>  	return r;
>  }
>  
> @@ -1314,8 +1321,10 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
>  
>  void kvm_selftest_arch_init(void)
>  {
> +	is_cpu_amd = cpu_vendor_string_is("AuthenticAMD");
>  }
>  
>  void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
>  {
> +	sync_global_to_guest(vm, is_cpu_amd);
>  }
> -- 
> 2.37.2.789.g6183377224-goog
> 

Re: [V2 PATCH 4/8] KVM: selftests: x86: Precompute the result for is_{intel,amd}_cpu()

[Vishal Annapurve]

On Wed, Sep 21, 2022 at 2:19 PM David Matlack <dmatlack@google.com> wrote:
>
> On Thu, Sep 15, 2022 at 12:04:44AM +0000, Vishal Annapurve wrote:
> > Cache the vendor CPU type in a global variable so that multiple calls
> > to is_intel_cpu() do not need to re-execute CPUID.
> >
> > Add cpu vendor check in kvm_hypercall() so that it executes correct
> > vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids
> > exit to KVM which anyway tries to patch the instruction according to
> > the cpu type.
>
> The commit shortlog makes no mention (nor even implies) that this commit
> adds AMD support to kvm_hypercall(). Please break this commit up into 2.
> One to precompute the result of is_{intel,amd}_cpu() and one to add AMD
> support to kvm_hypercall().
>
> If you really want to keep this as one commit (I don't know what the
> benefit would be), please change the shortlog and commit message to
> focus on the kvm_hypercall() change, as that is the real goal of this
> commit. The precomputation is arguably and implementation detail. e.g.
>

is_amd_cpu is used by guest code within fix_hypercall_test.c, just
caching the result will break the guest code execution. I have clubbed
these two changes together in order to ensure that is_amd_cpu works
fine for both host userspace and guest vm logic.

>   KVM: selftest: Add support for AMD to kvm_hypercall()
>
>   Make it possible to use kvm_hypercall() on AMD by checking if running
>   on an AMD CPU and, if so, using vmmcall instead of vmcall. In order to
>   avoid executing CPUID in the guest on every call t kvm_hypercall()
>   (which would be slow), pre-compute the result of is_{intel,amd}_cpu()
>   as part of kvm_selftest_arch_init() and sync it into the guest
>   after loading the ELF image.
>
> But again, it'd be cleaner just to split it up. Caching the result of
> is_{intel,amd}_cpu() is useful in its own right, independent of the
> kvm_hypercall() change.
>
> >
> > ...
> >
> > @@ -1314,8 +1321,10 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
> >
> >  void kvm_selftest_arch_init(void)
> >  {
> > +     is_cpu_amd = cpu_vendor_string_is("AuthenticAMD");
> >  }
> >
> >  void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
> >  {
> > +     sync_global_to_guest(vm, is_cpu_amd);
> >  }
> > --
> > 2.37.2.789.g6183377224-goog
> >

Re: [V2 PATCH 4/8] KVM: selftests: x86: Precompute the result for is_{intel,amd}_cpu()

[David Matlack]

On Mon, Sep 26, 2022 at 4:27 PM Vishal Annapurve <vannapurve@google.com> wrote:
>
> On Wed, Sep 21, 2022 at 2:19 PM David Matlack <dmatlack@google.com> wrote:
> >
> > On Thu, Sep 15, 2022 at 12:04:44AM +0000, Vishal Annapurve wrote:
> > > Cache the vendor CPU type in a global variable so that multiple calls
> > > to is_intel_cpu() do not need to re-execute CPUID.
> > >
> > > Add cpu vendor check in kvm_hypercall() so that it executes correct
> > > vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids
> > > exit to KVM which anyway tries to patch the instruction according to
> > > the cpu type.
> >
> > The commit shortlog makes no mention (nor even implies) that this commit
> > adds AMD support to kvm_hypercall(). Please break this commit up into 2.
> > One to precompute the result of is_{intel,amd}_cpu() and one to add AMD
> > support to kvm_hypercall().
> >
> > If you really want to keep this as one commit (I don't know what the
> > benefit would be), please change the shortlog and commit message to
> > focus on the kvm_hypercall() change, as that is the real goal of this
> > commit. The precomputation is arguably and implementation detail. e.g.
> >
>
> is_amd_cpu is used by guest code within fix_hypercall_test.c, just
> caching the result will break the guest code execution. I have clubbed
> these two changes together in order to ensure that is_amd_cpu works
> fine for both host userspace and guest vm logic.

Ah, so the sync_global_to_guest() part needs to go in the patch that
adds caching to is_amd_cpu().

But the point still stands that adding AMD support to kvm_hypercall()
is a logically independent change.

>
> >   KVM: selftest: Add support for AMD to kvm_hypercall()
> >
> >   Make it possible to use kvm_hypercall() on AMD by checking if running
> >   on an AMD CPU and, if so, using vmmcall instead of vmcall. In order to
> >   avoid executing CPUID in the guest on every call t kvm_hypercall()
> >   (which would be slow), pre-compute the result of is_{intel,amd}_cpu()
> >   as part of kvm_selftest_arch_init() and sync it into the guest
> >   after loading the ELF image.
> >
> > But again, it'd be cleaner just to split it up. Caching the result of
> > is_{intel,amd}_cpu() is useful in its own right, independent of the
> > kvm_hypercall() change.
> >
> > >
> > > ...
> > >
> > > @@ -1314,8 +1321,10 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
> > >
> > >  void kvm_selftest_arch_init(void)
> > >  {
> > > +     is_cpu_amd = cpu_vendor_string_is("AuthenticAMD");
> > >  }
> > >
> > >  void kvm_arch_post_vm_elf_load(struct kvm_vm *vm)
> > >  {
> > > +     sync_global_to_guest(vm, is_cpu_amd);
> > >  }
> > > --
> > > 2.37.2.789.g6183377224-goog
> > >

Re: [V2 PATCH 4/8] KVM: selftests: x86: Precompute the result for is_{intel,amd}_cpu()

[Vishal Annapurve]

On Mon, Sep 26, 2022 at 4:34 PM David Matlack <dmatlack@google.com> wrote:
>
> On Mon, Sep 26, 2022 at 4:27 PM Vishal Annapurve <vannapurve@google.com> wrote:
> >
> > On Wed, Sep 21, 2022 at 2:19 PM David Matlack <dmatlack@google.com> wrote:
> > > ...
> >
> > is_amd_cpu is used by guest code within fix_hypercall_test.c, just
> > caching the result will break the guest code execution. I have clubbed
> > these two changes together in order to ensure that is_amd_cpu works
> > fine for both host userspace and guest vm logic.
>
> Ah, so the sync_global_to_guest() part needs to go in the patch that
> adds caching to is_amd_cpu().
>
> But the point still stands that adding AMD support to kvm_hypercall()
> is a logically independent change.
>

I see what you mean. Will split this change into two in the next series.

> > ...
> > > > --
> > > > 2.37.2.789.g6183377224-goog
> > > >

Re: [V2 PATCH 4/8] KVM: selftests: x86: Precompute the result for is_{intel,amd}_cpu()

[David Matlack]

On Thu, Sep 15, 2022 at 12:04:44AM +0000, Vishal Annapurve wrote:
> Cache the vendor CPU type in a global variable so that multiple calls
> to is_intel_cpu() do not need to re-execute CPUID.
> 
> Add cpu vendor check in kvm_hypercall() so that it executes correct
> vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids
> exit to KVM which anyway tries to patch the instruction according to
> the cpu type.

Out of curiousity, why do we want to avoid this exit?

Re: [V2 PATCH 4/8] KVM: selftests: x86: Precompute the result for is_{intel,amd}_cpu()

[Vishal Annapurve]

On Wed, Sep 21, 2022 at 2:39 PM David Matlack <dmatlack@google.com> wrote:
>
> On Thu, Sep 15, 2022 at 12:04:44AM +0000, Vishal Annapurve wrote:
> > Cache the vendor CPU type in a global variable so that multiple calls
> > to is_intel_cpu() do not need to re-execute CPUID.
> >
> > Add cpu vendor check in kvm_hypercall() so that it executes correct
> > vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids
> > exit to KVM which anyway tries to patch the instruction according to
> > the cpu type.
>
> Out of curiousity, why do we want to avoid this exit?

Referring to the patch set posted for UPM selftests with
non-confidential VMs [1], vmcall patching will not work for selftests
executed with UPM feature enabled since guest memory can not be
modified by KVM. So I tried to add a kvm_hypercall implementation that
will execute the hypercall according to the cpu type.

Hypercall updates in this series are done to ensure that such a change
is done for all callers to allow consistency and avoid relying on KVM
behavior to patch the vmmcall/vmcall instruction.

[1] https://lore.kernel.org/lkml/20220819174659.2427983-5-vannapurve@google.com/

Re: [V2 PATCH 4/8] KVM: selftests: x86: Precompute the result for is_{intel,amd}_cpu()

[David Matlack]

On Mon, Sep 26, 2022 at 4:48 PM Vishal Annapurve <vannapurve@google.com> wrote:
>
> On Wed, Sep 21, 2022 at 2:39 PM David Matlack <dmatlack@google.com> wrote:
> >
> > On Thu, Sep 15, 2022 at 12:04:44AM +0000, Vishal Annapurve wrote:
> > > Cache the vendor CPU type in a global variable so that multiple calls
> > > to is_intel_cpu() do not need to re-execute CPUID.
> > >
> > > Add cpu vendor check in kvm_hypercall() so that it executes correct
> > > vmcall/vmmcall instruction when running on Intel/AMD hosts. This avoids
> > > exit to KVM which anyway tries to patch the instruction according to
> > > the cpu type.
> >
> > Out of curiousity, why do we want to avoid this exit?
>
> Referring to the patch set posted for UPM selftests with
> non-confidential VMs [1], vmcall patching will not work for selftests
> executed with UPM feature enabled since guest memory can not be
> modified by KVM. So I tried to add a kvm_hypercall implementation that
> will execute the hypercall according to the cpu type.
>
> Hypercall updates in this series are done to ensure that such a change
> is done for all callers to allow consistency and avoid relying on KVM
> behavior to patch the vmmcall/vmcall instruction.
>
> [1] https://lore.kernel.org/lkml/20220819174659.2427983-5-vannapurve@google.com/

Thanks! That makes a ton of sense. Please include that in the cover
letter and any of the commit messages that avoid hypercall patching.
That will help reviewers understand the context of why the changes are
being made, and help anyone reviewing the git history in the future
understand the same.

[V2 PATCH 5/8] KVM: selftests: x86: delete svm_vmcall_test

[Vishal Annapurve]

svm_vmcall_test is superseded by fix_hypercall_test.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vishal Annapurve <vannapurve@google.com>
---
 tools/testing/selftests/kvm/.gitignore        |  1 -
 .../selftests/kvm/x86_64/svm_vmcall_test.c    | 74 -------------------
 2 files changed, 75 deletions(-)
 delete mode 100644 tools/testing/selftests/kvm/x86_64/svm_vmcall_test.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index d625a3f83780..22e9a5b5488c 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -36,7 +36,6 @@
 /x86_64/sev_migrate_tests
 /x86_64/smm_test
 /x86_64/state_test
-/x86_64/svm_vmcall_test
 /x86_64/svm_int_ctl_test
 /x86_64/svm_nested_soft_inject_test
 /x86_64/sync_regs_test
diff --git a/tools/testing/selftests/kvm/x86_64/svm_vmcall_test.c b/tools/testing/selftests/kvm/x86_64/svm_vmcall_test.c
deleted file mode 100644
index c3ac45df7483..000000000000
--- a/tools/testing/selftests/kvm/x86_64/svm_vmcall_test.c
+++ /dev/null
@@ -1,74 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * svm_vmcall_test
- *
- * Copyright (C) 2020, Red Hat, Inc.
- *
- * Nested SVM testing: VMCALL
- */
-
-#include "test_util.h"
-#include "kvm_util.h"
-#include "processor.h"
-#include "svm_util.h"
-
-static void l2_guest_code(struct svm_test_data *svm)
-{
-	__asm__ __volatile__("vmcall");
-}
-
-static void l1_guest_code(struct svm_test_data *svm)
-{
-	#define L2_GUEST_STACK_SIZE 64
-	unsigned long l2_guest_stack[L2_GUEST_STACK_SIZE];
-	struct vmcb *vmcb = svm->vmcb;
-
-	/* Prepare for L2 execution. */
-	generic_svm_setup(svm, l2_guest_code,
-			  &l2_guest_stack[L2_GUEST_STACK_SIZE]);
-
-	run_guest(vmcb, svm->vmcb_gpa);
-
-	GUEST_ASSERT(vmcb->control.exit_code == SVM_EXIT_VMMCALL);
-	GUEST_DONE();
-}
-
-int main(int argc, char *argv[])
-{
-	struct kvm_vcpu *vcpu;
-	vm_vaddr_t svm_gva;
-	struct kvm_vm *vm;
-
-	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SVM));
-
-	vm = vm_create_with_one_vcpu(&vcpu, l1_guest_code);
-
-	vcpu_alloc_svm(vm, &svm_gva);
-	vcpu_args_set(vcpu, 1, svm_gva);
-
-	for (;;) {
-		volatile struct kvm_run *run = vcpu->run;
-		struct ucall uc;
-
-		vcpu_run(vcpu);
-		TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
-			    "Got exit_reason other than KVM_EXIT_IO: %u (%s)\n",
-			    run->exit_reason,
-			    exit_reason_str(run->exit_reason));
-
-		switch (get_ucall(vcpu, &uc)) {
-		case UCALL_ABORT:
-			REPORT_GUEST_ASSERT(uc);
-			/* NOT REACHED */
-		case UCALL_SYNC:
-			break;
-		case UCALL_DONE:
-			goto done;
-		default:
-			TEST_FAIL("Unknown ucall 0x%lx.", uc.cmd);
-		}
-	}
-done:
-	kvm_vm_free(vm);
-	return 0;
-}
-- 
2.37.2.789.g6183377224-goog

Re: [V2 PATCH 5/8] KVM: selftests: x86: delete svm_vmcall_test

[David Matlack]

On Thu, Sep 15, 2022 at 12:04:45AM +0000, Vishal Annapurve wrote:
> svm_vmcall_test is superseded by fix_hypercall_test.

Please provide a more detailed description of why svm_vmcall_test is
being dropped. e.g. What do you mean by "superseded" specifically?

I ask because this will be helpful context for future readers of this
commit. I also ask because it's not clear to me that fix_hypercall_test
is a 1:1 replacement of svm_vmcall_test. e.g. svm_vmcall_test sets up an
L2 while fix_hypercall_test just executes in L1.

> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> ---
>  tools/testing/selftests/kvm/.gitignore        |  1 -
>  .../selftests/kvm/x86_64/svm_vmcall_test.c    | 74 -------------------
>  2 files changed, 75 deletions(-)
>  delete mode 100644 tools/testing/selftests/kvm/x86_64/svm_vmcall_test.c
> 
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index d625a3f83780..22e9a5b5488c 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -36,7 +36,6 @@
>  /x86_64/sev_migrate_tests
>  /x86_64/smm_test
>  /x86_64/state_test
> -/x86_64/svm_vmcall_test
>  /x86_64/svm_int_ctl_test
>  /x86_64/svm_nested_soft_inject_test
>  /x86_64/sync_regs_test
> diff --git a/tools/testing/selftests/kvm/x86_64/svm_vmcall_test.c b/tools/testing/selftests/kvm/x86_64/svm_vmcall_test.c
> deleted file mode 100644
> index c3ac45df7483..000000000000
> --- a/tools/testing/selftests/kvm/x86_64/svm_vmcall_test.c
> +++ /dev/null
> @@ -1,74 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> - * svm_vmcall_test
> - *
> - * Copyright (C) 2020, Red Hat, Inc.
> - *
> - * Nested SVM testing: VMCALL
> - */
> -
> -#include "test_util.h"
> -#include "kvm_util.h"
> -#include "processor.h"
> -#include "svm_util.h"
> -
> -static void l2_guest_code(struct svm_test_data *svm)
> -{
> -	__asm__ __volatile__("vmcall");
> -}
> -
> -static void l1_guest_code(struct svm_test_data *svm)
> -{
> -	#define L2_GUEST_STACK_SIZE 64
> -	unsigned long l2_guest_stack[L2_GUEST_STACK_SIZE];
> -	struct vmcb *vmcb = svm->vmcb;
> -
> -	/* Prepare for L2 execution. */
> -	generic_svm_setup(svm, l2_guest_code,
> -			  &l2_guest_stack[L2_GUEST_STACK_SIZE]);
> -
> -	run_guest(vmcb, svm->vmcb_gpa);
> -
> -	GUEST_ASSERT(vmcb->control.exit_code == SVM_EXIT_VMMCALL);
> -	GUEST_DONE();
> -}
> -
> -int main(int argc, char *argv[])
> -{
> -	struct kvm_vcpu *vcpu;
> -	vm_vaddr_t svm_gva;
> -	struct kvm_vm *vm;
> -
> -	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SVM));
> -
> -	vm = vm_create_with_one_vcpu(&vcpu, l1_guest_code);
> -
> -	vcpu_alloc_svm(vm, &svm_gva);
> -	vcpu_args_set(vcpu, 1, svm_gva);
> -
> -	for (;;) {
> -		volatile struct kvm_run *run = vcpu->run;
> -		struct ucall uc;
> -
> -		vcpu_run(vcpu);
> -		TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
> -			    "Got exit_reason other than KVM_EXIT_IO: %u (%s)\n",
> -			    run->exit_reason,
> -			    exit_reason_str(run->exit_reason));
> -
> -		switch (get_ucall(vcpu, &uc)) {
> -		case UCALL_ABORT:
> -			REPORT_GUEST_ASSERT(uc);
> -			/* NOT REACHED */
> -		case UCALL_SYNC:
> -			break;
> -		case UCALL_DONE:
> -			goto done;
> -		default:
> -			TEST_FAIL("Unknown ucall 0x%lx.", uc.cmd);
> -		}
> -	}
> -done:
> -	kvm_vm_free(vm);
> -	return 0;
> -}
> -- 
> 2.37.2.789.g6183377224-goog
> 

Re: [V2 PATCH 5/8] KVM: selftests: x86: delete svm_vmcall_test

[Vishal Annapurve]

On Wed, Sep 21, 2022 at 2:34 PM David Matlack <dmatlack@google.com> wrote:
>
> On Thu, Sep 15, 2022 at 12:04:45AM +0000, Vishal Annapurve wrote:
> > svm_vmcall_test is superseded by fix_hypercall_test.
>
> Please provide a more detailed description of why svm_vmcall_test is
> being dropped. e.g. What do you mean by "superseded" specifically?
>
> I ask because this will be helpful context for future readers of this
> commit. I also ask because it's not clear to me that fix_hypercall_test
> is a 1:1 replacement of svm_vmcall_test. e.g. svm_vmcall_test sets up an
> L2 while fix_hypercall_test just executes in L1.
>

You are right, I earlier wrongly interpreted it to be just testing
vmcall, but it also does vmcall from L2.
I will exclude this change in the next series.

> >
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> > ---
> > --
> > 2.37.2.789.g6183377224-goog
> >

[V2 PATCH 6/8] KVM: selftests: x86: Execute cpu specific hypercall from nested guests

[Vishal Annapurve]

Execute vmcall/vmmcall from nested guests according to the cpu type.
This avoid exit to KVM which would anyway patch the hypercall
instruction according to the cpu type.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vishal Annapurve <vannapurve@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h      |  2 ++
 tools/testing/selftests/kvm/include/x86_64/vmx.h  |  9 ---------
 .../selftests/kvm/lib/x86_64/perf_test_util.c     |  2 +-
 .../testing/selftests/kvm/lib/x86_64/processor.c  | 15 +++++++++++++++
 tools/testing/selftests/kvm/x86_64/smm_test.c     |  2 +-
 tools/testing/selftests/kvm/x86_64/state_test.c   |  8 ++++----
 .../selftests/kvm/x86_64/vmx_dirty_log_test.c     |  2 +-
 .../kvm/x86_64/vmx_preemption_timer_test.c        |  2 +-
 8 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 0cbc71b7af50..18a8a6a2b786 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -833,6 +833,8 @@ void vm_set_page_table_entry(struct kvm_vm *vm, struct kvm_vcpu *vcpu,
 uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
 		       uint64_t a3);
 
+void nested_guest_vmcall(void);
+
 void __vm_xsave_require_permission(int bit, const char *name);
 
 #define vm_xsave_require_permission(perm)	\
diff --git a/tools/testing/selftests/kvm/include/x86_64/vmx.h b/tools/testing/selftests/kvm/include/x86_64/vmx.h
index 99fa1410964c..d8d4fd3353e5 100644
--- a/tools/testing/selftests/kvm/include/x86_64/vmx.h
+++ b/tools/testing/selftests/kvm/include/x86_64/vmx.h
@@ -480,15 +480,6 @@ static inline int vmresume(void)
 	return ret;
 }
 
-static inline void vmcall(void)
-{
-	/* Currently, L1 destroys our GPRs during vmexits.  */
-	__asm__ __volatile__("push %%rbp; vmcall; pop %%rbp" : : :
-			     "rax", "rbx", "rcx", "rdx",
-			     "rsi", "rdi", "r8", "r9", "r10", "r11", "r12",
-			     "r13", "r14", "r15");
-}
-
 static inline int vmread(uint64_t encoding, uint64_t *value)
 {
 	uint64_t tmp;
diff --git a/tools/testing/selftests/kvm/lib/x86_64/perf_test_util.c b/tools/testing/selftests/kvm/lib/x86_64/perf_test_util.c
index 0f344a7c89c4..b420b35b7f45 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/perf_test_util.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/perf_test_util.c
@@ -18,7 +18,7 @@
 void perf_test_l2_guest_code(uint64_t vcpu_id)
 {
 	perf_test_guest_code(vcpu_id);
-	vmcall();
+	nested_guest_vmcall();
 }
 
 extern char perf_test_l2_guest_entry[];
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index c0ae938772f6..e12c8b543b8f 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1195,6 +1195,21 @@ uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
 	return r;
 }
 
+void nested_guest_vmcall(void)
+{
+	/* Currently, L1 destroys our GPRs during vmexits.  */
+	if (is_amd_cpu())
+		__asm__ __volatile__("push %%rbp; vmmcall; pop %%rbp" : : :
+			     "rax", "rbx", "rcx", "rdx",
+			     "rsi", "rdi", "r8", "r9", "r10", "r11", "r12",
+			     "r13", "r14", "r15");
+	else
+		__asm__ __volatile__("push %%rbp; vmcall; pop %%rbp" : : :
+			     "rax", "rbx", "rcx", "rdx",
+			     "rsi", "rdi", "r8", "r9", "r10", "r11", "r12",
+			     "r13", "r14", "r15");
+}
+
 const struct kvm_cpuid2 *kvm_get_supported_hv_cpuid(void)
 {
 	static struct kvm_cpuid2 *cpuid;
diff --git a/tools/testing/selftests/kvm/x86_64/smm_test.c b/tools/testing/selftests/kvm/x86_64/smm_test.c
index 1f136a81858e..bf04c78c9c8e 100644
--- a/tools/testing/selftests/kvm/x86_64/smm_test.c
+++ b/tools/testing/selftests/kvm/x86_64/smm_test.c
@@ -61,7 +61,7 @@ static void l2_guest_code(void)
 
 	sync_with_host(10);
 
-	vmcall();
+	nested_guest_vmcall();
 }
 
 static void guest_code(void *arg)
diff --git a/tools/testing/selftests/kvm/x86_64/state_test.c b/tools/testing/selftests/kvm/x86_64/state_test.c
index ea578971fb9f..a9634c06dc60 100644
--- a/tools/testing/selftests/kvm/x86_64/state_test.c
+++ b/tools/testing/selftests/kvm/x86_64/state_test.c
@@ -26,10 +26,10 @@ void svm_l2_guest_code(void)
 {
 	GUEST_SYNC(4);
 	/* Exit to L1 */
-	vmcall();
+	nested_guest_vmcall();
 	GUEST_SYNC(6);
 	/* Done, exit to L1 and never come back.  */
-	vmcall();
+	nested_guest_vmcall();
 }
 
 static void svm_l1_guest_code(struct svm_test_data *svm)
@@ -57,7 +57,7 @@ void vmx_l2_guest_code(void)
 	GUEST_SYNC(6);
 
 	/* Exit to L1 */
-	vmcall();
+	nested_guest_vmcall();
 
 	/* L1 has now set up a shadow VMCS for us.  */
 	GUEST_ASSERT(vmreadz(GUEST_RIP) == 0xc0ffee);
@@ -70,7 +70,7 @@ void vmx_l2_guest_code(void)
 	GUEST_SYNC(12);
 
 	/* Done, exit to L1 and never come back.  */
-	vmcall();
+	nested_guest_vmcall();
 }
 
 static void vmx_l1_guest_code(struct vmx_pages *vmx_pages)
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_dirty_log_test.c b/tools/testing/selftests/kvm/x86_64/vmx_dirty_log_test.c
index 2d8c23d639f7..fa24e69a806c 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_dirty_log_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_dirty_log_test.c
@@ -42,7 +42,7 @@ static void l2_guest_code(void)
 	GUEST_SYNC(false);
 
 	/* Exit to L1 and never come back.  */
-	vmcall();
+	nested_guest_vmcall();
 }
 
 void l1_guest_code(struct vmx_pages *vmx)
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_preemption_timer_test.c b/tools/testing/selftests/kvm/x86_64/vmx_preemption_timer_test.c
index 0efdc05969a5..04bae6995344 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_preemption_timer_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_preemption_timer_test.c
@@ -38,7 +38,7 @@ void l2_guest_code(void)
 {
 	u64 vmx_pt_delta;
 
-	vmcall();
+	nested_guest_vmcall();
 	l2_vmx_pt_start = (rdtsc() >> vmx_pt_rate) << vmx_pt_rate;
 
 	/*
-- 
2.37.2.789.g6183377224-goog

[V2 PATCH 7/8] Kvm: selftests: x86: Execute cpu specific vmcall instruction

[Vishal Annapurve]

Update the vmcall instruction invocation to happen according to the cpu
type.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vishal Annapurve <vannapurve@google.com>
---
 tools/testing/selftests/kvm/include/x86_64/processor.h    | 8 ++++++++
 tools/testing/selftests/kvm/x86_64/vmx_apic_access_test.c | 2 +-
 .../selftests/kvm/x86_64/vmx_nested_tsc_scaling_test.c    | 2 +-
 tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c  | 2 +-
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 18a8a6a2b786..74893a7a80f8 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -833,6 +833,14 @@ void vm_set_page_table_entry(struct kvm_vm *vm, struct kvm_vcpu *vcpu,
 uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
 		       uint64_t a3);
 
+/*
+ * Execute vmcall instruction.
+ */
+static inline void vmcall(void)
+{
+	kvm_hypercall(0, 0, 0, 0, 0);
+}
+
 void nested_guest_vmcall(void);
 
 void __vm_xsave_require_permission(int bit, const char *name);
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_apic_access_test.c b/tools/testing/selftests/kvm/x86_64/vmx_apic_access_test.c
index 5abecf06329e..8180711c8684 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_apic_access_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_apic_access_test.c
@@ -31,7 +31,7 @@
 static void l2_guest_code(void)
 {
 	/* Exit to L1 */
-	__asm__ __volatile__("vmcall");
+	vmcall();
 }
 
 static void l1_guest_code(struct vmx_pages *vmx_pages, unsigned long high_gpa)
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_nested_tsc_scaling_test.c b/tools/testing/selftests/kvm/x86_64/vmx_nested_tsc_scaling_test.c
index 465a9434d61c..37da9d01d5d6 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_nested_tsc_scaling_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_nested_tsc_scaling_test.c
@@ -76,7 +76,7 @@ static void l2_guest_code(void)
 	check_tsc_freq(UCHECK_L2);
 
 	/* exit to L1 */
-	__asm__ __volatile__("vmcall");
+	vmcall();
 }
 
 static void l1_guest_code(struct vmx_pages *vmx_pages)
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c b/tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c
index 5943187e8594..00192f564d9b 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c
@@ -74,7 +74,7 @@ static void l2_guest_code(void)
 	check_ia32_tsc_adjust(-2 * TSC_ADJUST_VALUE);
 
 	/* Exit to L1 */
-	__asm__ __volatile__("vmcall");
+	vmcall();
 }
 
 static void l1_guest_code(struct vmx_pages *vmx_pages)
-- 
2.37.2.789.g6183377224-goog

Re: [V2 PATCH 7/8] Kvm: selftests: x86: Execute cpu specific vmcall instruction

[David Matlack]

On Thu, Sep 15, 2022 at 12:04:47AM +0000, Vishal Annapurve wrote:
> Update the vmcall instruction invocation to happen according to the cpu
> type.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> ---
>  tools/testing/selftests/kvm/include/x86_64/processor.h    | 8 ++++++++
>  tools/testing/selftests/kvm/x86_64/vmx_apic_access_test.c | 2 +-
>  .../selftests/kvm/x86_64/vmx_nested_tsc_scaling_test.c    | 2 +-
>  tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c  | 2 +-

What's the reason to use kvm_hypercall() for these tests? All of these
are Intel-specific. i.e. is_amd_cpu() will always return false.

>  4 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 18a8a6a2b786..74893a7a80f8 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -833,6 +833,14 @@ void vm_set_page_table_entry(struct kvm_vm *vm, struct kvm_vcpu *vcpu,
>  uint64_t kvm_hypercall(uint64_t nr, uint64_t a0, uint64_t a1, uint64_t a2,
>  		       uint64_t a3);
>  
> +/*
> + * Execute vmcall instruction.
> + */
> +static inline void vmcall(void)
> +{
> +	kvm_hypercall(0, 0, 0, 0, 0);
> +}
> +
>  void nested_guest_vmcall(void);
>  
>  void __vm_xsave_require_permission(int bit, const char *name);
> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_apic_access_test.c b/tools/testing/selftests/kvm/x86_64/vmx_apic_access_test.c
> index 5abecf06329e..8180711c8684 100644
> --- a/tools/testing/selftests/kvm/x86_64/vmx_apic_access_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/vmx_apic_access_test.c
> @@ -31,7 +31,7 @@
>  static void l2_guest_code(void)
>  {
>  	/* Exit to L1 */
> -	__asm__ __volatile__("vmcall");
> +	vmcall();
>  }
>  
>  static void l1_guest_code(struct vmx_pages *vmx_pages, unsigned long high_gpa)
> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_nested_tsc_scaling_test.c b/tools/testing/selftests/kvm/x86_64/vmx_nested_tsc_scaling_test.c
> index 465a9434d61c..37da9d01d5d6 100644
> --- a/tools/testing/selftests/kvm/x86_64/vmx_nested_tsc_scaling_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/vmx_nested_tsc_scaling_test.c
> @@ -76,7 +76,7 @@ static void l2_guest_code(void)
>  	check_tsc_freq(UCHECK_L2);
>  
>  	/* exit to L1 */
> -	__asm__ __volatile__("vmcall");
> +	vmcall();
>  }
>  
>  static void l1_guest_code(struct vmx_pages *vmx_pages)
> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c b/tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c
> index 5943187e8594..00192f564d9b 100644
> --- a/tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c
> @@ -74,7 +74,7 @@ static void l2_guest_code(void)
>  	check_ia32_tsc_adjust(-2 * TSC_ADJUST_VALUE);
>  
>  	/* Exit to L1 */
> -	__asm__ __volatile__("vmcall");
> +	vmcall();
>  }
>  
>  static void l1_guest_code(struct vmx_pages *vmx_pages)
> -- 
> 2.37.2.789.g6183377224-goog
> 

Re: [V2 PATCH 7/8] Kvm: selftests: x86: Execute cpu specific vmcall instruction

[Vishal Annapurve]

On Wed, Sep 21, 2022 at 2:43 PM David Matlack <dmatlack@google.com> wrote:
>
> On Thu, Sep 15, 2022 at 12:04:47AM +0000, Vishal Annapurve wrote:
> > Update the vmcall instruction invocation to happen according to the cpu
> > type.
> >
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> > ---
> >  tools/testing/selftests/kvm/include/x86_64/processor.h    | 8 ++++++++
> >  tools/testing/selftests/kvm/x86_64/vmx_apic_access_test.c | 2 +-
> >  .../selftests/kvm/x86_64/vmx_nested_tsc_scaling_test.c    | 2 +-
> >  tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c  | 2 +-
>
> What's the reason to use kvm_hypercall() for these tests? All of these
> are Intel-specific. i.e. is_amd_cpu() will always return false.
>

That's right. This change is attempting to have hypercalls from guest
code done via a common API as far as possible.

> >  4 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> > ...
> > --
> > 2.37.2.789.g6183377224-goog
> >

[V2 PATCH 8/8] KVM: selftests: x86: xen: Execute cpu specific vmcall instruction

[Vishal Annapurve]

Update xen specific hypercall invocation to execute cpu specific vmcall
instructions.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vishal Annapurve <vannapurve@google.com>
---
 .../selftests/kvm/x86_64/xen_shinfo_test.c    | 64 +++++++------------
 .../selftests/kvm/x86_64/xen_vmcall_test.c    | 14 ++--
 2 files changed, 34 insertions(+), 44 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
index 8a5cb800f50e..92ed07f1c772 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
@@ -145,6 +145,23 @@ static void guest_wait_for_irq(void)
 	guest_saw_irq = false;
 }
 
+static unsigned long vmcall_helper(unsigned long reg_a, unsigned long reg_di,
+	unsigned long reg_si)
+{
+	unsigned long ret;
+
+	if (is_amd_cpu())
+		__asm__ __volatile__ ("vmmcall" :
+			"=a" (ret) :
+			"a" (reg_a), "D" (reg_di), "S" (reg_si));
+	else
+		__asm__ __volatile__ ("vmcall" :
+			"=a" (ret) :
+			"a" (reg_a), "D" (reg_di), "S" (reg_si));
+
+	return ret;
+}
+
 static void guest_code(void)
 {
 	struct vcpu_runstate_info *rs = (void *)RUNSTATE_VADDR;
@@ -217,12 +234,7 @@ static void guest_code(void)
 	 * EVTCHNOP_send hypercall. */
 	unsigned long rax;
 	struct evtchn_send s = { .port = 127 };
-	__asm__ __volatile__ ("vmcall" :
-			      "=a" (rax) :
-			      "a" (__HYPERVISOR_event_channel_op),
-			      "D" (EVTCHNOP_send),
-			      "S" (&s));
-
+	rax = vmcall_helper(__HYPERVISOR_event_channel_op, EVTCHNOP_send, (unsigned long)&s);
 	GUEST_ASSERT(rax == 0);
 
 	guest_wait_for_irq();
@@ -232,12 +244,7 @@ static void guest_code(void)
 	/* Deliver "outbound" event channel to an eventfd which
 	 * happens to be one of our own irqfds. */
 	s.port = 197;
-	__asm__ __volatile__ ("vmcall" :
-			      "=a" (rax) :
-			      "a" (__HYPERVISOR_event_channel_op),
-			      "D" (EVTCHNOP_send),
-			      "S" (&s));
-
+	rax = vmcall_helper(__HYPERVISOR_event_channel_op, EVTCHNOP_send, (unsigned long)&s);
 	GUEST_ASSERT(rax == 0);
 
 	guest_wait_for_irq();
@@ -245,10 +252,7 @@ static void guest_code(void)
 	GUEST_SYNC(13);
 
 	/* Set a timer 100ms in the future. */
-	__asm__ __volatile__ ("vmcall" :
-			      "=a" (rax) :
-			      "a" (__HYPERVISOR_set_timer_op),
-			      "D" (rs->state_entry_time + 100000000));
+	rax = vmcall_helper(__HYPERVISOR_set_timer_op, (rs->state_entry_time + 100000000), 0);
 	GUEST_ASSERT(rax == 0);
 
 	GUEST_SYNC(14);
@@ -271,36 +275,21 @@ static void guest_code(void)
 		.timeout = 0,
 	};
 
-	__asm__ __volatile__ ("vmcall" :
-			      "=a" (rax) :
-			      "a" (__HYPERVISOR_sched_op),
-			      "D" (SCHEDOP_poll),
-			      "S" (&p));
-
+	rax = vmcall_helper(__HYPERVISOR_sched_op, SCHEDOP_poll, (unsigned long)&p);
 	GUEST_ASSERT(rax == 0);
 
 	GUEST_SYNC(17);
 
 	/* Poll for an unset port and wait for the timeout. */
 	p.timeout = 100000000;
-	__asm__ __volatile__ ("vmcall" :
-			      "=a" (rax) :
-			      "a" (__HYPERVISOR_sched_op),
-			      "D" (SCHEDOP_poll),
-			      "S" (&p));
-
+	rax = vmcall_helper(__HYPERVISOR_sched_op, SCHEDOP_poll, (unsigned long)&p);
 	GUEST_ASSERT(rax == 0);
 
 	GUEST_SYNC(18);
 
 	/* A timer will wake the masked port we're waiting on, while we poll */
 	p.timeout = 0;
-	__asm__ __volatile__ ("vmcall" :
-			      "=a" (rax) :
-			      "a" (__HYPERVISOR_sched_op),
-			      "D" (SCHEDOP_poll),
-			      "S" (&p));
-
+	rax = vmcall_helper(__HYPERVISOR_sched_op, SCHEDOP_poll, (unsigned long)&p);
 	GUEST_ASSERT(rax == 0);
 
 	GUEST_SYNC(19);
@@ -309,12 +298,7 @@ static void guest_code(void)
 	 * actual interrupt, while we're polling on a different port. */
 	ports[0]++;
 	p.timeout = 0;
-	__asm__ __volatile__ ("vmcall" :
-			      "=a" (rax) :
-			      "a" (__HYPERVISOR_sched_op),
-			      "D" (SCHEDOP_poll),
-			      "S" (&p));
-
+	rax = vmcall_helper(__HYPERVISOR_sched_op, SCHEDOP_poll, (unsigned long)&p);
 	GUEST_ASSERT(rax == 0);
 
 	guest_wait_for_irq();
diff --git a/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c b/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
index 88914d48c65e..e78f1b5d3af8 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
@@ -37,10 +37,16 @@ static void guest_code(void)
 	register unsigned long r9 __asm__("r9") = ARGVALUE(6);
 
 	/* First a direct invocation of 'vmcall' */
-	__asm__ __volatile__("vmcall" :
-			     "=a"(rax) :
-			     "a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
-			     "r"(r10), "r"(r8), "r"(r9));
+	if (is_amd_cpu())
+		__asm__ __volatile__("vmmcall" :
+			"=a"(rax) :
+			"a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
+			"r"(r10), "r"(r8), "r"(r9));
+	else
+		__asm__ __volatile__("vmcall" :
+			"=a"(rax) :
+			"a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
+			"r"(r10), "r"(r8), "r"(r9));
 	GUEST_ASSERT(rax == RETVALUE);
 
 	/* Fill in the Xen hypercall page */
-- 
2.37.2.789.g6183377224-goog

Re: [V2 PATCH 8/8] KVM: selftests: x86: xen: Execute cpu specific vmcall instruction

[David Matlack]

On Thu, Sep 15, 2022 at 12:04:48AM +0000, Vishal Annapurve wrote:
> Update xen specific hypercall invocation to execute cpu specific vmcall
> instructions.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> ---
>  .../selftests/kvm/x86_64/xen_shinfo_test.c    | 64 +++++++------------
>  .../selftests/kvm/x86_64/xen_vmcall_test.c    | 14 ++--
>  2 files changed, 34 insertions(+), 44 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
> index 8a5cb800f50e..92ed07f1c772 100644
> --- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
> @@ -145,6 +145,23 @@ static void guest_wait_for_irq(void)
>  	guest_saw_irq = false;
>  }
>  
> +static unsigned long vmcall_helper(unsigned long reg_a, unsigned long reg_di,
> +	unsigned long reg_si)
> +{
> +	unsigned long ret;
> +
> +	if (is_amd_cpu())
> +		__asm__ __volatile__ ("vmmcall" :
> +			"=a" (ret) :
> +			"a" (reg_a), "D" (reg_di), "S" (reg_si));
> +	else
> +		__asm__ __volatile__ ("vmcall" :
> +			"=a" (ret) :
> +			"a" (reg_a), "D" (reg_di), "S" (reg_si));
> +
> +	return ret;
> +}
> +
>  static void guest_code(void)
>  {
>  	struct vcpu_runstate_info *rs = (void *)RUNSTATE_VADDR;
> @@ -217,12 +234,7 @@ static void guest_code(void)
>  	 * EVTCHNOP_send hypercall. */
>  	unsigned long rax;
>  	struct evtchn_send s = { .port = 127 };
> -	__asm__ __volatile__ ("vmcall" :
> -			      "=a" (rax) :
> -			      "a" (__HYPERVISOR_event_channel_op),
> -			      "D" (EVTCHNOP_send),
> -			      "S" (&s));
> -
> +	rax = vmcall_helper(__HYPERVISOR_event_channel_op, EVTCHNOP_send, (unsigned long)&s);
>  	GUEST_ASSERT(rax == 0);
>  
>  	guest_wait_for_irq();
> @@ -232,12 +244,7 @@ static void guest_code(void)
>  	/* Deliver "outbound" event channel to an eventfd which
>  	 * happens to be one of our own irqfds. */
>  	s.port = 197;
> -	__asm__ __volatile__ ("vmcall" :
> -			      "=a" (rax) :
> -			      "a" (__HYPERVISOR_event_channel_op),
> -			      "D" (EVTCHNOP_send),
> -			      "S" (&s));
> -
> +	rax = vmcall_helper(__HYPERVISOR_event_channel_op, EVTCHNOP_send, (unsigned long)&s);
>  	GUEST_ASSERT(rax == 0);
>  
>  	guest_wait_for_irq();
> @@ -245,10 +252,7 @@ static void guest_code(void)
>  	GUEST_SYNC(13);
>  
>  	/* Set a timer 100ms in the future. */
> -	__asm__ __volatile__ ("vmcall" :
> -			      "=a" (rax) :
> -			      "a" (__HYPERVISOR_set_timer_op),
> -			      "D" (rs->state_entry_time + 100000000));
> +	rax = vmcall_helper(__HYPERVISOR_set_timer_op, (rs->state_entry_time + 100000000), 0);
>  	GUEST_ASSERT(rax == 0);
>  
>  	GUEST_SYNC(14);
> @@ -271,36 +275,21 @@ static void guest_code(void)
>  		.timeout = 0,
>  	};
>  
> -	__asm__ __volatile__ ("vmcall" :
> -			      "=a" (rax) :
> -			      "a" (__HYPERVISOR_sched_op),
> -			      "D" (SCHEDOP_poll),
> -			      "S" (&p));
> -
> +	rax = vmcall_helper(__HYPERVISOR_sched_op, SCHEDOP_poll, (unsigned long)&p);
>  	GUEST_ASSERT(rax == 0);
>  
>  	GUEST_SYNC(17);
>  
>  	/* Poll for an unset port and wait for the timeout. */
>  	p.timeout = 100000000;
> -	__asm__ __volatile__ ("vmcall" :
> -			      "=a" (rax) :
> -			      "a" (__HYPERVISOR_sched_op),
> -			      "D" (SCHEDOP_poll),
> -			      "S" (&p));
> -
> +	rax = vmcall_helper(__HYPERVISOR_sched_op, SCHEDOP_poll, (unsigned long)&p);
>  	GUEST_ASSERT(rax == 0);
>  
>  	GUEST_SYNC(18);
>  
>  	/* A timer will wake the masked port we're waiting on, while we poll */
>  	p.timeout = 0;
> -	__asm__ __volatile__ ("vmcall" :
> -			      "=a" (rax) :
> -			      "a" (__HYPERVISOR_sched_op),
> -			      "D" (SCHEDOP_poll),
> -			      "S" (&p));
> -
> +	rax = vmcall_helper(__HYPERVISOR_sched_op, SCHEDOP_poll, (unsigned long)&p);
>  	GUEST_ASSERT(rax == 0);
>  
>  	GUEST_SYNC(19);
> @@ -309,12 +298,7 @@ static void guest_code(void)
>  	 * actual interrupt, while we're polling on a different port. */
>  	ports[0]++;
>  	p.timeout = 0;
> -	__asm__ __volatile__ ("vmcall" :
> -			      "=a" (rax) :
> -			      "a" (__HYPERVISOR_sched_op),
> -			      "D" (SCHEDOP_poll),
> -			      "S" (&p));
> -
> +	rax = vmcall_helper(__HYPERVISOR_sched_op, SCHEDOP_poll, (unsigned long)&p);
>  	GUEST_ASSERT(rax == 0);
>  
>  	guest_wait_for_irq();
> diff --git a/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c b/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
> index 88914d48c65e..e78f1b5d3af8 100644
> --- a/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
> @@ -37,10 +37,16 @@ static void guest_code(void)
>  	register unsigned long r9 __asm__("r9") = ARGVALUE(6);
>  
>  	/* First a direct invocation of 'vmcall' */
> -	__asm__ __volatile__("vmcall" :
> -			     "=a"(rax) :
> -			     "a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
> -			     "r"(r10), "r"(r8), "r"(r9));
> +	if (is_amd_cpu())
> +		__asm__ __volatile__("vmmcall" :
> +			"=a"(rax) :
> +			"a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
> +			"r"(r10), "r"(r8), "r"(r9));
> +	else
> +		__asm__ __volatile__("vmcall" :
> +			"=a"(rax) :
> +			"a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
> +			"r"(r10), "r"(r8), "r"(r9));

Can we create common helper functions or macros for doing hypercalls to
reduce the amount of duplicated inline assembly?

>  	GUEST_ASSERT(rax == RETVALUE);
>  
>  	/* Fill in the Xen hypercall page */
> -- 
> 2.37.2.789.g6183377224-goog
> 

Re: [V2 PATCH 8/8] KVM: selftests: x86: xen: Execute cpu specific vmcall instruction

[Vishal Annapurve]

On Wed, Sep 21, 2022 at 2:47 PM David Matlack <dmatlack@google.com> wrote:
>
> On Thu, Sep 15, 2022 at 12:04:48AM +0000, Vishal Annapurve wrote:
> > Update xen specific hypercall invocation to execute cpu specific vmcall
> > instructions.
> >
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Vishal Annapurve <vannapurve@google.com>
> > ---
> >  .../selftests/kvm/x86_64/xen_shinfo_test.c    | 64 +++++++------------
> >  .../selftests/kvm/x86_64/xen_vmcall_test.c    | 14 ++--
> >  2 files changed, 34 insertions(+), 44 deletions(-)
> > ...
> > diff --git a/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c b/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
> > index 88914d48c65e..e78f1b5d3af8 100644
> > --- a/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
> > +++ b/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
> > @@ -37,10 +37,16 @@ static void guest_code(void)
> >       register unsigned long r9 __asm__("r9") = ARGVALUE(6);
> >
> >       /* First a direct invocation of 'vmcall' */
> > -     __asm__ __volatile__("vmcall" :
> > -                          "=a"(rax) :
> > -                          "a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
> > -                          "r"(r10), "r"(r8), "r"(r9));
> > +     if (is_amd_cpu())
> > +             __asm__ __volatile__("vmmcall" :
> > +                     "=a"(rax) :
> > +                     "a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
> > +                     "r"(r10), "r"(r8), "r"(r9));
> > +     else
> > +             __asm__ __volatile__("vmcall" :
> > +                     "=a"(rax) :
> > +                     "a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
> > +                     "r"(r10), "r"(r8), "r"(r9));
>
> Can we create common helper functions or macros for doing hypercalls to
> reduce the amount of duplicated inline assembly?
>

Ack, will fix this in the next series.

> >       GUEST_ASSERT(rax == RETVALUE);
> >
> >       /* Fill in the Xen hypercall page */
> > --
> > 2.37.2.789.g6183377224-goog
> >