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

[PATCH v6 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.

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 | 19 +++++++++++++
 xen/arch/x86/time.c               | 47 +++++++++++++++++++++++++++----
 2 files changed, 61 insertions(+), 5 deletions(-)

-- 
2.46.0

[PATCH v6 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 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 | 19 +++++++++++++++
 xen/arch/x86/time.c               | 39 ++++++++++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 959cf45b55d9..a4f916d0d33e 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2816,6 +2816,25 @@ 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.
+
 ### watchdog (x86)
 > `= force | <boolean>`
 
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 9588502f28e8..b97c81ebd3e9 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1552,6 +1552,35 @@ static const char *__init wallclock_type_to_string(void)
     return "";
 }
 
+static int __init cf_check parse_wallclock(const char *arg)
+{
+    if ( !arg )
+        return -EINVAL;
+
+    if ( !strcmp("auto", arg) )
+        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 +2556,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 v6 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 b97c81ebd3e9..e4fe03197381 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1590,14 +1590,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 v6 1/2] x86/time: introduce command line option to select wallclock

[Jan Beulich]

On 12.09.2024 13:15, Roger Pau Monne wrote:
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1552,6 +1552,35 @@ static const char *__init wallclock_type_to_string(void)
>      return "";
>  }
>  
> +static int __init cf_check parse_wallclock(const char *arg)
> +{
> +    if ( !arg )
> +        return -EINVAL;
> +
> +    if ( !strcmp("auto", arg) )
> +        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 +2556,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();

I don't want to stand in the way, and I can live with this form, so I'd like to
ask that EFI folks or Andrew provide the necessary A-b / R-b. I'd like to note
though that there continue to be quirks here. They may not be affecting the
overall result as long as we have only three possible wallclocks, but there
might be problems if a 4th one appeared.

With the EFI_RS check in the command line handler and assuming the default case
of no "efi=no-rs" on the command line, EFI_RS may still end up being off by the
time the command line is being parsed. With "wallclock=cmos wallclock=efi" this
would result in no probing and "cmos" chosen if there was that check in place.
With the logic as added here there will be probing in that case. Replace "cmos"
by a hypothetical non-default 4th wallclock type (i.e. probing picking "cmos"),
and I expect you can see how the result would then not necessarily be as
expected.

Jan

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

[Roger Pau Monné]

On Thu, Sep 12, 2024 at 01:57:00PM +0200, Jan Beulich wrote:
> On 12.09.2024 13:15, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1552,6 +1552,35 @@ static const char *__init wallclock_type_to_string(void)
> >      return "";
> >  }
> >  
> > +static int __init cf_check parse_wallclock(const char *arg)
> > +{
> > +    if ( !arg )
> > +        return -EINVAL;
> > +
> > +    if ( !strcmp("auto", arg) )
> > +        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 +2556,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();
> 
> I don't want to stand in the way, and I can live with this form, so I'd like to
> ask that EFI folks or Andrew provide the necessary A-b / R-b. I'd like to note
> though that there continue to be quirks here. They may not be affecting the
> overall result as long as we have only three possible wallclocks, but there
> might be problems if a 4th one appeared.
> 
> With the EFI_RS check in the command line handler and assuming the default case
> of no "efi=no-rs" on the command line, EFI_RS may still end up being off by the
> time the command line is being parsed. With "wallclock=cmos wallclock=efi" this
> would result in no probing and "cmos" chosen if there was that check in place.
> With the logic as added here there will be probing in that case. Replace "cmos"
> by a hypothetical non-default 4th wallclock type (i.e. probing picking "cmos"),
> and I expect you can see how the result would then not necessarily be as
> expected.

My expectation would be that if "wallclock=cmos wallclock=efi" is used
the last option overrides any previous one, and hence if that last
option is not valid the logic will fallback to the default selection
(in this case to probing).

Thinking about this, it might make sense to unconditionally set
wallclock_source = WALLCLOCK_UNSET at the start of parse_wallclock()
to avoid previous instances carrying over if later ones are not valid.

Thanks, Roger.

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

[Marek Marczykowski-Górecki]

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

On Thu, Sep 12, 2024 at 02:56:55PM +0200, Roger Pau Monné wrote:
> On Thu, Sep 12, 2024 at 01:57:00PM +0200, Jan Beulich wrote:
> > On 12.09.2024 13:15, Roger Pau Monne wrote:
> > > --- a/xen/arch/x86/time.c
> > > +++ b/xen/arch/x86/time.c
> > > @@ -1552,6 +1552,35 @@ static const char *__init wallclock_type_to_string(void)
> > >      return "";
> > >  }
> > >  
> > > +static int __init cf_check parse_wallclock(const char *arg)
> > > +{
> > > +    if ( !arg )
> > > +        return -EINVAL;
> > > +
> > > +    if ( !strcmp("auto", arg) )
> > > +        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 +2556,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();
> > 
> > I don't want to stand in the way, and I can live with this form, so I'd like to
> > ask that EFI folks or Andrew provide the necessary A-b / R-b. I'd like to note
> > though that there continue to be quirks here. They may not be affecting the
> > overall result as long as we have only three possible wallclocks, but there
> > might be problems if a 4th one appeared.
> > 
> > With the EFI_RS check in the command line handler and assuming the default case
> > of no "efi=no-rs" on the command line, EFI_RS may still end up being off by the
> > time the command line is being parsed. With "wallclock=cmos wallclock=efi" this
> > would result in no probing and "cmos" chosen if there was that check in place.
> > With the logic as added here there will be probing in that case. Replace "cmos"
> > by a hypothetical non-default 4th wallclock type (i.e. probing picking "cmos"),
> > and I expect you can see how the result would then not necessarily be as
> > expected.
> 
> My expectation would be that if "wallclock=cmos wallclock=efi" is used
> the last option overrides any previous one, and hence if that last
> option is not valid the logic will fallback to the default selection
> (in this case to probing).

That would be my expectation too. If some kind of preference would be
expected, it should looks like wallclock=efi,cmos, but I don't think we
need that.

> Thinking about this, it might make sense to unconditionally set
> wallclock_source = WALLCLOCK_UNSET at the start of parse_wallclock()
> to avoid previous instances carrying over if later ones are not valid.

This may be a good idea. But more importantly, the behavior should be
included in the option documentation (that if a selected value is not
available, it fallback to auto). And maybe a log message when that
happens (but I'm okay with skipping this one, as selected wallclock
source is logged already)?

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

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

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

[Roger Pau Monné]

On Thu, Sep 12, 2024 at 03:30:29PM +0200, Marek Marczykowski-Górecki wrote:
> On Thu, Sep 12, 2024 at 02:56:55PM +0200, Roger Pau Monné wrote:
> > On Thu, Sep 12, 2024 at 01:57:00PM +0200, Jan Beulich wrote:
> > > On 12.09.2024 13:15, Roger Pau Monne wrote:
> > > > --- a/xen/arch/x86/time.c
> > > > +++ b/xen/arch/x86/time.c
> > > > @@ -1552,6 +1552,35 @@ static const char *__init wallclock_type_to_string(void)
> > > >      return "";
> > > >  }
> > > >  
> > > > +static int __init cf_check parse_wallclock(const char *arg)
> > > > +{
> > > > +    if ( !arg )
> > > > +        return -EINVAL;
> > > > +
> > > > +    if ( !strcmp("auto", arg) )
> > > > +        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 +2556,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();
> > > 
> > > I don't want to stand in the way, and I can live with this form, so I'd like to
> > > ask that EFI folks or Andrew provide the necessary A-b / R-b. I'd like to note
> > > though that there continue to be quirks here. They may not be affecting the
> > > overall result as long as we have only three possible wallclocks, but there
> > > might be problems if a 4th one appeared.
> > > 
> > > With the EFI_RS check in the command line handler and assuming the default case
> > > of no "efi=no-rs" on the command line, EFI_RS may still end up being off by the
> > > time the command line is being parsed. With "wallclock=cmos wallclock=efi" this
> > > would result in no probing and "cmos" chosen if there was that check in place.
> > > With the logic as added here there will be probing in that case. Replace "cmos"
> > > by a hypothetical non-default 4th wallclock type (i.e. probing picking "cmos"),
> > > and I expect you can see how the result would then not necessarily be as
> > > expected.
> > 
> > My expectation would be that if "wallclock=cmos wallclock=efi" is used
> > the last option overrides any previous one, and hence if that last
> > option is not valid the logic will fallback to the default selection
> > (in this case to probing).
> 
> That would be my expectation too. If some kind of preference would be
> expected, it should looks like wallclock=efi,cmos, but I don't think we
> need that.
> 
> > Thinking about this, it might make sense to unconditionally set
> > wallclock_source = WALLCLOCK_UNSET at the start of parse_wallclock()
> > to avoid previous instances carrying over if later ones are not valid.
> 
> This may be a good idea. But more importantly, the behavior should be
> included in the option documentation (that if a selected value is not
> available, it fallback to auto). And maybe a log message when that
> happens (but I'm okay with skipping this one, as selected wallclock
> source is logged already)?

Thanks, would you be fine with:

### 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 not available Xen will default to `auto`.

Regards, Roger.

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

[Marek Marczykowski-Górecki]

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

On Thu, Sep 12, 2024 at 03:47:53PM +0200, Roger Pau Monné wrote:
> On Thu, Sep 12, 2024 at 03:30:29PM +0200, Marek Marczykowski-Górecki wrote:
> > On Thu, Sep 12, 2024 at 02:56:55PM +0200, Roger Pau Monné wrote:
> > > On Thu, Sep 12, 2024 at 01:57:00PM +0200, Jan Beulich wrote:
> > > > On 12.09.2024 13:15, Roger Pau Monne wrote:
> > > > > --- a/xen/arch/x86/time.c
> > > > > +++ b/xen/arch/x86/time.c
> > > > > @@ -1552,6 +1552,35 @@ static const char *__init wallclock_type_to_string(void)
> > > > >      return "";
> > > > >  }
> > > > >  
> > > > > +static int __init cf_check parse_wallclock(const char *arg)
> > > > > +{
> > > > > +    if ( !arg )
> > > > > +        return -EINVAL;
> > > > > +
> > > > > +    if ( !strcmp("auto", arg) )
> > > > > +        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 +2556,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();
> > > > 
> > > > I don't want to stand in the way, and I can live with this form, so I'd like to
> > > > ask that EFI folks or Andrew provide the necessary A-b / R-b. I'd like to note
> > > > though that there continue to be quirks here. They may not be affecting the
> > > > overall result as long as we have only three possible wallclocks, but there
> > > > might be problems if a 4th one appeared.
> > > > 
> > > > With the EFI_RS check in the command line handler and assuming the default case
> > > > of no "efi=no-rs" on the command line, EFI_RS may still end up being off by the
> > > > time the command line is being parsed. With "wallclock=cmos wallclock=efi" this
> > > > would result in no probing and "cmos" chosen if there was that check in place.
> > > > With the logic as added here there will be probing in that case. Replace "cmos"
> > > > by a hypothetical non-default 4th wallclock type (i.e. probing picking "cmos"),
> > > > and I expect you can see how the result would then not necessarily be as
> > > > expected.
> > > 
> > > My expectation would be that if "wallclock=cmos wallclock=efi" is used
> > > the last option overrides any previous one, and hence if that last
> > > option is not valid the logic will fallback to the default selection
> > > (in this case to probing).
> > 
> > That would be my expectation too. If some kind of preference would be
> > expected, it should looks like wallclock=efi,cmos, but I don't think we
> > need that.
> > 
> > > Thinking about this, it might make sense to unconditionally set
> > > wallclock_source = WALLCLOCK_UNSET at the start of parse_wallclock()
> > > to avoid previous instances carrying over if later ones are not valid.
> > 
> > This may be a good idea. But more importantly, the behavior should be
> > included in the option documentation (that if a selected value is not
> > available, it fallback to auto). And maybe a log message when that
> > happens (but I'm okay with skipping this one, as selected wallclock
> > source is logged already)?
> 
> Thanks, would you be fine with:
> 
> ### 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 not available Xen will default to `auto`.

Looks good.

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

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