[PATCH v4 0/3] x86/time: avoid early uses of NOW() to return zero

[PATCH v4 0/3] x86/time: avoid early uses of NOW() to return zero

[Jan Beulich]

1: time: add "NOW() good" indicator
2: x86/Intel: split model-specific freq calculation off of intel_log_freq()
3: x86/time: avoid early uses of NOW() to return zero

Jan

[PATCH v4 1/3] time: add "NOW() good" indicator

[Jan Beulich]

printk_start_of_line() checks for a value of 0 right now. In order to be
able to have NOW() return at least monotonically increasing values, that
needs replacing by an explicit indicator.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Arm and RISC-V may want to consider whether their initial get_cycles()
can't be moved yet earlier, such that the indicator also can be set
yet earlier.
---
v4: Add barriers.
v3: New.

--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -145,6 +145,8 @@ void __init preinit_xen_time(void)
         panic("Timer: Cannot initialize platform timer\n");
 
     boot_count = get_cycles();
+    smp_wmb();
+    NOW_good = true;
 }
 
 static void __init init_dt_xen_time(void)
--- a/xen/arch/riscv/time.c
+++ b/xen/arch/riscv/time.c
@@ -87,6 +87,8 @@ void __init preinit_xen_time(void)
         panic("%s: ACPI isn't supported\n", __func__);
 
     boot_clock_cycles = get_cycles();
+    smp_wmb();
+    NOW_good = true;
 
     /* set_xen_timer must have been set by sbi_init() already */
     ASSERT(set_xen_timer);
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -2660,6 +2660,7 @@ void __init early_time_init(void)
 
     set_time_scale(&t->tsc_scale, tmp);
     t->stamp.local_tsc = boot_tsc_stamp;
+    NOW_good = true;
 
     init_percpu_time();
 
--- a/xen/common/time.c
+++ b/xen/common/time.c
@@ -22,6 +22,8 @@
 #include <asm/div64.h>
 #include <asm/domain.h>
 
+bool __ro_after_init NOW_good;
+
 /* Nonzero if YEAR is a leap year (every 4 years,
    except every 100th isn't, and every 400th is).  */
 #define __isleap(year) \
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -975,11 +975,11 @@ static void printk_start_of_line(const c
         }
         /* fall through */
     case TSM_BOOT:
-        sec = NOW();
-        nsec = do_div(sec, 1000000000);
-
-        if ( sec | nsec )
+        if ( NOW_good )
         {
+            smp_rmb();
+            sec = NOW();
+            nsec = do_div(sec, 1000000000);
             snprintf(tstr, sizeof(tstr), "[%5"PRIu64".%06"PRIu64"] ",
                      sec, nsec / 1000);
             break;
--- a/xen/include/xen/time.h
+++ b/xen/include/xen/time.h
@@ -62,6 +62,12 @@ struct tm wallclock_time(uint64_t *ns);
 /* Chosen so (NOW() + delta) wont overflow without an uptime of 200 years */
 #define STIME_DELTA_MAX ((s_time_t)((uint64_t)~0ULL>>2))
 
+/*
+ * Indicator that the value returned by NOW() is good (earlier invocations may
+ * return zero or very small, merely monotonically increasing values).
+ */
+extern bool NOW_good;
+
 /* Explicitly OR with 1 just in case version number gets out of sync. */
 #define version_update_begin(v) (((v) + 1) | 1)
 #define version_update_end(v)   ((v) + 1)

[PATCH v4 2/3] x86/Intel: split model-specific freq calculation off of intel_log_freq()

[Jan Beulich]

..., for that logic to become reusable. While doing so undo the open-
coding of DIV_ROUND_UP(). Also switch to the new struct cpuinfo_x86 field
names.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
If Misra didn't dislike non-static functions without external callers, the
new function could be put below the old one, thus reducing churn and
improving readability of the diff (really I moved the code for the new
function up, but the diff representation is the other way around).
---
v3: New.

--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -476,51 +476,14 @@ static int num_cpu_cores(struct cpuinfo_
 		return 1;
 }
 
-static void intel_log_freq(const struct cpuinfo_x86 *c)
+static void intel_process_freq(const struct cpuinfo_x86 *c,
+                               unsigned int *min_mhz, unsigned int *max_mhz)
 {
-    unsigned int eax, ebx, ecx, edx, factor;
     uint64_t msrval;
     uint8_t max_ratio, min_ratio;
+    unsigned int factor;
 
-    if ( c->cpuid_level >= 0x15 )
-    {
-        cpuid(0x15, &eax, &ebx, &ecx, &edx);
-        if ( ecx && ebx && eax )
-        {
-            unsigned long long val = ecx;
-
-            val *= ebx;
-            printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n",
-                   smp_processor_id(), ecx, ebx, eax, val / eax);
-        }
-        else if ( ecx | eax | ebx )
-        {
-            printk("CPU%u: TSC:", smp_processor_id());
-            if ( ecx )
-                printk(" core: %u Hz", ecx);
-            if ( ebx && eax )
-                printk(" ratio: %u / %u", ebx, eax);
-            printk("\n");
-        }
-    }
-
-    if ( c->cpuid_level >= 0x16 )
-    {
-        cpuid(0x16, &eax, &ebx, &ecx, &edx);
-        if ( ecx | eax | ebx )
-        {
-            printk("CPU%u:", smp_processor_id());
-            if ( ecx )
-                printk(" bus: %u MHz", ecx);
-            if ( eax )
-                printk(" base: %u MHz", eax);
-            if ( ebx )
-                printk(" max: %u MHz", ebx);
-            printk("\n");
-        }
-    }
-
-    switch ( c->x86 )
+    switch ( c->family )
     {
         static const unsigned short core_factors[] =
             { 26667, 13333, 20000, 16667, 33333, 10000, 40000 };
@@ -533,7 +496,7 @@ static void intel_log_freq(const struct
         if ( !max_ratio )
             return;
 
-        switch ( c->x86_model )
+        switch ( c->model )
         {
         case 0x0e: /* Core */
         case 0x0f: case 0x16: case 0x17: case 0x1d: /* Core2 */
@@ -578,10 +541,61 @@ static void intel_log_freq(const struct
         return;
     }
 
+    if ( min_mhz )
+        *min_mhz = DIV_ROUND_UP(factor * min_ratio, 100);
+    *max_mhz = DIV_ROUND_UP(factor * max_ratio, 100);
+}
+
+static void intel_log_freq(const struct cpuinfo_x86 *c)
+{
+    unsigned int eax, ebx, ecx, edx, min_mhz = 0, max_mhz = 0;
+
+    if ( c->cpuid_level >= 0x15 )
+    {
+        cpuid(0x15, &eax, &ebx, &ecx, &edx);
+        if ( ecx && ebx && eax )
+        {
+            unsigned long long val = ecx;
+
+            val *= ebx;
+            printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n",
+                   smp_processor_id(), ecx, ebx, eax, val / eax);
+        }
+        else if ( ecx | eax | ebx )
+        {
+            printk("CPU%u: TSC:", smp_processor_id());
+            if ( ecx )
+                printk(" core: %u Hz", ecx);
+            if ( ebx && eax )
+                printk(" ratio: %u / %u", ebx, eax);
+            printk("\n");
+        }
+    }
+
+    if ( c->cpuid_level >= 0x16 )
+    {
+        cpuid(0x16, &eax, &ebx, &ecx, &edx);
+        if ( ecx | eax | ebx )
+        {
+            printk("CPU%u:", smp_processor_id());
+            if ( ecx )
+                printk(" bus: %u MHz", ecx);
+            if ( eax )
+                printk(" base: %u MHz", eax);
+            if ( ebx )
+                printk(" max: %u MHz", ebx);
+            printk("\n");
+        }
+    }
+
+    intel_process_freq(c, &min_mhz, &max_mhz);
+    if ( !max_mhz )
+        return;
+
     printk("CPU%u: ", smp_processor_id());
-    if ( min_ratio )
-        printk("%u ... ", (factor * min_ratio + 50) / 100);
-    printk("%u MHz\n", (factor * max_ratio + 50) / 100);
+    if ( min_mhz )
+        printk("%u ... ", min_mhz);
+    printk("%u MHz\n", max_mhz);
 }
 
 static void init_intel_perf(struct cpuinfo_x86 *c)

[PATCH v4 3/3] x86/time: avoid early uses of NOW() to return zero

[Jan Beulich]

Waiting loops like the one in flush_command_buffer() will degenerate to
infinite ones when used early enough for NOW() to still return constant
zero. Make sure the returned value at least monotonically increases. When
available, use nominal frequency values as initial approximation.

Do this only in get_s_time(), as producing a sane value in
get_s_time_fixed() for non-zero inputs won't be reasonably possible.
Put an assertion there.

Reported-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: While generally the mentioned waiting loops will take longer to time
     out, on a very fast CPU tight loops may time out too early.

RFC: On the 2nd pass through early_cpu_init() it may be okay to skip the
     new additions.

With "x86/time: set AP's TSC scale estimate earlier" the counter update
may not need to be atomic anymore, as then only the BSP can reasonably hit
that path.

I don't think Fixes: tags should be put here. If we did, we'd have to
enumerate all introductions of early uses of NOW() (or get_s_time()), with
the exception of those dealing with getting back 0 (which I expect is only
printk_start_of_line()). Will want backporting nevertheless (unless deemed
too risky).
---
v3: Use "high" / "max" freq if "nominal" isn't available. Set NOW_good.
v2: Add assertion to get_s_time_fixed(). Use nominal frequencies for very
    early setting, if available.

--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -19,6 +19,7 @@
 #include <asm/random.h>
 #include <asm/setup.h>
 #include <asm/shstk.h>
+#include <asm/time.h>
 #include <asm/xstate.h>
 
 #include <public/sysctl.h>
@@ -403,6 +404,36 @@ void __init early_cpu_init(bool verbose)
 				    &c->x86_capability[FEATURESET_7d1]);
 	}
 
+	if (c->cpuid_level >= 0x15) {
+		cpuid(0x15, &eax, &ebx, &ecx, &edx);
+
+		if (ecx && ebx && eax)
+			preset_tsc_scale(DIV_ROUND_UP(ecx * 1UL * ebx, eax));
+		else if (c->cpuid_level >= 0x16) {
+			/* Assume CPU base freq ≈ TSC freq. */
+			cpuid(0x16, &eax, &ebx, &ecx, &edx);
+			if (eax)
+				preset_tsc_scale(eax * 1000000UL);
+			else if (ebx) /* See preset_tsc_scale() for why. */
+				preset_tsc_scale(ebx * 1000000UL);
+		}
+	} else if (c->vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) {
+		unsigned int nom_mhz = 0, hi_mhz = 0;
+
+		amd_process_freq(c, NULL, &nom_mhz, &hi_mhz);
+		if (nom_mhz)
+			preset_tsc_scale(nom_mhz * 1000000UL);
+		else if (hi_mhz) /* See preset_tsc_scale() for why. */
+			preset_tsc_scale(hi_mhz * 1000000UL);
+	} else if (c->vendor & X86_VENDOR_INTEL) {
+		unsigned int hi_mhz = 0;
+
+		/* See preset_tsc_scale() for why. */
+		intel_process_freq(c, NULL, &hi_mhz);
+		if (hi_mhz)
+			preset_tsc_scale(hi_mhz * 1000000UL);
+	}
+
 	eax = cpuid_eax(0x80000000);
 	if ((eax >> 16) == 0x8000 && eax >= 0x80000008) {
 		ebx = eax >= 0x8000001f ? cpuid_ebx(0x8000001f) : 0;
--- a/xen/arch/x86/include/asm/time.h
+++ b/xen/arch/x86/include/asm/time.h
@@ -23,6 +23,7 @@ mktime (unsigned int year, unsigned int
 int time_suspend(void);
 int time_resume(void);
 
+void preset_tsc_scale(unsigned long freq);
 void init_percpu_time(void);
 void time_latch_stamps(void);
 
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -476,8 +476,8 @@ static int num_cpu_cores(struct cpuinfo_
 		return 1;
 }
 
-static void intel_process_freq(const struct cpuinfo_x86 *c,
-                               unsigned int *min_mhz, unsigned int *max_mhz)
+void intel_process_freq(const struct cpuinfo_x86 *c,
+                        unsigned int *min_mhz, unsigned int *max_mhz)
 {
     uint64_t msrval;
     uint8_t max_ratio, min_ratio;
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -417,6 +417,9 @@ static inline uint8_t get_cpu_family(uin
     return fam;
 }
 
+void intel_process_freq(const struct cpuinfo_x86 *c,
+                        unsigned int *min_mhz, unsigned int *max_mhz);
+
 #ifdef CONFIG_INTEL
 extern int8_t opt_tsx;
 extern bool rtm_disabled;
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1664,6 +1664,9 @@ s_time_t get_s_time_fixed(uint64_t at_ts
     const struct cpu_time *t = &this_cpu(cpu_time);
     uint64_t tsc, delta;
 
+    /* scale_delta() degenerates when the scale wasn't set yet. */
+    ASSERT(t->tsc_scale.mul_frac);
+
     if ( at_tsc )
         tsc = at_tsc;
     else
@@ -1679,6 +1682,20 @@ s_time_t get_s_time_fixed(uint64_t at_ts
 
 s_time_t get_s_time(void)
 {
+    /*
+     * Before the TSC scale is set, avoid returning constant 0 (or whatever
+     * this_cpu(cpu_time).stamp.local_stime is set to).  While the returned
+     * value is in no way representing time, it at least increases
+     * monotonically, thus avoiding e.g. waiting loops to degenerate to
+     * entirely infinite ones.
+     */
+    if ( unlikely(!this_cpu(cpu_time).tsc_scale.mul_frac) )
+    {
+        static s_time_t counter;
+
+        return arch_fetch_and_add(&counter, 1);
+    }
+
     return get_s_time_fixed(0);
 }
 
@@ -2632,6 +2649,22 @@ int __init init_xen_time(void)
     return 0;
 }
 
+/* BSP-only function to pre-set an approximate TSC scale. */
+void __init preset_tsc_scale(unsigned long freq)
+{
+    struct cpu_time *t = &this_cpu(cpu_time);
+
+    /*
+     * The incoming frequency is only approximate (nominal).  Increase it by
+     * 1% to make NOW() output rather a little too slow than too fast, thus
+     * avoiding a possible backwards jump once the final scale is set.
+     */
+    freq += DIV_ROUND_UP(freq, 100);
+
+    set_time_scale(&t->tsc_scale, freq);
+    t->stamp.local_tsc = boot_tsc_stamp;
+    NOW_good = true;
+}
 
 /* Early init function. */
 void __init early_time_init(void)
@@ -2649,6 +2682,9 @@ void __init early_time_init(void)
                    "TSC ADJUST set to %lx on boot CPU - clearing\n", tmp);
             wrmsrl(MSR_IA32_TSC_ADJUST, 0);
             boot_tsc_stamp -= tmp;
+
+            if ( t->stamp.local_tsc )
+                t->stamp.local_tsc -= tmp;
         }
     }
 

Re: [PATCH v4 1/3] time: add "NOW() good" indicator

[Oleksii Kurochko]

On 6/30/26 4:06 PM, Jan Beulich wrote:
> printk_start_of_line() checks for a value of 0 right now. In order to be
> able to have NOW() return at least monotonically increasing values, that
> needs replacing by an explicit indicator.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

LGTM:
  Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

Re: [PATCH v4 1/3] time: add "NOW() good" indicator

[Roger Pau Monné]

On Tue, Jun 30, 2026 at 04:06:00PM +0200, Jan Beulich wrote:
> printk_start_of_line() checks for a value of 0 right now. In order to be
> able to have NOW() return at least monotonically increasing values, that
> needs replacing by an explicit indicator.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Arm and RISC-V may want to consider whether their initial get_cycles()
> can't be moved yet earlier, such that the indicator also can be set
> yet earlier.
> ---
> v4: Add barriers.
> v3: New.
> 
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -145,6 +145,8 @@ void __init preinit_xen_time(void)
>          panic("Timer: Cannot initialize platform timer\n");
>  
>      boot_count = get_cycles();
> +    smp_wmb();
> +    NOW_good = true;
>  }
>  
>  static void __init init_dt_xen_time(void)
> --- a/xen/arch/riscv/time.c
> +++ b/xen/arch/riscv/time.c
> @@ -87,6 +87,8 @@ void __init preinit_xen_time(void)
>          panic("%s: ACPI isn't supported\n", __func__);
>  
>      boot_clock_cycles = get_cycles();
> +    smp_wmb();
> +    NOW_good = true;
>  
>      /* set_xen_timer must have been set by sbi_init() already */
>      ASSERT(set_xen_timer);
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -2660,6 +2660,7 @@ void __init early_time_init(void)
>  
>      set_time_scale(&t->tsc_scale, tmp);
>      t->stamp.local_tsc = boot_tsc_stamp;
> +    NOW_good = true;

Would you need a barrier here to ensure compiler doesn't re-order the
writes?  Maybe using ACCESS_ONCE(), or a smp_wmb() like it's used in
other arches?

Thanks, Roger.

Re: [PATCH v4 2/3] x86/Intel: split model-specific freq calculation off of intel_log_freq()

[Roger Pau Monné]

On Tue, Jun 30, 2026 at 04:06:21PM +0200, Jan Beulich wrote:
> ..., for that logic to become reusable. While doing so undo the open-
> coding of DIV_ROUND_UP(). Also switch to the new struct cpuinfo_x86 field
> names.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

AFAICT this doesn't introduce any functional change.

Acked-by: Roger Pau Monné <roger@xenproject.org>

Thanks, Roger.

Re: [PATCH v4 1/3] time: add "NOW() good" indicator

[Roger Pau Monné]

On Tue, Jun 30, 2026 at 04:06:00PM +0200, Jan Beulich wrote:
> printk_start_of_line() checks for a value of 0 right now. In order to be
> able to have NOW() return at least monotonically increasing values, that
> needs replacing by an explicit indicator.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Arm and RISC-V may want to consider whether their initial get_cycles()
> can't be moved yet earlier, such that the indicator also can be set
> yet earlier.
> ---
> v4: Add barriers.
> v3: New.
> 
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -145,6 +145,8 @@ void __init preinit_xen_time(void)
>          panic("Timer: Cannot initialize platform timer\n");
>  
>      boot_count = get_cycles();
> +    smp_wmb();
> +    NOW_good = true;
>  }
>  
>  static void __init init_dt_xen_time(void)
> --- a/xen/arch/riscv/time.c
> +++ b/xen/arch/riscv/time.c
> @@ -87,6 +87,8 @@ void __init preinit_xen_time(void)
>          panic("%s: ACPI isn't supported\n", __func__);
>  
>      boot_clock_cycles = get_cycles();
> +    smp_wmb();
> +    NOW_good = true;
>  
>      /* set_xen_timer must have been set by sbi_init() already */
>      ASSERT(set_xen_timer);
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -2660,6 +2660,7 @@ void __init early_time_init(void)
>  
>      set_time_scale(&t->tsc_scale, tmp);
>      t->stamp.local_tsc = boot_tsc_stamp;
> +    NOW_good = true;

Would you need a barrier here to ensure compiler doesn't re-order the
writes?  Maybe using ACCESS_ONCE(), or a smp_wmb() like it's used in
other arches?

Thanks, Roger.

Re: [PATCH v4 1/3] time: add "NOW() good" indicator

[Jan Beulich]

On 28.07.2026 10:17, Roger Pau Monné wrote:
> On Tue, Jun 30, 2026 at 04:06:00PM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -2660,6 +2660,7 @@ void __init early_time_init(void)
>>  
>>      set_time_scale(&t->tsc_scale, tmp);
>>      t->stamp.local_tsc = boot_tsc_stamp;
>> +    NOW_good = true;
> 
> Would you need a barrier here to ensure compiler doesn't re-order the
> writes?  Maybe using ACCESS_ONCE(), or a smp_wmb() like it's used in
> other arches?

Aiui it isn't needed here. The compiler can't re-order the two writes,
due to our use of -fno-strict-aliasing. The barrier is there on other
arch-es to avoid re-ordering in hardware.

Jan

Re: [PATCH v4 3/3] x86/time: avoid early uses of NOW() to return zero

[Roger Pau Monné]

On Tue, Jun 30, 2026 at 04:06:41PM +0200, Jan Beulich wrote:
> Waiting loops like the one in flush_command_buffer() will degenerate to
> infinite ones when used early enough for NOW() to still return constant
> zero. Make sure the returned value at least monotonically increases. When
> available, use nominal frequency values as initial approximation.
> 
> Do this only in get_s_time(), as producing a sane value in
> get_s_time_fixed() for non-zero inputs won't be reasonably possible.
> Put an assertion there.
> 
> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> RFC: While generally the mentioned waiting loops will take longer to time
>      out, on a very fast CPU tight loops may time out too early.

While we know this is not ideal, it's better than getting stuck in an
infinite NOW() loop without any timeout.  IMO it's best to timeout
early than not timeout at all.

> RFC: On the 2nd pass through early_cpu_init() it may be okay to skip the
>      new additions.

Possibly, yes, maybe add a static variable there to avoid re-doing?

> 
> With "x86/time: set AP's TSC scale estimate earlier" the counter update
> may not need to be atomic anymore, as then only the BSP can reasonably hit
> that path.
> 
> I don't think Fixes: tags should be put here. If we did, we'd have to
> enumerate all introductions of early uses of NOW() (or get_s_time()), with
> the exception of those dealing with getting back 0 (which I expect is only
> printk_start_of_line()). Will want backporting nevertheless (unless deemed
> too risky).
> ---
> v3: Use "high" / "max" freq if "nominal" isn't available. Set NOW_good.
> v2: Add assertion to get_s_time_fixed(). Use nominal frequencies for very
>     early setting, if available.
> 
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -19,6 +19,7 @@
>  #include <asm/random.h>
>  #include <asm/setup.h>
>  #include <asm/shstk.h>
> +#include <asm/time.h>
>  #include <asm/xstate.h>
>  
>  #include <public/sysctl.h>
> @@ -403,6 +404,36 @@ void __init early_cpu_init(bool verbose)
>  				    &c->x86_capability[FEATURESET_7d1]);
>  	}
>  
> +	if (c->cpuid_level >= 0x15) {
> +		cpuid(0x15, &eax, &ebx, &ecx, &edx);
> +
> +		if (ecx && ebx && eax)
> +			preset_tsc_scale(DIV_ROUND_UP(ecx * 1UL * ebx, eax));
> +		else if (c->cpuid_level >= 0x16) {
> +			/* Assume CPU base freq ≈ TSC freq. */
> +			cpuid(0x16, &eax, &ebx, &ecx, &edx);
> +			if (eax)
> +				preset_tsc_scale(eax * 1000000UL);
> +			else if (ebx) /* See preset_tsc_scale() for why. */
> +				preset_tsc_scale(ebx * 1000000UL);
> +		}
> +	} else if (c->vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) {
> +		unsigned int nom_mhz = 0, hi_mhz = 0;
> +
> +		amd_process_freq(c, NULL, &nom_mhz, &hi_mhz);
> +		if (nom_mhz)
> +			preset_tsc_scale(nom_mhz * 1000000UL);
> +		else if (hi_mhz) /* See preset_tsc_scale() for why. */
> +			preset_tsc_scale(hi_mhz * 1000000UL);
> +	} else if (c->vendor & X86_VENDOR_INTEL) {
> +		unsigned int hi_mhz = 0;
> +
> +		/* See preset_tsc_scale() for why. */

I would avoid those repeated "See preset_tsc_scale() for why."
comments, and simply state at the beginning of the block that either
the nominal or the higher reported frequencies will be used, as in the
worse case when using the high frequency the timer will run slower,
but not faster.

> +		intel_process_freq(c, NULL, &hi_mhz);
> +		if (hi_mhz)
> +			preset_tsc_scale(hi_mhz * 1000000UL);
> +	}
> +
>  	eax = cpuid_eax(0x80000000);
>  	if ((eax >> 16) == 0x8000 && eax >= 0x80000008) {
>  		ebx = eax >= 0x8000001f ? cpuid_ebx(0x8000001f) : 0;
> --- a/xen/arch/x86/include/asm/time.h
> +++ b/xen/arch/x86/include/asm/time.h
> @@ -23,6 +23,7 @@ mktime (unsigned int year, unsigned int
>  int time_suspend(void);
>  int time_resume(void);
>  
> +void preset_tsc_scale(unsigned long freq);
>  void init_percpu_time(void);
>  void time_latch_stamps(void);
>  
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -476,8 +476,8 @@ static int num_cpu_cores(struct cpuinfo_
>  		return 1;
>  }
>  
> -static void intel_process_freq(const struct cpuinfo_x86 *c,
> -                               unsigned int *min_mhz, unsigned int *max_mhz)
> +void intel_process_freq(const struct cpuinfo_x86 *c,
> +                        unsigned int *min_mhz, unsigned int *max_mhz)
>  {
>      uint64_t msrval;
>      uint8_t max_ratio, min_ratio;
> --- a/xen/arch/x86/include/asm/processor.h
> +++ b/xen/arch/x86/include/asm/processor.h
> @@ -417,6 +417,9 @@ static inline uint8_t get_cpu_family(uin
>      return fam;
>  }
>  
> +void intel_process_freq(const struct cpuinfo_x86 *c,
> +                        unsigned int *min_mhz, unsigned int *max_mhz);
> +
>  #ifdef CONFIG_INTEL
>  extern int8_t opt_tsx;
>  extern bool rtm_disabled;
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1664,6 +1664,9 @@ s_time_t get_s_time_fixed(uint64_t at_ts
>      const struct cpu_time *t = &this_cpu(cpu_time);
>      uint64_t tsc, delta;
>  
> +    /* scale_delta() degenerates when the scale wasn't set yet. */
> +    ASSERT(t->tsc_scale.mul_frac);

Hm, so for release builds we would just return 0 in get_s_time_fixed()
when called before the scale is initialized.  I guess that's as good
as we can do.  I wonder whether using BUG_ON() won't be better here,
but it's likely best to return 0 than plain crash.

Thanks, Roger.

Re: [PATCH v4 3/3] x86/time: avoid early uses of NOW() to return zero

[Roger Pau Monné]

On Tue, Jun 30, 2026 at 04:06:41PM +0200, Jan Beulich wrote:
> Waiting loops like the one in flush_command_buffer() will degenerate to
> infinite ones when used early enough for NOW() to still return constant
> zero. Make sure the returned value at least monotonically increases. When
> available, use nominal frequency values as initial approximation.
> 
> Do this only in get_s_time(), as producing a sane value in
> get_s_time_fixed() for non-zero inputs won't be reasonably possible.
> Put an assertion there.
> 
> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> RFC: While generally the mentioned waiting loops will take longer to time
>      out, on a very fast CPU tight loops may time out too early.

While we know this is not ideal, it's better than getting stuck in an
infinite NOW() loop without any timeout.  IMO it's best to timeout
early than not timeout at all.

> RFC: On the 2nd pass through early_cpu_init() it may be okay to skip the
>      new additions.

Possibly, yes, maybe add a static variable there to avoid re-doing?

> 
> With "x86/time: set AP's TSC scale estimate earlier" the counter update
> may not need to be atomic anymore, as then only the BSP can reasonably hit
> that path.
> 
> I don't think Fixes: tags should be put here. If we did, we'd have to
> enumerate all introductions of early uses of NOW() (or get_s_time()), with
> the exception of those dealing with getting back 0 (which I expect is only
> printk_start_of_line()). Will want backporting nevertheless (unless deemed
> too risky).
> ---
> v3: Use "high" / "max" freq if "nominal" isn't available. Set NOW_good.
> v2: Add assertion to get_s_time_fixed(). Use nominal frequencies for very
>     early setting, if available.
> 
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -19,6 +19,7 @@
>  #include <asm/random.h>
>  #include <asm/setup.h>
>  #include <asm/shstk.h>
> +#include <asm/time.h>
>  #include <asm/xstate.h>
>  
>  #include <public/sysctl.h>
> @@ -403,6 +404,36 @@ void __init early_cpu_init(bool verbose)
>  				    &c->x86_capability[FEATURESET_7d1]);
>  	}
>  
> +	if (c->cpuid_level >= 0x15) {
> +		cpuid(0x15, &eax, &ebx, &ecx, &edx);
> +
> +		if (ecx && ebx && eax)
> +			preset_tsc_scale(DIV_ROUND_UP(ecx * 1UL * ebx, eax));
> +		else if (c->cpuid_level >= 0x16) {
> +			/* Assume CPU base freq ≈ TSC freq. */
> +			cpuid(0x16, &eax, &ebx, &ecx, &edx);
> +			if (eax)
> +				preset_tsc_scale(eax * 1000000UL);
> +			else if (ebx) /* See preset_tsc_scale() for why. */
> +				preset_tsc_scale(ebx * 1000000UL);
> +		}
> +	} else if (c->vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) {
> +		unsigned int nom_mhz = 0, hi_mhz = 0;
> +
> +		amd_process_freq(c, NULL, &nom_mhz, &hi_mhz);
> +		if (nom_mhz)
> +			preset_tsc_scale(nom_mhz * 1000000UL);
> +		else if (hi_mhz) /* See preset_tsc_scale() for why. */
> +			preset_tsc_scale(hi_mhz * 1000000UL);
> +	} else if (c->vendor & X86_VENDOR_INTEL) {
> +		unsigned int hi_mhz = 0;
> +
> +		/* See preset_tsc_scale() for why. */

I would avoid those repeated "See preset_tsc_scale() for why."
comments, and simply state at the beginning of the block that either
the nominal or the higher reported frequencies will be used, as in the
worse case when using the high frequency the timer will run slower,
but not faster.

> +		intel_process_freq(c, NULL, &hi_mhz);
> +		if (hi_mhz)
> +			preset_tsc_scale(hi_mhz * 1000000UL);
> +	}
> +
>  	eax = cpuid_eax(0x80000000);
>  	if ((eax >> 16) == 0x8000 && eax >= 0x80000008) {
>  		ebx = eax >= 0x8000001f ? cpuid_ebx(0x8000001f) : 0;
> --- a/xen/arch/x86/include/asm/time.h
> +++ b/xen/arch/x86/include/asm/time.h
> @@ -23,6 +23,7 @@ mktime (unsigned int year, unsigned int
>  int time_suspend(void);
>  int time_resume(void);
>  
> +void preset_tsc_scale(unsigned long freq);
>  void init_percpu_time(void);
>  void time_latch_stamps(void);
>  
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -476,8 +476,8 @@ static int num_cpu_cores(struct cpuinfo_
>  		return 1;
>  }
>  
> -static void intel_process_freq(const struct cpuinfo_x86 *c,
> -                               unsigned int *min_mhz, unsigned int *max_mhz)
> +void intel_process_freq(const struct cpuinfo_x86 *c,
> +                        unsigned int *min_mhz, unsigned int *max_mhz)
>  {
>      uint64_t msrval;
>      uint8_t max_ratio, min_ratio;
> --- a/xen/arch/x86/include/asm/processor.h
> +++ b/xen/arch/x86/include/asm/processor.h
> @@ -417,6 +417,9 @@ static inline uint8_t get_cpu_family(uin
>      return fam;
>  }
>  
> +void intel_process_freq(const struct cpuinfo_x86 *c,
> +                        unsigned int *min_mhz, unsigned int *max_mhz);
> +
>  #ifdef CONFIG_INTEL
>  extern int8_t opt_tsx;
>  extern bool rtm_disabled;
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1664,6 +1664,9 @@ s_time_t get_s_time_fixed(uint64_t at_ts
>      const struct cpu_time *t = &this_cpu(cpu_time);
>      uint64_t tsc, delta;
>  
> +    /* scale_delta() degenerates when the scale wasn't set yet. */
> +    ASSERT(t->tsc_scale.mul_frac);

Hm, so for release builds we would just return 0 in get_s_time_fixed()
when called before the scale is initialized.  I guess that's as good
as we can do.  I wonder whether using BUG_ON() won't be better here,
but it's likely best to return 0 than plain crash.

Thanks, Roger.

Re: [PATCH v4 3/3] x86/time: avoid early uses of NOW() to return zero

[Jan Beulich]

On 28.07.2026 10:41, Roger Pau Monné wrote:
> On Tue, Jun 30, 2026 at 04:06:41PM +0200, Jan Beulich wrote:
>> Waiting loops like the one in flush_command_buffer() will degenerate to
>> infinite ones when used early enough for NOW() to still return constant
>> zero. Make sure the returned value at least monotonically increases. When
>> available, use nominal frequency values as initial approximation.
>>
>> Do this only in get_s_time(), as producing a sane value in
>> get_s_time_fixed() for non-zero inputs won't be reasonably possible.
>> Put an assertion there.
>>
>> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> RFC: While generally the mentioned waiting loops will take longer to time
>>      out, on a very fast CPU tight loops may time out too early.
> 
> While we know this is not ideal, it's better than getting stuck in an
> infinite NOW() loop without any timeout.  IMO it's best to timeout
> early than not timeout at all.

Good, thanks for confirming.

>> RFC: On the 2nd pass through early_cpu_init() it may be okay to skip the
>>      new additions.
> 
> Possibly, yes, maybe add a static variable there to avoid re-doing?

I don't think a static would be needed: We can key this off of the function
parameter.

>> @@ -403,6 +404,36 @@ void __init early_cpu_init(bool verbose)
>>  				    &c->x86_capability[FEATURESET_7d1]);
>>  	}
>>  
>> +	if (c->cpuid_level >= 0x15) {
>> +		cpuid(0x15, &eax, &ebx, &ecx, &edx);
>> +
>> +		if (ecx && ebx && eax)
>> +			preset_tsc_scale(DIV_ROUND_UP(ecx * 1UL * ebx, eax));
>> +		else if (c->cpuid_level >= 0x16) {
>> +			/* Assume CPU base freq ≈ TSC freq. */
>> +			cpuid(0x16, &eax, &ebx, &ecx, &edx);
>> +			if (eax)
>> +				preset_tsc_scale(eax * 1000000UL);
>> +			else if (ebx) /* See preset_tsc_scale() for why. */
>> +				preset_tsc_scale(ebx * 1000000UL);
>> +		}
>> +	} else if (c->vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) {
>> +		unsigned int nom_mhz = 0, hi_mhz = 0;
>> +
>> +		amd_process_freq(c, NULL, &nom_mhz, &hi_mhz);
>> +		if (nom_mhz)
>> +			preset_tsc_scale(nom_mhz * 1000000UL);
>> +		else if (hi_mhz) /* See preset_tsc_scale() for why. */
>> +			preset_tsc_scale(hi_mhz * 1000000UL);
>> +	} else if (c->vendor & X86_VENDOR_INTEL) {
>> +		unsigned int hi_mhz = 0;
>> +
>> +		/* See preset_tsc_scale() for why. */
> 
> I would avoid those repeated "See preset_tsc_scale() for why."
> comments, and simply state at the beginning of the block that either
> the nominal or the higher reported frequencies will be used, as in the
> worse case when using the high frequency the timer will run slower,
> but not faster.

Can do.

>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -1664,6 +1664,9 @@ s_time_t get_s_time_fixed(uint64_t at_ts
>>      const struct cpu_time *t = &this_cpu(cpu_time);
>>      uint64_t tsc, delta;
>>  
>> +    /* scale_delta() degenerates when the scale wasn't set yet. */
>> +    ASSERT(t->tsc_scale.mul_frac);
> 
> Hm, so for release builds we would just return 0 in get_s_time_fixed()
> when called before the scale is initialized.  I guess that's as good
> as we can do.  I wonder whether using BUG_ON() won't be better here,
> but it's likely best to return 0 than plain crash.

Yeah, crashing release builds because of this felt excessive to me.

Jan

Re: [PATCH v4 1/3] time: add "NOW() good" indicator

[Roger Pau Monné]

On Tue, Jul 28, 2026 at 10:24:38AM +0200, Jan Beulich wrote:
> On 28.07.2026 10:17, Roger Pau Monné wrote:
> > On Tue, Jun 30, 2026 at 04:06:00PM +0200, Jan Beulich wrote:
> >> --- a/xen/arch/x86/time.c
> >> +++ b/xen/arch/x86/time.c
> >> @@ -2660,6 +2660,7 @@ void __init early_time_init(void)
> >>  
> >>      set_time_scale(&t->tsc_scale, tmp);
> >>      t->stamp.local_tsc = boot_tsc_stamp;
> >> +    NOW_good = true;
> > 
> > Would you need a barrier here to ensure compiler doesn't re-order the
> > writes?  Maybe using ACCESS_ONCE(), or a smp_wmb() like it's used in
> > other arches?
> 
> Aiui it isn't needed here. The compiler can't re-order the two writes,
> due to our use of -fno-strict-aliasing. The barrier is there on other
> arch-es to avoid re-ordering in hardware.

You are the expert in compilers, but it was my understanding that
`no-strict-aliasing` only affects the ordering of pointers accesses,
but not plain variables.  IOW: NOW_good accesses could be reordered
because it's not a pointer.  Does the compiler consider t can point to
&NOW_good and hence it can't be reordered?

Thanks, Roger.

Re: [PATCH v4 1/3] time: add "NOW() good" indicator

[Jan Beulich]

On 28.07.2026 12:32, Roger Pau Monné wrote:
> On Tue, Jul 28, 2026 at 10:24:38AM +0200, Jan Beulich wrote:
>> On 28.07.2026 10:17, Roger Pau Monné wrote:
>>> On Tue, Jun 30, 2026 at 04:06:00PM +0200, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/time.c
>>>> +++ b/xen/arch/x86/time.c
>>>> @@ -2660,6 +2660,7 @@ void __init early_time_init(void)
>>>>  
>>>>      set_time_scale(&t->tsc_scale, tmp);
>>>>      t->stamp.local_tsc = boot_tsc_stamp;
>>>> +    NOW_good = true;
>>>
>>> Would you need a barrier here to ensure compiler doesn't re-order the
>>> writes?  Maybe using ACCESS_ONCE(), or a smp_wmb() like it's used in
>>> other arches?
>>
>> Aiui it isn't needed here. The compiler can't re-order the two writes,
>> due to our use of -fno-strict-aliasing. The barrier is there on other
>> arch-es to avoid re-ordering in hardware.
> 
> You are the expert in compilers, but it was my understanding that
> `no-strict-aliasing` only affects the ordering of pointers accesses,
> but not plain variables.  IOW: NOW_good accesses could be reordered
> because it's not a pointer.  Does the compiler consider t can point to
> &NOW_good and hence it can't be reordered?

Yes, that's my understanding of how this work. (Yet no, "expert" surely
is going too far.) Things would be different if NOW_good was a function-
local variable, I think.

Jan