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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.09.2026 11:15, Roger Pau Monné wrote:
> 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:
>>>> --- 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?

Crashing because of too late an .init.data access may require more
analysis than necessary though. I.e. if that was really the intention,
I think it should be a BUG_ON(), and I further think I'd prefer to leave
this to a separate patch (which I would likely ack, but which I'm not
sure I would be willing to write).

Jan