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

[Stefano Stabellini <sstabellini@kernel.org>]

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?


> 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.


> Also, looking at all the involved code, I once again wonder whether
> all the is_hvm_*() there shouldn't be has_hvm_container_*().

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