On Tue, Jan 06, 2026 at 02:58:11PM +0100, Jan Beulich wrote:
> Callers may pass in TSC values from before the local TSC stamp was last
> updated (this would in particular be the case when the TSC was latched, a
> time rendezvous occurs, and the latched value is used only afterwards).
> scale_delta(), otoh, deals with unsigned values, and hence would treat
> negative incoming deltas as huge positive values, yielding entirely bogus
> return values.
>
> Fixes: 88e64cb785c1 ("x86/HVM: use fixed TSC value when saving or restoring
> domain")
> Reported-by: Антон Марков <[email protected]>
> Signed-off-by: Jan Beulich <[email protected]>
> ---
> An alternative might be to have scale_delta() deal with signed deltas, yet
> that seemed more risky to me.
I won't go that route, the caller can figure out the signedness of the
result as needed.
> There could likely be more Fixes: tags; the one used is the oldest
> applicable one, from what I can tell.
>
> 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.
> 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.
> 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);
> 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()?
> --- 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?
> + 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]>
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.
Thanks, Roger.
