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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 22 Apr 2016, Jan Beulich wrote:
> >>> On 22.04.16 at 12:08, <sstabellini@xxxxxxxxxx> wrote:
> > On Fri, 22 Apr 2016, Jan Beulich wrote:
> >> >>> On 21.04.16 at 15:29, <sstabellini@xxxxxxxxxx> 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);
> }

This works, thanks! I'll resend a patch along these lines with your authorship.

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