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

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é <[email protected]>
>> Signed-off-by: Jan Beulich <[email protected]>
>> ---
>> 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