It was discovered that 'hyperv_clock' fails miserably when the system is
using an unsupported (by KVM) clocksource, e.g. 'kvm-clock'. The root cause
of the failure is that 'hyperv_clock' doesn't actually check which clocksource
is currently in use. Other tests (kvm_clock_test, vmx_nested_tsc_scaling_test)
have the required check but each test does it on its own.
Generalize clocksource checking infrastructure, make all three clocksource
dependent tests run with 'tsc' and 'hyperv_clocksource_tsc_page', and skip
gracefully when run in an unsupported configuration.
The last patch of the series is a loosely related minor nitpick for KVM
code itself.
Vitaly Kuznetsov (5):
KVM: selftests: Generalize check_clocksource() from kvm_clock_test
KVM: selftests: Use generic sys_clocksource_is_tsc() in
vmx_nested_tsc_scaling_test
KVM: selftests: Run clocksource dependent tests with
hyperv_clocksource_tsc_page too
KVM: selftests: Make hyperv_clock require TSC based system clocksource
KVM: x86: Make gtod_is_based_on_tsc() return 'bool'
arch/x86/kvm/x86.c | 2 +-
.../testing/selftests/kvm/include/test_util.h | 2 +
.../selftests/kvm/include/x86_64/processor.h | 2 +
tools/testing/selftests/kvm/lib/test_util.c | 25 ++++++++++++
.../selftests/kvm/lib/x86_64/processor.c | 15 ++++++++
.../selftests/kvm/x86_64/hyperv_clock.c | 1 +
.../selftests/kvm/x86_64/kvm_clock_test.c | 38 +------------------
.../kvm/x86_64/vmx_nested_tsc_scaling_test.c | 19 +---------
8 files changed, 48 insertions(+), 56 deletions(-)
base-commit: 7f26fea9bc085290e3731501f4f8fc5b82b9d615
--
2.43.0
Despite its name, system_has_stable_tsc() just checks that system
clocksource is 'tsc'; this can now be done with generic
sys_clocksource_is_tsc().
No functional change intended.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
.../kvm/x86_64/vmx_nested_tsc_scaling_test.c | 19 +------------------
1 file changed, 1 insertion(+), 18 deletions(-)
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 e710b6e7fb38..93b0a850a240 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
@@ -116,23 +116,6 @@ static void l1_guest_code(struct vmx_pages *vmx_pages)
GUEST_DONE();
}
-static bool system_has_stable_tsc(void)
-{
- bool tsc_is_stable;
- FILE *fp;
- char buf[4];
-
- fp = fopen("/sys/devices/system/clocksource/clocksource0/current_clocksource", "r");
- if (fp == NULL)
- return false;
-
- tsc_is_stable = fgets(buf, sizeof(buf), fp) &&
- !strncmp(buf, "tsc", sizeof(buf));
-
- fclose(fp);
- return tsc_is_stable;
-}
-
int main(int argc, char *argv[])
{
struct kvm_vcpu *vcpu;
@@ -148,7 +131,7 @@ int main(int argc, char *argv[])
TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_VMX));
TEST_REQUIRE(kvm_has_cap(KVM_CAP_TSC_CONTROL));
- TEST_REQUIRE(system_has_stable_tsc());
+ TEST_REQUIRE(sys_clocksource_is_tsc());
/*
* We set L1's scale factor to be a random number from 2 to 10.
--
2.43.0
KVM's 'gtod_is_based_on_tsc()' recognizes two clocksources: 'tsc' and
'hyperv_clocksource_tsc_page' and enables kvmclock in 'masterclock'
mode when either is in use. Transform 'sys_clocksource_is_tsc()' into
'sys_clocksource_is_based_on_tsc()' to support the later. This affects
two tests: kvm_clock_test and vmx_nested_tsc_scaling_test, both seem
to work well when system clocksource is 'hyperv_clocksource_tsc_page'.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
tools/testing/selftests/kvm/include/x86_64/processor.h | 2 +-
tools/testing/selftests/kvm/lib/x86_64/processor.c | 4 +++-
tools/testing/selftests/kvm/x86_64/kvm_clock_test.c | 2 +-
.../selftests/kvm/x86_64/vmx_nested_tsc_scaling_test.c | 2 +-
4 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 01eec72e0d3e..5bca8c947c82 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -1271,6 +1271,6 @@ void virt_map_level(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
#define PFERR_GUEST_PAGE_MASK BIT_ULL(PFERR_GUEST_PAGE_BIT)
#define PFERR_IMPLICIT_ACCESS BIT_ULL(PFERR_IMPLICIT_ACCESS_BIT)
-bool sys_clocksource_is_tsc(void);
+bool sys_clocksource_is_based_on_tsc(void);
#endif /* SELFTEST_KVM_PROCESSOR_H */
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index b0c64667803d..70969d374e5b 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1300,13 +1300,15 @@ void kvm_selftest_arch_init(void)
host_cpu_is_amd = this_cpu_is_amd();
}
-bool sys_clocksource_is_tsc(void)
+bool sys_clocksource_is_based_on_tsc(void)
{
char *clk_name = sys_get_cur_clocksource();
bool ret = false;
if (!strcmp(clk_name, "tsc\n"))
ret = true;
+ else if (!strcmp(clk_name, "hyperv_clocksource_tsc_page\n"))
+ ret = true;
free(clk_name);
diff --git a/tools/testing/selftests/kvm/x86_64/kvm_clock_test.c b/tools/testing/selftests/kvm/x86_64/kvm_clock_test.c
index 9deee8556b5c..eb1c12fb7656 100644
--- a/tools/testing/selftests/kvm/x86_64/kvm_clock_test.c
+++ b/tools/testing/selftests/kvm/x86_64/kvm_clock_test.c
@@ -143,7 +143,7 @@ int main(void)
flags = kvm_check_cap(KVM_CAP_ADJUST_CLOCK);
TEST_REQUIRE(flags & KVM_CLOCK_REALTIME);
- TEST_REQUIRE(sys_clocksource_is_tsc());
+ TEST_REQUIRE(sys_clocksource_is_based_on_tsc());
vm = vm_create_with_one_vcpu(&vcpu, guest_main);
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 93b0a850a240..1759fa5cb3f2 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
@@ -131,7 +131,7 @@ int main(int argc, char *argv[])
TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_VMX));
TEST_REQUIRE(kvm_has_cap(KVM_CAP_TSC_CONTROL));
- TEST_REQUIRE(sys_clocksource_is_tsc());
+ TEST_REQUIRE(sys_clocksource_is_based_on_tsc());
/*
* We set L1's scale factor to be a random number from 2 to 10.
--
2.43.0
Several existing x86 selftests need to check that the underlying system
clocksource is TSC or based on TSC but every test implements its own
check. As a first step towards unification, extract check_clocksource()
from kvm_clock_test and split it into two functions: arch-neutral
'sys_get_cur_clocksource()' and x86-specific 'sys_clocksource_is_tsc()'.
Fix a couple of pre-existing issues in kvm_clock_test: memory leakage in
check_clocksource() and using TEST_ASSERT() instead of TEST_REQUIRE().
The change also makes the test fail when system clocksource can't be read
from sysfs.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
.../testing/selftests/kvm/include/test_util.h | 2 +
.../selftests/kvm/include/x86_64/processor.h | 2 +
tools/testing/selftests/kvm/lib/test_util.c | 25 ++++++++++++
.../selftests/kvm/lib/x86_64/processor.c | 13 +++++++
.../selftests/kvm/x86_64/kvm_clock_test.c | 38 +------------------
5 files changed, 43 insertions(+), 37 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
index 71a41fa924b7..50a5e31ba8da 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -195,4 +195,6 @@ __printf(3, 4) int guest_snprintf(char *buf, int n, const char *fmt, ...);
char *strdup_printf(const char *fmt, ...) __attribute__((format(printf, 1, 2), nonnull(1)));
+char *sys_get_cur_clocksource(void);
+
#endif /* SELFTEST_KVM_TEST_UTIL_H */
diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index a84863503fcb..01eec72e0d3e 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -1271,4 +1271,6 @@ void virt_map_level(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
#define PFERR_GUEST_PAGE_MASK BIT_ULL(PFERR_GUEST_PAGE_BIT)
#define PFERR_IMPLICIT_ACCESS BIT_ULL(PFERR_IMPLICIT_ACCESS_BIT)
+bool sys_clocksource_is_tsc(void);
+
#endif /* SELFTEST_KVM_PROCESSOR_H */
diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
index 5d7f28b02d73..b8cb20cf61e9 100644
--- a/tools/testing/selftests/kvm/lib/test_util.c
+++ b/tools/testing/selftests/kvm/lib/test_util.c
@@ -392,3 +392,28 @@ char *strdup_printf(const char *fmt, ...)
return str;
}
+
+#define CLOCKSOURCE_PATH "/sys/devices/system/clocksource/clocksource0/current_clocksource"
+
+char *sys_get_cur_clocksource(void)
+{
+ char *clk_name;
+ struct stat st;
+ FILE *fp;
+
+ fp = fopen(CLOCKSOURCE_PATH, "r");
+ TEST_ASSERT(fp, "failed to open clocksource file, errno: %d", errno);
+
+ TEST_ASSERT(!fstat(fileno(fp), &st), "failed to stat clocksource file, errno: %d",
+ errno);
+
+ clk_name = malloc(st.st_size);
+ TEST_ASSERT(clk_name, "failed to allocate buffer to read file\n");
+
+ TEST_ASSERT(fgets(clk_name, st.st_size, fp), "failed to read clocksource file: %d",
+ ferror(fp));
+
+ fclose(fp);
+
+ return clk_name;
+}
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index d8288374078e..b0c64667803d 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1299,3 +1299,16 @@ void kvm_selftest_arch_init(void)
host_cpu_is_intel = this_cpu_is_intel();
host_cpu_is_amd = this_cpu_is_amd();
}
+
+bool sys_clocksource_is_tsc(void)
+{
+ char *clk_name = sys_get_cur_clocksource();
+ bool ret = false;
+
+ if (!strcmp(clk_name, "tsc\n"))
+ ret = true;
+
+ free(clk_name);
+
+ return ret;
+}
diff --git a/tools/testing/selftests/kvm/x86_64/kvm_clock_test.c b/tools/testing/selftests/kvm/x86_64/kvm_clock_test.c
index 1778704360a6..9deee8556b5c 100644
--- a/tools/testing/selftests/kvm/x86_64/kvm_clock_test.c
+++ b/tools/testing/selftests/kvm/x86_64/kvm_clock_test.c
@@ -132,42 +132,6 @@ static void enter_guest(struct kvm_vcpu *vcpu)
}
}
-#define CLOCKSOURCE_PATH "/sys/devices/system/clocksource/clocksource0/current_clocksource"
-
-static void check_clocksource(void)
-{
- char *clk_name;
- struct stat st;
- FILE *fp;
-
- fp = fopen(CLOCKSOURCE_PATH, "r");
- if (!fp) {
- pr_info("failed to open clocksource file: %d; assuming TSC.\n",
- errno);
- return;
- }
-
- if (fstat(fileno(fp), &st)) {
- pr_info("failed to stat clocksource file: %d; assuming TSC.\n",
- errno);
- goto out;
- }
-
- clk_name = malloc(st.st_size);
- TEST_ASSERT(clk_name, "failed to allocate buffer to read file\n");
-
- if (!fgets(clk_name, st.st_size, fp)) {
- pr_info("failed to read clocksource file: %d; assuming TSC.\n",
- ferror(fp));
- goto out;
- }
-
- TEST_ASSERT(!strncmp(clk_name, "tsc\n", st.st_size),
- "clocksource not supported: %s", clk_name);
-out:
- fclose(fp);
-}
-
int main(void)
{
struct kvm_vcpu *vcpu;
@@ -179,7 +143,7 @@ int main(void)
flags = kvm_check_cap(KVM_CAP_ADJUST_CLOCK);
TEST_REQUIRE(flags & KVM_CLOCK_REALTIME);
- check_clocksource();
+ TEST_REQUIRE(sys_clocksource_is_tsc());
vm = vm_create_with_one_vcpu(&vcpu, guest_main);
--
2.43.0
KVM sets up Hyper-V TSC page clocksource for its guests when system
clocksource is 'based on TSC' (see gtod_is_based_on_tsc()), running
hyperv_clock with any other clocksource leads to imminent failure.
Add the missing requirement to make the test skip gracefully.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
tools/testing/selftests/kvm/x86_64/hyperv_clock.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_clock.c b/tools/testing/selftests/kvm/x86_64/hyperv_clock.c
index f5e1e98f04f9..02cc3c560566 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_clock.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_clock.c
@@ -212,6 +212,7 @@ int main(void)
int stage;
TEST_REQUIRE(kvm_has_cap(KVM_CAP_HYPERV_TIME));
+ TEST_REQUIRE(sys_clocksource_is_based_on_tsc());
vm = vm_create_with_one_vcpu(&vcpu, guest_main);
--
2.43.0
gtod_is_based_on_tsc() is boolean in nature, i.e. it returns '1' for good
clocksources and '0' otherwise. Moreover, its result is used raw by
kvm_get_time_and_clockread()/kvm_get_walltime_and_clockread() which are
'bool'.
No functional change intended.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
arch/x86/kvm/x86.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 27e23714e960..2aba11eb58ac 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2507,7 +2507,7 @@ static u64 compute_guest_tsc(struct kvm_vcpu *vcpu, s64 kernel_ns)
}
#ifdef CONFIG_X86_64
-static inline int gtod_is_based_on_tsc(int mode)
+static inline bool gtod_is_based_on_tsc(int mode)
{
return mode == VDSO_CLOCKMODE_TSC || mode == VDSO_CLOCKMODE_HVCLOCK;
}
--
2.43.0
Vitaly Kuznetsov <[email protected]> writes:
> It was discovered that 'hyperv_clock' fails miserably when the system is
> using an unsupported (by KVM) clocksource, e.g. 'kvm-clock'. The root cause
> of the failure is that 'hyperv_clock' doesn't actually check which clocksource
> is currently in use. Other tests (kvm_clock_test, vmx_nested_tsc_scaling_test)
> have the required check but each test does it on its own.
>
> Generalize clocksource checking infrastructure, make all three clocksource
> dependent tests run with 'tsc' and 'hyperv_clocksource_tsc_page', and skip
> gracefully when run in an unsupported configuration.
>
> The last patch of the series is a loosely related minor nitpick for KVM
> code itself.
>
> Vitaly Kuznetsov (5):
> KVM: selftests: Generalize check_clocksource() from kvm_clock_test
> KVM: selftests: Use generic sys_clocksource_is_tsc() in
> vmx_nested_tsc_scaling_test
> KVM: selftests: Run clocksource dependent tests with
> hyperv_clocksource_tsc_page too
> KVM: selftests: Make hyperv_clock require TSC based system clocksource
> KVM: x86: Make gtod_is_based_on_tsc() return 'bool'
Ping)
--
Vitaly
On Tue, Jan 09, 2024, Vitaly Kuznetsov wrote:
> +bool sys_clocksource_is_tsc(void)
> +{
> + char *clk_name = sys_get_cur_clocksource();
> + bool ret = false;
> +
> + if (!strcmp(clk_name, "tsc\n"))
> + ret = true;
This can more simply be:
bool ret = !strcmp(clk_name, "tsc\n");
and then
bool ret = !strcmp(clk_name, "tsc\n") ||
!strcmp(clk_name, "hyperv_clocksource_tsc_page\n");
when the Hyper-V variant comes along. I'll fixup when applying unless you
violently disagree.
On Tue, 09 Jan 2024 15:11:16 +0100, Vitaly Kuznetsov wrote:
> It was discovered that 'hyperv_clock' fails miserably when the system is
> using an unsupported (by KVM) clocksource, e.g. 'kvm-clock'. The root cause
> of the failure is that 'hyperv_clock' doesn't actually check which clocksource
> is currently in use. Other tests (kvm_clock_test, vmx_nested_tsc_scaling_test)
> have the required check but each test does it on its own.
>
> Generalize clocksource checking infrastructure, make all three clocksource
> dependent tests run with 'tsc' and 'hyperv_clocksource_tsc_page', and skip
> gracefully when run in an unsupported configuration.
>
> [...]
Applied to kvm-x86 selftests, thanks!
[1/5] KVM: selftests: Generalize check_clocksource() from kvm_clock_test
https://github.com/kvm-x86/linux/commit/449d0d6ccf55
[2/5] KVM: selftests: Use generic sys_clocksource_is_tsc() in vmx_nested_tsc_scaling_test
https://github.com/kvm-x86/linux/commit/a79036441a68
[3/5] KVM: selftests: Run clocksource dependent tests with hyperv_clocksource_tsc_page too
https://github.com/kvm-x86/linux/commit/436e6e541cb2
[4/5] KVM: selftests: Make hyperv_clock require TSC based system clocksource
https://github.com/kvm-x86/linux/commit/14fce852a14b
[5/5] KVM: x86: Make gtod_is_based_on_tsc() return 'bool'
https://github.com/kvm-x86/linux/commit/57cc53712934
--
https://github.com/kvm-x86/linux/tree/next
On Tue, Jan 30, 2024, Sean Christopherson wrote:
> On Tue, 09 Jan 2024 15:11:16 +0100, Vitaly Kuznetsov wrote:
> > It was discovered that 'hyperv_clock' fails miserably when the system is
> > using an unsupported (by KVM) clocksource, e.g. 'kvm-clock'. The root cause
> > of the failure is that 'hyperv_clock' doesn't actually check which clocksource
> > is currently in use. Other tests (kvm_clock_test, vmx_nested_tsc_scaling_test)
> > have the required check but each test does it on its own.
> >
> > Generalize clocksource checking infrastructure, make all three clocksource
> > dependent tests run with 'tsc' and 'hyperv_clocksource_tsc_page', and skip
> > gracefully when run in an unsupported configuration.
> >
> > [...]
>
> Applied to kvm-x86 selftests, thanks!
>
> [1/5] KVM: selftests: Generalize check_clocksource() from kvm_clock_test
> https://github.com/kvm-x86/linux/commit/449d0d6ccf55
> [2/5] KVM: selftests: Use generic sys_clocksource_is_tsc() in vmx_nested_tsc_scaling_test
> https://github.com/kvm-x86/linux/commit/a79036441a68
> [3/5] KVM: selftests: Run clocksource dependent tests with hyperv_clocksource_tsc_page too
> https://github.com/kvm-x86/linux/commit/436e6e541cb2
> [4/5] KVM: selftests: Make hyperv_clock require TSC based system clocksource
> https://github.com/kvm-x86/linux/commit/14fce852a14b
> [5/5] KVM: x86: Make gtod_is_based_on_tsc() return 'bool'
> https://github.com/kvm-x86/linux/commit/57cc53712934
FYI, I dropped the xen_shinfo patch, and past me wasn't clever enough to make sure
that patch was applied last. New hashes are:
[1/5] KVM: selftests: Generalize check_clocksource() from kvm_clock_test
https://github.com/kvm-x86/linux/commit/e440c5f2e3e6
[2/5] KVM: selftests: Use generic sys_clocksource_is_tsc() in vmx_nested_tsc_scaling_test
https://github.com/kvm-x86/linux/commit/410cb01ead5b
[3/5] KVM: selftests: Run clocksource dependent tests with hyperv_clocksource_tsc_page too
https://github.com/kvm-x86/linux/commit/09951bf2cbb3
[4/5] KVM: selftests: Make hyperv_clock require TSC based system clocksource
https://github.com/kvm-x86/linux/commit/b6831a108be1
[5/5] KVM: x86: Make gtod_is_based_on_tsc() return 'bool'
https://github.com/kvm-x86/linux/commit/9e62797fd7e8