Re: [PATCH v2 1/2] KVM: x86: hyper-v: Properly divide maybe-negative 'hv_clock->system_time' in compute_tsc_page_parameters()

[Marcelo Tosatti <mtosatti@redhat.com>]

On Mon, Mar 29, 2021 at 05:24:13PM +0000, Sean Christopherson wrote:
> 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@redhat.com>
> > ---
> >  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?

IMHO hv_clock.system_time represents an unsigned value:

        system_time:
                a host notion of monotonic time, including sleep
                time at the time this structure was last updated. Unit is
                nanoseconds.


Delta between values is not transmitted through this variable, 
so unclear what negative values would mean.