[PATCH] x86/time: adjust handling of negative delta in stime2tsc()

[PATCH] x86/time: adjust handling of negative delta in stime2tsc()

[Jan Beulich]

When we cap negative values to 0 (see code comment as to why), going
through scale_delta() is pointless - it'll return 0 anyway. Therefore make
the call conditional (and then also the one to scale_reciprocal()), adding
a comment as to why there is this capping.

Modernize types used while there, and switch to usiong initializers for
the local variables.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Adding likely() to the conditional here does make a difference. Question
is whether to do so, seeing that there looks to be a possibility (of
unknown frequency) for the delta to be non-positive.

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1176,20 +1176,26 @@ uint64_t __init calibrate_apic_timer(voi
     return elapsed * CALIBRATE_FRAC;
 }
 
-u64 stime2tsc(s_time_t stime)
+uint64_t stime2tsc(s_time_t stime)
 {
-    struct cpu_time *t;
-    struct time_scale sys_to_tsc;
-    s_time_t stime_delta;
+    const struct cpu_time *t = &this_cpu(cpu_time);
+    s_time_t stime_delta = stime - t->stamp.local_stime;
+    int64_t delta = 0;
 
-    t = &this_cpu(cpu_time);
-    sys_to_tsc = scale_reciprocal(t->tsc_scale);
+    /*
+     * While for reprogram_timer() the capping at 0 isn't relevant (the returned
+     * value is likely in the past anyway then, by the time it is used), for
+     * cstate_restore_tsc() this is relevant: We need to avoid moving the TSC
+     * backwards (relative to when it may last have been read).
+     */
+    if ( stime_delta > 0 )
+    {
+        struct time_scale sys_to_tsc = scale_reciprocal(t->tsc_scale);
 
-    stime_delta = stime - t->stamp.local_stime;
-    if ( stime_delta < 0 )
-        stime_delta = 0;
+        delta = scale_delta(stime_delta, &sys_to_tsc);
+    }
 
-    return t->stamp.local_tsc + scale_delta(stime_delta, &sys_to_tsc);
+    return t->stamp.local_tsc + delta;
 }
 
 void cstate_restore_tsc(void)

Re: [PATCH] x86/time: adjust handling of negative delta in stime2tsc()

[Roger Pau Monné]

On Tue, Feb 10, 2026 at 11:04:59AM +0100, Jan Beulich wrote:
> When we cap negative values to 0 (see code comment as to why), going
> through scale_delta() is pointless - it'll return 0 anyway. Therefore make
> the call conditional (and then also the one to scale_reciprocal()), adding
> a comment as to why there is this capping.
> 
> Modernize types used while there, and switch to usiong initializers for
> the local variables.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Adding likely() to the conditional here does make a difference. Question
> is whether to do so, seeing that there looks to be a possibility (of
> unknown frequency) for the delta to be non-positive.

Hm, what I've done lately with {un}likely() is not attempting to
optimize for the most taken path, but rather use it to force the
compiler to optimize the fast path, if the function has one.  The slow
path will be slow anyway, and hence any compiler optimization should
be towards making the fast path possibly faster IMO.

This function doesn't seem to have any fast (or slow) paths, so I
would leave it as-is.

> 
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1176,20 +1176,26 @@ uint64_t __init calibrate_apic_timer(voi
>      return elapsed * CALIBRATE_FRAC;
>  }
>  
> -u64 stime2tsc(s_time_t stime)
> +uint64_t stime2tsc(s_time_t stime)
>  {
> -    struct cpu_time *t;
> -    struct time_scale sys_to_tsc;
> -    s_time_t stime_delta;
> +    const struct cpu_time *t = &this_cpu(cpu_time);
> +    s_time_t stime_delta = stime - t->stamp.local_stime;
> +    int64_t delta = 0;

Why do you make delta a signed integer, the value returned by
scale_delta() is unsigned.

>  
> -    t = &this_cpu(cpu_time);
> -    sys_to_tsc = scale_reciprocal(t->tsc_scale);
> +    /*
> +     * While for reprogram_timer() the capping at 0 isn't relevant (the returned

The capping might want mentioning in the function prototype, as maybe
new users expect stime2tsc() to return TSC values from times in the
past.

Otherwise LGTM.

Thanks, Roger.

Re: [PATCH] x86/time: adjust handling of negative delta in stime2tsc()

[Jan Beulich]

On 25.03.2026 18:57, Roger Pau Monné wrote:
> On Tue, Feb 10, 2026 at 11:04:59AM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -1176,20 +1176,26 @@ uint64_t __init calibrate_apic_timer(voi
>>      return elapsed * CALIBRATE_FRAC;
>>  }
>>  
>> -u64 stime2tsc(s_time_t stime)
>> +uint64_t stime2tsc(s_time_t stime)
>>  {
>> -    struct cpu_time *t;
>> -    struct time_scale sys_to_tsc;
>> -    s_time_t stime_delta;
>> +    const struct cpu_time *t = &this_cpu(cpu_time);
>> +    s_time_t stime_delta = stime - t->stamp.local_stime;
>> +    int64_t delta = 0;
> 
> Why do you make delta a signed integer, the value returned by
> scale_delta() is unsigned.

I guess purely mechanically, as deltas usually want to be signed. Changed.

>> -    t = &this_cpu(cpu_time);
>> -    sys_to_tsc = scale_reciprocal(t->tsc_scale);
>> +    /*
>> +     * While for reprogram_timer() the capping at 0 isn't relevant (the returned
> 
> The capping might want mentioning in the function prototype, as maybe
> new users expect stime2tsc() to return TSC values from times in the
> past.

Can do, sure.

Jan