Paolo Bonzini <pbonzini@xxxxxxxxxx> 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
