| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZjPrlLNNGNh2mOmW@google.com>
Date: Thu, 2 May 2024 12:37:56 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Thomas Huth <thuth@...hat.com>
Cc: kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
Shuah Khan <shuah@...nel.org>, Muhammad Usama Anjum <usama.anjum@...labora.com>
Subject: Re: [PATCH v2] KVM: selftests: Use TAP interface in the
set_memory_region test
On Fri, Apr 26, 2024, Thomas Huth wrote:
> 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. (Note: We are not using the KVM_ONE_VCPU_TEST_SUITE()
> macro here since these tests are creating their VMs with the
> vm_create_barebones() function, not with vm_create_with_one_vcpu())
>
> Reviewed-by: Andrew Jones <ajones@...tanamicro.com>
> Signed-off-by: Thomas Huth <thuth@...hat.com>
> ---
> v2:
> - Rebase to linux-next branch
> - Make "loops" variable static
> - Added Andrew's Reviewed-by
>
> .../selftests/kvm/set_memory_region_test.c | 86 +++++++++----------
> 1 file changed, 42 insertions(+), 44 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
> index 68c899d27561..a5c9bee5235a 100644
> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c
> @@ -16,6 +16,7 @@
> #include <test_util.h>
> #include <kvm_util.h>
> #include <processor.h>
> +#include "kselftest_harness.h"
>
> /*
> * s390x needs at least 1MB alignment, and the x86_64 MOVE/DELETE tests need a
> @@ -38,6 +39,8 @@ extern const uint64_t final_rip_end;
>
> static sem_t vcpu_ready;
>
> +static int loops;
..
> -static void test_add_overlapping_private_memory_regions(void)
> +TEST(add_overlapping_private_memory_regions)
> {
> struct kvm_vm *vm;
> int memfd;
> int r;
>
> - pr_info("Testing ADD of overlapping KVM_MEM_GUEST_MEMFD memory regions\n");
> + if (!has_cap_guest_memfd())
> + SKIP(return, "Missing KVM_MEM_GUEST_MEMFD / KVM_X86_SW_PROTECTED_VM");
I like that we can actually report sub-tests as being skipped, but I don't like
having multiple ways to express requirements. And IMO, this is much less readable
than TEST_REQUIRE(has_cap_guest_memfd());
AIUI, each test runs in a child process, so TEST_REQUIRE() can simply exit(), it
just needs to avoid ksft_exit_skip() so that a sub-test doesn't spit out the full
test summary.
And if using exit() isn't an option, setjmp()+longjmp() will do the trick (I got
that working for KVM_ONE_VCPU_TEST() before I realized tests run as a child).
The below is lightly tested, but I think it does what we want?
I also think we would effectively forbid direct use of TEST(). Partly because
it's effectively necessary to use TEST_REQUIRE(), but also so that all tests will
have an existing single point of contact if we need/want to make similar changes
in the future.
Lastly, would using a fixture allow throwing "loops" into a structure that is
passed to each sub-test? Having the global is obviously not a big deal, but it'd
be nice if the early conversions to the TAP-friendly framework demonstrate the
"right" way to do things, because they'll inevitably become the blueprint for all
future conversions.
---
From: Sean Christopherson <seanjc@...gle.com>
Date: Thu, 2 May 2024 12:32:04 -0700
Subject: [PATCH] KVM: selftests: Allow using TEST_REQUIRE in kselftest harness
testcases
TODO: write me
Signed-off-by: Sean Christopherson <seanjc@...gle.com>
---
.../selftests/kvm/include/kvm_test_harness.h | 4 ++++
.../testing/selftests/kvm/include/test_util.h | 24 +++++++++++++++----
tools/testing/selftests/kvm/lib/kvm_util.c | 2 ++
.../selftests/kvm/x86_64/vmx_pmu_caps_test.c | 3 +--
4 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/kvm_test_harness.h b/tools/testing/selftests/kvm/include/kvm_test_harness.h
index 8f7c6858e8e2..eda1c08c7c2b 100644
--- a/tools/testing/selftests/kvm/include/kvm_test_harness.h
+++ b/tools/testing/selftests/kvm/include/kvm_test_harness.h
@@ -9,6 +9,7 @@
#define SELFTEST_KVM_TEST_HARNESS_H
#include "kselftest_harness.h"
+#include "test_util.h"
#define KVM_ONE_VCPU_TEST_SUITE(name) \
FIXTURE(name) { \
@@ -29,7 +30,10 @@ static void __suite##_##test(struct kvm_vcpu *vcpu); \
TEST_F(suite, test) \
{ \
vcpu_arch_set_entry_point(self->vcpu, guestcode); \
+ \
+ kvm_is_sub_test = true; \
__suite##_##test(self->vcpu); \
+ kvm_is_sub_test = NULL; \
} \
static void __suite##_##test(struct kvm_vcpu *vcpu)
diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
index 3e473058849f..64c9f128fef4 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -36,10 +36,26 @@ static inline int _no_printf(const char *format, ...) { return 0; }
#endif
void __printf(1, 2) print_skip(const char *fmt, ...);
-#define __TEST_REQUIRE(f, fmt, ...) \
-do { \
- if (!(f)) \
- ksft_exit_skip("- " fmt "\n", ##__VA_ARGS__); \
+
+extern bool kvm_is_sub_test;
+
+/*
+ * Skip the test if a required capability/feature/whatever is not available,
+ * e.g. due to lack of support in the underlying hardware, running against an
+ * older kernel/KVM, etc. Use ksft_test_result_skip() for sub-tests to avoid
+ * spuriously printing the summary of the entire test suite. Note, sub-tests
+ * run in a child process, and so can exit() directly, e.g. don't need to
+ * longjmp() out or do something similar to avoid killing the test as a whole.
+ */
+#define __TEST_REQUIRE(f, fmt, ...) \
+do { \
+ if (!(f)) { \
+ if (kvm_is_sub_test) { \
+ ksft_test_result_skip("- " fmt "\n", ##__VA_ARGS__); \
+ exit(KSFT_SKIP); \
+ } \
+ ksft_exit_skip("- " fmt "\n", ##__VA_ARGS__); \
+ } \
} while (0)
#define TEST_REQUIRE(f) __TEST_REQUIRE(f, "Requirement not met: %s", #f)
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 6b2158655baa..4b24c454fd33 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -19,6 +19,8 @@
#define KVM_UTIL_MIN_PFN 2
+bool kvm_is_sub_test;
+
uint32_t guest_random_seed;
struct guest_random_state guest_rng;
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 7c92536551cc..a58e0b1c2ee5 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
@@ -195,8 +195,7 @@ KVM_ONE_VCPU_TEST(vmx_pmu_caps, lbr_perf_capabilities, guest_code)
{
int r;
- if (!host_cap.lbr_format)
- return;
+ TEST_REQUIRE(host_cap.lbr_format);
vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.capabilities);
vcpu_set_msr(vcpu, MSR_LBR_TOS, 7);
base-commit: 2489e6c9ebb57d6d0e98936479b5f586201379c7
--
Powered by blists - more mailing lists