Re: [PATCH] xen/time: fix system_time for vtsc=1 PV guests

["Jan Beulich" <JBeulich@suse.com>]

>>> On 22.04.16 at 12:08, <sstabellini@kernel.org> wrote:
> On Fri, 22 Apr 2016, Jan Beulich wrote:
>> >>> On 21.04.16 at 15:29, <sstabellini@kernel.org> wrote:
>> > --- a/xen/arch/x86/time.c
>> > +++ b/xen/arch/x86/time.c
>> > @@ -784,7 +784,7 @@ static void __update_vcpu_system_time(struct vcpu *v, 
> int force)
>> >      struct cpu_time       *t;
>> >      struct vcpu_time_info *u, _u = {};
>> >      struct domain *d = v->domain;
>> > -    s_time_t tsc_stamp;
>> > +    s_time_t stime_stamp, tsc_stamp = 0;
>> 
>> I don't see why the initializer needs adding here.
> 
> Ops, sorry, I developed the patch against 4.6, the useless
> initialization derives from it.
> 
> 
>> > @@ -807,7 +808,11 @@ static void __update_vcpu_system_time(struct vcpu *v, 
> int force)
>> >                  tsc_stamp = -gtime_to_gtsc(d, -stime);
>> >          }
>> >          else
>> > +        {
>> >              tsc_stamp = gtime_to_gtsc(d, stime);
>> > +            if (!tsc_stamp)
>> 
>> Coding style.
>> 
>> > +                stime_stamp = d->arch.vtsc_offset;
>> > +        }
>> 
>> While I can see this being the right thing for getting the two stamps
>> in sync, is that really helping the guest? Time ought to be not moving
>> forward until getting past vtsc_offset afaict, and that can't be good.
> 
> It helps a lot in my test case: without this Linux hangs due to lost
> timer interrupts (because they are set in the past).
> 
> 
>> I.e. it would seem to me that it's gtime_to_gtsc() that needs
>> adjustment to properly deal with time < d->arch.vtsc_offset.
> 
> I agree that it would be nice to fix gtime_to_gtsc, but how do you
> suggest to do it?

See below.

>> Plus I can't see why, in the worst case, the gTSC value can't be
>> wrapped through zero into negative (or really huge positive) range:
>> Such TSC values are certainly not invalid, and guests shouldn't really
>> make assumptions on TSC values being in the small positive range
>> when they boot.
> 
> Am I understanding correctly that you are suggesting to let the
> subtraction in gtime_to_gtsc return a negative -- actually a wrapped
> around positive?  Something like:
> 
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index 7a01c90..896fd9f 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1757,8 +1757,8 @@ custom_param("tsc", tsc_parse);
>  u64 gtime_to_gtsc(struct domain *d, u64 time)
>  {
>      if ( !is_hvm_domain(d) )
> -        time = max_t(s64, time - d->arch.vtsc_offset, 0);
> -    return scale_delta(time, &d->arch.ns_to_vtsc);
> +        time = time - d->arch.vtsc_offset;
> +    return scale_delta(time2, &d->arch.ns_to_vtsc);
>  }
>  
> Unfortunately that wouldn't solve the problem because of the scaling.

Of course. I thought more along the lines of

u64 gtime_to_gtsc(struct domain *d, u64 time)
{
    if ( !is_hvm_domain(d) )
    {
        if ( time < d->arch.vtsc_offset )
             return -scale_delta(d->arch.vtsc_offset - time, &d->arch.ns_to_vtsc);
        time -= d->arch.vtsc_offset;
    }
    return scale_delta(time, &d->arch.ns_to_vtsc);
}

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel