Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 15, 2020 at 01:49:22PM +0100, Jan Beulich wrote:
> On 15.01.2020 12:53, Roger Pau Monné wrote:
> > On Wed, Jan 15, 2020 at 12:40:27PM +0100, Jan Beulich wrote:
> >> On 15.01.2020 10:47, Roger Pau Monné wrote:
> >>> On Tue, Jan 14, 2020 at 07:36:21PM +0000, Igor Druzhinin wrote:
> >>>> --- a/xen/arch/x86/time.c
> >>>> +++ b/xen/arch/x86/time.c
> >>>> @@ -955,10 +955,16 @@ u64 stime2tsc(s_time_t stime)
> >>>>  
> >>>>  void cstate_restore_tsc(void)
> >>>>  {
> >>>> +    struct cpu_time *t = &this_cpu(cpu_time);
> >>>> +
> >>>>      if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
> >>>>          return;
> >>>>  
> >>>> -    write_tsc(stime2tsc(read_platform_stime(NULL)));
> >>>> +    t->stamp.master_stime = read_platform_stime(NULL);
> >>>> +    t->stamp.local_tsc = stime2tsc(t->stamp.master_stime);
> >>>> +    t->stamp.local_stime = t->stamp.master_stime;
> >>>> +
> >>>> +    write_tsc(t->stamp.local_tsc);
> >>>
> >>> In order to avoid the TSC write (and the likely associated vmexit),
> >>> could you instead do:
> >>>
> >>> t->stamp.local_stime = t->stamp.master_stime = read_platform_stime(NULL);
> >>> t->stamp.local_tsc = rdtsc_ordered();
> >>>
> >>> I think it should achieve the same as it syncs the local TSC stamp and
> >>> times, would avoid the TSC write and slightly simplifies the logic.
> >>
> >> Wouldn't this result in guests possibly observing the TSC moving
> >> backwards?
> > 
> > Isn't local_tsc storing a TSC value read from the same CPU always, and
> > hence could only go backwards if rdtsc actually goes backwards?
> 
> For one I have to admit I was (mistakenly) thinking of wakeup
> from S states more than that from C states. So assuming the
> TSC indeed only stops (but won't get e.g. restarted), backwards
> moves ought to be excluded.

Even if the TSC was restarted I think my proposed approach should be
fine. The only requirement is that the stored TSC stamp must always be
behind than the value returned by rdtsc. See get_s_time_fixed: as
long as the delta is positive the returned time should be correct.

> What I'm then worried about is too
> little progress observable by guests. The PV time protocol
> ought to be fine in this regard (and consumers of raw TSC values
> are on their own anyway), but wouldn't you need to update TSC
> offsets of HVM guests in order to compensate for the elapsed
> time?

That will be done when the HVM vCPU gets scheduled in as part of the
update_vcpu_system_time call AFAICT. cstate_restore_tsc will always be
called with the idle vCPU context, and hence there's always going to
be a vCPU switch before scheduling anything else.

> > Ie: cpu_frequency_change seems to do something similar, together with
> > a re-adjusting of the time scale, but doesn't perform any TSC write.
> 
> A P-state change at most alters the the tick rate, but wouldn't
> stop or even reset the TSC (afaict).

Right, just wanted to point out that the cpu_time stamp can be
updated without having to write to the TSC. Anyway, not sure it's very
relevant or useful, so forget this reference.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel