Re: [PATCH 1/2] KVM: x86: hyper-v: Forbid unsigned hv_clock->system_time to go negative after KVM_REQ_CLOCK_UPDATE

[Vitaly Kuznetsov <vkuznets@redhat.com>]

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