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 12:36:08PM +0000, Igor Druzhinin wrote:
> On 15/01/2020 09:47, Roger Pau Monné wrote:
> > On Tue, Jan 14, 2020 at 07:36:21PM +0000, Igor Druzhinin wrote:
> >> If ITSC is not available on CPU (e.g if running nested as PV shim)
> >> then X86_FEATURE_NONSTOP_TSC is not advertised in certain cases, i.e.
> >> all AMD and some old Intel processors. In which case TSC would need to
> >> be restored on CPU from platform time by Xen upon exiting deep C-states.
> >>
> >> As platform time might be behind the last TSC stamp recorded for the
> >> current CPU, invariant of TSC stamp being always behind local TSC counter
> >> is violated. This has an effect of get_s_time() going negative resulting
> >> in eventual system hang or crash.
> >>
> >> Fix this issue by updating local TSC stamp along with TSC counter write.
> > 
> > Thanks! I haven't seen such issue because I've been running the shim
> > with nomigrate in order to prevent the vTSC overhead.
> > 
> >>
> >> Signed-off-by: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
> >> ---
> >> This caused reliable hangs of shim domains with multiple vCPUs on all AMD
> >> systems. The problem got also reproduced on bare-metal by artifically
> >> masking ITSC feature bit. The proposed fix has been verified for both
> >> cases.
> >> ---
> >>  xen/arch/x86/time.c | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> >> index e79cb4d..f6b26f8 100644
> >> --- 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 in that case RDTSC might return something behind platform time
> which is not right I guess.

The TSC and the platform time are completely independent from Xen's
PoV, you can have a platform time not based on the TSC (ie: PIT, HPET
or PM), and hence there's no direct relation between both.

The TSC is used as a way to get an approximate platform time based on
the last platform time value and the TSC delta between than value and
the current TSC value, I assume that's done because reading the TSC is
much cheaper than reading the platform time.

As long as the platform time and the TSC stamps are both updated at the
same time it should be fine.

Roger.

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