Sean Christopherson <seanjc@xxxxxxxxxx> writes: > On Mon, Mar 29, 2021, Vitaly Kuznetsov wrote: >> 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. >> >> Use div_s64() to get the proper result when dividing maybe-negative >> 'hv_clock.system_time' by 100. >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> >> --- >> arch/x86/kvm/hyperv.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c >> index f98370a39936..0529b892f634 100644 >> --- a/arch/x86/kvm/hyperv.c >> +++ b/arch/x86/kvm/hyperv.c >> @@ -1070,10 +1070,13 @@ 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 -= >> + /* >> + * Note: 'hv_clock->system_time' despite being 'u64' can hold a negative >> + * value here, thus div_s64(). >> + */ > > Will anything break if hv_clock.system_time is made a s64? > I think no, but pvclock-abi.h says: " * These structs MUST NOT be changed. * They are the ABI between hypervisor and guest OS. * Both Xen and KVM are using this. " so I was kind of scared to suggest a change... -- Vitaly
