[PATCH v5 0/4] x86/time: improvements to wallclock logic

[PATCH v5 0/4] 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 (4):
  x86/time: pull cmos_rtc_probe outside of function and rename
  x86/time: introduce probing logic for the wallclock
  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               | 152 ++++++++++++++++++++++++------
 2 files changed, 144 insertions(+), 27 deletions(-)

-- 
2.46.0

[PATCH v5 1/4] x86/time: pull cmos_rtc_probe outside of function and rename

[Roger Pau Monne]

Rename cmos_rtc_probe to opt_cmos_rtc_probe in order to better describe it
being a command line option, and rename cmos_probe() function to
cmos_rtc_probe().

Also move opt_cmos_rtc_probe to being a static global variable in preparation
for further changes that will require the variable being global to the file.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v4:
 - New in this version.
---
 xen/arch/x86/time.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index f37300946e4e..ec48805a2239 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1291,7 +1291,10 @@ static bool __get_cmos_time(struct rtc_time *rtc)
     return t1 <= SECONDS(1) && t2 < MILLISECS(3);
 }
 
-static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
+static bool __read_mostly opt_cmos_rtc_probe;
+boolean_param("cmos-rtc-probe", opt_cmos_rtc_probe);
+
+static bool cmos_rtc_probe(struct rtc_time *rtc_p)
 {
     unsigned int seconds = 60;
 
@@ -1300,7 +1303,7 @@ static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
         bool success = __get_cmos_time(rtc_p);
         struct rtc_time rtc = *rtc_p;
 
-        if ( likely(!cmos_rtc_probe) )
+        if ( likely(!opt_cmos_rtc_probe) )
             return true;
 
         if ( !success ||
@@ -1332,8 +1335,6 @@ static unsigned long get_cmos_time(void)
 {
     unsigned long res;
     struct rtc_time rtc;
-    static bool __read_mostly cmos_rtc_probe;
-    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
 
     if ( efi_enabled(EFI_RS) )
     {
@@ -1343,12 +1344,12 @@ static unsigned long get_cmos_time(void)
     }
 
     if ( likely(!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)) )
-        cmos_rtc_probe = false;
-    else if ( system_state < SYS_STATE_smp_boot && !cmos_rtc_probe )
+        opt_cmos_rtc_probe = false;
+    else if ( system_state < SYS_STATE_smp_boot && !opt_cmos_rtc_probe )
         panic("System with no CMOS RTC advertised must be booted from EFI"
               " (or with command line option \"cmos-rtc-probe\")\n");
 
-    if ( !cmos_probe(&rtc, cmos_rtc_probe) )
+    if ( !cmos_rtc_probe(&rtc) )
         panic("No CMOS RTC found - system must be booted from EFI\n");
 
     return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
-- 
2.46.0

[PATCH v5 2/4] x86/time: introduce probing logic for the wallclock

[Roger Pau Monne]

Adding such probing allows to clearly separate init vs runtime code, and to
place the probing logic into the init section for the CMOS case.  Note both
the Xen shared_info page wallclock, and the EFI wallclock don't really have any
probing-specific logic.  The shared_info wallclock will always be there if
booted as a Xen guest, while the EFI_GET_TIME method probing relies on checking
if it returns a value different than 0.

The panic message printed when Xen is unable to find a viable wallclock source
has been adjusted slightly, I believe the printed guidance still provides the
same amount of information to the user.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v3:
 - Remove ASSERT from cmos_read().
 - Change indentation of panic message arguments in probe_wallclock().
 - Add __init attribute.

Changes since v2:
 - New in this version.
---
 xen/arch/x86/time.c | 118 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 92 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index ec48805a2239..1dcbd9f520f5 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1291,20 +1291,23 @@ static bool __get_cmos_time(struct rtc_time *rtc)
     return t1 <= SECONDS(1) && t2 < MILLISECS(3);
 }
 
-static bool __read_mostly opt_cmos_rtc_probe;
+static bool __initdata opt_cmos_rtc_probe;
 boolean_param("cmos-rtc-probe", opt_cmos_rtc_probe);
 
-static bool cmos_rtc_probe(struct rtc_time *rtc_p)
+static bool __init cmos_rtc_probe(void)
 {
     unsigned int seconds = 60;
 
+    if ( !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) )
+        return true;
+
+    if ( !opt_cmos_rtc_probe )
+        return false;
+
     for ( ; ; )
     {
-        bool success = __get_cmos_time(rtc_p);
-        struct rtc_time rtc = *rtc_p;
-
-        if ( likely(!opt_cmos_rtc_probe) )
-            return true;
+        struct rtc_time rtc;
+        bool success = __get_cmos_time(&rtc);
 
         if ( !success ||
              rtc.sec >= 60 || rtc.min >= 60 || rtc.hour >= 24 ||
@@ -1331,26 +1334,12 @@ static bool cmos_rtc_probe(struct rtc_time *rtc_p)
     return false;
 }
 
-static unsigned long get_cmos_time(void)
+
+static unsigned long cmos_rtc_read(void)
 {
-    unsigned long res;
     struct rtc_time rtc;
 
-    if ( efi_enabled(EFI_RS) )
-    {
-        res = efi_get_time();
-        if ( res )
-            return res;
-    }
-
-    if ( likely(!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)) )
-        opt_cmos_rtc_probe = false;
-    else if ( system_state < SYS_STATE_smp_boot && !opt_cmos_rtc_probe )
-        panic("System with no CMOS RTC advertised must be booted from EFI"
-              " (or with command line option \"cmos-rtc-probe\")\n");
-
-    if ( !cmos_rtc_probe(&rtc) )
-        panic("No CMOS RTC found - system must be booted from EFI\n");
+    __get_cmos_time(&rtc);
 
     return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
 }
@@ -1533,12 +1522,85 @@ void rtc_guest_write(unsigned int port, unsigned int data)
     }
 }
 
-static unsigned long get_wallclock_time(void)
+static enum {
+    WALLCLOCK_UNSET,
+    WALLCLOCK_XEN,
+    WALLCLOCK_CMOS,
+    WALLCLOCK_EFI,
+} wallclock_source __ro_after_init;
+
+static const char *__init wallclock_type_to_string(void)
+{
+    switch ( wallclock_source )
+    {
+    case WALLCLOCK_XEN:
+        return "XEN";
+
+    case WALLCLOCK_CMOS:
+        return "CMOS RTC";
+
+    case WALLCLOCK_EFI:
+        return "EFI";
+
+    case WALLCLOCK_UNSET:
+        return "UNSET";
+    }
+
+    ASSERT_UNREACHABLE();
+    return "";
+}
+
+static void __init probe_wallclock(void)
 {
+    ASSERT(wallclock_source == WALLCLOCK_UNSET);
+
     if ( xen_guest )
+    {
+        wallclock_source = WALLCLOCK_XEN;
+        return;
+    }
+    if ( efi_enabled(EFI_RS) && efi_get_time() )
+    {
+        wallclock_source = WALLCLOCK_EFI;
+        return;
+    }
+    if ( cmos_rtc_probe() )
+    {
+        wallclock_source = WALLCLOCK_CMOS;
+        return;
+    }
+
+    panic("No usable wallclock found, probed:%s%s%s\n%s",
+          !opt_cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
+          opt_cmos_rtc_probe ? " CMOS" : "",
+          efi_enabled(EFI_RS) ? " EFI" : "",
+          !opt_cmos_rtc_probe
+              ? "Try with command line option \"cmos-rtc-probe\"\n"
+              : !efi_enabled(EFI_RS)
+                  ? "System must be booted from EFI\n"
+                  : "");
+}
+
+static unsigned long get_wallclock_time(void)
+{
+    switch ( wallclock_source )
+    {
+    case WALLCLOCK_XEN:
         return read_xen_wallclock();
 
-    return get_cmos_time();
+    case WALLCLOCK_CMOS:
+        return cmos_rtc_read();
+
+    case WALLCLOCK_EFI:
+        return efi_get_time();
+
+    case WALLCLOCK_UNSET:
+        /* Unexpected state - handled by the ASSERT_UNREACHABLE() below. */
+        break;
+    }
+
+    ASSERT_UNREACHABLE();
+    return 0;
 }
 
 /***************************************************************************
@@ -2463,6 +2525,10 @@ int __init init_xen_time(void)
 
     open_softirq(TIME_CALIBRATE_SOFTIRQ, local_time_calibration);
 
+    probe_wallclock();
+
+    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());
 
-- 
2.46.0

[PATCH v5 3/4] 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.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
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               | 33 ++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 0a66e1ee2d7e..1944b9d8eb9d 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2821,6 +2821,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 1dcbd9f520f5..c6d3f19a56d1 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1550,6 +1550,36 @@ 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) )
+    {
+        if ( !efi_enabled(EFI_RS) )
+            return -EINVAL;
+
+        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);
@@ -2525,7 +2555,8 @@ int __init init_xen_time(void)
 
     open_softirq(TIME_CALIBRATE_SOFTIRQ, local_time_calibration);
 
-    probe_wallclock();
+    if ( wallclock_source == WALLCLOCK_UNSET )
+        probe_wallclock();
 
     printk(XENLOG_INFO "Wallclock source: %s\n", wallclock_type_to_string());
 
-- 
2.46.0

[PATCH v5 4/4] 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 c6d3f19a56d1..8567eb2a98e3 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1589,14 +1589,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 v5 1/4] x86/time: pull cmos_rtc_probe outside of function and rename

[Jan Beulich]

On 09.09.2024 16:54, Roger Pau Monne wrote:
> Rename cmos_rtc_probe to opt_cmos_rtc_probe in order to better describe it
> being a command line option, and rename cmos_probe() function to
> cmos_rtc_probe().
> 
> Also move opt_cmos_rtc_probe to being a static global variable in preparation
> for further changes that will require the variable being global to the file.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v5 2/4] x86/time: introduce probing logic for the wallclock

[Jan Beulich]

On 09.09.2024 16:54, Roger Pau Monne wrote:
> Adding such probing allows to clearly separate init vs runtime code, and to
> place the probing logic into the init section for the CMOS case.  Note both
> the Xen shared_info page wallclock, and the EFI wallclock don't really have any
> probing-specific logic.  The shared_info wallclock will always be there if
> booted as a Xen guest, while the EFI_GET_TIME method probing relies on checking
> if it returns a value different than 0.
> 
> The panic message printed when Xen is unable to find a viable wallclock source
> has been adjusted slightly, I believe the printed guidance still provides the
> same amount of information to the user.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v5 3/4] x86/time: introduce command line option to select wallclock

[Jan Beulich]

On 09.09.2024 16:54, 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.

Perhaps add "respectively"?

> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1550,6 +1550,36 @@ 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) )
> +    {
> +        if ( !efi_enabled(EFI_RS) )
> +            return -EINVAL;

I'm afraid there's a problem here, and I'm sorry for not paying attention
earlier: EFI_RS is possibly affected by "efi=" (and hence may change after
this code ran). (It can also be cleared if ->SetVirtualAddressMap() fails,
but I think that's strictly ahead of command line parsing.)

Jan

Re: [PATCH v5 3/4] x86/time: introduce command line option to select wallclock

[Roger Pau Monné]

On Tue, Sep 10, 2024 at 11:32:05AM +0200, Jan Beulich wrote:
> On 09.09.2024 16:54, 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.
> 
> Perhaps add "respectively"?

Sure.

> 
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1550,6 +1550,36 @@ 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) )
> > +    {
> > +        if ( !efi_enabled(EFI_RS) )
> > +            return -EINVAL;
> 
> I'm afraid there's a problem here, and I'm sorry for not paying attention
> earlier: EFI_RS is possibly affected by "efi=" (and hence may change after
> this code ran). (It can also be cleared if ->SetVirtualAddressMap() fails,
> but I think that's strictly ahead of command line parsing.)

Hm, I see, thanks for noticing.  Anyone using 'efi=no-rs
wallclock=efi' likely deserves to be punished.  Would you be fine with
adding the following in init_xen_time():

    /*
     * EFI run time services can be disabled form 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();

Thanks, Roger.

Re: [PATCH v5 3/4] x86/time: introduce command line option to select wallclock

[Jan Beulich]

On 10.09.2024 15:10, Roger Pau Monné wrote:
> On Tue, Sep 10, 2024 at 11:32:05AM +0200, Jan Beulich wrote:
>> On 09.09.2024 16:54, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/time.c
>>> +++ b/xen/arch/x86/time.c
>>> @@ -1550,6 +1550,36 @@ 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) )
>>> +    {
>>> +        if ( !efi_enabled(EFI_RS) )
>>> +            return -EINVAL;
>>
>> I'm afraid there's a problem here, and I'm sorry for not paying attention
>> earlier: EFI_RS is possibly affected by "efi=" (and hence may change after
>> this code ran). (It can also be cleared if ->SetVirtualAddressMap() fails,
>> but I think that's strictly ahead of command line parsing.)
> 
> Hm, I see, thanks for noticing.  Anyone using 'efi=no-rs
> wallclock=efi' likely deserves to be punished.

Well, if you don't want to actually do this ;-) then ...

>  Would you be fine with
> adding the following in init_xen_time():
> 
>     /*
>      * EFI run time services can be disabled form 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();

... this is probably the best we can do (nit: s/form/from/ in the comment;
maybe also "..., hence the check done as part of option parsing may not
suffice" or some such). The subsequent log message should be sufficient to
tell them what's going on.

Jan

Re: [PATCH v5 3/4] x86/time: introduce command line option to select wallclock

[Roger Pau Monné]

On Tue, Sep 10, 2024 at 03:49:52PM +0200, Jan Beulich wrote:
> On 10.09.2024 15:10, Roger Pau Monné wrote:
> > On Tue, Sep 10, 2024 at 11:32:05AM +0200, Jan Beulich wrote:
> >> On 09.09.2024 16:54, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/time.c
> >>> +++ b/xen/arch/x86/time.c
> >>> @@ -1550,6 +1550,36 @@ 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) )
> >>> +    {
> >>> +        if ( !efi_enabled(EFI_RS) )
> >>> +            return -EINVAL;
> >>
> >> I'm afraid there's a problem here, and I'm sorry for not paying attention
> >> earlier: EFI_RS is possibly affected by "efi=" (and hence may change after
> >> this code ran). (It can also be cleared if ->SetVirtualAddressMap() fails,
> >> but I think that's strictly ahead of command line parsing.)
> > 
> > Hm, I see, thanks for noticing.  Anyone using 'efi=no-rs
> > wallclock=efi' likely deserves to be punished.
> 
> Well, if you don't want to actually do this ;-) then ...

It's not too complicated to attempt to arrange for something half sane
even if the user provided options are nonsense.  I've seen people
accumulate all kind of crap on the command line "just because I've
read it online".

> >  Would you be fine with
> > adding the following in init_xen_time():
> > 
> >     /*
> >      * EFI run time services can be disabled form 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();
> 
> ... this is probably the best we can do (nit: s/form/from/ in the comment;
> maybe also "..., hence the check done as part of option parsing may not
> suffice" or some such).

I didn't put in my previous reply, but I've removed the efi_enabled()
check from the option parsing and instead added this comment:

        /*
         * Checking if run-time services are available must be done after
         * command line parsing.
         */

I don't think there's much point in doing the check in
parse_wallclock() if it's not reliable, so your reference in the
comment to "the check done as part of option parsing" is no longer
valid.

Thanks, Roger.

Re: [PATCH v5 3/4] x86/time: introduce command line option to select wallclock

[Jan Beulich]

On 10.09.2024 16:24, Roger Pau Monné wrote:
> On Tue, Sep 10, 2024 at 03:49:52PM +0200, Jan Beulich wrote:
>> On 10.09.2024 15:10, Roger Pau Monné wrote:
>>>  Would you be fine with
>>> adding the following in init_xen_time():
>>>
>>>     /*
>>>      * EFI run time services can be disabled form 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();
>>
>> ... this is probably the best we can do (nit: s/form/from/ in the comment;
>> maybe also "..., hence the check done as part of option parsing may not
>> suffice" or some such).
> 
> I didn't put in my previous reply, but I've removed the efi_enabled()
> check from the option parsing and instead added this comment:
> 
>         /*
>          * Checking if run-time services are available must be done after
>          * command line parsing.
>          */
> 
> I don't think there's much point in doing the check in
> parse_wallclock() if it's not reliable, so your reference in the
> comment to "the check done as part of option parsing" is no longer
> valid.

Hmm. Rejecting the option if we can is reasonable imo. "efi=rs" can imo only
sensibly be used to override an earlier "efi=no-rs". Hence what we see while
parsing the wallclock option gives us at least reasonable grounds to reject
the option if EFI_RS is already clear. We then merely fail to reject the
option if the flag is cleared later.

Yet in the end I'd be happy to leave this particular aspect to you and the
EFI maintainers.

Jan

Re: [PATCH v5 3/4] x86/time: introduce command line option to select wallclock

[Roger Pau Monné]

On Tue, Sep 10, 2024 at 04:29:43PM +0200, Jan Beulich wrote:
> On 10.09.2024 16:24, Roger Pau Monné wrote:
> > On Tue, Sep 10, 2024 at 03:49:52PM +0200, Jan Beulich wrote:
> >> On 10.09.2024 15:10, Roger Pau Monné wrote:
> >>>  Would you be fine with
> >>> adding the following in init_xen_time():
> >>>
> >>>     /*
> >>>      * EFI run time services can be disabled form 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();
> >>
> >> ... this is probably the best we can do (nit: s/form/from/ in the comment;
> >> maybe also "..., hence the check done as part of option parsing may not
> >> suffice" or some such).
> > 
> > I didn't put in my previous reply, but I've removed the efi_enabled()
> > check from the option parsing and instead added this comment:
> > 
> >         /*
> >          * Checking if run-time services are available must be done after
> >          * command line parsing.
> >          */
> > 
> > I don't think there's much point in doing the check in
> > parse_wallclock() if it's not reliable, so your reference in the
> > comment to "the check done as part of option parsing" is no longer
> > valid.
> 
> Hmm. Rejecting the option if we can is reasonable imo. "efi=rs" can imo only
> sensibly be used to override an earlier "efi=no-rs". Hence what we see while
> parsing the wallclock option gives us at least reasonable grounds to reject
> the option if EFI_RS is already clear. We then merely fail to reject the
> option if the flag is cleared later.

I won't strongly argue about it, but I think having a non-reliable
check in parse_wallclock() is confusing.  I would have to add a
comment there anyway to note that depending on the position of the efi
and wallclock parameters the check for EFI_RS might not be effective -
at which point I think it's best to unify the check so it's uniformly
performed in init_xen_time().

> Yet in the end I'd be happy to leave this particular aspect to you and the
> EFI maintainers.

Thanks again for the feedback.

Roger.