[PATCH v7 0/2] x86/time: improvements to wallclock logic

[PATCH v7 0/2] x86/time: improvements to wallclock logic

[Roger Pau Monne]

Hello,

This series started as an attempt to change the default wallclock
preference from EFI_GET_TIME to CMOS RTC, but has grown quite a lot.
Last remaining fixes, compared to v6 just some minor changes to the
option documentation and handling.

Thanks, Roger.

Roger Pau Monne (2):
  x86/time: introduce command line option to select wallclock
  x86/time: prefer CMOS over EFI_GET_TIME

 docs/misc/xen-command-line.pandoc | 21 +++++++++++++
 xen/arch/x86/time.c               | 49 +++++++++++++++++++++++++++----
 2 files changed, 65 insertions(+), 5 deletions(-)

-- 
2.46.0

[PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Roger Pau Monne]

Allow setting the used wallclock from the command line.  When the option is set
to a value different than `auto` the probing is bypassed and the selected
implementation is used (as long as it's available).

The `xen` and `efi` options require being booted as a Xen guest (with Xen guest
supported built-in) or from UEFI firmware respectively.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v6:
 - Clarify documentation regarding repeated using of the wallclock command line
   option.

Changes since v5:
 - Do EFI run-time services checking after command line parsing.

Changes since v3:
 - Note xen option is only available if Xen guest support it built.
 - Fix typo.
---
 docs/misc/xen-command-line.pandoc | 21 ++++++++++++++++
 xen/arch/x86/time.c               | 41 ++++++++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 959cf45b55d9..2a9b3b9b8975 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2816,6 +2816,27 @@ vwfi to `native` reduces irq latency significantly. It can also lead to
 suboptimal scheduling decisions, but only when the system is
 oversubscribed (i.e., in total there are more vCPUs than pCPUs).
 
+### wallclock (x86)
+> `= auto | xen | cmos | efi`
+
+> Default: `auto`
+
+Allow forcing the usage of a specific wallclock source.
+
+ * `auto` let the hypervisor select the clocksource based on internal
+   heuristics.
+
+ * `xen` force usage of the Xen shared_info wallclock when booted as a Xen
+   guest.  This option is only available if the hypervisor was compiled with
+   `CONFIG_XEN_GUEST` enabled.
+
+ * `cmos` force usage of the CMOS RTC wallclock.
+
+ * `efi` force usage of the EFI_GET_TIME run-time method when booted from EFI
+   firmware.
+
+If the selected option is invalid or not available Xen will default to `auto`.
+
 ### watchdog (x86)
 > `= force | <boolean>`
 
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 29b026735e5d..e4751684951e 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1552,6 +1552,37 @@ static const char *__init wallclock_type_to_string(void)
     return "";
 }
 
+static int __init cf_check parse_wallclock(const char *arg)
+{
+    wallclock_source = WALLCLOCK_UNSET;
+
+    if ( !arg )
+        return -EINVAL;
+
+    if ( !strcmp("auto", arg) )
+        ASSERT(wallclock_source == WALLCLOCK_UNSET);
+    else if ( !strcmp("xen", arg) )
+    {
+        if ( !xen_guest )
+            return -EINVAL;
+
+        wallclock_source = WALLCLOCK_XEN;
+    }
+    else if ( !strcmp("cmos", arg) )
+        wallclock_source = WALLCLOCK_CMOS;
+    else if ( !strcmp("efi", arg) )
+        /*
+         * Checking if run-time services are available must be done after
+         * command line parsing.
+         */
+        wallclock_source = WALLCLOCK_EFI;
+    else
+        return -EINVAL;
+
+    return 0;
+}
+custom_param("wallclock", parse_wallclock);
+
 static void __init probe_wallclock(void)
 {
     ASSERT(wallclock_source == WALLCLOCK_UNSET);
@@ -2527,7 +2558,15 @@ int __init init_xen_time(void)
 
     open_softirq(TIME_CALIBRATE_SOFTIRQ, local_time_calibration);
 
-    probe_wallclock();
+    /*
+     * EFI run time services can be disabled from the command line, hence the
+     * check for them cannot be done as part of the wallclock option parsing.
+     */
+    if ( wallclock_source == WALLCLOCK_EFI && !efi_enabled(EFI_RS) )
+        wallclock_source = WALLCLOCK_UNSET;
+
+    if ( wallclock_source == WALLCLOCK_UNSET )
+        probe_wallclock();
 
     printk(XENLOG_INFO "Wallclock source: %s\n", wallclock_type_to_string());
 
-- 
2.46.0

[PATCH v7 2/2] x86/time: prefer CMOS over EFI_GET_TIME

[Roger Pau Monne]

The EFI_GET_TIME implementation is well known to be broken for many firmware
implementations, for Xen the result on such implementations are:

----[ Xen-4.19-unstable  x86_64  debug=y  Tainted:   C    ]----
CPU:    0
RIP:    e008:[<0000000062ccfa70>] 0000000062ccfa70
[...]
Xen call trace:
   [<0000000062ccfa70>] R 0000000062ccfa70
   [<00000000732e9a3f>] S 00000000732e9a3f
   [<ffff82d04034f34f>] F arch/x86/time.c#get_cmos_time+0x1b3/0x26e
   [<ffff82d04045926f>] F init_xen_time+0x28/0xa4
   [<ffff82d040454bc4>] F __start_xen+0x1ee7/0x2578
   [<ffff82d040203334>] F __high_start+0x94/0xa0

Pagetable walk from 0000000062ccfa70:
 L4[0x000] = 000000207ef1c063 ffffffffffffffff
 L3[0x001] = 000000005d6c0063 ffffffffffffffff
 L2[0x116] = 8000000062c001e3 ffffffffffffffff (PSE)

****************************************
Panic on CPU 0:
FATAL PAGE FAULT
[error_code=0011]
Faulting linear address: 0000000062ccfa70
****************************************

Swap the preference to default to CMOS first, and EFI later, in an attempt to
use EFI_GET_TIME as a last resort option only.  Note that Linux for example
doesn't allow calling the get_time method, and instead provides a dummy handler
that unconditionally returns EFI_UNSUPPORTED on x86-64.

Such change in the preferences requires some re-arranging of the function
logic, so that panic messages with workaround suggestions are suitably printed.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Updated to match previous changes.
---
 xen/arch/x86/time.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index e4751684951e..b86e4d58b40c 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1592,14 +1592,14 @@ static void __init probe_wallclock(void)
         wallclock_source = WALLCLOCK_XEN;
         return;
     }
-    if ( efi_enabled(EFI_RS) && efi_get_time() )
+    if ( cmos_rtc_probe() )
     {
-        wallclock_source = WALLCLOCK_EFI;
+        wallclock_source = WALLCLOCK_CMOS;
         return;
     }
-    if ( cmos_rtc_probe() )
+    if ( efi_enabled(EFI_RS) && efi_get_time() )
     {
-        wallclock_source = WALLCLOCK_CMOS;
+        wallclock_source = WALLCLOCK_EFI;
         return;
     }
 
-- 
2.46.0

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Jan Beulich]

On 13.09.2024 09:59, Roger Pau Monne wrote:
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1552,6 +1552,37 @@ static const char *__init wallclock_type_to_string(void)
>      return "";
>  }
>  
> +static int __init cf_check parse_wallclock(const char *arg)
> +{
> +    wallclock_source = WALLCLOCK_UNSET;

With this ...

> +    if ( !arg )
> +        return -EINVAL;
> +
> +    if ( !strcmp("auto", arg) )
> +        ASSERT(wallclock_source == WALLCLOCK_UNSET);

... I'm not convinced this is (still) needed.

Jan

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Roger Pau Monné]

On Fri, Sep 13, 2024 at 02:38:14PM +0200, Jan Beulich wrote:
> On 13.09.2024 09:59, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1552,6 +1552,37 @@ static const char *__init wallclock_type_to_string(void)
> >      return "";
> >  }
> >  
> > +static int __init cf_check parse_wallclock(const char *arg)
> > +{
> > +    wallclock_source = WALLCLOCK_UNSET;
> 
> With this ...
> 
> > +    if ( !arg )
> > +        return -EINVAL;
> > +
> > +    if ( !strcmp("auto", arg) )
> > +        ASSERT(wallclock_source == WALLCLOCK_UNSET);
> 
> ... I'm not convinced this is (still) needed.

It reduces to an empty statement in release builds, and is IMO clearer
code wise.  I could replace the assert with a comment, but IMO the
assert conveys the same information in a more compact format and
provides extra safety in case the code is changed and wallclock_source
is no longer initialized to the expected value.

Thanks, Roger.

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Jan Beulich]

On 16.09.2024 09:50, Roger Pau Monné wrote:
> On Fri, Sep 13, 2024 at 02:38:14PM +0200, Jan Beulich wrote:
>> On 13.09.2024 09:59, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/time.c
>>> +++ b/xen/arch/x86/time.c
>>> @@ -1552,6 +1552,37 @@ static const char *__init wallclock_type_to_string(void)
>>>      return "";
>>>  }
>>>  
>>> +static int __init cf_check parse_wallclock(const char *arg)
>>> +{
>>> +    wallclock_source = WALLCLOCK_UNSET;
>>
>> With this ...
>>
>>> +    if ( !arg )
>>> +        return -EINVAL;
>>> +
>>> +    if ( !strcmp("auto", arg) )
>>> +        ASSERT(wallclock_source == WALLCLOCK_UNSET);
>>
>> ... I'm not convinced this is (still) needed.
> 
> It reduces to an empty statement in release builds, and is IMO clearer
> code wise.  I could replace the assert with a comment, but IMO the
> assert conveys the same information in a more compact format and
> provides extra safety in case the code is changed and wallclock_source
> is no longer initialized to the expected value.

Well, so be it then.

Jan

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Andrew Cooper]

On 13/09/2024 8:59 am, Roger Pau Monne wrote:
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 959cf45b55d9..2a9b3b9b8975 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2816,6 +2816,27 @@ vwfi to `native` reduces irq latency significantly. It can also lead to
>  suboptimal scheduling decisions, but only when the system is
>  oversubscribed (i.e., in total there are more vCPUs than pCPUs).
>  
> +### wallclock (x86)
> +> `= auto | xen | cmos | efi`
> +
> +> Default: `auto`
> +
> +Allow forcing the usage of a specific wallclock source.
> +
> + * `auto` let the hypervisor select the clocksource based on internal
> +   heuristics.
> +
> + * `xen` force usage of the Xen shared_info wallclock when booted as a Xen
> +   guest.  This option is only available if the hypervisor was compiled with
> +   `CONFIG_XEN_GUEST` enabled.
> +
> + * `cmos` force usage of the CMOS RTC wallclock.
> +
> + * `efi` force usage of the EFI_GET_TIME run-time method when booted from EFI
> +   firmware.

For both `xen` and `efi`, something should be said about "if selected
and not satisfied, Xen falls back to other heuristics".

> +
> +If the selected option is invalid or not available Xen will default to `auto`.

I'm afraid that I'm firmly of the opinion that "auto" on the cmdline is
unnecessary complexity.  Auto is the default, and doesn't need
specifying explicitly.  That in turn simplifies ...

> +
>  ### watchdog (x86)
>  > `= force | <boolean>`
>  
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index 29b026735e5d..e4751684951e 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1552,6 +1552,37 @@ static const char *__init wallclock_type_to_string(void)
>      return "";
>  }
>  
> +static int __init cf_check parse_wallclock(const char *arg)
> +{
> +    wallclock_source = WALLCLOCK_UNSET;
> +
> +    if ( !arg )
> +        return -EINVAL;
> +
> +    if ( !strcmp("auto", arg) )
> +        ASSERT(wallclock_source == WALLCLOCK_UNSET);

... this.

Hitting this assert will manifest as a system reboot/hang with no
information on serial/VGA, because all of this runs prior to getting up
the console.  You only get to see it on a PVH boot because we bodge the
Xen console into default-existence.

So, ASSERT()/etc really need avoiding wherever possible in cmdline parsing.

In this case, all it serves to do is break examples like `wallclock=xen
wallclock=auto` case, which is unlike our latest-takes-precedence
behaviour everywhere else.

> +    else if ( !strcmp("xen", arg) )
> +    {
> +        if ( !xen_guest )

We don't normally treat this path as an error when parsing (we know what
it is, even if we can't action it).  Instead, there's no_config_param()
to be more friendly (for PVH at least).

It's a bit awkward, but this should do:

    {
#ifdef CONFIG_XEN_GUEST
        wallclock_source = WALLCLOCK_XEN;
#else
        no_config_param("XEN_GUEST", "wallclock", s, ss);
#endif
    }

There probably wants to be something similar for EFI, although it's not
a plain CONFIG so it might be more tricky.

~Andrew

Re: [PATCH v7 2/2] x86/time: prefer CMOS over EFI_GET_TIME

[Andrew Cooper]

On 13/09/2024 8:59 am, Roger Pau Monne wrote:
> The EFI_GET_TIME implementation is well known to be broken for many firmware
> implementations, for Xen the result on such implementations are:
>
> ----[ Xen-4.19-unstable  x86_64  debug=y  Tainted:   C    ]----
> CPU:    0
> RIP:    e008:[<0000000062ccfa70>] 0000000062ccfa70
> [...]
> Xen call trace:
>    [<0000000062ccfa70>] R 0000000062ccfa70
>    [<00000000732e9a3f>] S 00000000732e9a3f
>    [<ffff82d04034f34f>] F arch/x86/time.c#get_cmos_time+0x1b3/0x26e
>    [<ffff82d04045926f>] F init_xen_time+0x28/0xa4
>    [<ffff82d040454bc4>] F __start_xen+0x1ee7/0x2578
>    [<ffff82d040203334>] F __high_start+0x94/0xa0
>
> Pagetable walk from 0000000062ccfa70:
>  L4[0x000] = 000000207ef1c063 ffffffffffffffff
>  L3[0x001] = 000000005d6c0063 ffffffffffffffff
>  L2[0x116] = 8000000062c001e3 ffffffffffffffff (PSE)
>
> ****************************************
> Panic on CPU 0:
> FATAL PAGE FAULT
> [error_code=0011]
> Faulting linear address: 0000000062ccfa70
> ****************************************
>
> Swap the preference to default to CMOS first, and EFI later, in an attempt to
> use EFI_GET_TIME as a last resort option only.  Note that Linux for example
> doesn't allow calling the get_time method, and instead provides a dummy handler
> that unconditionally returns EFI_UNSUPPORTED on x86-64.
>
> Such change in the preferences requires some re-arranging of the function
> logic, so that panic messages with workaround suggestions are suitably printed.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

This brings us better-in-line with the rest of the world on the matter.

~Andrew

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 4226 bytes --]

On Mon, Sep 16, 2024 at 02:11:08PM +0100, Andrew Cooper wrote:
> On 13/09/2024 8:59 am, Roger Pau Monne wrote:
> > diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> > index 959cf45b55d9..2a9b3b9b8975 100644
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -2816,6 +2816,27 @@ vwfi to `native` reduces irq latency significantly. It can also lead to
> >  suboptimal scheduling decisions, but only when the system is
> >  oversubscribed (i.e., in total there are more vCPUs than pCPUs).
> >  
> > +### wallclock (x86)
> > +> `= auto | xen | cmos | efi`
> > +
> > +> Default: `auto`
> > +
> > +Allow forcing the usage of a specific wallclock source.
> > +
> > + * `auto` let the hypervisor select the clocksource based on internal
> > +   heuristics.
> > +
> > + * `xen` force usage of the Xen shared_info wallclock when booted as a Xen
> > +   guest.  This option is only available if the hypervisor was compiled with
> > +   `CONFIG_XEN_GUEST` enabled.
> > +
> > + * `cmos` force usage of the CMOS RTC wallclock.
> > +
> > + * `efi` force usage of the EFI_GET_TIME run-time method when booted from EFI
> > +   firmware.
> 
> For both `xen` and `efi`, something should be said about "if selected
> and not satisfied, Xen falls back to other heuristics".
> 
> > +
> > +If the selected option is invalid or not available Xen will default to `auto`.
> 
> I'm afraid that I'm firmly of the opinion that "auto" on the cmdline is
> unnecessary complexity.  Auto is the default, and doesn't need
> specifying explicitly.  That in turn simplifies ...

What about overriding earlier choice? For example overriding a built-in
cmdline? That said, with the current code, the same can be achieved with
wallclock=foo, and living with the warning in boot log...

> > +
> >  ### watchdog (x86)
> >  > `= force | <boolean>`
> >  
> > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> > index 29b026735e5d..e4751684951e 100644
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1552,6 +1552,37 @@ static const char *__init wallclock_type_to_string(void)
> >      return "";
> >  }
> >  
> > +static int __init cf_check parse_wallclock(const char *arg)
> > +{
> > +    wallclock_source = WALLCLOCK_UNSET;
> > +
> > +    if ( !arg )
> > +        return -EINVAL;
> > +
> > +    if ( !strcmp("auto", arg) )
> > +        ASSERT(wallclock_source == WALLCLOCK_UNSET);
> 
> ... this.
> 
> Hitting this assert will manifest as a system reboot/hang with no
> information on serial/VGA, because all of this runs prior to getting up
> the console.  You only get to see it on a PVH boot because we bodge the
> Xen console into default-existence.

This assert is no-op as wallclock_source is unconditionally set to WALLCLOCK_UNSET few lines above.

> So, ASSERT()/etc really need avoiding wherever possible in cmdline parsing.
> 
> In this case, all it serves to do is break examples like `wallclock=xen
> wallclock=auto` case, which is unlike our latest-takes-precedence
> behaviour everywhere else.
> 
> > +    else if ( !strcmp("xen", arg) )
> > +    {
> > +        if ( !xen_guest )
> 
> We don't normally treat this path as an error when parsing (we know what
> it is, even if we can't action it).  Instead, there's no_config_param()
> to be more friendly (for PVH at least).
> 
> It's a bit awkward, but this should do:
> 
>     {
> #ifdef CONFIG_XEN_GUEST
>         wallclock_source = WALLCLOCK_XEN;
> #else
>         no_config_param("XEN_GUEST", "wallclock", s, ss);
> #endif
>     }

Can you boot the binary build with CONFIG_XEN_GUEST=y as native? If so,
the above will not be enough, a runtime check is needed anyway.

> There probably wants to be something similar for EFI, although it's not
> a plain CONFIG so it might be more tricky.

It needs to be runtime check here even more. Not only because of
different boot modes, but due to interaction with efi=no-rs (or any
other reason for not having runtime services). See the comment there.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Roger Pau Monné]

On Mon, Sep 16, 2024 at 03:20:56PM +0200, Marek Marczykowski-Górecki wrote:
> On Mon, Sep 16, 2024 at 02:11:08PM +0100, Andrew Cooper wrote:
> > On 13/09/2024 8:59 am, Roger Pau Monne wrote:
> > > diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> > > index 959cf45b55d9..2a9b3b9b8975 100644
> > > --- a/docs/misc/xen-command-line.pandoc
> > > +++ b/docs/misc/xen-command-line.pandoc
> > > @@ -2816,6 +2816,27 @@ vwfi to `native` reduces irq latency significantly. It can also lead to
> > >  suboptimal scheduling decisions, but only when the system is
> > >  oversubscribed (i.e., in total there are more vCPUs than pCPUs).
> > >  
> > > +### wallclock (x86)
> > > +> `= auto | xen | cmos | efi`
> > > +
> > > +> Default: `auto`
> > > +
> > > +Allow forcing the usage of a specific wallclock source.
> > > +
> > > + * `auto` let the hypervisor select the clocksource based on internal
> > > +   heuristics.
> > > +
> > > + * `xen` force usage of the Xen shared_info wallclock when booted as a Xen
> > > +   guest.  This option is only available if the hypervisor was compiled with
> > > +   `CONFIG_XEN_GUEST` enabled.
> > > +
> > > + * `cmos` force usage of the CMOS RTC wallclock.
> > > +
> > > + * `efi` force usage of the EFI_GET_TIME run-time method when booted from EFI
> > > +   firmware.
> > 
> > For both `xen` and `efi`, something should be said about "if selected
> > and not satisfied, Xen falls back to other heuristics".

There's a line just below that notes this: "If the selected option is
invalid or not available Xen will default to `auto`."  I think it's
clearer than having to comment on every specific option.

> > > +
> > > +If the selected option is invalid or not available Xen will default to `auto`.
> > 
> > I'm afraid that I'm firmly of the opinion that "auto" on the cmdline is
> > unnecessary complexity.  Auto is the default, and doesn't need
> > specifying explicitly.  That in turn simplifies ...
> 
> What about overriding earlier choice? For example overriding a built-in
> cmdline? That said, with the current code, the same can be achieved with
> wallclock=foo, and living with the warning in boot log...

It's IMO weird to ask the users to use wallclock=foo to override a
previous command line wallclock option and fallback to the default
behavior.

> > > +
> > >  ### watchdog (x86)
> > >  > `= force | <boolean>`
> > >  
> > > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> > > index 29b026735e5d..e4751684951e 100644
> > > --- a/xen/arch/x86/time.c
> > > +++ b/xen/arch/x86/time.c
> > > @@ -1552,6 +1552,37 @@ static const char *__init wallclock_type_to_string(void)
> > >      return "";
> > >  }
> > >  
> > > +static int __init cf_check parse_wallclock(const char *arg)
> > > +{
> > > +    wallclock_source = WALLCLOCK_UNSET;
> > > +
> > > +    if ( !arg )
> > > +        return -EINVAL;
> > > +
> > > +    if ( !strcmp("auto", arg) )
> > > +        ASSERT(wallclock_source == WALLCLOCK_UNSET);
> > 
> > ... this.
> > 
> > Hitting this assert will manifest as a system reboot/hang with no
> > information on serial/VGA, because all of this runs prior to getting up
> > the console.  You only get to see it on a PVH boot because we bodge the
> > Xen console into default-existence.
> 
> This assert is no-op as wallclock_source is unconditionally set to WALLCLOCK_UNSET few lines above.

As mentioned to Jan - I find it nicer than adding an empty statement.

> > So, ASSERT()/etc really need avoiding wherever possible in cmdline parsing.
> > 
> > In this case, all it serves to do is break examples like `wallclock=xen
> > wallclock=auto` case, which is unlike our latest-takes-precedence
> > behaviour everywhere else.
> > 
> > > +    else if ( !strcmp("xen", arg) )
> > > +    {
> > > +        if ( !xen_guest )
> > 
> > We don't normally treat this path as an error when parsing (we know what
> > it is, even if we can't action it).  Instead, there's no_config_param()
> > to be more friendly (for PVH at least).
> > 
> > It's a bit awkward, but this should do:
> > 
> >     {
> > #ifdef CONFIG_XEN_GUEST
> >         wallclock_source = WALLCLOCK_XEN;
> > #else
> >         no_config_param("XEN_GUEST", "wallclock", s, ss);
> > #endif
> >     }
> 
> Can you boot the binary build with CONFIG_XEN_GUEST=y as native? If so,
> the above will not be enough, a runtime check is needed anyway.
> 
> > There probably wants to be something similar for EFI, although it's not
> > a plain CONFIG so it might be more tricky.
> 
> It needs to be runtime check here even more. Not only because of
> different boot modes, but due to interaction with efi=no-rs (or any
> other reason for not having runtime services). See the comment there.

I agree with Marek, no_config_param() is not enough here: Xen needs to
ensure it has been booted as a Xen guest, or that EFI run-time
services are enabled in order to ensure the selected clock source is
available.

Thanks, Roger.

Re: [PATCH v7 2/2] x86/time: prefer CMOS over EFI_GET_TIME

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]

On Fri, Sep 13, 2024 at 09:59:07AM +0200, Roger Pau Monne wrote:
> The EFI_GET_TIME implementation is well known to be broken for many firmware
> implementations, for Xen the result on such implementations are:
> 
> ----[ Xen-4.19-unstable  x86_64  debug=y  Tainted:   C    ]----
> CPU:    0
> RIP:    e008:[<0000000062ccfa70>] 0000000062ccfa70
> [...]
> Xen call trace:
>    [<0000000062ccfa70>] R 0000000062ccfa70
>    [<00000000732e9a3f>] S 00000000732e9a3f
>    [<ffff82d04034f34f>] F arch/x86/time.c#get_cmos_time+0x1b3/0x26e
>    [<ffff82d04045926f>] F init_xen_time+0x28/0xa4
>    [<ffff82d040454bc4>] F __start_xen+0x1ee7/0x2578
>    [<ffff82d040203334>] F __high_start+0x94/0xa0
> 
> Pagetable walk from 0000000062ccfa70:
>  L4[0x000] = 000000207ef1c063 ffffffffffffffff
>  L3[0x001] = 000000005d6c0063 ffffffffffffffff
>  L2[0x116] = 8000000062c001e3 ffffffffffffffff (PSE)
> 
> ****************************************
> Panic on CPU 0:
> FATAL PAGE FAULT
> [error_code=0011]
> Faulting linear address: 0000000062ccfa70
> ****************************************
> 
> Swap the preference to default to CMOS first, and EFI later, in an attempt to
> use EFI_GET_TIME as a last resort option only.  Note that Linux for example
> doesn't allow calling the get_time method, and instead provides a dummy handler
> that unconditionally returns EFI_UNSUPPORTED on x86-64.
> 
> Such change in the preferences requires some re-arranging of the function
> logic, so that panic messages with workaround suggestions are suitably printed.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Since this changes behavior for running on EFI,
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> ---
> Changes since v2:
>  - Updated to match previous changes.
> ---
>  xen/arch/x86/time.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index e4751684951e..b86e4d58b40c 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1592,14 +1592,14 @@ static void __init probe_wallclock(void)
>          wallclock_source = WALLCLOCK_XEN;
>          return;
>      }
> -    if ( efi_enabled(EFI_RS) && efi_get_time() )
> +    if ( cmos_rtc_probe() )
>      {
> -        wallclock_source = WALLCLOCK_EFI;
> +        wallclock_source = WALLCLOCK_CMOS;
>          return;
>      }
> -    if ( cmos_rtc_probe() )
> +    if ( efi_enabled(EFI_RS) && efi_get_time() )
>      {
> -        wallclock_source = WALLCLOCK_CMOS;
> +        wallclock_source = WALLCLOCK_EFI;
>          return;
>      }
>  
> -- 
> 2.46.0
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 4312 bytes --]

On Fri, Sep 13, 2024 at 09:59:06AM +0200, Roger Pau Monne wrote:
> Allow setting the used wallclock from the command line.  When the option is set
> to a value different than `auto` the probing is bypassed and the selected
> implementation is used (as long as it's available).
> 
> The `xen` and `efi` options require being booted as a Xen guest (with Xen guest
> supported built-in) or from UEFI firmware respectively.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> ---
> Changes since v6:
>  - Clarify documentation regarding repeated using of the wallclock command line
>    option.
> 
> Changes since v5:
>  - Do EFI run-time services checking after command line parsing.
> 
> Changes since v3:
>  - Note xen option is only available if Xen guest support it built.
>  - Fix typo.
> ---
>  docs/misc/xen-command-line.pandoc | 21 ++++++++++++++++
>  xen/arch/x86/time.c               | 41 ++++++++++++++++++++++++++++++-
>  2 files changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 959cf45b55d9..2a9b3b9b8975 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2816,6 +2816,27 @@ vwfi to `native` reduces irq latency significantly. It can also lead to
>  suboptimal scheduling decisions, but only when the system is
>  oversubscribed (i.e., in total there are more vCPUs than pCPUs).
>  
> +### wallclock (x86)
> +> `= auto | xen | cmos | efi`
> +
> +> Default: `auto`
> +
> +Allow forcing the usage of a specific wallclock source.
> +
> + * `auto` let the hypervisor select the clocksource based on internal
> +   heuristics.
> +
> + * `xen` force usage of the Xen shared_info wallclock when booted as a Xen
> +   guest.  This option is only available if the hypervisor was compiled with
> +   `CONFIG_XEN_GUEST` enabled.
> +
> + * `cmos` force usage of the CMOS RTC wallclock.
> +
> + * `efi` force usage of the EFI_GET_TIME run-time method when booted from EFI
> +   firmware.
> +
> +If the selected option is invalid or not available Xen will default to `auto`.
> +
>  ### watchdog (x86)
>  > `= force | <boolean>`
>  
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index 29b026735e5d..e4751684951e 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1552,6 +1552,37 @@ static const char *__init wallclock_type_to_string(void)
>      return "";
>  }
>  
> +static int __init cf_check parse_wallclock(const char *arg)
> +{
> +    wallclock_source = WALLCLOCK_UNSET;
> +
> +    if ( !arg )
> +        return -EINVAL;
> +
> +    if ( !strcmp("auto", arg) )
> +        ASSERT(wallclock_source == WALLCLOCK_UNSET);
> +    else if ( !strcmp("xen", arg) )
> +    {
> +        if ( !xen_guest )
> +            return -EINVAL;
> +
> +        wallclock_source = WALLCLOCK_XEN;
> +    }
> +    else if ( !strcmp("cmos", arg) )
> +        wallclock_source = WALLCLOCK_CMOS;
> +    else if ( !strcmp("efi", arg) )
> +        /*
> +         * Checking if run-time services are available must be done after
> +         * command line parsing.
> +         */
> +        wallclock_source = WALLCLOCK_EFI;
> +    else
> +        return -EINVAL;
> +
> +    return 0;
> +}
> +custom_param("wallclock", parse_wallclock);
> +
>  static void __init probe_wallclock(void)
>  {
>      ASSERT(wallclock_source == WALLCLOCK_UNSET);
> @@ -2527,7 +2558,15 @@ int __init init_xen_time(void)
>  
>      open_softirq(TIME_CALIBRATE_SOFTIRQ, local_time_calibration);
>  
> -    probe_wallclock();
> +    /*
> +     * EFI run time services can be disabled from the command line, hence the
> +     * check for them cannot be done as part of the wallclock option parsing.
> +     */
> +    if ( wallclock_source == WALLCLOCK_EFI && !efi_enabled(EFI_RS) )
> +        wallclock_source = WALLCLOCK_UNSET;
> +
> +    if ( wallclock_source == WALLCLOCK_UNSET )
> +        probe_wallclock();
>  
>      printk(XENLOG_INFO "Wallclock source: %s\n", wallclock_type_to_string());
>  
> -- 
> 2.46.0
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Roger Pau Monné]

On Mon, Jan 13, 2025 at 05:07:55PM +0100, Marek Marczykowski-Górecki wrote:
> On Fri, Sep 13, 2024 at 09:59:06AM +0200, Roger Pau Monne wrote:
> > Allow setting the used wallclock from the command line.  When the option is set
> > to a value different than `auto` the probing is bypassed and the selected
> > implementation is used (as long as it's available).
> > 
> > The `xen` and `efi` options require being booted as a Xen guest (with Xen guest
> > supported built-in) or from UEFI firmware respectively.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Thanks for the review.

Oleksii, can I get your opinion as Release Manager about whether this
(and the following patch) would be suitable for committing to staging
given the current release state?

It's a workaround for broken EFI implementations that many downstreams
carry on their patch queue.

Thanks, Roger.

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]


On 1/13/25 6:52 PM, Roger Pau Monné wrote:
> On Mon, Jan 13, 2025 at 05:07:55PM +0100, Marek Marczykowski-Górecki wrote:
>> On Fri, Sep 13, 2024 at 09:59:06AM +0200, Roger Pau Monne wrote:
>>> Allow setting the used wallclock from the command line.  When the option is set
>>> to a value different than `auto` the probing is bypassed and the selected
>>> implementation is used (as long as it's available).
>>>
>>> The `xen` and `efi` options require being booted as a Xen guest (with Xen guest
>>> supported built-in) or from UEFI firmware respectively.
>>>
>>> Signed-off-by: Roger Pau Monné<roger.pau@citrix.com>
>> Reviewed-by: Marek Marczykowski-Górecki<marmarek@invisiblethingslab.com>
> Thanks for the review.
>
> Oleksii, can I get your opinion as Release Manager about whether this
> (and the following patch) would be suitable for committing to staging
> given the current release state?
>
> It's a workaround for broken EFI implementations that many downstreams
> carry on their patch queue.

Based on your commit message, I understand this as addressing a bug ( but not very critical
as IIUC downstreams have the similar patch on their side ). Therefore, if it has been properly
reviewed and tested, we should consider including it in the current release.

IIUC, setting the wallclock to EFI should align with the behavior Xen had previously.
It might be preferable to use that argument as the default for a while, allowing others to verify the "auto"
value over time. After that, we could consider making "auto" the default.
That said, I am not particularly strict about following this approach.

~ Oleksii


>
> Thanks, Roger.

[-- Attachment #2: Type: text/html, Size: 2606 bytes --]

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Roger Pau Monné]

On Tue, Jan 14, 2025 at 12:12:03PM +0100, Oleksii Kurochko wrote:
> 
> On 1/13/25 6:52 PM, Roger Pau Monné wrote:
> > On Mon, Jan 13, 2025 at 05:07:55PM +0100, Marek Marczykowski-Górecki wrote:
> > > On Fri, Sep 13, 2024 at 09:59:06AM +0200, Roger Pau Monne wrote:
> > > > Allow setting the used wallclock from the command line.  When the option is set
> > > > to a value different than `auto` the probing is bypassed and the selected
> > > > implementation is used (as long as it's available).
> > > > 
> > > > The `xen` and `efi` options require being booted as a Xen guest (with Xen guest
> > > > supported built-in) or from UEFI firmware respectively.
> > > > 
> > > > Signed-off-by: Roger Pau Monné<roger.pau@citrix.com>
> > > Reviewed-by: Marek Marczykowski-Górecki<marmarek@invisiblethingslab.com>
> > Thanks for the review.
> > 
> > Oleksii, can I get your opinion as Release Manager about whether this
> > (and the following patch) would be suitable for committing to staging
> > given the current release state?
> > 
> > It's a workaround for broken EFI implementations that many downstreams
> > carry on their patch queue.
> 
> Based on your commit message, I understand this as addressing a bug ( but not very critical
> as IIUC downstreams have the similar patch on their side ). Therefore, if it has been properly
> reviewed and tested, we should consider including it in the current release.

IIRC at least Qubes, XenServer and XCP-ng have a patch that achieves
the same behavior as proposed here.

> IIUC, setting the wallclock to EFI should align with the behavior Xen had previously.
> It might be preferable to use that argument as the default for a while, allowing others to verify the "auto"
> value over time. After that, we could consider making "auto" the default.
> That said, I am not particularly strict about following this approach.

We cannot really set efi as the default, as it would break when
booting on legacy BIOS systems.

We could take only patch 1 and leave patch 2 after Xen 4.20 has
branched, but at that point I would see little benefit in having just
patch 1.

I don't have a strong opinion, but downstreams have been complaining
about Xen behavior regarding the usage of EFI_GET_TIME, so it might be
good to not ship yet another release with such allegedly broken
behavior.

Let me know what you think, as I would need a formal Release-Ack if
this is to be committed.

Thanks, Roger.

> ~ Oleksii
> 
> 
> > 
> > Thanks, Roger.

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 2875 bytes --]


On 1/14/25 12:27 PM, Roger Pau Monné wrote:
> On Tue, Jan 14, 2025 at 12:12:03PM +0100, Oleksii Kurochko wrote:
>> On 1/13/25 6:52 PM, Roger Pau Monné wrote:
>>> On Mon, Jan 13, 2025 at 05:07:55PM +0100, Marek Marczykowski-Górecki wrote:
>>>> On Fri, Sep 13, 2024 at 09:59:06AM +0200, Roger Pau Monne wrote:
>>>>> Allow setting the used wallclock from the command line.  When the option is set
>>>>> to a value different than `auto` the probing is bypassed and the selected
>>>>> implementation is used (as long as it's available).
>>>>>
>>>>> The `xen` and `efi` options require being booted as a Xen guest (with Xen guest
>>>>> supported built-in) or from UEFI firmware respectively.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné<roger.pau@citrix.com>
>>>> Reviewed-by: Marek Marczykowski-Górecki<marmarek@invisiblethingslab.com>
>>> Thanks for the review.
>>>
>>> Oleksii, can I get your opinion as Release Manager about whether this
>>> (and the following patch) would be suitable for committing to staging
>>> given the current release state?
>>>
>>> It's a workaround for broken EFI implementations that many downstreams
>>> carry on their patch queue.
>> Based on your commit message, I understand this as addressing a bug ( but not very critical
>> as IIUC downstreams have the similar patch on their side ). Therefore, if it has been properly
>> reviewed and tested, we should consider including it in the current release.
> IIRC at least Qubes, XenServer and XCP-ng have a patch that achieves
> the same behavior as proposed here.
>
>> IIUC, setting the wallclock to EFI should align with the behavior Xen had previously.
>> It might be preferable to use that argument as the default for a while, allowing others to verify the "auto"
>> value over time. After that, we could consider making "auto" the default.
>> That said, I am not particularly strict about following this approach.
> We cannot really set efi as the default, as it would break when
> booting on legacy BIOS systems.
>
> We could take only patch 1 and leave patch 2 after Xen 4.20 has
> branched, but at that point I would see little benefit in having just
> patch 1.

I don't see a lot of benefit of comitting only the one patch either.


>
> I don't have a strong opinion, but downstreams have been complaining
> about Xen behavior regarding the usage of EFI_GET_TIME, so it might be
> good to not ship yet another release with such allegedly broken
> behavior.

Agree with that. As  I mentioned above I consider it as a bug and based on
that several mentioned above downstreams have the similar patch I could consider
that as tested approach so ..

>
> Let me know what you think, as I would need a formal Release-Ack if
> this is to be committed.

... R-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>.

Thanks.

~ Oleksii

>
> Thanks, Roger.
>
>> ~ Oleksii
>>
>>
>>> Thanks, Roger.

[-- Attachment #2: Type: text/html, Size: 4929 bytes --]

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 3255 bytes --]


On 1/14/25 12:40 PM, Oleksii Kurochko wrote:
>
>
> On 1/14/25 12:27 PM, Roger Pau Monné wrote:
>> On Tue, Jan 14, 2025 at 12:12:03PM +0100, Oleksii Kurochko wrote:
>>> On 1/13/25 6:52 PM, Roger Pau Monné wrote:
>>>> On Mon, Jan 13, 2025 at 05:07:55PM +0100, Marek Marczykowski-Górecki wrote:
>>>>> On Fri, Sep 13, 2024 at 09:59:06AM +0200, Roger Pau Monne wrote:
>>>>>> Allow setting the used wallclock from the command line.  When the option is set
>>>>>> to a value different than `auto` the probing is bypassed and the selected
>>>>>> implementation is used (as long as it's available).
>>>>>>
>>>>>> The `xen` and `efi` options require being booted as a Xen guest (with Xen guest
>>>>>> supported built-in) or from UEFI firmware respectively.
>>>>>>
>>>>>> Signed-off-by: Roger Pau Monné<roger.pau@citrix.com>
>>>>> Reviewed-by: Marek Marczykowski-Górecki<marmarek@invisiblethingslab.com>
>>>> Thanks for the review.
>>>>
>>>> Oleksii, can I get your opinion as Release Manager about whether this
>>>> (and the following patch) would be suitable for committing to staging
>>>> given the current release state?
>>>>
>>>> It's a workaround for broken EFI implementations that many downstreams
>>>> carry on their patch queue.
>>> Based on your commit message, I understand this as addressing a bug ( but not very critical
>>> as IIUC downstreams have the similar patch on their side ). Therefore, if it has been properly
>>> reviewed and tested, we should consider including it in the current release.
>> IIRC at least Qubes, XenServer and XCP-ng have a patch that achieves
>> the same behavior as proposed here.
>>
>>> IIUC, setting the wallclock to EFI should align with the behavior Xen had previously.
>>> It might be preferable to use that argument as the default for a while, allowing others to verify the "auto"
>>> value over time. After that, we could consider making "auto" the default.
>>> That said, I am not particularly strict about following this approach.
>> We cannot really set efi as the default, as it would break when
>> booting on legacy BIOS systems.
>>
>> We could take only patch 1 and leave patch 2 after Xen 4.20 has
>> branched, but at that point I would see little benefit in having just
>> patch 1.
> I don't see a lot of benefit of comitting only the one patch either.
>
>
>> I don't have a strong opinion, but downstreams have been complaining
>> about Xen behavior regarding the usage of EFI_GET_TIME, so it might be
>> good to not ship yet another release with such allegedly broken
>> behavior.
> Agree with that. As  I mentioned above I consider it as a bug and based on
> that several mentioned above downstreams have the similar patch I could consider
> that as tested approach so ..
>> Let me know what you think, as I would need a formal Release-Ack if
>> this is to be committed.
> ... R-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>.

Sorry for the noise.

I missed to add that it would also be nice to mention IMO ( that now we have wallclock parameter )
in CHANGELOG so downstreams will receive "notification" that Xen provides a workaround for the mentioned
issue in the patch series.

Thanks.

~ Oleksii

>
> Thanks.
> ~ Oleksii
>> Thanks, Roger.
>>
>>> ~ Oleksii
>>>
>>>
>>>> Thanks, Roger.

[-- Attachment #2: Type: text/html, Size: 5951 bytes --]

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Roger Pau Monné]

On Tue, Jan 14, 2025 at 12:44:39PM +0100, Oleksii Kurochko wrote:
> 
> On 1/14/25 12:40 PM, Oleksii Kurochko wrote:
> > 
> > 
> > On 1/14/25 12:27 PM, Roger Pau Monné wrote:
> > > On Tue, Jan 14, 2025 at 12:12:03PM +0100, Oleksii Kurochko wrote:
> > > > On 1/13/25 6:52 PM, Roger Pau Monné wrote:
> > > > > On Mon, Jan 13, 2025 at 05:07:55PM +0100, Marek Marczykowski-Górecki wrote:
> > > > > > On Fri, Sep 13, 2024 at 09:59:06AM +0200, Roger Pau Monne wrote:
> > > > > > > Allow setting the used wallclock from the command line.  When the option is set
> > > > > > > to a value different than `auto` the probing is bypassed and the selected
> > > > > > > implementation is used (as long as it's available).
> > > > > > > 
> > > > > > > The `xen` and `efi` options require being booted as a Xen guest (with Xen guest
> > > > > > > supported built-in) or from UEFI firmware respectively.
> > > > > > > 
> > > > > > > Signed-off-by: Roger Pau Monné<roger.pau@citrix.com>
> > > > > > Reviewed-by: Marek Marczykowski-Górecki<marmarek@invisiblethingslab.com>
> > > > > Thanks for the review.
> > > > > 
> > > > > Oleksii, can I get your opinion as Release Manager about whether this
> > > > > (and the following patch) would be suitable for committing to staging
> > > > > given the current release state?
> > > > > 
> > > > > It's a workaround for broken EFI implementations that many downstreams
> > > > > carry on their patch queue.
> > > > Based on your commit message, I understand this as addressing a bug ( but not very critical
> > > > as IIUC downstreams have the similar patch on their side ). Therefore, if it has been properly
> > > > reviewed and tested, we should consider including it in the current release.
> > > IIRC at least Qubes, XenServer and XCP-ng have a patch that achieves
> > > the same behavior as proposed here.
> > > 
> > > > IIUC, setting the wallclock to EFI should align with the behavior Xen had previously.
> > > > It might be preferable to use that argument as the default for a while, allowing others to verify the "auto"
> > > > value over time. After that, we could consider making "auto" the default.
> > > > That said, I am not particularly strict about following this approach.
> > > We cannot really set efi as the default, as it would break when
> > > booting on legacy BIOS systems.
> > > 
> > > We could take only patch 1 and leave patch 2 after Xen 4.20 has
> > > branched, but at that point I would see little benefit in having just
> > > patch 1.
> > I don't see a lot of benefit of comitting only the one patch either.
> > 
> > 
> > > I don't have a strong opinion, but downstreams have been complaining
> > > about Xen behavior regarding the usage of EFI_GET_TIME, so it might be
> > > good to not ship yet another release with such allegedly broken
> > > behavior.
> > Agree with that. As  I mentioned above I consider it as a bug and based on
> > that several mentioned above downstreams have the similar patch I could consider
> > that as tested approach so ..
> > > Let me know what you think, as I would need a formal Release-Ack if
> > > this is to be committed.
> > ... R-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>.
> 
> Sorry for the noise.
> 
> I missed to add that it would also be nice to mention IMO ( that now we have wallclock parameter )
> in CHANGELOG so downstreams will receive "notification" that Xen provides a workaround for the mentioned
> issue in the patch series.

Indeed.  Would you be OK with adding the following chunk to patch 2?

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8507e6556a56..1de1d1eca17f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
    leaving this to the guest kernel to do in guest context.
  - On x86:
    - Prefer ACPI reboot over UEFI ResetSystem() run time service call.
+   - Prefer CMOS over EFI_GET_TIME as time source.
    - Switched the xAPIC flat driver to use physical destination mode for external
      interrupts instead of logical destination mode.
 
@@ -24,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
    - Support for LLC (Last Level Cache) coloring.
  - On x86:
    - xl suspend/resume subcommands.
+   - `wallclock` command line option to select time source.
 
 ### Removed
  - On x86:

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 4483 bytes --]


On 1/14/25 1:13 PM, Roger Pau Monné wrote:
> On Tue, Jan 14, 2025 at 12:44:39PM +0100, Oleksii Kurochko wrote:
>> On 1/14/25 12:40 PM, Oleksii Kurochko wrote:
>>>
>>> On 1/14/25 12:27 PM, Roger Pau Monné wrote:
>>>> On Tue, Jan 14, 2025 at 12:12:03PM +0100, Oleksii Kurochko wrote:
>>>>> On 1/13/25 6:52 PM, Roger Pau Monné wrote:
>>>>>> On Mon, Jan 13, 2025 at 05:07:55PM +0100, Marek Marczykowski-Górecki wrote:
>>>>>>> On Fri, Sep 13, 2024 at 09:59:06AM +0200, Roger Pau Monne wrote:
>>>>>>>> Allow setting the used wallclock from the command line.  When the option is set
>>>>>>>> to a value different than `auto` the probing is bypassed and the selected
>>>>>>>> implementation is used (as long as it's available).
>>>>>>>>
>>>>>>>> The `xen` and `efi` options require being booted as a Xen guest (with Xen guest
>>>>>>>> supported built-in) or from UEFI firmware respectively.
>>>>>>>>
>>>>>>>> Signed-off-by: Roger Pau Monné<roger.pau@citrix.com>
>>>>>>> Reviewed-by: Marek Marczykowski-Górecki<marmarek@invisiblethingslab.com>
>>>>>> Thanks for the review.
>>>>>>
>>>>>> Oleksii, can I get your opinion as Release Manager about whether this
>>>>>> (and the following patch) would be suitable for committing to staging
>>>>>> given the current release state?
>>>>>>
>>>>>> It's a workaround for broken EFI implementations that many downstreams
>>>>>> carry on their patch queue.
>>>>> Based on your commit message, I understand this as addressing a bug ( but not very critical
>>>>> as IIUC downstreams have the similar patch on their side ). Therefore, if it has been properly
>>>>> reviewed and tested, we should consider including it in the current release.
>>>> IIRC at least Qubes, XenServer and XCP-ng have a patch that achieves
>>>> the same behavior as proposed here.
>>>>
>>>>> IIUC, setting the wallclock to EFI should align with the behavior Xen had previously.
>>>>> It might be preferable to use that argument as the default for a while, allowing others to verify the "auto"
>>>>> value over time. After that, we could consider making "auto" the default.
>>>>> That said, I am not particularly strict about following this approach.
>>>> We cannot really set efi as the default, as it would break when
>>>> booting on legacy BIOS systems.
>>>>
>>>> We could take only patch 1 and leave patch 2 after Xen 4.20 has
>>>> branched, but at that point I would see little benefit in having just
>>>> patch 1.
>>> I don't see a lot of benefit of comitting only the one patch either.
>>>
>>>
>>>> I don't have a strong opinion, but downstreams have been complaining
>>>> about Xen behavior regarding the usage of EFI_GET_TIME, so it might be
>>>> good to not ship yet another release with such allegedly broken
>>>> behavior.
>>> Agree with that. As  I mentioned above I consider it as a bug and based on
>>> that several mentioned above downstreams have the similar patch I could consider
>>> that as tested approach so ..
>>>> Let me know what you think, as I would need a formal Release-Ack if
>>>> this is to be committed.
>>> ... R-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>.
>> Sorry for the noise.
>>
>> I missed to add that it would also be nice to mention IMO ( that now we have wallclock parameter )
>> in CHANGELOG so downstreams will receive "notification" that Xen provides a workaround for the mentioned
>> issue in the patch series.
> Indeed.  Would you be OK with adding the following chunk to patch 2?
>
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 8507e6556a56..1de1d1eca17f 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>      leaving this to the guest kernel to do in guest context.
>    - On x86:
>      - Prefer ACPI reboot over UEFI ResetSystem() run time service call.
> +   - Prefer CMOS over EFI_GET_TIME as time source.
>      - Switched the xAPIC flat driver to use physical destination mode for external
>        interrupts instead of logical destination mode.
>   
> @@ -24,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>      - Support for LLC (Last Level Cache) coloring.
>    - On x86:
>      - xl suspend/resume subcommands.
> +   - `wallclock` command line option to select time source.

What about of adding word 'Add' before `wallclock`?

Other LGTM: Acked-By: Oleksii Kurochko<oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

>   
>   ### Removed
>    - On x86:

[-- Attachment #2: Type: text/html, Size: 6823 bytes --]

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Roger Pau Monné]

On Tue, Jan 14, 2025 at 03:23:21PM +0100, Oleksii Kurochko wrote:
> 
> On 1/14/25 1:13 PM, Roger Pau Monné wrote:
> > On Tue, Jan 14, 2025 at 12:44:39PM +0100, Oleksii Kurochko wrote:
> > > On 1/14/25 12:40 PM, Oleksii Kurochko wrote:
> > > > 
> > > > On 1/14/25 12:27 PM, Roger Pau Monné wrote:
> > > > > On Tue, Jan 14, 2025 at 12:12:03PM +0100, Oleksii Kurochko wrote:
> > > > > > On 1/13/25 6:52 PM, Roger Pau Monné wrote:
> > > > > > > On Mon, Jan 13, 2025 at 05:07:55PM +0100, Marek Marczykowski-Górecki wrote:
> > > > > > > > On Fri, Sep 13, 2024 at 09:59:06AM +0200, Roger Pau Monne wrote:
> > > > > > > > > Allow setting the used wallclock from the command line.  When the option is set
> > > > > > > > > to a value different than `auto` the probing is bypassed and the selected
> > > > > > > > > implementation is used (as long as it's available).
> > > > > > > > > 
> > > > > > > > > The `xen` and `efi` options require being booted as a Xen guest (with Xen guest
> > > > > > > > > supported built-in) or from UEFI firmware respectively.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Roger Pau Monné<roger.pau@citrix.com>
> > > > > > > > Reviewed-by: Marek Marczykowski-Górecki<marmarek@invisiblethingslab.com>
> > > > > > > Thanks for the review.
> > > > > > > 
> > > > > > > Oleksii, can I get your opinion as Release Manager about whether this
> > > > > > > (and the following patch) would be suitable for committing to staging
> > > > > > > given the current release state?
> > > > > > > 
> > > > > > > It's a workaround for broken EFI implementations that many downstreams
> > > > > > > carry on their patch queue.
> > > > > > Based on your commit message, I understand this as addressing a bug ( but not very critical
> > > > > > as IIUC downstreams have the similar patch on their side ). Therefore, if it has been properly
> > > > > > reviewed and tested, we should consider including it in the current release.
> > > > > IIRC at least Qubes, XenServer and XCP-ng have a patch that achieves
> > > > > the same behavior as proposed here.
> > > > > 
> > > > > > IIUC, setting the wallclock to EFI should align with the behavior Xen had previously.
> > > > > > It might be preferable to use that argument as the default for a while, allowing others to verify the "auto"
> > > > > > value over time. After that, we could consider making "auto" the default.
> > > > > > That said, I am not particularly strict about following this approach.
> > > > > We cannot really set efi as the default, as it would break when
> > > > > booting on legacy BIOS systems.
> > > > > 
> > > > > We could take only patch 1 and leave patch 2 after Xen 4.20 has
> > > > > branched, but at that point I would see little benefit in having just
> > > > > patch 1.
> > > > I don't see a lot of benefit of comitting only the one patch either.
> > > > 
> > > > 
> > > > > I don't have a strong opinion, but downstreams have been complaining
> > > > > about Xen behavior regarding the usage of EFI_GET_TIME, so it might be
> > > > > good to not ship yet another release with such allegedly broken
> > > > > behavior.
> > > > Agree with that. As  I mentioned above I consider it as a bug and based on
> > > > that several mentioned above downstreams have the similar patch I could consider
> > > > that as tested approach so ..
> > > > > Let me know what you think, as I would need a formal Release-Ack if
> > > > > this is to be committed.
> > > > ... R-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>.
> > > Sorry for the noise.
> > > 
> > > I missed to add that it would also be nice to mention IMO ( that now we have wallclock parameter )
> > > in CHANGELOG so downstreams will receive "notification" that Xen provides a workaround for the mentioned
> > > issue in the patch series.
> > Indeed.  Would you be OK with adding the following chunk to patch 2?
> > 
> > diff --git a/CHANGELOG.md b/CHANGELOG.md
> > index 8507e6556a56..1de1d1eca17f 100644
> > --- a/CHANGELOG.md
> > +++ b/CHANGELOG.md
> > @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
> >      leaving this to the guest kernel to do in guest context.
> >    - On x86:
> >      - Prefer ACPI reboot over UEFI ResetSystem() run time service call.
> > +   - Prefer CMOS over EFI_GET_TIME as time source.
> >      - Switched the xAPIC flat driver to use physical destination mode for external
> >        interrupts instead of logical destination mode.
> > @@ -24,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
> >      - Support for LLC (Last Level Cache) coloring.
> >    - On x86:
> >      - xl suspend/resume subcommands.
> > +   - `wallclock` command line option to select time source.
> 
> What about of adding word 'Add' before `wallclock`?

It's in the "Added" section, so I thought it would be redundant.

> Other LGTM: Acked-By: Oleksii Kurochko<oleksii.kurochko@gmail.com>

Let me know if you would still like me to prepend "Add" to the above
item.

Thanks, Roger.

Re: [PATCH v7 1/2] x86/time: introduce command line option to select wallclock

[Oleksii Kurochko]

On 1/14/25 3:58 PM, Roger Pau Monné wrote:
> On Tue, Jan 14, 2025 at 03:23:21PM +0100, Oleksii Kurochko wrote:
>> On 1/14/25 1:13 PM, Roger Pau Monné wrote:
>>> On Tue, Jan 14, 2025 at 12:44:39PM +0100, Oleksii Kurochko wrote:
>>>> On 1/14/25 12:40 PM, Oleksii Kurochko wrote:
>>>>> On 1/14/25 12:27 PM, Roger Pau Monné wrote:
>>>>>> On Tue, Jan 14, 2025 at 12:12:03PM +0100, Oleksii Kurochko wrote:
>>>>>>> On 1/13/25 6:52 PM, Roger Pau Monné wrote:
>>>>>>>> On Mon, Jan 13, 2025 at 05:07:55PM +0100, Marek Marczykowski-Górecki wrote:
>>>>>>>>> On Fri, Sep 13, 2024 at 09:59:06AM +0200, Roger Pau Monne wrote:
>>>>>>>>>> Allow setting the used wallclock from the command line.  When the option is set
>>>>>>>>>> to a value different than `auto` the probing is bypassed and the selected
>>>>>>>>>> implementation is used (as long as it's available).
>>>>>>>>>>
>>>>>>>>>> The `xen` and `efi` options require being booted as a Xen guest (with Xen guest
>>>>>>>>>> supported built-in) or from UEFI firmware respectively.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Roger Pau Monné<roger.pau@citrix.com>
>>>>>>>>> Reviewed-by: Marek Marczykowski-Górecki<marmarek@invisiblethingslab.com>
>>>>>>>> Thanks for the review.
>>>>>>>>
>>>>>>>> Oleksii, can I get your opinion as Release Manager about whether this
>>>>>>>> (and the following patch) would be suitable for committing to staging
>>>>>>>> given the current release state?
>>>>>>>>
>>>>>>>> It's a workaround for broken EFI implementations that many downstreams
>>>>>>>> carry on their patch queue.
>>>>>>> Based on your commit message, I understand this as addressing a bug ( but not very critical
>>>>>>> as IIUC downstreams have the similar patch on their side ). Therefore, if it has been properly
>>>>>>> reviewed and tested, we should consider including it in the current release.
>>>>>> IIRC at least Qubes, XenServer and XCP-ng have a patch that achieves
>>>>>> the same behavior as proposed here.
>>>>>>
>>>>>>> IIUC, setting the wallclock to EFI should align with the behavior Xen had previously.
>>>>>>> It might be preferable to use that argument as the default for a while, allowing others to verify the "auto"
>>>>>>> value over time. After that, we could consider making "auto" the default.
>>>>>>> That said, I am not particularly strict about following this approach.
>>>>>> We cannot really set efi as the default, as it would break when
>>>>>> booting on legacy BIOS systems.
>>>>>>
>>>>>> We could take only patch 1 and leave patch 2 after Xen 4.20 has
>>>>>> branched, but at that point I would see little benefit in having just
>>>>>> patch 1.
>>>>> I don't see a lot of benefit of comitting only the one patch either.
>>>>>
>>>>>
>>>>>> I don't have a strong opinion, but downstreams have been complaining
>>>>>> about Xen behavior regarding the usage of EFI_GET_TIME, so it might be
>>>>>> good to not ship yet another release with such allegedly broken
>>>>>> behavior.
>>>>> Agree with that. As  I mentioned above I consider it as a bug and based on
>>>>> that several mentioned above downstreams have the similar patch I could consider
>>>>> that as tested approach so ..
>>>>>> Let me know what you think, as I would need a formal Release-Ack if
>>>>>> this is to be committed.
>>>>> ... R-Acked-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>.
>>>> Sorry for the noise.
>>>>
>>>> I missed to add that it would also be nice to mention IMO ( that now we have wallclock parameter )
>>>> in CHANGELOG so downstreams will receive "notification" that Xen provides a workaround for the mentioned
>>>> issue in the patch series.
>>> Indeed.  Would you be OK with adding the following chunk to patch 2?
>>>
>>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>>> index 8507e6556a56..1de1d1eca17f 100644
>>> --- a/CHANGELOG.md
>>> +++ b/CHANGELOG.md
>>> @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>>>       leaving this to the guest kernel to do in guest context.
>>>     - On x86:
>>>       - Prefer ACPI reboot over UEFI ResetSystem() run time service call.
>>> +   - Prefer CMOS over EFI_GET_TIME as time source.
>>>       - Switched the xAPIC flat driver to use physical destination mode for external
>>>         interrupts instead of logical destination mode.
>>> @@ -24,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>>>       - Support for LLC (Last Level Cache) coloring.
>>>     - On x86:
>>>       - xl suspend/resume subcommands.
>>> +   - `wallclock` command line option to select time source.
>> What about of adding word 'Add' before `wallclock`?
> It's in the "Added" section, so I thought it would be redundant.
>
>> Other LGTM: Acked-By: Oleksii Kurochko<oleksii.kurochko@gmail.com>
> Let me know if you would still like me to prepend "Add" to the above
> item.

Oh, then it makes sense. We can go without "Add" then.


~ Oleksii

>
> Thanks, Roger.