On 14.01.2026 18:49, Roger Pau Monné wrote: > On Tue, Jan 06, 2026 at 02:58:11PM +0100, Jan Beulich wrote: >> A similar issue looks to exist in read_xen_timer() and its read_cycle() >> helper, if we're scheduled out (and beck in) between reading of the TSC >> and calculation of the delta (involving ->tsc_timestamp). Am I >> overlooking anything there? > > If we are scheduled out in the middle, and the ->tsc_timestamp is > updated, ->version would also be bumped, and hence the loop will be > restarted. I don't think there's an issue there.
Oh, indeed - I was too focused on the read_cycle() alone. That may go wrong, but as you say the result then simply won't be used. >> stime2tsc() guards against negative deltas by using 0 instead; I'm not >> quite sure that's correct either. > > Hm, we should likely do the same for stime2tsc() that you do for > get_s_time_fixed(). Given the current callers I think we might be > safe, but it's a risk. Will do then. >> amd_check_erratum_1474() (next to its call to tsc_ticks2ns()) has a >> comment towards the TSC being "sane", but is that correct? Due to >> TSC_ADJUST, rdtsc() may well return a huge value (and the TSC would then >> wrap through 0 at some point). Shouldn't we subtract boot_tsc_stamp before >> calling tsc_ticks2ns()? > > amd_check_erratum_1474() runs after early_time_init(), which would > have cleared any TSC_ADJUST offset AFAICT. There's a note in the > initcall to that regard: > > /* > * Must be executed after early_time_init() for tsc_ticks2ns() to have been > * calibrated. That prevents us doing the check in init_amd(). > */ > presmp_initcall(amd_check_erratum_1474); Hmm, I should have written "Due to e.g. TSC_ADJUST". Firmware may also have played other games with MSR_TSC. >> A similar issue looks to exist in tsc_get_info(), again when rdtsc() >> possibly returns a huge value due to TSC_ADJUST. Once again I wonder >> whether we shouldn't subtract boot_tsc_stamp. > > I would expect tsc_get_info() to also get called exclusively after > early_time_init()? Same here then (obviously). >> --- a/xen/arch/x86/time.c >> +++ b/xen/arch/x86/time.c >> @@ -1649,8 +1649,13 @@ s_time_t get_s_time_fixed(u64 at_tsc) >> tsc = at_tsc; >> else >> tsc = rdtsc_ordered(); >> - delta = tsc - t->stamp.local_tsc; >> - return t->stamp.local_stime + scale_delta(delta, &t->tsc_scale); >> + >> + if ( tsc >= t->stamp.local_tsc ) > > Should we hint the compiler this is the likely path? I was wondering too, but was erring on the side of "no" primarily because of Andrew's general preference towards no likely(). I'd be happy to add it here. >> + delta = scale_delta(tsc - t->stamp.local_tsc, &t->tsc_scale); >> + else >> + delta = -scale_delta(t->stamp.local_tsc - tsc, &t->tsc_scale); >> + >> + return t->stamp.local_stime + delta; > > LGTM: > > Reviewed-by: Roger Pau Monné <[email protected]> Thanks. > However I see Anton still has concerns that this patch doesn't fully > fix the issue he reported, and I'm afraid I don't really understand > what's going on. I will have to take a more detailed look at the > thread, or possibly attempt to reproduce myself. Largely the same here. Jan
