* [PATCH v5 0/2] x86/time: avoid early uses of NOW() to return zero
@ 2026-08-19 11:44 Jan Beulich
2026-08-19 11:45 ` [PATCH v5 1/2] x86/CPU: re-arrange tail of early_cpu_init() Jan Beulich
2026-08-19 11:45 ` [PATCH v5 2/2] x86/time: avoid early uses of NOW() to return zero Jan Beulich
0 siblings, 2 replies; 9+ messages in thread
From: Jan Beulich @ 2026-08-19 11:44 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Andrew Cooper, Roger Pau Monné, Teddy Astie
1: CPU: re-arrange tail of early_cpu_init()
2: time: avoid early uses of NOW() to return zero
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 1/2] x86/CPU: re-arrange tail of early_cpu_init()
2026-08-19 11:44 [PATCH v5 0/2] x86/time: avoid early uses of NOW() to return zero Jan Beulich
@ 2026-08-19 11:45 ` Jan Beulich
2026-09-02 7:32 ` Roger Pau Monné
2026-08-19 11:45 ` [PATCH v5 2/2] x86/time: avoid early uses of NOW() to return zero Jan Beulich
1 sibling, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2026-08-19 11:45 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Andrew Cooper, Roger Pau Monné, Teddy Astie
Some early setup doesn't need re-doing after ucode load. Move the call to
initialize_cpu_data() slightly up and add a conditional return point.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The parameter being named "verbose" may be a little irritating for this
use, yet renaming would incur extra churn.
Clearly an alternative would be to split the function. I can't, however,
seem to be able to think of a good name for the part that would be invoked
post-ucode-loading. Maybe early_cpu_reinit(), except that calling that
from early_cpu_init() then still feel somewhat odd.
---
v5: New.
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -432,10 +432,18 @@ void __init early_cpu_init(bool verbose)
paddr_bits -= (ebx >> 6) & 0x3f;
}
+ initialize_cpu_data(0);
+
+ if (!verbose)
+ return;
+
+ /*
+ * Work which doesn't need repeating after microcode load goes below
+ * here.
+ */
+
if (!(c->vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)))
park_offline_cpus = opt_mce;
-
- initialize_cpu_data(0);
}
void reset_cpuinfo(struct cpuinfo_x86 *c, bool keep_basic)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 2/2] x86/time: avoid early uses of NOW() to return zero
2026-08-19 11:44 [PATCH v5 0/2] x86/time: avoid early uses of NOW() to return zero Jan Beulich
2026-08-19 11:45 ` [PATCH v5 1/2] x86/CPU: re-arrange tail of early_cpu_init() Jan Beulich
@ 2026-08-19 11:45 ` Jan Beulich
2026-09-02 7:54 ` Roger Pau Monné
1 sibling, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2026-08-19 11:45 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Andrew Cooper, Roger Pau Monné, Teddy Astie
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.
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).
---
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;
+
+ 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;
}
}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] x86/CPU: re-arrange tail of early_cpu_init()
2026-08-19 11:45 ` [PATCH v5 1/2] x86/CPU: re-arrange tail of early_cpu_init() Jan Beulich
@ 2026-09-02 7:32 ` Roger Pau Monné
0 siblings, 0 replies; 9+ messages in thread
From: Roger Pau Monné @ 2026-09-02 7:32 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel@lists.xenproject.org, Andrew Cooper, Teddy Astie
On Wed, Aug 19, 2026 at 01:45:31PM +0200, Jan Beulich wrote:
> Some early setup doesn't need re-doing after ucode load. Move the call to
> initialize_cpu_data() slightly up and add a conditional return point.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger@xenproject.org>
> ---
> The parameter being named "verbose" may be a little irritating for this
> use, yet renaming would incur extra churn.
>
> Clearly an alternative would be to split the function. I can't, however,
> seem to be able to think of a good name for the part that would be invoked
> post-ucode-loading. Maybe early_cpu_reinit(), except that calling that
> from early_cpu_init() then still feel somewhat odd.
No strong opinion. The code that doesn't need redoing seems so little
that splitting this might not (yet?) be necessary.
Thanks, Roger.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 2/2] x86/time: avoid early uses of NOW() to return zero
2026-08-19 11:45 ` [PATCH v5 2/2] x86/time: avoid early uses of NOW() to return zero Jan Beulich
@ 2026-09-02 7:54 ` Roger Pau Monné
2026-09-02 8:40 ` Jan Beulich
2026-09-02 14:20 ` Jan Beulich
0 siblings, 2 replies; 9+ messages in thread
From: Roger Pau Monné @ 2026-09-02 7:54 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel@lists.xenproject.org, Andrew Cooper, Teddy Astie
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@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger@xenporject.org>
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 2/2] x86/time: avoid early uses of NOW() to return zero
2026-09-02 7:54 ` Roger Pau Monné
@ 2026-09-02 8:40 ` Jan Beulich
2026-09-02 9:15 ` Roger Pau Monné
2026-09-02 14:20 ` Jan Beulich
1 sibling, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2026-09-02 8:40 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel@lists.xenproject.org, Andrew Cooper, Teddy Astie
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@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Acked-by: Roger Pau Monné <roger@xenporject.org>
I assume you won't mind if I correct the typo in the domain name.
>> --- 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.
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 2/2] x86/time: avoid early uses of NOW() to return zero
2026-09-02 8:40 ` Jan Beulich
@ 2026-09-02 9:15 ` Roger Pau Monné
2026-09-02 9:31 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: Roger Pau Monné @ 2026-09-02 9:15 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel@lists.xenproject.org, Andrew Cooper, Teddy Astie
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@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > Acked-by: Roger Pau Monné <roger@xenporject.org>
>
> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 2/2] x86/time: avoid early uses of NOW() to return zero
2026-09-02 9:15 ` Roger Pau Monné
@ 2026-09-02 9:31 ` Jan Beulich
0 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2026-09-02 9:31 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel@lists.xenproject.org, Andrew Cooper, Teddy Astie
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 2/2] x86/time: avoid early uses of NOW() to return zero
2026-09-02 7:54 ` Roger Pau Monné
2026-09-02 8:40 ` Jan Beulich
@ 2026-09-02 14:20 ` Jan Beulich
1 sibling, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2026-09-02 14:20 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel@lists.xenproject.org, Andrew Cooper, Teddy Astie
On 02.09.2026 09:54, Roger Pau Monné wrote:
> On Wed, Aug 19, 2026 at 01:45:56PM +0200, Jan Beulich wrote:
>> 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).
Question is how to find a good balance between pulling in prereqs (there
are quite a few, I think) vs sacrificing some functionality. Or whether
to make it an all-or-nothing decision.
Jan
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-09-02 14:20 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-19 11:44 [PATCH v5 0/2] x86/time: avoid early uses of NOW() to return zero Jan Beulich
2026-08-19 11:45 ` [PATCH v5 1/2] x86/CPU: re-arrange tail of early_cpu_init() Jan Beulich
2026-09-02 7:32 ` Roger Pau Monné
2026-08-19 11:45 ` [PATCH v5 2/2] x86/time: avoid early uses of NOW() to return zero Jan Beulich
2026-09-02 7:54 ` Roger Pau Monné
2026-09-02 8:40 ` Jan Beulich
2026-09-02 9:15 ` Roger Pau Monné
2026-09-02 9:31 ` Jan Beulich
2026-09-02 14:20 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.