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

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Wed, Aug 19, 2026 at 01:45:56PM +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@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Roger Pau Monné <roger@xxxxxxxxxxxxxx>

One minor comment below.

> ---
> 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.
> 
> 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).

I agree with backporting, current behavior of getting stuck in an
infinite loop is worse than timing out earlier than expected (which is
the worse outcome of this patch).

> ---
> v5: Move addition to early_cpu_init() down. Adjust commentary there.
> 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>
> @@ -444,6 +445,39 @@ void __init early_cpu_init(bool verbose)
>  
>       if (!(c->vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)))
>               park_offline_cpus = opt_mce;
> +
> +     /*
> +      * If nominal freq isn't available, use highest, thus causing NOW()
> +      * output to move more slowly.  See preset_tsc_scale().
> +      */
> +     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)
> +                             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)
> +                     preset_tsc_scale(hi_mhz * 1000000UL);
> +     } else if (c->vendor & X86_VENDOR_INTEL) {
> +             unsigned int hi_mhz = 0;
> +
> +             intel_process_freq(c, NULL, &hi_mhz);
> +             if (hi_mhz)
> +                     preset_tsc_scale(hi_mhz * 1000000UL);
> +     }
>  }
>  
>  void reset_cpuinfo(struct cpuinfo_x86 *c, bool keep_basic)
> --- 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;

Not that it matters much, but counter can probably be __initdata?

Thanks, Roger.