* [PATCH 0/2] KVM: x86: hyper-v: Fix TSC page update after KVM_SET_CLOCK(0) call
@ 2021-03-26 15:55 Vitaly Kuznetsov
2021-03-26 15:55 ` [PATCH 1/2] KVM: x86: hyper-v: Forbid unsigned hv_clock->system_time to go negative after KVM_REQ_CLOCK_UPDATE Vitaly Kuznetsov
2021-03-26 15:55 ` [PATCH 2/2] selftests: kvm: Check that TSC page value is small after KVM_SET_CLOCK(0) Vitaly Kuznetsov
0 siblings, 2 replies; 5+ messages in thread
From: Vitaly Kuznetsov @ 2021-03-26 15:55 UTC (permalink / raw)
To: kvm, Paolo Bonzini
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Marcelo Tosatti
I discovered that after KVM_SET_CLOCK(0) TSC page value in the guest can
go through the roof and apparently we have a signedness issue when the
update is performed. Fix the issue and add a selftest.
Vitaly Kuznetsov (2):
KVM: x86: hyper-v: Forbid unsigned hv_clock->system_time to go
negative after KVM_REQ_CLOCK_UPDATE
selftests: kvm: Check that TSC page value is small after
KVM_SET_CLOCK(0)
arch/x86/kvm/x86.c | 10 +++++++++-
tools/testing/selftests/kvm/x86_64/hyperv_clock.c | 13 +++++++++++--
2 files changed, 20 insertions(+), 3 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] KVM: x86: hyper-v: Forbid unsigned hv_clock->system_time to go negative after KVM_REQ_CLOCK_UPDATE
2021-03-26 15:55 [PATCH 0/2] KVM: x86: hyper-v: Fix TSC page update after KVM_SET_CLOCK(0) call Vitaly Kuznetsov
@ 2021-03-26 15:55 ` Vitaly Kuznetsov
2021-03-26 16:54 ` Paolo Bonzini
2021-03-26 15:55 ` [PATCH 2/2] selftests: kvm: Check that TSC page value is small after KVM_SET_CLOCK(0) Vitaly Kuznetsov
1 sibling, 1 reply; 5+ messages in thread
From: Vitaly Kuznetsov @ 2021-03-26 15:55 UTC (permalink / raw)
To: kvm, Paolo Bonzini
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Marcelo Tosatti
When guest time is reset with KVM_SET_CLOCK(0), it is possible for
hv_clock->system_time to become a small negative number. This happens
because in KVM_SET_CLOCK handling we set kvm->arch.kvmclock_offset based
on get_kvmclock_ns(kvm) but when KVM_REQ_CLOCK_UPDATE is handled,
kvm_guest_time_update() does
hv_clock.system_time = ka->master_kernel_ns + v->kvm->arch.kvmclock_offset;
And 'master_kernel_ns' represents the last time when masterclock
got updated, it can precede KVM_SET_CLOCK() call. Normally, this is not a
problem, the difference is very small, e.g. I'm observing
hv_clock.system_time = -70 ns. The issue comes from the fact that
'hv_clock.system_time' is stored as unsigned and 'system_time / 100' in
compute_tsc_page_parameters() becomes a very big number.
Forbid 'hv_clock.system_time' to go negative in kvm_guest_time_update().
A similar computation in get_kvmclock_ns() seems fine and doesn't require
the quirk.
Alternatively, we could've used 'master_kernel_ns' when computing
'arch.kvmclock_offset' but that would reduce the precision for normal
cases a bit. Another solution is to cast 'hv_clock.system_time' to
's64' in compute_tsc_page_parameters() but it seems we also use
'hv_clock.system_time' in trace_kvm_pvclock_update() as unsigned.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
arch/x86/kvm/x86.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fe806e894212..320da7912375 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2742,7 +2742,15 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
}
vcpu->hv_clock.tsc_timestamp = tsc_timestamp;
- vcpu->hv_clock.system_time = kernel_ns + v->kvm->arch.kvmclock_offset;
+
+ /*
+ * 'kvmclock_offset' can be negative and its absolute value can be
+ * slightly greater than 'kernel_ns' because when KVM_SET_CLOCK is
+ * handled, we use more precise get_kvmclock_ns() there. Make sure
+ * unsigned 'system_time' doesn't go negative.
+ */
+ vcpu->hv_clock.system_time = max(kernel_ns + v->kvm->arch.kvmclock_offset,
+ (s64)0);
vcpu->last_guest_tsc = tsc_timestamp;
/* If the host uses TSC clocksource, then it is stable */
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] selftests: kvm: Check that TSC page value is small after KVM_SET_CLOCK(0)
2021-03-26 15:55 [PATCH 0/2] KVM: x86: hyper-v: Fix TSC page update after KVM_SET_CLOCK(0) call Vitaly Kuznetsov
2021-03-26 15:55 ` [PATCH 1/2] KVM: x86: hyper-v: Forbid unsigned hv_clock->system_time to go negative after KVM_REQ_CLOCK_UPDATE Vitaly Kuznetsov
@ 2021-03-26 15:55 ` Vitaly Kuznetsov
1 sibling, 0 replies; 5+ messages in thread
From: Vitaly Kuznetsov @ 2021-03-26 15:55 UTC (permalink / raw)
To: kvm, Paolo Bonzini
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Marcelo Tosatti
Add a test for the issue when KVM_SET_CLOCK(0) call could cause
TSC page value to go very big because of a signedness issue around
hv_clock->system_time.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
tools/testing/selftests/kvm/x86_64/hyperv_clock.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_clock.c b/tools/testing/selftests/kvm/x86_64/hyperv_clock.c
index ffbc4555c6e2..7f1d2765572c 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_clock.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_clock.c
@@ -80,19 +80,24 @@ static inline void check_tsc_msr_rdtsc(void)
GUEST_ASSERT(delta_ns * 100 < (t2 - t1) * 100);
}
+static inline u64 get_tscpage_ts(struct ms_hyperv_tsc_page *tsc_page)
+{
+ return mul_u64_u64_shr64(rdtsc(), tsc_page->tsc_scale) + tsc_page->tsc_offset;
+}
+
static inline void check_tsc_msr_tsc_page(struct ms_hyperv_tsc_page *tsc_page)
{
u64 r1, r2, t1, t2;
/* Compare TSC page clocksource with HV_X64_MSR_TIME_REF_COUNT */
- t1 = mul_u64_u64_shr64(rdtsc(), tsc_page->tsc_scale) + tsc_page->tsc_offset;
+ t1 = get_tscpage_ts(tsc_page);
r1 = rdmsr(HV_X64_MSR_TIME_REF_COUNT);
/* 10 ms tolerance */
GUEST_ASSERT(r1 >= t1 && r1 - t1 < 100000);
nop_loop();
- t2 = mul_u64_u64_shr64(rdtsc(), tsc_page->tsc_scale) + tsc_page->tsc_offset;
+ t2 = get_tscpage_ts(tsc_page);
r2 = rdmsr(HV_X64_MSR_TIME_REF_COUNT);
GUEST_ASSERT(r2 >= t1 && r2 - t2 < 100000);
}
@@ -130,7 +135,11 @@ static void guest_main(struct ms_hyperv_tsc_page *tsc_page, vm_paddr_t tsc_page_
tsc_offset = tsc_page->tsc_offset;
/* Call KVM_SET_CLOCK from userspace, check that TSC page was updated */
+
GUEST_SYNC(7);
+ /* Sanity check TSC page timestamp, it should be close to 0 */
+ GUEST_ASSERT(get_tscpage_ts(tsc_page) < 100000);
+
GUEST_ASSERT(tsc_page->tsc_offset != tsc_offset);
nop_loop();
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] KVM: x86: hyper-v: Forbid unsigned hv_clock->system_time to go negative after KVM_REQ_CLOCK_UPDATE
2021-03-26 15:55 ` [PATCH 1/2] KVM: x86: hyper-v: Forbid unsigned hv_clock->system_time to go negative after KVM_REQ_CLOCK_UPDATE Vitaly Kuznetsov
@ 2021-03-26 16:54 ` Paolo Bonzini
2021-03-29 8:58 ` Vitaly Kuznetsov
0 siblings, 1 reply; 5+ messages in thread
From: Paolo Bonzini @ 2021-03-26 16:54 UTC (permalink / raw)
To: Vitaly Kuznetsov, kvm
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Marcelo Tosatti
On 26/03/21 16:55, Vitaly Kuznetsov wrote:
> Another solution is to cast 'hv_clock.system_time' to
> 's64' in compute_tsc_page_parameters() but it seems we also use
> 'hv_clock.system_time' in trace_kvm_pvclock_update() as unsigned.
I think that is better. There is no reason really to clamp the value to
to 0, while we know already that tsc_ref->tsc_offset can be either
positive or negative. So treating hv_clock->system_time as signed
before the division would make sense.
It should be just
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 58fa8c029867..e573e987f41b 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1070,9 +1070,7 @@ static bool compute_tsc_page_parameters(struct pvclock_vcpu_time_info *hv_clock,
hv_clock->tsc_to_system_mul,
100);
- tsc_ref->tsc_offset = hv_clock->system_time;
- do_div(tsc_ref->tsc_offset, 100);
- tsc_ref->tsc_offset -=
+ tsc_ref->tsc_offset = div_s64(hv_clock->system_time, 100) -
mul_u64_u64_shr(hv_clock->tsc_timestamp, tsc_ref->tsc_scale, 64);
return true;
}
right? The test passes for me with this change.
Paolo
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] KVM: x86: hyper-v: Forbid unsigned hv_clock->system_time to go negative after KVM_REQ_CLOCK_UPDATE
2021-03-26 16:54 ` Paolo Bonzini
@ 2021-03-29 8:58 ` Vitaly Kuznetsov
0 siblings, 0 replies; 5+ messages in thread
From: Vitaly Kuznetsov @ 2021-03-29 8:58 UTC (permalink / raw)
To: Paolo Bonzini, kvm
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Marcelo Tosatti
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 26/03/21 16:55, Vitaly Kuznetsov wrote:
>> Another solution is to cast 'hv_clock.system_time' to
>> 's64' in compute_tsc_page_parameters() but it seems we also use
>> 'hv_clock.system_time' in trace_kvm_pvclock_update() as unsigned.
>
> I think that is better. There is no reason really to clamp the value to
> to 0, while we know already that tsc_ref->tsc_offset can be either
> positive or negative. So treating hv_clock->system_time as signed
> before the division would make sense.
>
> It should be just
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 58fa8c029867..e573e987f41b 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1070,9 +1070,7 @@ static bool compute_tsc_page_parameters(struct pvclock_vcpu_time_info *hv_clock,
> hv_clock->tsc_to_system_mul,
> 100);
>
> - tsc_ref->tsc_offset = hv_clock->system_time;
> - do_div(tsc_ref->tsc_offset, 100);
> - tsc_ref->tsc_offset -=
> + tsc_ref->tsc_offset = div_s64(hv_clock->system_time, 100) -
> mul_u64_u64_shr(hv_clock->tsc_timestamp, tsc_ref->tsc_scale, 64);
> return true;
> }
>
> right? The test passes for me with this change.
Right,
in fact that's how v0 (which I've never sent out) of the patch looked
like but then I relalized that the fact that unsigned
'hv_clock->system_time' can sometimes keep a negative value is a
'gotcha' which may cause issues in the future.
I'll re-test and send v2, thanks!
--
Vitaly
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-03-29 9:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-26 15:55 [PATCH 0/2] KVM: x86: hyper-v: Fix TSC page update after KVM_SET_CLOCK(0) call Vitaly Kuznetsov
2021-03-26 15:55 ` [PATCH 1/2] KVM: x86: hyper-v: Forbid unsigned hv_clock->system_time to go negative after KVM_REQ_CLOCK_UPDATE Vitaly Kuznetsov
2021-03-26 16:54 ` Paolo Bonzini
2021-03-29 8:58 ` Vitaly Kuznetsov
2021-03-26 15:55 ` [PATCH 2/2] selftests: kvm: Check that TSC page value is small after KVM_SET_CLOCK(0) Vitaly Kuznetsov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).