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

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Wed, Sep 02, 2026 at 10:40:04AM +0200, Jan Beulich wrote:
> On 02.09.2026 09:54, Roger Pau Monné wrote:
> > 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>
> 
> I assume you won't mind if I correct the typo in the domain name.

Sure, please 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);
> >> +
> >>      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?
> 
> I wanted to play safe here: In get_s_time_fixed(), release builds won't
> crash if the assertion wasn't there, but would trigger in a
> corresponding debug build. In that (unexpected) situation, we'd crash
> here (when .init.* was unmapped) if __initdata was used.

That's kind of what I was aiming at: we possibly do want to crash hard
if Xen is still using the fake counter after initialization?  Nothing
good can come out of running guests without the TSC scaling being set,
as the PV clock exposed won't be functional either.  Likely resulting
in guests relying on it also getting stuck because mul_frac == 0?

Thanks, Roger.