As a comment next to one of the invocations states, get_wallclock_time()
can take over a second. The order of evaluation of function arguments is
in principle unspecified; in practice at least gcc looks to be evaluating
them from last to first. Hence with NOW() invoked first, the respective
value passed to do_settime() can be off by over a second (which is in
contrast to __get_cmos_time() attempting to get the time exactly after an
update, i.e. [pretty] precisely at a seconds boundary).

This also addresses a Misra C:2012 rule 13.2 ("The value of an expression
and its persistent side-effects shall be the same under all permitted
evaluation orders") violation each.

Fixes: f64134cdb81c ("x86: Fix time_resume() to notify all domains of wallclock change")
Fixes: 0bfcf984b727 ("x86: Reintroduce clocksource=tsc")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Of course the time it takes to do all the CMOS reads (or whichever else
wallclock time source is in use) also results in an inaccuracy. For
__get_cmos_time() this might be solvable by having it latch NOW() before
doing the 6 reads, but in particular for efi_get_time() there's hardly
anything we can do.

As to Misra rule 13.2: tagging.ecl lists the rule as clean. I also can't
find any deviation for the two instances fixed here. What am I missing?

For __get_cmos_time(), tangentially: Wouldn't we better use the
century byte if available? As it stands, things will break in 2070. Which
is a long way out, yes, but still. (Of course this would mean a 7th slow
I/O port write/read pair.)

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -2575,6 +2575,8 @@ __initcall(verify_tsc_reliability);
 /* Late init function (after interrupts are enabled). */
 int __init init_xen_time(void)
 {
+    unsigned long wc;
+
     tsc_check_writability();
 
     open_softirq(TIME_CALIBRATE_SOFTIRQ, local_time_calibration);
@@ -2592,7 +2594,8 @@ int __init init_xen_time(void)
     printk(XENLOG_INFO "Wallclock source: %s\n", wallclock_type_to_string());
 
     /* NB. get_wallclock_time() can take over one second to execute. */
-    do_settime(get_wallclock_time(), 0, NOW());
+    wc = get_wallclock_time();
+    do_settime(wc, 0,  NOW());
 
     /* Finish platform timer initialization. */
     try_platform_timer_tail();
@@ -2745,6 +2748,8 @@ int time_suspend(void)
 
 int time_resume(void)
 {
+    unsigned long wc;
+
     preinit_pit();
 
     resume_platform_timer();
@@ -2756,7 +2761,8 @@ int time_resume(void)
 
     set_timer(&calibration_timer, NOW() + EPOCH);
 
-    do_settime(get_wallclock_time() + cmos_utc_offset, 0, NOW());
+    wc = get_wallclock_time();
+    do_settime(wc + cmos_utc_offset, 0, NOW());
 
     update_vcpu_system_time(current);

On Wed, May 06, 2026 at 11:35:45AM +0200, Jan Beulich wrote:
> As a comment next to one of the invocations states, get_wallclock_time()
> can take over a second. The order of evaluation of function arguments is
> in principle unspecified; in practice at least gcc looks to be evaluating
> them from last to first. Hence with NOW() invoked first, the respective
> value passed to do_settime() can be off by over a second (which is in
> contrast to __get_cmos_time() attempting to get the time exactly after an
> update, i.e. [pretty] precisely at a seconds boundary).
> 
> This also addresses a Misra C:2012 rule 13.2 ("The value of an expression
> and its persistent side-effects shall be the same under all permitted
> evaluation orders") violation each.
> 
> Fixes: f64134cdb81c ("x86: Fix time_resume() to notify all domains of wallclock change")
> Fixes: 0bfcf984b727 ("x86: Reintroduce clocksource=tsc")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

On Wed, May 06, 2026 at 11:35:45AM +0200, Jan Beulich wrote:
> As a comment next to one of the invocations states, get_wallclock_time()
> can take over a second. The order of evaluation of function arguments is
> in principle unspecified; in practice at least gcc looks to be evaluating
> them from last to first. Hence with NOW() invoked first, the respective
> value passed to do_settime() can be off by over a second (which is in
> contrast to __get_cmos_time() attempting to get the time exactly after an
> update, i.e. [pretty] precisely at a seconds boundary).
> 
> This also addresses a Misra C:2012 rule 13.2 ("The value of an expression
> and its persistent side-effects shall be the same under all permitted
> evaluation orders") violation each.
> 
> Fixes: f64134cdb81c ("x86: Fix time_resume() to notify all domains of wallclock change")
> Fixes: 0bfcf984b727 ("x86: Reintroduce clocksource=tsc")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Of course the time it takes to do all the CMOS reads (or whichever else
> wallclock time source is in use) also results in an inaccuracy. For
> __get_cmos_time() this might be solvable by having it latch NOW() before
> doing the 6 reads, but in particular for efi_get_time() there's hardly
> anything we can do.
> 
> As to Misra rule 13.2: tagging.ecl lists the rule as clean. I also can't
> find any deviation for the two instances fixed here. What am I missing?
> 
> For __get_cmos_time(), tangentially: Wouldn't we better use the
> century byte if available? As it stands, things will break in 2070. Which
> is a long way out, yes, but still. (Of course this would mean a 7th slow
> I/O port write/read pair.)

Seems fine to me.

One further note: in __get_cmos_time() I think we want to read
backwards, so start with century then year and so on, so the last read
is seconds, as to avoid extra skew.

Thanks, Roger.

On 06.05.2026 13:15, Roger Pau Monné wrote:
> On Wed, May 06, 2026 at 11:35:45AM +0200, Jan Beulich wrote:
>> As a comment next to one of the invocations states, get_wallclock_time()
>> can take over a second. The order of evaluation of function arguments is
>> in principle unspecified; in practice at least gcc looks to be evaluating
>> them from last to first. Hence with NOW() invoked first, the respective
>> value passed to do_settime() can be off by over a second (which is in
>> contrast to __get_cmos_time() attempting to get the time exactly after an
>> update, i.e. [pretty] precisely at a seconds boundary).
>>
>> This also addresses a Misra C:2012 rule 13.2 ("The value of an expression
>> and its persistent side-effects shall be the same under all permitted
>> evaluation orders") violation each.
>>
>> Fixes: f64134cdb81c ("x86: Fix time_resume() to notify all domains of wallclock change")
>> Fixes: 0bfcf984b727 ("x86: Reintroduce clocksource=tsc")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Of course the time it takes to do all the CMOS reads (or whichever else
>> wallclock time source is in use) also results in an inaccuracy. For
>> __get_cmos_time() this might be solvable by having it latch NOW() before
>> doing the 6 reads, but in particular for efi_get_time() there's hardly
>> anything we can do.
>>
>> As to Misra rule 13.2: tagging.ecl lists the rule as clean. I also can't
>> find any deviation for the two instances fixed here. What am I missing?
>>
>> For __get_cmos_time(), tangentially: Wouldn't we better use the
>> century byte if available? As it stands, things will break in 2070. Which
>> is a long way out, yes, but still. (Of course this would mean a 7th slow
>> I/O port write/read pair.)
> 
> Seems fine to me.

I'll make a patch, provided I can figure out under what conditions the byte
is present / valid (hopefully that won't involve ACPI AML).

> One further note: in __get_cmos_time() I think we want to read
> backwards, so start with century then year and so on, so the last read
> is seconds, as to avoid extra skew.

I don't think this matters. It would be awkward if those reads took over a
second. Plus if it did, the UIP flag would transiently be set, in which
case doing the reads wouldn't be valid in the first place. Arguably this
can in principle happen if a SMI hits in the middle, and its handling
takes excessively long.

Jan

On Wed, May 06, 2026 at 01:45:36PM +0200, Jan Beulich wrote:
> On 06.05.2026 13:15, Roger Pau Monné wrote:
> > On Wed, May 06, 2026 at 11:35:45AM +0200, Jan Beulich wrote:
> >> As a comment next to one of the invocations states, get_wallclock_time()
> >> can take over a second. The order of evaluation of function arguments is
> >> in principle unspecified; in practice at least gcc looks to be evaluating
> >> them from last to first. Hence with NOW() invoked first, the respective
> >> value passed to do_settime() can be off by over a second (which is in
> >> contrast to __get_cmos_time() attempting to get the time exactly after an
> >> update, i.e. [pretty] precisely at a seconds boundary).
> >>
> >> This also addresses a Misra C:2012 rule 13.2 ("The value of an expression
> >> and its persistent side-effects shall be the same under all permitted
> >> evaluation orders") violation each.
> >>
> >> Fixes: f64134cdb81c ("x86: Fix time_resume() to notify all domains of wallclock change")
> >> Fixes: 0bfcf984b727 ("x86: Reintroduce clocksource=tsc")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> Of course the time it takes to do all the CMOS reads (or whichever else
> >> wallclock time source is in use) also results in an inaccuracy. For
> >> __get_cmos_time() this might be solvable by having it latch NOW() before
> >> doing the 6 reads, but in particular for efi_get_time() there's hardly
> >> anything we can do.
> >>
> >> As to Misra rule 13.2: tagging.ecl lists the rule as clean. I also can't
> >> find any deviation for the two instances fixed here. What am I missing?
> >>
> >> For __get_cmos_time(), tangentially: Wouldn't we better use the
> >> century byte if available? As it stands, things will break in 2070. Which
> >> is a long way out, yes, but still. (Of course this would mean a 7th slow
> >> I/O port write/read pair.)
> > 
> > Seems fine to me.
> 
> I'll make a patch, provided I can figure out under what conditions the byte
> is present / valid (hopefully that won't involve ACPI AML).

It's in the ACPI FADT table AFAIK.

> > One further note: in __get_cmos_time() I think we want to read
> > backwards, so start with century then year and so on, so the last read
> > is seconds, as to avoid extra skew.
> 
> I don't think this matters. It would be awkward if those reads took over a
> second. Plus if it did, the UIP flag would transiently be set, in which
> case doing the reads wouldn't be valid in the first place. Arguably this
> can in principle happen if a SMI hits in the middle, and its handling
> takes excessively long.

Oh, I see, indeed.

Thanks, Roger.

On 06/05/2026 10:35 am, Jan Beulich wrote:
> As a comment next to one of the invocations states, get_wallclock_time()
> can take over a second. The order of evaluation of function arguments is
> in principle unspecified; in practice at least gcc looks to be evaluating
> them from last to first. Hence with NOW() invoked first, the respective
> value passed to do_settime() can be off by over a second (which is in
> contrast to __get_cmos_time() attempting to get the time exactly after an
> update, i.e. [pretty] precisely at a seconds boundary).
>
> This also addresses a Misra C:2012 rule 13.2 ("The value of an expression
> and its persistent side-effects shall be the same under all permitted
> evaluation orders") violation each.
>
> Fixes: f64134cdb81c ("x86: Fix time_resume() to notify all domains of wallclock change")
> Fixes: 0bfcf984b727 ("x86: Reintroduce clocksource=tsc")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Of course the time it takes to do all the CMOS reads (or whichever else
> wallclock time source is in use) also results in an inaccuracy. For
> __get_cmos_time() this might be solvable by having it latch NOW() before
> doing the 6 reads, but in particular for efi_get_time() there's hardly
> anything we can do.
>
> As to Misra rule 13.2: tagging.ecl lists the rule as clean. I also can't
> find any deviation for the two instances fixed here. What am I missing?

From deviations.ecl:

-doc_begin="The following file is imported from Linux: ignore for now."
-file_tag+={adopted_time_r8_3,"^xen/arch/x86/time\\.c$"}

I've said before and it bears repeating.  This claim is false and should
never have been put in to start with, and time.c is not impacted file.

~Andrew

On 06/05/2026 10:47 am, Andrew Cooper wrote:
> On 06/05/2026 10:35 am, Jan Beulich wrote:
>> As a comment next to one of the invocations states, get_wallclock_time()
>> can take over a second. The order of evaluation of function arguments is
>> in principle unspecified; in practice at least gcc looks to be evaluating
>> them from last to first. Hence with NOW() invoked first, the respective
>> value passed to do_settime() can be off by over a second (which is in
>> contrast to __get_cmos_time() attempting to get the time exactly after an
>> update, i.e. [pretty] precisely at a seconds boundary).
>>
>> This also addresses a Misra C:2012 rule 13.2 ("The value of an expression
>> and its persistent side-effects shall be the same under all permitted
>> evaluation orders") violation each.
>>
>> Fixes: f64134cdb81c ("x86: Fix time_resume() to notify all domains of wallclock change")
>> Fixes: 0bfcf984b727 ("x86: Reintroduce clocksource=tsc")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Of course the time it takes to do all the CMOS reads (or whichever else
>> wallclock time source is in use) also results in an inaccuracy. For
>> __get_cmos_time() this might be solvable by having it latch NOW() before
>> doing the 6 reads, but in particular for efi_get_time() there's hardly
>> anything we can do.
>>
>> As to Misra rule 13.2: tagging.ecl lists the rule as clean. I also can't
>> find any deviation for the two instances fixed here. What am I missing?
> From deviations.ecl:
>
> -doc_begin="The following file is imported from Linux: ignore for now."
> -file_tag+={adopted_time_r8_3,"^xen/arch/x86/time\\.c$"}
>
> I've said before and it bears repeating.  This claim is false and should
> never have been put in to start with, and time.c is not impacted file.

is not the only*

~Andrew

On 2026-05-06 11:48, Andrew Cooper wrote:
> On 06/05/2026 10:47 am, Andrew Cooper wrote:
>> On 06/05/2026 10:35 am, Jan Beulich wrote:
>>> As a comment next to one of the invocations states, 
>>> get_wallclock_time()
>>> can take over a second. The order of evaluation of function arguments 
>>> is
>>> in principle unspecified; in practice at least gcc looks to be 
>>> evaluating
>>> them from last to first. Hence with NOW() invoked first, the 
>>> respective
>>> value passed to do_settime() can be off by over a second (which is in
>>> contrast to __get_cmos_time() attempting to get the time exactly 
>>> after an
>>> update, i.e. [pretty] precisely at a seconds boundary).
>>> 
>>> This also addresses a Misra C:2012 rule 13.2 ("The value of an 
>>> expression
>>> and its persistent side-effects shall be the same under all permitted
>>> evaluation orders") violation each.
>>> 
>>> Fixes: f64134cdb81c ("x86: Fix time_resume() to notify all domains of 
>>> wallclock change")
>>> Fixes: 0bfcf984b727 ("x86: Reintroduce clocksource=tsc")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> Of course the time it takes to do all the CMOS reads (or whichever 
>>> else
>>> wallclock time source is in use) also results in an inaccuracy. For
>>> __get_cmos_time() this might be solvable by having it latch NOW() 
>>> before
>>> doing the 6 reads, but in particular for efi_get_time() there's 
>>> hardly
>>> anything we can do.
>>> 
>>> As to Misra rule 13.2: tagging.ecl lists the rule as clean. I also 
>>> can't
>>> find any deviation for the two instances fixed here. What am I 
>>> missing?
>> From deviations.ecl:
>> 
>> -doc_begin="The following file is imported from Linux: ignore for 
>> now."
>> -file_tag+={adopted_time_r8_3,"^xen/arch/x86/time\\.c$"}
>> 
>> I've said before and it bears repeating.  This claim is false and 
>> should
>> never have been put in to start with, and time.c is not impacted file.
> 
> is not the only*
> 

Patches welcome :)

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253