Basic idea of this series is now to use the kselftest_harness.h
framework to get TAP output in the tests, so that it is easier
for the user to see what is going on, and e.g. to be able to
detect whether a certain test is part of the test binary or not
(which is useful when tests get extended in the course of time).
Since most tests also need a vcpu, we introduce our own macros
to define such tests, so we don't have to repeat this code all
over the place.
v3:
- Add patch from Sean to allow setting vCPU's entry points seperately
- Let each test define the entry point via KVM_ONE_VCPU_TEST(), don't
do it globally from KVM_ONE_VCPU_TEST_SUITE()
v2:
- Dropped the "Rename the ASSERT_EQ macro" patch (already merged)
- Split the fixes in the sync_regs_test into separate patches
(see the first two patches)
- Introduce the KVM_ONE_VCPU_TEST_SUITE() macro as suggested
by Sean (see third patch) and use it in the following patches
- Add a new patch to convert vmx_pmu_caps_test.c, too
Sean Christopherson (1):
KVM: selftests: Move setting a vCPU's entry point to a dedicated API
Thomas Huth (7):
KVM: selftests: x86: sync_regs_test: Use vcpu_run() where appropriate
KVM: selftests: x86: sync_regs_test: Get regs structure before
modifying it
KVM: selftests: Add a macro to define a test with one vcpu
KVM: selftests: x86: Use TAP interface in the sync_regs test
KVM: selftests: x86: Use TAP interface in the fix_hypercall test
KVM: selftests: x86: Use TAP interface in the vmx_pmu_caps test
KVM: selftests: x86: Use TAP interface in the userspace_msr_exit test
.../selftests/kvm/include/kvm_test_harness.h | 36 ++++++
.../selftests/kvm/include/kvm_util_base.h | 11 +-
.../selftests/kvm/lib/aarch64/processor.c | 23 +++-
.../selftests/kvm/lib/riscv/processor.c | 9 +-
.../selftests/kvm/lib/s390x/processor.c | 13 +-
.../selftests/kvm/lib/x86_64/processor.c | 13 +-
.../selftests/kvm/x86_64/fix_hypercall_test.c | 27 ++--
.../selftests/kvm/x86_64/sync_regs_test.c | 121 +++++++++++++-----
.../kvm/x86_64/userspace_msr_exit_test.c | 52 ++------
.../selftests/kvm/x86_64/vmx_pmu_caps_test.c | 50 ++------
10 files changed, 215 insertions(+), 140 deletions(-)
create mode 100644 tools/testing/selftests/kvm/include/kvm_test_harness.h
--
2.43.0
From: Sean Christopherson <[email protected]>
Extract the code to set a vCPU's entry point out of vm_arch_vcpu_add() and
into a new API, vcpu_arch_set_entry_point(). Providing a separate API
will allow creating a KVM selftests hardness that can handle tests that
use different entry points for sub-tests, whereas *requiring* the entry
point to be specified at vCPU creation makes it difficult to create a
generic harness, e.g. the boilerplate setup/teardown can't easily create
and destroy the VM and vCPUs.
Signed-off-by: Sean Christopherson <[email protected]>
Signed-off-by: Thomas Huth <[email protected]>
---
.../selftests/kvm/include/kvm_util_base.h | 11 +++++----
.../selftests/kvm/lib/aarch64/processor.c | 23 ++++++++++++++-----
.../selftests/kvm/lib/riscv/processor.c | 9 +++++---
.../selftests/kvm/lib/s390x/processor.c | 13 ++++++-----
.../selftests/kvm/lib/x86_64/processor.c | 13 ++++++++---
5 files changed, 47 insertions(+), 22 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 9e5afc472c142..a6e7738a8db73 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -969,15 +969,18 @@ static inline void vcpu_dump(FILE *stream, struct kvm_vcpu *vcpu,
* Input Args:
* vm - Virtual Machine
* vcpu_id - The id of the VCPU to add to the VM.
- * guest_code - The vCPU's entry point
*/
-struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
- void *guest_code);
+struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id);
+void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code);
static inline struct kvm_vcpu *vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
void *guest_code)
{
- return vm_arch_vcpu_add(vm, vcpu_id, guest_code);
+ struct kvm_vcpu *vcpu = vm_arch_vcpu_add(vm, vcpu_id);
+
+ vcpu_arch_set_entry_point(vcpu, guest_code);
+
+ return vcpu;
}
/* Re-create a vCPU after restarting a VM, e.g. for state save/restore tests. */
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index 41c776b642c0c..05b6c40e3655f 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -365,8 +365,13 @@ void vcpu_arch_dump(FILE *stream, struct kvm_vcpu *vcpu, uint8_t indent)
indent, "", pstate, pc);
}
-struct kvm_vcpu *aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
- struct kvm_vcpu_init *init, void *guest_code)
+void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code)
+{
+ vcpu_set_reg(vcpu, ARM64_CORE_REG(regs.pc), (uint64_t)guest_code);
+}
+
+static struct kvm_vcpu *__aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
+ struct kvm_vcpu_init *init)
{
size_t stack_size;
uint64_t stack_vaddr;
@@ -381,15 +386,21 @@ struct kvm_vcpu *aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
aarch64_vcpu_setup(vcpu, init);
vcpu_set_reg(vcpu, ARM64_CORE_REG(sp_el1), stack_vaddr + stack_size);
- vcpu_set_reg(vcpu, ARM64_CORE_REG(regs.pc), (uint64_t)guest_code);
+}
+
+struct kvm_vcpu *aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
+ struct kvm_vcpu_init *init, void *guest_code)
+{
+ struct kvm_vcpu *vcpu = __aarch64_vcpu_add(vm, vcpu_id, init);
+
+ vcpu_arch_set_entry_point(vcpu, guest_code);
return vcpu;
}
-struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
- void *guest_code)
+struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id)
{
- return aarch64_vcpu_add(vm, vcpu_id, NULL, guest_code);
+ return __aarch64_vcpu_add(vm, vcpu_id, NULL);
}
void vcpu_args_set(struct kvm_vcpu *vcpu, unsigned int num, ...)
diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c
index 7ca736fb41940..c993947f07823 100644
--- a/tools/testing/selftests/kvm/lib/riscv/processor.c
+++ b/tools/testing/selftests/kvm/lib/riscv/processor.c
@@ -277,8 +277,12 @@ static void __aligned(16) guest_unexp_trap(void)
0, 0, 0, 0, 0, 0);
}
-struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
- void *guest_code)
+void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code)
+{
+ vcpu_set_reg(vcpu, RISCV_CORE_REG(regs.pc), (unsigned long)guest_code);
+}
+
+struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id)
{
int r;
size_t stack_size;
@@ -312,7 +316,6 @@ struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
/* Setup stack pointer and program counter of guest */
vcpu_set_reg(vcpu, RISCV_CORE_REG(regs.sp), stack_vaddr + stack_size);
- vcpu_set_reg(vcpu, RISCV_CORE_REG(regs.pc), (unsigned long)guest_code);
/* Setup default exception vector of guest */
vcpu_set_reg(vcpu, RISCV_GENERAL_CSR_REG(stvec), (unsigned long)guest_unexp_trap);
diff --git a/tools/testing/selftests/kvm/lib/s390x/processor.c b/tools/testing/selftests/kvm/lib/s390x/processor.c
index 15945121daf17..cd5301cc9788a 100644
--- a/tools/testing/selftests/kvm/lib/s390x/processor.c
+++ b/tools/testing/selftests/kvm/lib/s390x/processor.c
@@ -155,15 +155,18 @@ void virt_arch_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
virt_dump_region(stream, vm, indent, vm->pgd);
}
-struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
- void *guest_code)
+void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code)
+{
+ vcpu->run->psw_addr = (uintptr_t)guest_code;
+}
+
+struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id)
{
size_t stack_size = DEFAULT_STACK_PGS * getpagesize();
uint64_t stack_vaddr;
struct kvm_regs regs;
struct kvm_sregs sregs;
struct kvm_vcpu *vcpu;
- struct kvm_run *run;
TEST_ASSERT(vm->page_size == 4096, "Unsupported page size: 0x%x",
vm->page_size);
@@ -184,9 +187,7 @@ struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
sregs.crs[1] = vm->pgd | 0xf; /* Primary region table */
vcpu_sregs_set(vcpu, &sregs);
- run = vcpu->run;
- run->psw_mask = 0x0400000180000000ULL; /* DAT enabled + 64 bit mode */
- run->psw_addr = (uintptr_t)guest_code;
+ vcpu->run->psw_mask = 0x0400000180000000ULL; /* DAT enabled + 64 bit mode */
return vcpu;
}
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index d8288374078e4..b9b6cb730a088 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -562,8 +562,16 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm)
sync_global_to_guest(vm, host_cpu_is_amd);
}
-struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
- void *guest_code)
+void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code)
+{
+ struct kvm_regs regs;
+
+ vcpu_regs_get(vcpu, ®s);
+ regs.rip = (unsigned long) guest_code;
+ vcpu_regs_set(vcpu, ®s);
+}
+
+struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id)
{
struct kvm_mp_state mp_state;
struct kvm_regs regs;
@@ -597,7 +605,6 @@ struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
vcpu_regs_get(vcpu, ®s);
regs.rflags = regs.rflags | 0x2;
regs.rsp = stack_vaddr;
- regs.rip = (unsigned long) guest_code;
vcpu_regs_set(vcpu, ®s);
/* Setup the MP state */
--
2.43.0
The regs structure just accidentally contains the right values
from the previous test in the spot where we want to change rbx.
It's cleaner if we properly initialize the structure here before
using it.
Suggested-by: Sean Christopherson <[email protected]>
Signed-off-by: Thomas Huth <[email protected]>
---
tools/testing/selftests/kvm/x86_64/sync_regs_test.c | 1 +
1 file changed, 1 insertion(+)
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 8c3898cf79b31..1cd19dfa0046c 100644
--- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
@@ -315,6 +315,7 @@ int main(int argc, char *argv[])
run->kvm_valid_regs = 0;
run->kvm_dirty_regs = 0;
run->s.regs.regs.rbx = 0xAAAA;
+ vcpu_regs_get(vcpu, ®s);
regs.rbx = 0xBAC0;
vcpu_regs_set(vcpu, ®s);
vcpu_run(vcpu);
--
2.43.0
The sync_regs test currently does not have any output (unless one
of the TEST_ASSERT statement fails), so it's hard to say for a user
whether a certain new sub-test has been included in the binary or
not. Let's make this a little bit more user-friendly and include
some TAP output via the kselftest_harness.h / kvm_test_harness.h
interface.
To be able to use the interface, we have to break up the huge main()
function here in more fine grained parts - then we can use the new
KVM_ONE_VCPU_TEST() macro to define the individual tests. Since these
are run with a separate VM now, we have also to make sure to create
the expected state at the beginning of each test, so some parts grow
a little bit - which should be OK considering that the individual
tests are more self-contained now.
Suggested-by: David Matlack <[email protected]>
Suggested-by: Sean Christopherson <[email protected]>
Signed-off-by: Thomas Huth <[email protected]>
---
.../selftests/kvm/x86_64/sync_regs_test.c | 110 +++++++++++++-----
1 file changed, 84 insertions(+), 26 deletions(-)
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 1cd19dfa0046c..67f78c0a58a51 100644
--- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
@@ -17,6 +17,7 @@
#include <sys/ioctl.h>
#include <pthread.h>
+#include "kvm_test_harness.h"
#include "test_util.h"
#include "kvm_util.h"
#include "processor.h"
@@ -41,6 +42,8 @@ void guest_code(void)
: "rax", "rbx");
}
+KVM_ONE_VCPU_TEST_SUITE(sync_regs_test);
+
static void compare_regs(struct kvm_regs *left, struct kvm_regs *right)
{
#define REG_COMPARE(reg) \
@@ -152,18 +155,15 @@ static noinline void *race_sregs_cr4(void *arg)
return NULL;
}
-static void race_sync_regs(void *racer)
+static void race_sync_regs(struct kvm_vcpu *vcpu, void *racer)
{
const time_t TIMEOUT = 2; /* seconds, roughly */
struct kvm_x86_state *state;
struct kvm_translation tr;
- struct kvm_vcpu *vcpu;
struct kvm_run *run;
- struct kvm_vm *vm;
pthread_t thread;
time_t t;
- vm = vm_create_with_one_vcpu(&vcpu, guest_code);
run = vcpu->run;
run->kvm_valid_regs = KVM_SYNC_X86_SREGS;
@@ -205,26 +205,12 @@ static void race_sync_regs(void *racer)
TEST_ASSERT_EQ(pthread_join(thread, NULL), 0);
kvm_x86_state_cleanup(state);
- kvm_vm_free(vm);
}
-int main(int argc, char *argv[])
+KVM_ONE_VCPU_TEST(sync_regs_test, read_invalid, guest_code)
{
- struct kvm_vcpu *vcpu;
- struct kvm_vm *vm;
- struct kvm_run *run;
- struct kvm_regs regs;
- struct kvm_sregs sregs;
- struct kvm_vcpu_events events;
- int rv, cap;
-
- cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
- TEST_REQUIRE((cap & TEST_SYNC_FIELDS) == TEST_SYNC_FIELDS);
- TEST_REQUIRE(!(cap & INVALID_SYNC_FIELD));
-
- vm = vm_create_with_one_vcpu(&vcpu, guest_code);
-
- run = vcpu->run;
+ struct kvm_run *run = vcpu->run;
+ int rv;
/* Request reading invalid register set from VCPU. */
run->kvm_valid_regs = INVALID_SYNC_FIELD;
@@ -240,6 +226,12 @@ int main(int argc, char *argv[])
"Invalid kvm_valid_regs did not cause expected KVM_RUN error: %d\n",
rv);
run->kvm_valid_regs = 0;
+}
+
+KVM_ONE_VCPU_TEST(sync_regs_test, set_invalid, guest_code)
+{
+ struct kvm_run *run = vcpu->run;
+ int rv;
/* Request setting invalid register set into VCPU. */
run->kvm_dirty_regs = INVALID_SYNC_FIELD;
@@ -255,6 +247,14 @@ int main(int argc, char *argv[])
"Invalid kvm_dirty_regs did not cause expected KVM_RUN error: %d\n",
rv);
run->kvm_dirty_regs = 0;
+}
+
+KVM_ONE_VCPU_TEST(sync_regs_test, req_and_verify_all_valid, guest_code)
+{
+ struct kvm_run *run = vcpu->run;
+ struct kvm_vcpu_events events;
+ struct kvm_sregs sregs;
+ struct kvm_regs regs;
/* Request and verify all valid register sets. */
/* TODO: BUILD TIME CHECK: TEST_ASSERT(KVM_SYNC_X86_NUM_FIELDS != 3); */
@@ -270,6 +270,19 @@ int main(int argc, char *argv[])
vcpu_events_get(vcpu, &events);
compare_vcpu_events(&events, &run->s.regs.events);
+}
+
+KVM_ONE_VCPU_TEST(sync_regs_test, set_and_verify_various, guest_code)
+{
+ struct kvm_run *run = vcpu->run;
+ struct kvm_vcpu_events events;
+ struct kvm_sregs sregs;
+ struct kvm_regs regs;
+
+ /* Run once to get register set */
+ run->kvm_valid_regs = TEST_SYNC_FIELDS;
+ vcpu_run(vcpu);
+ TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
/* Set and verify various register values. */
run->s.regs.regs.rbx = 0xBAD1DEA;
@@ -295,6 +308,11 @@ int main(int argc, char *argv[])
vcpu_events_get(vcpu, &events);
compare_vcpu_events(&events, &run->s.regs.events);
+}
+
+KVM_ONE_VCPU_TEST(sync_regs_test, clear_kvm_dirty_regs_bits, guest_code)
+{
+ struct kvm_run *run = vcpu->run;
/* Clear kvm_dirty_regs bits, verify new s.regs values are
* overwritten with existing guest values.
@@ -307,6 +325,17 @@ int main(int argc, char *argv[])
TEST_ASSERT(run->s.regs.regs.rbx != 0xDEADBEEF,
"rbx sync regs value incorrect 0x%llx.",
run->s.regs.regs.rbx);
+}
+
+KVM_ONE_VCPU_TEST(sync_regs_test, clear_kvm_valid_and_dirty_regs, guest_code)
+{
+ struct kvm_run *run = vcpu->run;
+ struct kvm_regs regs;
+
+ /* Run once to get register set */
+ run->kvm_valid_regs = TEST_SYNC_FIELDS;
+ vcpu_run(vcpu);
+ TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
/* Clear kvm_valid_regs bits and kvm_dirty_bits.
* Verify s.regs values are not overwritten with existing guest values
@@ -327,6 +356,17 @@ int main(int argc, char *argv[])
TEST_ASSERT(regs.rbx == 0xBAC0 + 1,
"rbx guest value incorrect 0x%llx.",
regs.rbx);
+}
+
+KVM_ONE_VCPU_TEST(sync_regs_test, clear_kvm_valid_regs_bits, guest_code)
+{
+ struct kvm_run *run = vcpu->run;
+ struct kvm_regs regs;
+
+ /* Run once to get register set */
+ run->kvm_valid_regs = TEST_SYNC_FIELDS;
+ vcpu_run(vcpu);
+ TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
/* Clear kvm_valid_regs bits. Verify s.regs values are not overwritten
* with existing guest values but that guest values are overwritten
@@ -344,12 +384,30 @@ int main(int argc, char *argv[])
TEST_ASSERT(regs.rbx == 0xBBBB + 1,
"rbx guest value incorrect 0x%llx.",
regs.rbx);
+}
+
+KVM_ONE_VCPU_TEST(sync_regs_test, race_cr4, guest_code)
+{
+ race_sync_regs(vcpu, race_sregs_cr4);
+}
+
+KVM_ONE_VCPU_TEST(sync_regs_test, race_exc, guest_code)
+{
+ race_sync_regs(vcpu, race_events_exc);
+}
- kvm_vm_free(vm);
+KVM_ONE_VCPU_TEST(sync_regs_test, race_inj_pen, guest_code)
+{
+ race_sync_regs(vcpu, race_events_inj_pen);
+}
+
+int main(int argc, char *argv[])
+{
+ int cap;
- race_sync_regs(race_sregs_cr4);
- race_sync_regs(race_events_exc);
- race_sync_regs(race_events_inj_pen);
+ cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
+ TEST_REQUIRE((cap & TEST_SYNC_FIELDS) == TEST_SYNC_FIELDS);
+ TEST_REQUIRE(!(cap & INVALID_SYNC_FIELD));
- return 0;
+ return test_harness_run(argc, argv);
}
--
2.43.0
Use the kvm_test_harness.h interface in this test to get TAP
output, so that it is easier for the user to see what the test
is doing.
Signed-off-by: Thomas Huth <[email protected]>
---
.../selftests/kvm/x86_64/fix_hypercall_test.c | 27 ++++++++++++-------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
index 0f728f05ea82f..f3c2239228b10 100644
--- a/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
+++ b/tools/testing/selftests/kvm/x86_64/fix_hypercall_test.c
@@ -9,6 +9,7 @@
#include <linux/stringify.h>
#include <stdint.h>
+#include "kvm_test_harness.h"
#include "apic.h"
#include "test_util.h"
#include "kvm_util.h"
@@ -83,6 +84,8 @@ static void guest_main(void)
GUEST_DONE();
}
+KVM_ONE_VCPU_TEST_SUITE(fix_hypercall);
+
static void enter_guest(struct kvm_vcpu *vcpu)
{
struct kvm_run *run = vcpu->run;
@@ -103,14 +106,11 @@ static void enter_guest(struct kvm_vcpu *vcpu)
}
}
-static void test_fix_hypercall(bool disable_quirk)
+static void test_fix_hypercall(struct kvm_vcpu *vcpu, bool disable_quirk)
{
- struct kvm_vcpu *vcpu;
- struct kvm_vm *vm;
-
- vm = vm_create_with_one_vcpu(&vcpu, guest_main);
+ struct kvm_vm *vm = vcpu->vm;
- vm_init_descriptor_tables(vcpu->vm);
+ vm_init_descriptor_tables(vm);
vcpu_init_descriptor_tables(vcpu);
vm_install_exception_handler(vcpu->vm, UD_VECTOR, guest_ud_handler);
@@ -126,10 +126,19 @@ static void test_fix_hypercall(bool disable_quirk)
enter_guest(vcpu);
}
-int main(void)
+KVM_ONE_VCPU_TEST(fix_hypercall, enable_quirk, guest_main)
+{
+ test_fix_hypercall(vcpu, false);
+}
+
+KVM_ONE_VCPU_TEST(fix_hypercall, disable_quirk, guest_main)
+{
+ test_fix_hypercall(vcpu, true);
+}
+
+int main(int argc, char *argv[])
{
TEST_REQUIRE(kvm_check_cap(KVM_CAP_DISABLE_QUIRKS2) & KVM_X86_QUIRK_FIX_HYPERCALL_INSN);
- test_fix_hypercall(false);
- test_fix_hypercall(true);
+ return test_harness_run(argc, argv);
}
--
2.43.0
Use the kvm_test_harness.h interface in this test to get TAP
output, so that it is easier for the user to see what the test
is doing.
Signed-off-by: Thomas Huth <[email protected]>
---
.../selftests/kvm/x86_64/vmx_pmu_caps_test.c | 50 ++++---------------
1 file changed, 11 insertions(+), 39 deletions(-)
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
index 2a8d4ac2f0204..11953c3ded756 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
@@ -15,6 +15,7 @@
#include <linux/bitmap.h>
+#include "kvm_test_harness.h"
#include "kvm_util.h"
#include "vmx.h"
@@ -32,7 +33,7 @@ union perf_capabilities {
u64 anythread_deprecated:1;
};
u64 capabilities;
-};
+} host_cap;
/*
* The LBR format and most PEBS features are immutable, all other features are
@@ -73,19 +74,19 @@ static void guest_code(uint64_t current_val)
GUEST_DONE();
}
+KVM_ONE_VCPU_TEST_SUITE(vmx_pmu_caps);
+
/*
* Verify that guest WRMSRs to PERF_CAPABILITIES #GP regardless of the value
* written, that the guest always sees the userspace controlled value, and that
* PERF_CAPABILITIES is immutable after KVM_RUN.
*/
-static void test_guest_wrmsr_perf_capabilities(union perf_capabilities host_cap)
+KVM_ONE_VCPU_TEST(vmx_pmu_caps, guest_wrmsr_perf_capabilities, guest_code)
{
- struct kvm_vcpu *vcpu;
- struct kvm_vm *vm = vm_create_with_one_vcpu(&vcpu, guest_code);
struct ucall uc;
int r, i;
- vm_init_descriptor_tables(vm);
+ vm_init_descriptor_tables(vcpu->vm);
vcpu_init_descriptor_tables(vcpu);
vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.capabilities);
@@ -117,31 +118,21 @@ static void test_guest_wrmsr_perf_capabilities(union perf_capabilities host_cap)
TEST_ASSERT(!r, "Post-KVM_RUN write '0x%llx'didn't fail",
host_cap.capabilities ^ BIT_ULL(i));
}
-
- kvm_vm_free(vm);
}
/*
* Verify KVM allows writing PERF_CAPABILITIES with all KVM-supported features
* enabled, as well as '0' (to disable all features).
*/
-static void test_basic_perf_capabilities(union perf_capabilities host_cap)
+KVM_ONE_VCPU_TEST(vmx_pmu_caps, basic_perf_capabilities, guest_code)
{
- struct kvm_vcpu *vcpu;
- struct kvm_vm *vm = vm_create_with_one_vcpu(&vcpu, NULL);
-
vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, 0);
vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.capabilities);
-
- kvm_vm_free(vm);
}
-static void test_fungible_perf_capabilities(union perf_capabilities host_cap)
+KVM_ONE_VCPU_TEST(vmx_pmu_caps, fungible_perf_capabilities, guest_code)
{
const uint64_t fungible_caps = host_cap.capabilities & ~immutable_caps.capabilities;
-
- struct kvm_vcpu *vcpu;
- struct kvm_vm *vm = vm_create_with_one_vcpu(&vcpu, NULL);
int bit;
for_each_set_bit(bit, &fungible_caps, 64) {
@@ -150,8 +141,6 @@ static void test_fungible_perf_capabilities(union perf_capabilities host_cap)
host_cap.capabilities & ~BIT_ULL(bit));
}
vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.capabilities);
-
- kvm_vm_free(vm);
}
/*
@@ -160,14 +149,11 @@ static void test_fungible_perf_capabilities(union perf_capabilities host_cap)
* separately as they are multi-bit values, e.g. toggling or setting a single
* bit can generate a false positive without dedicated safeguards.
*/
-static void test_immutable_perf_capabilities(union perf_capabilities host_cap)
+KVM_ONE_VCPU_TEST(vmx_pmu_caps, immutable_perf_capabilities, guest_code)
{
const uint64_t reserved_caps = (~host_cap.capabilities |
immutable_caps.capabilities) &
~format_caps.capabilities;
-
- struct kvm_vcpu *vcpu;
- struct kvm_vm *vm = vm_create_with_one_vcpu(&vcpu, NULL);
union perf_capabilities val = host_cap;
int r, bit;
@@ -201,8 +187,6 @@ static void test_immutable_perf_capabilities(union perf_capabilities host_cap)
TEST_ASSERT(!r, "Bad PEBS FMT = 0x%x didn't fail, host = 0x%x",
val.pebs_format, host_cap.pebs_format);
}
-
- kvm_vm_free(vm);
}
/*
@@ -211,17 +195,13 @@ static void test_immutable_perf_capabilities(union perf_capabilities host_cap)
* LBR_TOS as those bits are writable across all uarch implementations (arch
* LBRs will need to poke a different MSR).
*/
-static void test_lbr_perf_capabilities(union perf_capabilities host_cap)
+KVM_ONE_VCPU_TEST(vmx_pmu_caps, lbr_perf_capabilities, guest_code)
{
- struct kvm_vcpu *vcpu;
- struct kvm_vm *vm;
int r;
if (!host_cap.lbr_format)
return;
- vm = vm_create_with_one_vcpu(&vcpu, NULL);
-
vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.capabilities);
vcpu_set_msr(vcpu, MSR_LBR_TOS, 7);
@@ -229,14 +209,10 @@ static void test_lbr_perf_capabilities(union perf_capabilities host_cap)
r = _vcpu_set_msr(vcpu, MSR_LBR_TOS, 7);
TEST_ASSERT(!r, "Writing LBR_TOS should fail after disabling vPMU");
-
- kvm_vm_free(vm);
}
int main(int argc, char *argv[])
{
- union perf_capabilities host_cap;
-
TEST_REQUIRE(get_kvm_param_bool("enable_pmu"));
TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_PDCM));
@@ -248,9 +224,5 @@ int main(int argc, char *argv[])
TEST_ASSERT(host_cap.full_width_write,
"Full-width writes should always be supported");
- test_basic_perf_capabilities(host_cap);
- test_fungible_perf_capabilities(host_cap);
- test_immutable_perf_capabilities(host_cap);
- test_guest_wrmsr_perf_capabilities(host_cap);
- test_lbr_perf_capabilities(host_cap);
+ return test_harness_run(argc, argv);
}
--
2.43.0
Use the kselftest_harness.h interface in this test to get TAP
output, so that it is easier for the user to see what the test
is doing.
Signed-off-by: Thomas Huth <[email protected]>
---
.../kvm/x86_64/userspace_msr_exit_test.c | 52 +++++--------------
1 file changed, 13 insertions(+), 39 deletions(-)
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 3533dc2fbfeeb..9591a5fd54d7c 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
@@ -8,6 +8,7 @@
#define _GNU_SOURCE /* for program_invocation_short_name */
#include <sys/ioctl.h>
+#include "kvm_test_harness.h"
#include "test_util.h"
#include "kvm_util.h"
#include "vmx.h"
@@ -527,14 +528,13 @@ static void run_guest_then_process_ucall_done(struct kvm_vcpu *vcpu)
process_ucall_done(vcpu);
}
-static void test_msr_filter_allow(void)
+KVM_ONE_VCPU_TEST_SUITE(user_msr);
+
+KVM_ONE_VCPU_TEST(user_msr, msr_filter_allow, guest_code_filter_allow)
{
- struct kvm_vcpu *vcpu;
- struct kvm_vm *vm;
+ struct kvm_vm *vm = vcpu->vm;
int rc;
- vm = vm_create_with_one_vcpu(&vcpu, guest_code_filter_allow);
-
rc = kvm_check_cap(KVM_CAP_X86_USER_SPACE_MSR);
TEST_ASSERT(rc, "KVM_CAP_X86_USER_SPACE_MSR is available");
vm_enable_cap(vm, KVM_CAP_X86_USER_SPACE_MSR, KVM_MSR_EXIT_REASON_FILTER);
@@ -585,8 +585,6 @@ static void test_msr_filter_allow(void)
} else {
printf("To run the instruction emulated tests set the module parameter 'kvm.force_emulation_prefix=1'\n");
}
-
- kvm_vm_free(vm);
}
static int handle_ucall(struct kvm_vcpu *vcpu)
@@ -646,16 +644,12 @@ static void handle_wrmsr(struct kvm_run *run)
}
}
-static void test_msr_filter_deny(void)
+KVM_ONE_VCPU_TEST(user_msr, msr_filter_deny, guest_code_filter_deny)
{
- struct kvm_vcpu *vcpu;
- struct kvm_vm *vm;
- struct kvm_run *run;
+ struct kvm_vm *vm = vcpu->vm;
+ struct kvm_run *run = vcpu->run;
int rc;
- vm = vm_create_with_one_vcpu(&vcpu, guest_code_filter_deny);
- run = vcpu->run;
-
rc = kvm_check_cap(KVM_CAP_X86_USER_SPACE_MSR);
TEST_ASSERT(rc, "KVM_CAP_X86_USER_SPACE_MSR is available");
vm_enable_cap(vm, KVM_CAP_X86_USER_SPACE_MSR, KVM_MSR_EXIT_REASON_INVAL |
@@ -689,18 +683,13 @@ static void test_msr_filter_deny(void)
done:
TEST_ASSERT(msr_reads == 4, "Handled 4 rdmsr in user space");
TEST_ASSERT(msr_writes == 3, "Handled 3 wrmsr in user space");
-
- kvm_vm_free(vm);
}
-static void test_msr_permission_bitmap(void)
+KVM_ONE_VCPU_TEST(user_msr, msr_permission_bitmap, guest_code_permission_bitmap)
{
- struct kvm_vcpu *vcpu;
- struct kvm_vm *vm;
+ struct kvm_vm *vm = vcpu->vm;
int rc;
- vm = vm_create_with_one_vcpu(&vcpu, guest_code_permission_bitmap);
-
rc = kvm_check_cap(KVM_CAP_X86_USER_SPACE_MSR);
TEST_ASSERT(rc, "KVM_CAP_X86_USER_SPACE_MSR is available");
vm_enable_cap(vm, KVM_CAP_X86_USER_SPACE_MSR, KVM_MSR_EXIT_REASON_FILTER);
@@ -715,8 +704,6 @@ static void test_msr_permission_bitmap(void)
vm_ioctl(vm, KVM_X86_SET_MSR_FILTER, &filter_gs);
run_guest_then_process_rdmsr(vcpu, MSR_GS_BASE);
run_guest_then_process_ucall_done(vcpu);
-
- kvm_vm_free(vm);
}
#define test_user_exit_msr_ioctl(vm, cmd, arg, flag, valid_mask) \
@@ -786,31 +773,18 @@ static void run_msr_filter_flag_test(struct kvm_vm *vm)
}
/* Test that attempts to write to the unused bits in a flag fails. */
-static void test_user_exit_msr_flags(void)
+KVM_ONE_VCPU_TEST(user_msr, user_exit_msr_flags, NULL)
{
- struct kvm_vcpu *vcpu;
- struct kvm_vm *vm;
-
- vm = vm_create_with_one_vcpu(&vcpu, NULL);
+ struct kvm_vm *vm = vcpu->vm;
/* Test flags for KVM_CAP_X86_USER_SPACE_MSR. */
run_user_space_msr_flag_test(vm);
/* Test flags and range flags for KVM_X86_SET_MSR_FILTER. */
run_msr_filter_flag_test(vm);
-
- kvm_vm_free(vm);
}
int main(int argc, char *argv[])
{
- test_msr_filter_allow();
-
- test_msr_filter_deny();
-
- test_msr_permission_bitmap();
-
- test_user_exit_msr_flags();
-
- return 0;
+ return test_harness_run(argc, argv);
}
--
2.43.0
Most tests are currently not giving any proper output for the user
to see how much sub-tests have already been run, or whether new
sub-tests are part of a binary or not. So it would be good to
support TAP output in the KVM selftests. There is already a nice
framework for this in the kselftest_harness.h header which we can
use. But since we also need a vcpu in most KVM selftests, it also
makes sense to introduce our own wrapper around this which takes
care of creating a VM with one vcpu, so we don't have to repeat
this boilerplate in each and every test. Thus let's introduce
a KVM_ONE_VCPU_TEST() macro here which takes care of this.
Suggested-by: Sean Christopherson <[email protected]>
Link: https://lore.kernel.org/all/Y2v+B3xxYKJSM%[email protected]/
Signed-off-by: Thomas Huth <[email protected]>
---
.../selftests/kvm/include/kvm_test_harness.h | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
create mode 100644 tools/testing/selftests/kvm/include/kvm_test_harness.h
diff --git a/tools/testing/selftests/kvm/include/kvm_test_harness.h b/tools/testing/selftests/kvm/include/kvm_test_harness.h
new file mode 100644
index 0000000000000..8f7c6858e8e2d
--- /dev/null
+++ b/tools/testing/selftests/kvm/include/kvm_test_harness.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Macros for defining a KVM test
+ *
+ * Copyright (C) 2022, Google LLC.
+ */
+
+#ifndef SELFTEST_KVM_TEST_HARNESS_H
+#define SELFTEST_KVM_TEST_HARNESS_H
+
+#include "kselftest_harness.h"
+
+#define KVM_ONE_VCPU_TEST_SUITE(name) \
+ FIXTURE(name) { \
+ struct kvm_vcpu *vcpu; \
+ }; \
+ \
+ FIXTURE_SETUP(name) { \
+ (void)vm_create_with_one_vcpu(&self->vcpu, NULL); \
+ } \
+ \
+ FIXTURE_TEARDOWN(name) { \
+ kvm_vm_free(self->vcpu->vm); \
+ }
+
+#define KVM_ONE_VCPU_TEST(suite, test, guestcode) \
+static void __suite##_##test(struct kvm_vcpu *vcpu); \
+ \
+TEST_F(suite, test) \
+{ \
+ vcpu_arch_set_entry_point(self->vcpu, guestcode); \
+ __suite##_##test(self->vcpu); \
+} \
+static void __suite##_##test(struct kvm_vcpu *vcpu)
+
+#endif /* SELFTEST_KVM_TEST_HARNESS_H */
--
2.43.0
On Thu, 08 Feb 2024 21:48:36 +0100, Thomas Huth wrote:
> Basic idea of this series is now to use the kselftest_harness.h
> framework to get TAP output in the tests, so that it is easier
> for the user to see what is going on, and e.g. to be able to
> detect whether a certain test is part of the test binary or not
> (which is useful when tests get extended in the course of time).
>
> Since most tests also need a vcpu, we introduce our own macros
> to define such tests, so we don't have to repeat this code all
> over the place.
>
> [...]
OMG, you didn't tell me this allows sub-tests to run after a failed test!
That alone is worth the conversion :-)
There's definitely a few enhancements we'll want to make, but this is more than
good enough as a starting point.
Applied to kvm-x86 selftests, thanks!
[1/8] KVM: selftests: x86: sync_regs_test: Use vcpu_run() where appropriate
https://github.com/kvm-x86/linux/commit/e10086285659
[2/8] KVM: selftests: x86: sync_regs_test: Get regs structure before modifying it
https://github.com/kvm-x86/linux/commit/221d65449453
[3/8] KVM: selftests: Move setting a vCPU's entry point to a dedicated API
https://github.com/kvm-x86/linux/commit/8ef192609f14
[4/8] KVM: selftests: Add a macro to define a test with one vcpu
https://github.com/kvm-x86/linux/commit/992178c7219c
[5/8] KVM: selftests: x86: Use TAP interface in the sync_regs test
https://github.com/kvm-x86/linux/commit/04941eb15439
[6/8] KVM: selftests: x86: Use TAP interface in the fix_hypercall test
https://github.com/kvm-x86/linux/commit/69fb12492005
[7/8] KVM: selftests: x86: Use TAP interface in the vmx_pmu_caps test
https://github.com/kvm-x86/linux/commit/200f604dfd07
[8/8] KVM: selftests: x86: Use TAP interface in the userspace_msr_exit test
https://github.com/kvm-x86/linux/commit/8fd14fc541c7
--
https://github.com/kvm-x86/linux/tree/next
On Thu, Feb 08, 2024 at 09:48:39PM +0100, Thomas Huth wrote:
> From: Sean Christopherson <[email protected]>
>
> Extract the code to set a vCPU's entry point out of vm_arch_vcpu_add() and
> into a new API, vcpu_arch_set_entry_point(). Providing a separate API
> will allow creating a KVM selftests hardness that can handle tests that
> use different entry points for sub-tests, whereas *requiring* the entry
> point to be specified at vCPU creation makes it difficult to create a
> generic harness, e.g. the boilerplate setup/teardown can't easily create
> and destroy the VM and vCPUs.
With today's -next I'm seeing most of the KVM selftests failing on an
arm64 defconfig with:
# ==== Test Assertion Failure ====
# include/kvm_util_base.h:677: !ret
# pid=735 tid=735 errno=9 - Bad file descriptor
# 1 0x0000000000410937: vcpu_set_reg at kvm_util_base.h:677 (discriminator 4)
# 2 (inlined by) vcpu_arch_set_entry_point at processor.c:370 (discriminator 4)
# 3 0x0000000000407bab: vm_vcpu_add at kvm_util_base.h:981
# 4 (inlined by) __vm_create_with_vcpus at kvm_util.c:419
# 5 (inlined by) __vm_create_shape_with_one_vcpu at kvm_util.c:432
# 6 0x000000000040187b: __vm_create_with_one_vcpu at kvm_util_base.h:892
# 7 (inlined by) vm_create_with_one_vcpu at kvm_util_base.h:899
# 8 (inlined by) main at aarch32_id_regs.c:158
# 9 0x0000007fbcbe6dc3: ?? ??:0
# 10 0x0000007fbcbe6e97: ?? ??:0
# 11 0x0000000000401f2f: _start at ??:?
# KVM_SET_ONE_REG failed, rc: -1 errno: 9 (Bad file descriptor)
and a bisect pointed to this commit which does look plausibly relevant.
Note that while this was bisected with plain arm64 defconfig and the KVM
selftests fragment was not enabled, but enabling the KVM fragment gave
the same result as would be expected based on the options enabled by the
fragment. We're also seeing an alternative failure pattern where the
tests segfault when run in a different environment, I'm also tracking
that down but I suspect these are the same issue.
A full log from a sample failing run can be seen here.
https://lava.sirena.org.uk/scheduler/job/645026#L1581
Bisect log:
git bisect start
# good: [75d8cf735082728a5dfb7e46926ee184851cc519] Merge branch 'for-linux-next-fixes' of git://anongit.freedesktop.org/drm/drm-misc
git bisect good 75d8cf735082728a5dfb7e46926ee184851cc519
# bad: [20af1ca418d2c0b11bc2a1fe8c0c88f67bcc2a7e] Add linux-next specific files for 20240228
git bisect bad 20af1ca418d2c0b11bc2a1fe8c0c88f67bcc2a7e
# good: [1322f1801e59dddce10591d602d246c1bf49990c] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git bisect good 1322f1801e59dddce10591d602d246c1bf49990c
# good: [f996a1cab1c3547a0bd2edf0daa7a71eddec9b58] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git
git bisect good f996a1cab1c3547a0bd2edf0daa7a71eddec9b58
# bad: [22e19d7b30a88dc9e7b315935f44fb2a6c6bf7bf] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git
git bisect bad 22e19d7b30a88dc9e7b315935f44fb2a6c6bf7bf
# good: [f9ad77051d5d45000848e87650a382995adf7e50] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
git bisect good f9ad77051d5d45000848e87650a382995adf7e50
# bad: [6e9a1d8a249374b0c8ff9472f30f160c98881519] Merge branch 'next' of https://github.com/kvm-x86/linux.git
git bisect bad 6e9a1d8a249374b0c8ff9472f30f160c98881519
# bad: [f3ac6b5aec49c3f8ced623802ee9efa6484263eb] Merge branch 'xen'
git bisect bad f3ac6b5aec49c3f8ced623802ee9efa6484263eb
# good: [938ccbf4327f38cec365986136e349486ddbb005] Merge branch 'pmu'
git bisect good 938ccbf4327f38cec365986136e349486ddbb005
# bad: [f3750b0c7f6e48b0adfb9bd2419de4a3c604ca68] KVM: selftests: Add a basic SEV-ES smoke test
git bisect bad f3750b0c7f6e48b0adfb9bd2419de4a3c604ca68
# bad: [992178c7219caa0bcdaa5c0ce59615b12da21662] KVM: selftests: Add a macro to define a test with one vcpu
git bisect bad 992178c7219caa0bcdaa5c0ce59615b12da21662
# good: [71cd774ad2f98d4c78bc868e7e55397810be3540] KVM: s390: move s390-specific structs to uapi/asm/kvm.h
git bisect good 71cd774ad2f98d4c78bc868e7e55397810be3540
# good: [db7d6fbc10447090bab8691a907a7c383ec66f58] KVM: remove unnecessary #ifdef
git bisect good db7d6fbc10447090bab8691a907a7c383ec66f58
# good: [221d65449453846bbf6801d0ecf7dfdf4f413ad9] KVM: selftests: x86: sync_regs_test: Get regs structure before modifying it
git bisect good 221d65449453846bbf6801d0ecf7dfdf4f413ad9
# bad: [8ef192609f14272b7bd6fc3a553ebe02d1133cd0] KVM: selftests: Move setting a vCPU's entry point to a dedicated API
git bisect bad 8ef192609f14272b7bd6fc3a553ebe02d1133cd0
# first bad commit: [8ef192609f14272b7bd6fc3a553ebe02d1133cd0] KVM: selftests: Move setting a vCPU's entry point to a dedicated API
On Wed, Feb 28, 2024, Mark Brown wrote:
> On Thu, Feb 08, 2024 at 09:48:39PM +0100, Thomas Huth wrote:
> > From: Sean Christopherson <[email protected]>
> >
> > Extract the code to set a vCPU's entry point out of vm_arch_vcpu_add() and
> > into a new API, vcpu_arch_set_entry_point(). Providing a separate API
> > will allow creating a KVM selftests hardness that can handle tests that
> > use different entry points for sub-tests, whereas *requiring* the entry
> > point to be specified at vCPU creation makes it difficult to create a
> > generic harness, e.g. the boilerplate setup/teardown can't easily create
> > and destroy the VM and vCPUs.
>
> With today's -next I'm seeing most of the KVM selftests failing on an
> arm64 defconfig with:
>
> # ==== Test Assertion Failure ====
> # include/kvm_util_base.h:677: !ret
> # pid=735 tid=735 errno=9 - Bad file descriptor
> # 1 0x0000000000410937: vcpu_set_reg at kvm_util_base.h:677 (discriminator 4)
> # 2 (inlined by) vcpu_arch_set_entry_point at processor.c:370 (discriminator 4)
> # 3 0x0000000000407bab: vm_vcpu_add at kvm_util_base.h:981
> # 4 (inlined by) __vm_create_with_vcpus at kvm_util.c:419
> # 5 (inlined by) __vm_create_shape_with_one_vcpu at kvm_util.c:432
> # 6 0x000000000040187b: __vm_create_with_one_vcpu at kvm_util_base.h:892
> # 7 (inlined by) vm_create_with_one_vcpu at kvm_util_base.h:899
> # 8 (inlined by) main at aarch32_id_regs.c:158
> # 9 0x0000007fbcbe6dc3: ?? ??:0
> # 10 0x0000007fbcbe6e97: ?? ??:0
> # 11 0x0000000000401f2f: _start at ??:?
> # KVM_SET_ONE_REG failed, rc: -1 errno: 9 (Bad file descriptor)
>
> and a bisect pointed to this commit which does look plausibly relevant.
>
> Note that while this was bisected with plain arm64 defconfig and the KVM
> selftests fragment was not enabled, but enabling the KVM fragment gave
> the same result as would be expected based on the options enabled by the
> fragment. We're also seeing an alternative failure pattern where the
> tests segfault when run in a different environment, I'm also tracking
> that down but I suspect these are the same issue.
Gah, my bad, I should have at least tested on ARM since I have easy access to
such hardware. If I can't figure out what's going wrong in the next few hours,
I'll drop this series and we can try again for 6.10.
Sorry :-/
> A full log from a sample failing run can be seen here.
>
> https://lava.sirena.org.uk/scheduler/job/645026#L1581
>
> Bisect log:
>
> git bisect start
> # good: [75d8cf735082728a5dfb7e46926ee184851cc519] Merge branch 'for-linux-next-fixes' of git://anongit.freedesktop.org/drm/drm-misc
> git bisect good 75d8cf735082728a5dfb7e46926ee184851cc519
> # bad: [20af1ca418d2c0b11bc2a1fe8c0c88f67bcc2a7e] Add linux-next specific files for 20240228
> git bisect bad 20af1ca418d2c0b11bc2a1fe8c0c88f67bcc2a7e
> # good: [1322f1801e59dddce10591d602d246c1bf49990c] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
> git bisect good 1322f1801e59dddce10591d602d246c1bf49990c
> # good: [f996a1cab1c3547a0bd2edf0daa7a71eddec9b58] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git
> git bisect good f996a1cab1c3547a0bd2edf0daa7a71eddec9b58
> # bad: [22e19d7b30a88dc9e7b315935f44fb2a6c6bf7bf] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git
> git bisect bad 22e19d7b30a88dc9e7b315935f44fb2a6c6bf7bf
> # good: [f9ad77051d5d45000848e87650a382995adf7e50] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
> git bisect good f9ad77051d5d45000848e87650a382995adf7e50
> # bad: [6e9a1d8a249374b0c8ff9472f30f160c98881519] Merge branch 'next' of https://github.com/kvm-x86/linux.git
> git bisect bad 6e9a1d8a249374b0c8ff9472f30f160c98881519
> # bad: [f3ac6b5aec49c3f8ced623802ee9efa6484263eb] Merge branch 'xen'
> git bisect bad f3ac6b5aec49c3f8ced623802ee9efa6484263eb
> # good: [938ccbf4327f38cec365986136e349486ddbb005] Merge branch 'pmu'
> git bisect good 938ccbf4327f38cec365986136e349486ddbb005
> # bad: [f3750b0c7f6e48b0adfb9bd2419de4a3c604ca68] KVM: selftests: Add a basic SEV-ES smoke test
> git bisect bad f3750b0c7f6e48b0adfb9bd2419de4a3c604ca68
> # bad: [992178c7219caa0bcdaa5c0ce59615b12da21662] KVM: selftests: Add a macro to define a test with one vcpu
> git bisect bad 992178c7219caa0bcdaa5c0ce59615b12da21662
> # good: [71cd774ad2f98d4c78bc868e7e55397810be3540] KVM: s390: move s390-specific structs to uapi/asm/kvm.h
> git bisect good 71cd774ad2f98d4c78bc868e7e55397810be3540
> # good: [db7d6fbc10447090bab8691a907a7c383ec66f58] KVM: remove unnecessary #ifdef
> git bisect good db7d6fbc10447090bab8691a907a7c383ec66f58
> # good: [221d65449453846bbf6801d0ecf7dfdf4f413ad9] KVM: selftests: x86: sync_regs_test: Get regs structure before modifying it
> git bisect good 221d65449453846bbf6801d0ecf7dfdf4f413ad9
> # bad: [8ef192609f14272b7bd6fc3a553ebe02d1133cd0] KVM: selftests: Move setting a vCPU's entry point to a dedicated API
> git bisect bad 8ef192609f14272b7bd6fc3a553ebe02d1133cd0
> # first bad commit: [8ef192609f14272b7bd6fc3a553ebe02d1133cd0] KVM: selftests: Move setting a vCPU's entry point to a dedicated API
+cc Raghavendra
Hey,
On Wed, Feb 28, 2024 at 01:19:48PM -0800, Sean Christopherson wrote:
> but due to a different issue that is fixed in the kvm-arm tree[*], but not in mine,
> I built without -Werror and didn't see the new warn in the sea of GUEST_PRINTF
> warnings.
>
> Ugh, and I still can't enable -Werror, because there are unused functions in
> aarch64/vpmu_counter_access.c
>
> aarch64/vpmu_counter_access.c:96:20: error: unused function 'enable_counter' [-Werror,-Wunused-function]
> static inline void enable_counter(int idx)
> ^
> aarch64/vpmu_counter_access.c:104:20: error: unused function 'disable_counter' [-Werror,-Wunused-function]
> static inline void disable_counter(int idx)
> ^
> 2 errors generated.
> make: *** [Makefile:278: /usr/local/google/home/seanjc/go/src/kernel.org/nox/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.o] Error 1
> make: *** Waiting for unfinished jobs....
>
> Commit 49f31cff9c533d264659356b90445023b04e10fb failed to build with 'make-clang make-arm make -j128'.
>
> Oliver/Marc, any thoughts on how you want to fix the unused function warnings?
> As evidenced by this goof, being able to compile with -Werror is super helpful.
Are these the only remaining warnings we have in the arm64 selftests
build?
Faster than me paging this test back in: Raghu, are we missing any test
cases upstream that these helpers were intended for? If no, mind sending
a patch to get rid of them?
> And another question: is there any reason to not force -Werror for selftests?
Nothing comes to mind. We need to bite the bullet and make the switch.
There might be breakage, but we can certainly handle that.
--
Thanks,
Oliver
On Mon, Feb 26, 2024, Sean Christopherson wrote:
> On Thu, 08 Feb 2024 21:48:36 +0100, Thomas Huth wrote:
> > Basic idea of this series is now to use the kselftest_harness.h
> > framework to get TAP output in the tests, so that it is easier
> > for the user to see what is going on, and e.g. to be able to
> > detect whether a certain test is part of the test binary or not
> > (which is useful when tests get extended in the course of time).
> >
> > Since most tests also need a vcpu, we introduce our own macros
> > to define such tests, so we don't have to repeat this code all
> > over the place.
> >
> > [...]
>
> OMG, you didn't tell me this allows sub-tests to run after a failed test!
> That alone is worth the conversion :-)
>
> There's definitely a few enhancements we'll want to make, but this is more than
> good enough as a starting point.
>
> Applied to kvm-x86 selftests, thanks!
>
> [1/8] KVM: selftests: x86: sync_regs_test: Use vcpu_run() where appropriate
> https://github.com/kvm-x86/linux/commit/e10086285659
> [2/8] KVM: selftests: x86: sync_regs_test: Get regs structure before modifying it
> https://github.com/kvm-x86/linux/commit/221d65449453
> [3/8] KVM: selftests: Move setting a vCPU's entry point to a dedicated API
> https://github.com/kvm-x86/linux/commit/8ef192609f14
> [4/8] KVM: selftests: Add a macro to define a test with one vcpu
> https://github.com/kvm-x86/linux/commit/992178c7219c
> [5/8] KVM: selftests: x86: Use TAP interface in the sync_regs test
> https://github.com/kvm-x86/linux/commit/04941eb15439
> [6/8] KVM: selftests: x86: Use TAP interface in the fix_hypercall test
> https://github.com/kvm-x86/linux/commit/69fb12492005
> [7/8] KVM: selftests: x86: Use TAP interface in the vmx_pmu_caps test
> https://github.com/kvm-x86/linux/commit/200f604dfd07
> [8/8] KVM: selftests: x86: Use TAP interface in the userspace_msr_exit test
> https://github.com/kvm-x86/linux/commit/8fd14fc541c7
FYI, the hashes have changed for patches 3-8, as I forced pushed to fix an ARM
goof in patch 3.
[1/8] KVM: selftests: x86: sync_regs_test: Use vcpu_run() where appropriate
https://github.com/kvm-x86/linux/commit/e10086285659
[2/8] KVM: selftests: x86: sync_regs_test: Get regs structure before modifying it
https://github.com/kvm-x86/linux/commit/221d65449453
[3/8] KVM: selftests: Move setting a vCPU's entry point to a dedicated API
https://github.com/kvm-x86/linux/commit/53a43dd48f8e
[4/8] KVM: selftests: Add a macro to define a test with one vcpu
https://github.com/kvm-x86/linux/commit/55f2cf88486c
[5/8] KVM: selftests: x86: Use TAP interface in the sync_regs test
https://github.com/kvm-x86/linux/commit/ba97ed0af6fe
[6/8] KVM: selftests: x86: Use TAP interface in the fix_hypercall test
https://github.com/kvm-x86/linux/commit/a6983e8f5fab
[7/8] KVM: selftests: x86: Use TAP interface in the vmx_pmu_caps test
https://github.com/kvm-x86/linux/commit/de1b03f25f3b
[8/8] KVM: selftests: x86: Use TAP interface in the userspace_msr_exit test
https://github.com/kvm-x86/linux/commit/8d251856d425
I really should fix the CC list _before_ drafting a reply...
On Wed, Feb 28, 2024 at 09:29:56PM +0000, Oliver Upton wrote:
> +cc Raghavendra
See below :)
> Hey,
>
> On Wed, Feb 28, 2024 at 01:19:48PM -0800, Sean Christopherson wrote:
> > but due to a different issue that is fixed in the kvm-arm tree[*], but not in mine,
> > I built without -Werror and didn't see the new warn in the sea of GUEST_PRINTF
> > warnings.
> >
> > Ugh, and I still can't enable -Werror, because there are unused functions in
> > aarch64/vpmu_counter_access.c
> >
> > aarch64/vpmu_counter_access.c:96:20: error: unused function 'enable_counter' [-Werror,-Wunused-function]
> > static inline void enable_counter(int idx)
> > ^
> > aarch64/vpmu_counter_access.c:104:20: error: unused function 'disable_counter' [-Werror,-Wunused-function]
> > static inline void disable_counter(int idx)
> > ^
> > 2 errors generated.
> > make: *** [Makefile:278: /usr/local/google/home/seanjc/go/src/kernel.org/nox/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.o] Error 1
> > make: *** Waiting for unfinished jobs....
> >
> > Commit 49f31cff9c533d264659356b90445023b04e10fb failed to build with 'make-clang make-arm make -j128'.
> >
> > Oliver/Marc, any thoughts on how you want to fix the unused function warnings?
> > As evidenced by this goof, being able to compile with -Werror is super helpful.
>
> Are these the only remaining warnings we have in the arm64 selftests
> build?
>
> Faster than me paging this test back in: Raghu, are we missing any test
> cases upstream that these helpers were intended for? If no, mind sending
> a patch to get rid of them?
>
> > And another question: is there any reason to not force -Werror for selftests?
>
> Nothing comes to mind. We need to bite the bullet and make the switch.
> There might be breakage, but we can certainly handle that.
>
> --
> Thanks,
> Oliver
--
Thanks,
Oliver
On Wed, Feb 28, 2024, Oliver Upton wrote:
> +cc Raghavendra
>
> Hey,
>
> On Wed, Feb 28, 2024 at 01:19:48PM -0800, Sean Christopherson wrote:
> > but due to a different issue that is fixed in the kvm-arm tree[*], but not in mine,
> > I built without -Werror and didn't see the new warn in the sea of GUEST_PRINTF
> > warnings.
> >
> > Ugh, and I still can't enable -Werror, because there are unused functions in
> > aarch64/vpmu_counter_access.c
> >
> > aarch64/vpmu_counter_access.c:96:20: error: unused function 'enable_counter' [-Werror,-Wunused-function]
> > static inline void enable_counter(int idx)
> > ^
> > aarch64/vpmu_counter_access.c:104:20: error: unused function 'disable_counter' [-Werror,-Wunused-function]
> > static inline void disable_counter(int idx)
> > ^
> > 2 errors generated.
> > make: *** [Makefile:278: /usr/local/google/home/seanjc/go/src/kernel.org/nox/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.o] Error 1
> > make: *** Waiting for unfinished jobs....
> >
> > Commit 49f31cff9c533d264659356b90445023b04e10fb failed to build with 'make-clang make-arm make -j128'.
> >
> > Oliver/Marc, any thoughts on how you want to fix the unused function warnings?
> > As evidenced by this goof, being able to compile with -Werror is super helpful.
>
> Are these the only remaining warnings we have in the arm64 selftests
> build?
Yep, unless I've missed something, this is the only outstanding warning across
all architectures that support selftests (sans LoongArch and PPC, which are
pending).
On Wed, Feb 28, 2024 at 01:19:48PM -0800, Sean Christopherson wrote:
> I'll squash the above and force push.
Thanks, I'll give that a spin (likely tomorrow at this point).
> And another question: is there any reason to not force -Werror for selftests?
Enabling it by default can cause a bunch of fragility when people are
running different compilers, there's things like all the different clang
versions, and it's especially common to run into issues when you've got
a shiny new compiler on a modern distro but need to go look at older
kernels for some reason. You then get issues bisecting things since you
get regions where the build was broken due to -Werror which may or may
not be pointing at the runtime issue you were looking for (particularly
likely to be hiding things if the build issue is in a different test).
Hey Oliver,
+cc Shaoqin
On Wed, Feb 28, 2024 at 1:30 PM Oliver Upton <[email protected]> wrote:
>
> +cc Raghavendra
>
> Hey,
>
> On Wed, Feb 28, 2024 at 01:19:48PM -0800, Sean Christopherson wrote:
> > but due to a different issue that is fixed in the kvm-arm tree[*], but not in mine,
> > I built without -Werror and didn't see the new warn in the sea of GUEST_PRINTF
> > warnings.
> >
> > Ugh, and I still can't enable -Werror, because there are unused functions in
> > aarch64/vpmu_counter_access.c
> >
> > aarch64/vpmu_counter_access.c:96:20: error: unused function 'enable_counter' [-Werror,-Wunused-function]
> > static inline void enable_counter(int idx)
> > ^
> > aarch64/vpmu_counter_access.c:104:20: error: unused function 'disable_counter' [-Werror,-Wunused-function]
> > static inline void disable_counter(int idx)
> > ^
> > 2 errors generated.
> > make: *** [Makefile:278: /usr/local/google/home/seanjc/go/src/kernel.org/nox/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.o] Error 1
> > make: *** Waiting for unfinished jobs....
> >
> > Commit 49f31cff9c533d264659356b90445023b04e10fb failed to build with 'make-clang make-arm make -j128'.
> >
> > Oliver/Marc, any thoughts on how you want to fix the unused function warnings?
> > As evidenced by this goof, being able to compile with -Werror is super helpful.
>
> Are these the only remaining warnings we have in the arm64 selftests
> build?
>
> Faster than me paging this test back in: Raghu, are we missing any test
> cases upstream that these helpers were intended for? If no, mind sending
> a patch to get rid of them?
>
I sent out a patch in the past to get rid of them [1], but Shaoqin is
currently making an effort to (fix and) use them in their tests [2].
While we are still reviewing the series, we can apply [1] to unblock
enabling -Werror and Shaqoqin can re-introduce the functions as
needed. But, it's your call.
Thank you.
Raghavendra
[1]: https://lore.kernel.org/lkml/[email protected]/T/
[2]: https://lore.kernel.org/all/[email protected]/
On Wed, Feb 28, 2024 at 03:00:03PM -0800, Raghavendra Rao Ananta wrote:
[...]
> I sent out a patch in the past to get rid of them [1], but Shaoqin is
> currently making an effort to (fix and) use them in their tests [2].
> While we are still reviewing the series, we can apply [1] to unblock
> enabling -Werror and Shaqoqin can re-introduce the functions as
> needed. But, it's your call.
Thanks for the brief, now I remember :) Agreed, let's just delete these
upstream. These accessors are simple anyway, and easy to re-review in
Shaoqin's tests.
Sean -- I'm going to pick up [1] and throw it on the branch with your
cleanup.
--
Thanks,
Oliver
On Wed, Feb 28, 2024 at 01:19:48PM -0800, Sean Christopherson wrote:
> @@ -386,6 +386,7 @@ static struct kvm_vcpu *__aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
> aarch64_vcpu_setup(vcpu, init);
>
> vcpu_set_reg(vcpu, ARM64_CORE_REG(sp_el1), stack_vaddr + stack_size);
> + return vcpu;
> }
>
> struct kvm_vcpu *aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
>
> I'll squash the above and force push.
Confirmed that this fixes the originally reported issue.
Oliver and/or Marc, question for y'all towards the bottom.
On Wed, Feb 28, 2024, Sean Christopherson wrote:
> On Wed, Feb 28, 2024, Mark Brown wrote:
> > On Thu, Feb 08, 2024 at 09:48:39PM +0100, Thomas Huth wrote:
> > > From: Sean Christopherson <[email protected]>
> > >
> > > Extract the code to set a vCPU's entry point out of vm_arch_vcpu_add() and
> > > into a new API, vcpu_arch_set_entry_point(). Providing a separate API
> > > will allow creating a KVM selftests hardness that can handle tests that
> > > use different entry points for sub-tests, whereas *requiring* the entry
> > > point to be specified at vCPU creation makes it difficult to create a
> > > generic harness, e.g. the boilerplate setup/teardown can't easily create
> > > and destroy the VM and vCPUs.
> >
> > With today's -next I'm seeing most of the KVM selftests failing on an
> > arm64 defconfig with:
> >
> > # ==== Test Assertion Failure ====
> > # include/kvm_util_base.h:677: !ret
> > # pid=735 tid=735 errno=9 - Bad file descriptor
> > # 1 0x0000000000410937: vcpu_set_reg at kvm_util_base.h:677 (discriminator 4)
> > # 2 (inlined by) vcpu_arch_set_entry_point at processor.c:370 (discriminator 4)
> > # 3 0x0000000000407bab: vm_vcpu_add at kvm_util_base.h:981
> > # 4 (inlined by) __vm_create_with_vcpus at kvm_util.c:419
> > # 5 (inlined by) __vm_create_shape_with_one_vcpu at kvm_util.c:432
> > # 6 0x000000000040187b: __vm_create_with_one_vcpu at kvm_util_baseh:892
> > # 7 (inlined by) vm_create_with_one_vcpu at kvm_util_base.h:899
> > # 8 (inlined by) main at aarch32_id_regs.c:158
> > # 9 0x0000007fbcbe6dc3: ?? ??:0
> > # 10 0x0000007fbcbe6e97: ?? ??:0
> > # 11 0x0000000000401f2f: _start at ??:?
> > # KVM_SET_ONE_REG failed, rc: -1 errno: 9 (Bad file descriptor)
> >
> > and a bisect pointed to this commit which does look plausibly relevant.
> >
> > Note that while this was bisected with plain arm64 defconfig and the KVM
> > selftests fragment was not enabled, but enabling the KVM fragment gave
> > the same result as would be expected based on the options enabled by the
> > fragment. We're also seeing an alternative failure pattern where the
> > tests segfault when run in a different environment, I'm also tracking
> > that down but I suspect these are the same issue.
>
> Gah, my bad, I should have at least tested on ARM since I have easy access to
> such hardware. If I can't figure out what's going wrong in the next few hours,
> I'll drop this series and we can try again for 6.10.
>
> Sorry :-/
/facepalm
The inner helper doesn't return the vCPU, and by dumb (bad) luck, selftests end
up trying to use fd=0.
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index ed4ab29f4fad..a9eb17295be4 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -386,6 +386,7 @@ static struct kvm_vcpu *__aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
aarch64_vcpu_setup(vcpu, init);
vcpu_set_reg(vcpu, ARM64_CORE_REG(sp_el1), stack_vaddr + stack_size);
+ return vcpu;
}
struct kvm_vcpu *aarch64_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
I'll squash the above and force push.
In my defense, I would have caught this when build-testing, as the compiler does
warn...
lib/aarch64/processor.c -o /usr/local/google/home/seanjc/go/src/kernel.org/nox/tools/testing/selftests/kvm/lib/aarch64/processor.o
lib/aarch64/processor.c: In function ‘__aarch64_vcpu_add’:
lib/aarch64/processor.c:389:1: warning: no return statement in function returning non-void [-Wreturn-type]
389 | }
| ^
At top level:
cc1: note: unrecognized command-line option ‘-Wno-gnu-variable-sized-type-not-at-end’ may have been intended to silence earlier diagnostics
but due to a different issue that is fixed in the kvm-arm tree[*], but not in mine,
I built without -Werror and didn't see the new warn in the sea of GUEST_PRINTF
warnings.
Ugh, and I still can't enable -Werror, because there are unused functions in
aarch64/vpmu_counter_access.c
aarch64/vpmu_counter_access.c:96:20: error: unused function 'enable_counter' [-Werror,-Wunused-function]
static inline void enable_counter(int idx)
^
aarch64/vpmu_counter_access.c:104:20: error: unused function 'disable_counter' [-Werror,-Wunused-function]
static inline void disable_counter(int idx)
^
2 errors generated.
make: *** [Makefile:278: /usr/local/google/home/seanjc/go/src/kernel.org/nox/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.o] Error 1
make: *** Waiting for unfinished jobs....
Commit 49f31cff9c533d264659356b90445023b04e10fb failed to build with 'make-clang make-arm make -j128'.
Oliver/Marc, any thoughts on how you want to fix the unused function warnings?
As evidenced by this goof, being able to compile with -Werror is super helpful.
And another question: is there any reason to not force -Werror for selftests?
[*] https://lore.kernel.org/all/[email protected]