[PATCH v3 0/7] x86/time: improvements to wallclock logic

[PATCH v3 0/7] 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.
First 3 patches should be non-functional changes, mostly chopping the
current logic into smaller functions so that in patch 4 the probing vs
runtime wallclock logic can be split.

Patch 5 changes the preference to use CMOS RTC even when booted from EFI
firmware.

Finally patches 6 introduces a new command line option to bypass the
probing an allow specifying which wallclock source to use on the command
line.  Patch 7 enables CMOS RTC probing by default.

Thanks, Roger.

Roger Pau Monne (7):
  x86/time: introduce helper to fetch Xen wallclock when running as a
    guest
  x86/time: move CMOS edge detection into read helper
  x86/time: split CMOS read and probe logic into function
  x86/time: introduce probing logic for the wallclock
  x86/time: prefer CMOS over EFI_GET_TIME
  x86/time: introduce command line option to select wallclock
  x86/time: probe the CMOS RTC by default

 docs/misc/xen-command-line.pandoc |  28 +++-
 xen/arch/x86/time.c               | 238 +++++++++++++++++++++++-------
 2 files changed, 208 insertions(+), 58 deletions(-)

-- 
2.46.0

[PATCH v3 1/7] x86/time: introduce helper to fetch Xen wallclock when running as a guest

[Roger Pau Monne]

Move the current code in get_wallclock_time() to fetch the Xen wallclock
information from the shared page when running as a guest into a separate
helper.

No functional change intended.

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

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index a97d78484105..d022db4bd4a0 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -785,6 +785,31 @@ static struct platform_timesource __initdata_cf_clobber plt_xen_timer =
     .resume = resume_xen_timer,
     .counter_bits = 63,
 };
+
+static unsigned long read_xen_wallclock(void)
+{
+    struct shared_info *sh_info = XEN_shared_info;
+    uint32_t wc_version;
+    uint64_t wc_sec;
+
+    ASSERT(xen_guest);
+
+    do {
+        wc_version = sh_info->wc_version & ~1;
+        smp_rmb();
+
+        wc_sec  = sh_info->wc_sec;
+        smp_rmb();
+    } while ( wc_version != sh_info->wc_version );
+
+    return wc_sec + read_xen_timer() / 1000000000;
+}
+#else
+static unsigned long read_xen_wallclock(void)
+{
+    ASSERT_UNREACHABLE();
+    return 0;
+}
 #endif
 
 #ifdef CONFIG_HYPERV_GUEST
@@ -1497,24 +1522,8 @@ void rtc_guest_write(unsigned int port, unsigned int data)
 
 static unsigned long get_wallclock_time(void)
 {
-#ifdef CONFIG_XEN_GUEST
     if ( xen_guest )
-    {
-        struct shared_info *sh_info = XEN_shared_info;
-        uint32_t wc_version;
-        uint64_t wc_sec;
-
-        do {
-            wc_version = sh_info->wc_version & ~1;
-            smp_rmb();
-
-            wc_sec  = sh_info->wc_sec;
-            smp_rmb();
-        } while ( wc_version != sh_info->wc_version );
-
-        return wc_sec + read_xen_timer() / 1000000000;
-    }
-#endif
+        return read_xen_wallclock();
 
     return get_cmos_time();
 }
-- 
2.46.0

Re: [PATCH v3 1/7] x86/time: introduce helper to fetch Xen wallclock when running as a guest

[Jan Beulich]

On 03.09.2024 15:02, Roger Pau Monne wrote:
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -785,6 +785,31 @@ static struct platform_timesource __initdata_cf_clobber plt_xen_timer =
>      .resume = resume_xen_timer,
>      .counter_bits = 63,
>  };
> +
> +static unsigned long read_xen_wallclock(void)
> +{
> +    struct shared_info *sh_info = XEN_shared_info;
> +    uint32_t wc_version;
> +    uint64_t wc_sec;
> +
> +    ASSERT(xen_guest);
> +
> +    do {
> +        wc_version = sh_info->wc_version & ~1;
> +        smp_rmb();
> +
> +        wc_sec  = sh_info->wc_sec;
> +        smp_rmb();
> +    } while ( wc_version != sh_info->wc_version );
> +
> +    return wc_sec + read_xen_timer() / 1000000000;
> +}
> +#else
> +static unsigned long read_xen_wallclock(void)
> +{
> +    ASSERT_UNREACHABLE();
> +    return 0;
> +}
>  #endif

I understand you try to re-use an existing #ifdef here, but I wonder if I
could talk you into not doing so and instead placing the #ifdef inside the
(then single) function body. Less redundancy, less room for mistakes /
oversights.

Jan

Re: [PATCH v3 1/7] x86/time: introduce helper to fetch Xen wallclock when running as a guest

[Roger Pau Monné]

On Tue, Sep 03, 2024 at 04:48:41PM +0200, Jan Beulich wrote:
> On 03.09.2024 15:02, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -785,6 +785,31 @@ static struct platform_timesource __initdata_cf_clobber plt_xen_timer =
> >      .resume = resume_xen_timer,
> >      .counter_bits = 63,
> >  };
> > +
> > +static unsigned long read_xen_wallclock(void)
> > +{
> > +    struct shared_info *sh_info = XEN_shared_info;
> > +    uint32_t wc_version;
> > +    uint64_t wc_sec;
> > +
> > +    ASSERT(xen_guest);
> > +
> > +    do {
> > +        wc_version = sh_info->wc_version & ~1;
> > +        smp_rmb();
> > +
> > +        wc_sec  = sh_info->wc_sec;
> > +        smp_rmb();
> > +    } while ( wc_version != sh_info->wc_version );
> > +
> > +    return wc_sec + read_xen_timer() / 1000000000;
> > +}
> > +#else
> > +static unsigned long read_xen_wallclock(void)
> > +{
> > +    ASSERT_UNREACHABLE();
> > +    return 0;
> > +}
> >  #endif
> 
> I understand you try to re-use an existing #ifdef here, but I wonder if I
> could talk you into not doing so and instead placing the #ifdef inside the
> (then single) function body. Less redundancy, less room for mistakes /
> oversights.

I don't mind much, I've assumed reducing the number of #ifdefs blocks
was better, but I don't have a strong opinion.  Initially I just kept
the #ifdef block in get_wallclock_time().

Thanks, Roger.

[PATCH v3 2/7] x86/time: move CMOS edge detection into read helper

[Roger Pau Monne]

Move the logic that ensures the CMOS RTC data is read just after it's been
updated into the __get_cmos_time() function that does the register reads.  This
requires returning a boolean from __get_cmos_time() to signal whether the read
has been successfully performed after an update.

The goal, albeit not accomplished by this patch, is to be able to split the
probing and the reading of the CMOS RTC data into two separate functions.

No functional change intended.

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

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index d022db4bd4a0..2a64687bf45b 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1247,8 +1247,26 @@ struct rtc_time {
     unsigned int year, mon, day, hour, min, sec;
 };
 
-static void __get_cmos_time(struct rtc_time *rtc)
+static bool __get_cmos_time(struct rtc_time *rtc)
 {
+    s_time_t start, t1, t2;
+    unsigned long flags;
+
+    spin_lock_irqsave(&rtc_lock, flags);
+
+    /* read RTC exactly on falling edge of update flag */
+    start = NOW();
+    do { /* may take up to 1 second... */
+        t1 = NOW() - start;
+    } while ( !(CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) &&
+              t1 <= SECONDS(1) );
+
+    start = NOW();
+    do { /* must try at least 2.228 ms */
+        t2 = NOW() - start;
+    } while ( (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) &&
+              t2 < MILLISECS(3) );
+
     rtc->sec  = CMOS_READ(RTC_SECONDS);
     rtc->min  = CMOS_READ(RTC_MINUTES);
     rtc->hour = CMOS_READ(RTC_HOURS);
@@ -1268,11 +1286,15 @@ static void __get_cmos_time(struct rtc_time *rtc)
 
     if ( (rtc->year += 1900) < 1970 )
         rtc->year += 100;
+
+    spin_unlock_irqrestore(&rtc_lock, flags);
+
+    return t1 <= SECONDS(1) && t2 < MILLISECS(3);
 }
 
 static unsigned long get_cmos_time(void)
 {
-    unsigned long res, flags;
+    unsigned long res;
     struct rtc_time rtc;
     unsigned int seconds = 60;
     static bool __read_mostly cmos_rtc_probe;
@@ -1293,29 +1315,9 @@ static unsigned long get_cmos_time(void)
 
     for ( ; ; )
     {
-        s_time_t start, t1, t2;
-
-        spin_lock_irqsave(&rtc_lock, flags);
-
-        /* read RTC exactly on falling edge of update flag */
-        start = NOW();
-        do { /* may take up to 1 second... */
-            t1 = NOW() - start;
-        } while ( !(CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) &&
-                  t1 <= SECONDS(1) );
-
-        start = NOW();
-        do { /* must try at least 2.228 ms */
-            t2 = NOW() - start;
-        } while ( (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) &&
-                  t2 < MILLISECS(3) );
-
-        __get_cmos_time(&rtc);
-
-        spin_unlock_irqrestore(&rtc_lock, flags);
+        bool success = __get_cmos_time(&rtc);
 
-        if ( likely(!cmos_rtc_probe) ||
-             t1 > SECONDS(1) || t2 >= MILLISECS(3) ||
+        if ( likely(!cmos_rtc_probe) || !success ||
              rtc.sec >= 60 || rtc.min >= 60 || rtc.hour >= 24 ||
              !rtc.day || rtc.day > 31 ||
              !rtc.mon || rtc.mon > 12 )
-- 
2.46.0

Re: [PATCH v3 2/7] x86/time: move CMOS edge detection into read helper

[Jan Beulich]

On 03.09.2024 15:02, Roger Pau Monne wrote:
> Move the logic that ensures the CMOS RTC data is read just after it's been
> updated into the __get_cmos_time() function that does the register reads.  This
> requires returning a boolean from __get_cmos_time() to signal whether the read
> has been successfully performed after an update.

Considering the limited use of both function, that's probably fine a change
to make, despite me otherwise thinking that this is the slightly wrong move.
I'd generally expect __get_cmos_time() to be usable without that checking,
so long as the results are interpreted appropriately.

> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1247,8 +1247,26 @@ struct rtc_time {
>      unsigned int year, mon, day, hour, min, sec;
>  };
>  
> -static void __get_cmos_time(struct rtc_time *rtc)
> +static bool __get_cmos_time(struct rtc_time *rtc)
>  {
> +    s_time_t start, t1, t2;
> +    unsigned long flags;
> +
> +    spin_lock_irqsave(&rtc_lock, flags);
> +
> +    /* read RTC exactly on falling edge of update flag */
> +    start = NOW();
> +    do { /* may take up to 1 second... */
> +        t1 = NOW() - start;
> +    } while ( !(CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) &&
> +              t1 <= SECONDS(1) );
> +
> +    start = NOW();
> +    do { /* must try at least 2.228 ms */
> +        t2 = NOW() - start;
> +    } while ( (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) &&
> +              t2 < MILLISECS(3) );
> +
>      rtc->sec  = CMOS_READ(RTC_SECONDS);
>      rtc->min  = CMOS_READ(RTC_MINUTES);
>      rtc->hour = CMOS_READ(RTC_HOURS);
> @@ -1268,11 +1286,15 @@ static void __get_cmos_time(struct rtc_time *rtc)
>  
>      if ( (rtc->year += 1900) < 1970 )
>          rtc->year += 100;
> +
> +    spin_unlock_irqrestore(&rtc_lock, flags);

Imo this unlock wants placing at least ahead of the if() in context. The
lock needs to protect only the port accesses, not any of the calculations.

Jan

Re: [PATCH v3 2/7] x86/time: move CMOS edge detection into read helper

[Roger Pau Monné]

On Tue, Sep 03, 2024 at 05:02:21PM +0200, Jan Beulich wrote:
> On 03.09.2024 15:02, Roger Pau Monne wrote:
> > Move the logic that ensures the CMOS RTC data is read just after it's been
> > updated into the __get_cmos_time() function that does the register reads.  This
> > requires returning a boolean from __get_cmos_time() to signal whether the read
> > has been successfully performed after an update.
> 
> Considering the limited use of both function, that's probably fine a change
> to make, despite me otherwise thinking that this is the slightly wrong move.
> I'd generally expect __get_cmos_time() to be usable without that checking,
> so long as the results are interpreted appropriately.

I've expanded the commit message a bit to reflect what you mention
here.

> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1247,8 +1247,26 @@ struct rtc_time {
> >      unsigned int year, mon, day, hour, min, sec;
> >  };
> >  
> > -static void __get_cmos_time(struct rtc_time *rtc)
> > +static bool __get_cmos_time(struct rtc_time *rtc)
> >  {
> > +    s_time_t start, t1, t2;
> > +    unsigned long flags;
> > +
> > +    spin_lock_irqsave(&rtc_lock, flags);
> > +
> > +    /* read RTC exactly on falling edge of update flag */
> > +    start = NOW();
> > +    do { /* may take up to 1 second... */
> > +        t1 = NOW() - start;
> > +    } while ( !(CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) &&
> > +              t1 <= SECONDS(1) );
> > +
> > +    start = NOW();
> > +    do { /* must try at least 2.228 ms */
> > +        t2 = NOW() - start;
> > +    } while ( (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) &&
> > +              t2 < MILLISECS(3) );
> > +
> >      rtc->sec  = CMOS_READ(RTC_SECONDS);
> >      rtc->min  = CMOS_READ(RTC_MINUTES);
> >      rtc->hour = CMOS_READ(RTC_HOURS);
> > @@ -1268,11 +1286,15 @@ static void __get_cmos_time(struct rtc_time *rtc)
> >  
> >      if ( (rtc->year += 1900) < 1970 )
> >          rtc->year += 100;
> > +
> > +    spin_unlock_irqrestore(&rtc_lock, flags);
> 
> Imo this unlock wants placing at least ahead of the if() in context. The
> lock needs to protect only the port accesses, not any of the calculations.

I could even cache the value of CMOS_READ(RTC_CONTROL) ahead of the
check, so that the drop could be dropped earlier, but I'm not going to
do it here.

Thanks, Roger.

[PATCH v3 3/7] x86/time: split CMOS read and probe logic into function

[Roger Pau Monne]

The current logic to probe for the CMOS RTC is open-coded in get_cmos_time(),
move it to a separate function that both serves the purpose of testing for the
CMOS RTC existence and returning its value.

The goal is to be able to split the probing and the reading logic into separate
helpers, and putting the current logic in a separate function helps simplifying
further changes.

A transient *rtc_p variable is introduced as a parameter to the function, that
will be removed by further changes.

No functional change intended.

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

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 2a64687bf45b..10840757b22c 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1292,45 +1292,32 @@ static bool __get_cmos_time(struct rtc_time *rtc)
     return t1 <= SECONDS(1) && t2 < MILLISECS(3);
 }
 
-static unsigned long get_cmos_time(void)
+static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
 {
-    unsigned long res;
-    struct rtc_time rtc;
     unsigned int seconds = 60;
-    static bool __read_mostly cmos_rtc_probe;
-    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
-
-    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)) )
-        cmos_rtc_probe = false;
-    else if ( system_state < SYS_STATE_smp_boot && !cmos_rtc_probe )
-        panic("System with no CMOS RTC advertised must be booted from EFI"
-              " (or with command line option \"cmos-rtc-probe\")\n");
 
     for ( ; ; )
     {
-        bool success = __get_cmos_time(&rtc);
+        bool success = __get_cmos_time(rtc_p);
+        struct rtc_time rtc = *rtc_p;
 
-        if ( likely(!cmos_rtc_probe) || !success ||
+        if ( likely(!cmos_rtc_probe) )
+            return true;
+
+        if ( !success ||
              rtc.sec >= 60 || rtc.min >= 60 || rtc.hour >= 24 ||
              !rtc.day || rtc.day > 31 ||
              !rtc.mon || rtc.mon > 12 )
-            break;
+            return false;
 
         if ( seconds < 60 )
         {
             if ( rtc.sec != seconds )
             {
-                cmos_rtc_probe = false;
                 acpi_gbl_FADT.boot_flags &= ~ACPI_FADT_NO_CMOS_RTC;
+                return true;
             }
-            break;
+            return false;
         }
 
         process_pending_softirqs();
@@ -1338,7 +1325,31 @@ static unsigned long get_cmos_time(void)
         seconds = rtc.sec;
     }
 
-    if ( unlikely(cmos_rtc_probe) )
+    ASSERT_UNREACHABLE();
+    return false;
+}
+
+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) )
+    {
+        res = efi_get_time();
+        if ( res )
+            return res;
+    }
+
+    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 )
+        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) )
         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

Re: [PATCH v3 3/7] x86/time: split CMOS read and probe logic into function

[Jan Beulich]

On 03.09.2024 15:02, Roger Pau Monne wrote:
> The current logic to probe for the CMOS RTC is open-coded in get_cmos_time(),
> move it to a separate function that both serves the purpose of testing for the
> CMOS RTC existence and returning its value.
> 
> The goal is to be able to split the probing and the reading logic into separate
> helpers, and putting the current logic in a separate function helps simplifying
> further changes.
> 
> A transient *rtc_p variable is introduced as a parameter to the function, that
> will be removed by further changes.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

This looks like a straight transformation, except - as noted before - for ...

> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1292,45 +1292,32 @@ static bool __get_cmos_time(struct rtc_time *rtc)
>      return t1 <= SECONDS(1) && t2 < MILLISECS(3);
>  }
>  
> -static unsigned long get_cmos_time(void)
> +static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
>  {
> -    unsigned long res;
> -    struct rtc_time rtc;
>      unsigned int seconds = 60;
> -    static bool __read_mostly cmos_rtc_probe;
> -    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
> -
> -    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)) )
> -        cmos_rtc_probe = false;
> -    else if ( system_state < SYS_STATE_smp_boot && !cmos_rtc_probe )
> -        panic("System with no CMOS RTC advertised must be booted from EFI"
> -              " (or with command line option \"cmos-rtc-probe\")\n");
>  
>      for ( ; ; )
>      {
> -        bool success = __get_cmos_time(&rtc);
> +        bool success = __get_cmos_time(rtc_p);
> +        struct rtc_time rtc = *rtc_p;
>  
> -        if ( likely(!cmos_rtc_probe) || !success ||
> +        if ( likely(!cmos_rtc_probe) )
> +            return true;
> +
> +        if ( !success ||
>               rtc.sec >= 60 || rtc.min >= 60 || rtc.hour >= 24 ||
>               !rtc.day || rtc.day > 31 ||
>               !rtc.mon || rtc.mon > 12 )
> -            break;
> +            return false;
>  
>          if ( seconds < 60 )
>          {
>              if ( rtc.sec != seconds )
>              {
> -                cmos_rtc_probe = false;

... the removal of this line. As asked for before, can the somewhat sub-optimal
new behavior (with the static, which now lives in another function, being
cleared only the next time round) please be at least mentioned in the
description?

>                  acpi_gbl_FADT.boot_flags &= ~ACPI_FADT_NO_CMOS_RTC;
> +                return true;
>              }
> -            break;
> +            return false;
>          }
>  
>          process_pending_softirqs();
> @@ -1338,7 +1325,31 @@ static unsigned long get_cmos_time(void)
>          seconds = rtc.sec;
>      }
>  
> -    if ( unlikely(cmos_rtc_probe) )
> +    ASSERT_UNREACHABLE();
> +    return false;
> +}
> +
> +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) )
> +    {
> +        res = efi_get_time();
> +        if ( res )
> +            return res;
> +    }
> +
> +    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 )
> +        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) )
>          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);

Having seen the series up to here (and already by the previous patch) I think
I see now where we disagree about the conditional-ness of the probing: I
suppose you deem only the 2nd and possible subsequent iterations of the loop
in (now) cmos_probe() as "probing", whereas I consider all of what that
function now contains as exactly that.

The difference is becoming more pronounced with the subsequent change of
preferring CMOS over EFI time: With EFI (with or without ACPI) there never
was a guarantee that a CMOS clock would exist. Prior to the introduction of
the ACPI_FADT_NO_CMOS_RTC flag the was a de-facto guarantee that PC-like
systems would have one. And vendors abusing the flag made us probe, despite
the port accesses being problematic until we know there actually is a CMOS
(RTC) there. Hence why I was suggesting that there be a way to bypass the
CMOS accesses altogether at least when booted from EFI (as is the case
right now, just without the need for any user override).

Jan

Re: [PATCH v3 3/7] x86/time: split CMOS read and probe logic into function

[Roger Pau Monné]

On Tue, Sep 03, 2024 at 05:16:44PM +0200, Jan Beulich wrote:
> On 03.09.2024 15:02, Roger Pau Monne wrote:
> > The current logic to probe for the CMOS RTC is open-coded in get_cmos_time(),
> > move it to a separate function that both serves the purpose of testing for the
> > CMOS RTC existence and returning its value.
> > 
> > The goal is to be able to split the probing and the reading logic into separate
> > helpers, and putting the current logic in a separate function helps simplifying
> > further changes.
> > 
> > A transient *rtc_p variable is introduced as a parameter to the function, that
> > will be removed by further changes.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> This looks like a straight transformation, except - as noted before - for ...
> 
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1292,45 +1292,32 @@ static bool __get_cmos_time(struct rtc_time *rtc)
> >      return t1 <= SECONDS(1) && t2 < MILLISECS(3);
> >  }
> >  
> > -static unsigned long get_cmos_time(void)
> > +static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
> >  {
> > -    unsigned long res;
> > -    struct rtc_time rtc;
> >      unsigned int seconds = 60;
> > -    static bool __read_mostly cmos_rtc_probe;
> > -    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
> > -
> > -    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)) )
> > -        cmos_rtc_probe = false;
> > -    else if ( system_state < SYS_STATE_smp_boot && !cmos_rtc_probe )
> > -        panic("System with no CMOS RTC advertised must be booted from EFI"
> > -              " (or with command line option \"cmos-rtc-probe\")\n");
> >  
> >      for ( ; ; )
> >      {
> > -        bool success = __get_cmos_time(&rtc);
> > +        bool success = __get_cmos_time(rtc_p);
> > +        struct rtc_time rtc = *rtc_p;
> >  
> > -        if ( likely(!cmos_rtc_probe) || !success ||
> > +        if ( likely(!cmos_rtc_probe) )
> > +            return true;
> > +
> > +        if ( !success ||
> >               rtc.sec >= 60 || rtc.min >= 60 || rtc.hour >= 24 ||
> >               !rtc.day || rtc.day > 31 ||
> >               !rtc.mon || rtc.mon > 12 )
> > -            break;
> > +            return false;
> >  
> >          if ( seconds < 60 )
> >          {
> >              if ( rtc.sec != seconds )
> >              {
> > -                cmos_rtc_probe = false;
> 
> ... the removal of this line. As asked for before, can the somewhat sub-optimal
> new behavior (with the static, which now lives in another function, being
> cleared only the next time round) please be at least mentioned in the
> description?

Sure, sorry I've failed to do this here.  Such weird behavior is
transient, and will go away wit the next patch IIRC, once the probing
is properly split from the run-time reading of the CMOS RTC.

> >                  acpi_gbl_FADT.boot_flags &= ~ACPI_FADT_NO_CMOS_RTC;
> > +                return true;
> >              }
> > -            break;
> > +            return false;
> >          }
> >  
> >          process_pending_softirqs();
> > @@ -1338,7 +1325,31 @@ static unsigned long get_cmos_time(void)
> >          seconds = rtc.sec;
> >      }
> >  
> > -    if ( unlikely(cmos_rtc_probe) )
> > +    ASSERT_UNREACHABLE();
> > +    return false;
> > +}
> > +
> > +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) )
> > +    {
> > +        res = efi_get_time();
> > +        if ( res )
> > +            return res;
> > +    }
> > +
> > +    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 )
> > +        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) )
> >          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);
> 
> Having seen the series up to here (and already by the previous patch) I think
> I see now where we disagree about the conditional-ness of the probing: I
> suppose you deem only the 2nd and possible subsequent iterations of the loop
> in (now) cmos_probe() as "probing", whereas I consider all of what that
> function now contains as exactly that.
> 
> The difference is becoming more pronounced with the subsequent change of
> preferring CMOS over EFI time: With EFI (with or without ACPI) there never
> was a guarantee that a CMOS clock would exist. Prior to the introduction of
> the ACPI_FADT_NO_CMOS_RTC flag the was a de-facto guarantee that PC-like
> systems would have one. And vendors abusing the flag made us probe, despite
> the port accesses being problematic until we know there actually is a CMOS
> (RTC) there. Hence why I was suggesting that there be a way to bypass the
> CMOS accesses altogether at least when booted from EFI (as is the case
> right now, just without the need for any user override).

With further patches you could use wallclock=efi in order to bypass
any probing of the CMOS.

Also note that the current logic here still keeps the reading of the
CMOS limited to when ACPI_FADT_NO_CMOS_RTC is not set or when
cmos-rtc-probe is present on the command line.

Thanks, Roger.

[PATCH v3 4/7] 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 v2:
 - New in this version.
---
 xen/arch/x86/time.c | 116 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 92 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 10840757b22c..8402131d7b6a 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1292,14 +1292,23 @@ 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 __initdata cmos_rtc_probe;
+boolean_param("cmos-rtc-probe", cmos_rtc_probe);
+
+static bool __init cmos_probe(void)
 {
     unsigned int seconds = 60;
 
+    if ( !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) )
+        return true;
+
+    if ( !cmos_rtc_probe )
+        return false;
+
     for ( ; ; )
     {
-        bool success = __get_cmos_time(rtc_p);
-        struct rtc_time rtc = *rtc_p;
+        struct rtc_time rtc;
+        bool success = __get_cmos_time(&rtc);
 
         if ( likely(!cmos_rtc_probe) )
             return true;
@@ -1329,28 +1338,13 @@ static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
     return false;
 }
 
-static unsigned long get_cmos_time(void)
+
+static unsigned long cmos_read(void)
 {
-    unsigned long res;
     struct rtc_time rtc;
-    static bool __read_mostly cmos_rtc_probe;
-    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
+    bool success = __get_cmos_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)) )
-        cmos_rtc_probe = false;
-    else if ( system_state < SYS_STATE_smp_boot && !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) )
-        panic("No CMOS RTC found - system must be booted from EFI\n");
+    ASSERT(success);
 
     return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
 }
@@ -1533,12 +1527,82 @@ 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 *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_probe() )
+    {
+        wallclock_source = WALLCLOCK_CMOS;
+        return;
+    }
+
+    panic("No usable wallclock found, probed:%s%s%s\n%s",
+          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
+          cmos_rtc_probe ? " CMOS" : "",
+          efi_enabled(EFI_RS) ? " EFI" : "",
+          !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_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 +2527,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

Re: [PATCH v3 4/7] x86/time: introduce probing logic for the wallclock

[Jan Beulich]

On 03.09.2024 15:03, 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>

Looks a little involved, but I'm largely fine with it; just a couple of
more or less cosmetic remarks:

> @@ -1329,28 +1338,13 @@ static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
>      return false;
>  }
>  
> -static unsigned long get_cmos_time(void)
> +
> +static unsigned long cmos_read(void)
>  {
> -    unsigned long res;
>      struct rtc_time rtc;
> -    static bool __read_mostly cmos_rtc_probe;
> -    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
> +    bool success = __get_cmos_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)) )
> -        cmos_rtc_probe = false;
> -    else if ( system_state < SYS_STATE_smp_boot && !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) )
> -        panic("No CMOS RTC found - system must be booted from EFI\n");
> +    ASSERT(success);

I'm not convinced of this assertion: It's either too much (compared to
what we had so far) or not enough, considering the behavior ...

>      return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
>  }

... with a release build.

> @@ -1533,12 +1527,82 @@ 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 *wallclock_type_to_string(void)

__init ?

>  {
> +    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_probe() )
> +    {
> +        wallclock_source = WALLCLOCK_CMOS;
> +        return;
> +    }
> +
> +    panic("No usable wallclock found, probed:%s%s%s\n%s",
> +          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
> +          cmos_rtc_probe ? " CMOS" : "",
> +          efi_enabled(EFI_RS) ? " EFI" : "",
> +          !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
> +           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n" : "");

This last argument is sufficiently complex that I think it is pretty
important for the question marks and colons to respectively align with
one another, even if this may mean one or two more lines of code.

Jan

Re: [PATCH v3 4/7] x86/time: introduce probing logic for the wallclock

[Roger Pau Monné]

On Tue, Sep 03, 2024 at 05:32:27PM +0200, Jan Beulich wrote:
> On 03.09.2024 15:03, 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>
> 
> Looks a little involved, but I'm largely fine with it; just a couple of
> more or less cosmetic remarks:
> 
> > @@ -1329,28 +1338,13 @@ static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
> >      return false;
> >  }
> >  
> > -static unsigned long get_cmos_time(void)
> > +
> > +static unsigned long cmos_read(void)
> >  {
> > -    unsigned long res;
> >      struct rtc_time rtc;
> > -    static bool __read_mostly cmos_rtc_probe;
> > -    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
> > +    bool success = __get_cmos_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)) )
> > -        cmos_rtc_probe = false;
> > -    else if ( system_state < SYS_STATE_smp_boot && !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) )
> > -        panic("No CMOS RTC found - system must be booted from EFI\n");
> > +    ASSERT(success);
> 
> I'm not convinced of this assertion: It's either too much (compared to
> what we had so far) or not enough, considering the behavior ...
> 
> >      return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
> >  }
> 
> ... with a release build.

My reasoning was that on a debug build we want to spot any such
issues (as it's likely a symptom the RTC is misbehaving?) but on a release
build we should rather return an incorrect wallclock time rather than
panicking.  I can remove the ASSERT and local variable altogether if
you prefer.

> 
> > @@ -1533,12 +1527,82 @@ 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 *wallclock_type_to_string(void)
> 
> __init ?
> 
> >  {
> > +    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_probe() )
> > +    {
> > +        wallclock_source = WALLCLOCK_CMOS;
> > +        return;
> > +    }
> > +
> > +    panic("No usable wallclock found, probed:%s%s%s\n%s",
> > +          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
> > +          cmos_rtc_probe ? " CMOS" : "",
> > +          efi_enabled(EFI_RS) ? " EFI" : "",
> > +          !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
> > +           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n" : "");
> 
> This last argument is sufficiently complex that I think it is pretty
> important for the question marks and colons to respectively align with
> one another, even if this may mean one or two more lines of code.

I had it that way originally, but then it seemed the extra
indentation made it less readable.  Will see how can I adjust it, my
preference would be for:

    panic("No usable wallclock found, probed:%s%s%s\n%s",
          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
          cmos_rtc_probe ? " CMOS" : "",
          efi_enabled(EFI_RS) ? " EFI" : "",
          !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
                          : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
                                                 : "");

But that exceeds the 80 columns limit.

Thanks, Roger.

Re: [PATCH v3 4/7] x86/time: introduce probing logic for the wallclock

[Jan Beulich]

On 04.09.2024 12:58, Roger Pau Monné wrote:
> On Tue, Sep 03, 2024 at 05:32:27PM +0200, Jan Beulich wrote:
>> On 03.09.2024 15:03, Roger Pau Monne wrote:
>>> @@ -1329,28 +1338,13 @@ static bool cmos_probe(struct rtc_time *rtc_p, bool cmos_rtc_probe)
>>>      return false;
>>>  }
>>>  
>>> -static unsigned long get_cmos_time(void)
>>> +
>>> +static unsigned long cmos_read(void)
>>>  {
>>> -    unsigned long res;
>>>      struct rtc_time rtc;
>>> -    static bool __read_mostly cmos_rtc_probe;
>>> -    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
>>> +    bool success = __get_cmos_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)) )
>>> -        cmos_rtc_probe = false;
>>> -    else if ( system_state < SYS_STATE_smp_boot && !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) )
>>> -        panic("No CMOS RTC found - system must be booted from EFI\n");
>>> +    ASSERT(success);
>>
>> I'm not convinced of this assertion: It's either too much (compared to
>> what we had so far) or not enough, considering the behavior ...
>>
>>>      return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
>>>  }
>>
>> ... with a release build.
> 
> My reasoning was that on a debug build we want to spot any such
> issues (as it's likely a symptom the RTC is misbehaving?) but on a release
> build we should rather return an incorrect wallclock time rather than
> panicking.  I can remove the ASSERT and local variable altogether if
> you prefer.

I would prefer that, yes, but I also won't insist.

>>> @@ -1533,12 +1527,82 @@ 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 *wallclock_type_to_string(void)
>>
>> __init ?
>>
>>>  {
>>> +    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_probe() )
>>> +    {
>>> +        wallclock_source = WALLCLOCK_CMOS;
>>> +        return;
>>> +    }
>>> +
>>> +    panic("No usable wallclock found, probed:%s%s%s\n%s",
>>> +          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>>> +          cmos_rtc_probe ? " CMOS" : "",
>>> +          efi_enabled(EFI_RS) ? " EFI" : "",
>>> +          !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
>>> +           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n" : "");
>>
>> This last argument is sufficiently complex that I think it is pretty
>> important for the question marks and colons to respectively align with
>> one another, even if this may mean one or two more lines of code.
> 
> I had it that way originally, but then it seemed the extra
> indentation made it less readable.  Will see how can I adjust it, my
> preference would be for:
> 
>     panic("No usable wallclock found, probed:%s%s%s\n%s",
>           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>           cmos_rtc_probe ? " CMOS" : "",
>           efi_enabled(EFI_RS) ? " EFI" : "",
>           !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
>                           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
>                                                  : "");
> 
> But that exceeds the 80 columns limit.

Right, formally the above would be my preference, too. Here two shorter-
lines alternatives:

    panic("No usable wallclock found, probed:%s%s%s\n%s",
          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
          cmos_rtc_probe ? " CMOS" : "",
          efi_enabled(EFI_RS) ? " EFI" : "",
          !cmos_rtc_probe
          ? "Try with command line option \"cmos-rtc-probe\"\n"
          : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
                                 : "");

    panic("No usable wallclock found, probed:%s%s%s\n%s",
          !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
          cmos_rtc_probe ? " CMOS" : "",
          efi_enabled(EFI_RS) ? " EFI" : "",
          !cmos_rtc_probe
              ? "Try with command line option \"cmos-rtc-probe\"\n"
              : !efi_enabled(EFI_RS)
                  ? "System must be booted from EFI\n"
                  : "");

Either of these or anything more or less similar will do imo, just as
long as the ? vs : alignment is there.

One thing I notice only now: The trailing %s will be a little odd if
the "" variant is used in the last argument. That'll produce "(XEN) "
with nothing following in the log. Which usually is a sign of some
strange breakage.

Jan

Re: [PATCH v3 4/7] x86/time: introduce probing logic for the wallclock

[Roger Pau Monné]

On Wed, Sep 04, 2024 at 01:49:36PM +0200, Jan Beulich wrote:
> On 04.09.2024 12:58, Roger Pau Monné wrote:
> > I had it that way originally, but then it seemed the extra
> > indentation made it less readable.  Will see how can I adjust it, my
> > preference would be for:
> > 
> >     panic("No usable wallclock found, probed:%s%s%s\n%s",
> >           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
> >           cmos_rtc_probe ? " CMOS" : "",
> >           efi_enabled(EFI_RS) ? " EFI" : "",
> >           !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
> >                           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
> >                                                  : "");
> > 
> > But that exceeds the 80 columns limit.
> 
> Right, formally the above would be my preference, too. Here two shorter-
> lines alternatives:
> 
>     panic("No usable wallclock found, probed:%s%s%s\n%s",
>           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>           cmos_rtc_probe ? " CMOS" : "",
>           efi_enabled(EFI_RS) ? " EFI" : "",
>           !cmos_rtc_probe
>           ? "Try with command line option \"cmos-rtc-probe\"\n"
>           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
>                                  : "");
> 
>     panic("No usable wallclock found, probed:%s%s%s\n%s",
>           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>           cmos_rtc_probe ? " CMOS" : "",
>           efi_enabled(EFI_RS) ? " EFI" : "",
>           !cmos_rtc_probe
>               ? "Try with command line option \"cmos-rtc-probe\"\n"
>               : !efi_enabled(EFI_RS)
>                   ? "System must be booted from EFI\n"
>                   : "");
> 
> Either of these or anything more or less similar will do imo, just as
> long as the ? vs : alignment is there.

I think I prefer the second variant, as indentation is clearer there.

> 
> One thing I notice only now: The trailing %s will be a little odd if
> the "" variant is used in the last argument. That'll produce "(XEN) "
> with nothing following in the log. Which usually is a sign of some
> strange breakage.

I've tested this and it doesn't produce an extra newline if the string
parameter is "".  IOW:

printk("FOO\n%s", "");

Results in:

(XEN) [    2.230603] TSC deadline timer enabled
(XEN) [    2.235654] FOO
(XEN) [    2.238682] Wallclock source: EFI

Thanks, Roger.

Re: [PATCH v3 4/7] x86/time: introduce probing logic for the wallclock

[Jan Beulich]

On 04.09.2024 14:30, Roger Pau Monné wrote:
> On Wed, Sep 04, 2024 at 01:49:36PM +0200, Jan Beulich wrote:
>> On 04.09.2024 12:58, Roger Pau Monné wrote:
>>> I had it that way originally, but then it seemed the extra
>>> indentation made it less readable.  Will see how can I adjust it, my
>>> preference would be for:
>>>
>>>     panic("No usable wallclock found, probed:%s%s%s\n%s",
>>>           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>>>           cmos_rtc_probe ? " CMOS" : "",
>>>           efi_enabled(EFI_RS) ? " EFI" : "",
>>>           !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
>>>                           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
>>>                                                  : "");
>>>
>>> But that exceeds the 80 columns limit.
>>
>> Right, formally the above would be my preference, too. Here two shorter-
>> lines alternatives:
>>
>>     panic("No usable wallclock found, probed:%s%s%s\n%s",
>>           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>>           cmos_rtc_probe ? " CMOS" : "",
>>           efi_enabled(EFI_RS) ? " EFI" : "",
>>           !cmos_rtc_probe
>>           ? "Try with command line option \"cmos-rtc-probe\"\n"
>>           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n"
>>                                  : "");
>>
>>     panic("No usable wallclock found, probed:%s%s%s\n%s",
>>           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
>>           cmos_rtc_probe ? " CMOS" : "",
>>           efi_enabled(EFI_RS) ? " EFI" : "",
>>           !cmos_rtc_probe
>>               ? "Try with command line option \"cmos-rtc-probe\"\n"
>>               : !efi_enabled(EFI_RS)
>>                   ? "System must be booted from EFI\n"
>>                   : "");
>>
>> Either of these or anything more or less similar will do imo, just as
>> long as the ? vs : alignment is there.
> 
> I think I prefer the second variant, as indentation is clearer there.
> 
>>
>> One thing I notice only now: The trailing %s will be a little odd if
>> the "" variant is used in the last argument. That'll produce "(XEN) "
>> with nothing following in the log. Which usually is a sign of some
>> strange breakage.
> 
> I've tested this and it doesn't produce an extra newline if the string
> parameter is "".  IOW:
> 
> printk("FOO\n%s", "");
> 
> Results in:
> 
> (XEN) [    2.230603] TSC deadline timer enabled
> (XEN) [    2.235654] FOO
> (XEN) [    2.238682] Wallclock source: EFI

Oh, my mistake. Format string processing of course comes before the
determination of line breaks within what is to be output.

Jan

[PATCH v3 5/7] 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 8402131d7b6a..da3fd1555041 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1564,14 +1564,14 @@ static void __init probe_wallclock(void)
         wallclock_source = WALLCLOCK_XEN;
         return;
     }
-    if ( efi_enabled(EFI_RS) && efi_get_time() )
+    if ( cmos_probe() )
     {
-        wallclock_source = WALLCLOCK_EFI;
+        wallclock_source = WALLCLOCK_CMOS;
         return;
     }
-    if ( cmos_probe() )
+    if ( efi_enabled(EFI_RS) && efi_get_time() )
     {
-        wallclock_source = WALLCLOCK_CMOS;
+        wallclock_source = WALLCLOCK_EFI;
         return;
     }
 
-- 
2.46.0

[PATCH v3 6/7] 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>
---
 docs/misc/xen-command-line.pandoc | 18 +++++++++++++++++
 xen/arch/x86/time.c               | 33 ++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 0a66e1ee2d7e..23de922b9705 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2821,6 +2821,24 @@ 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.
+
+ * `cmos` force usage of the CMOS RTC wallclock.
+
+ * `efi` force usage of the EFI_GFET_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 da3fd1555041..6e19c666d13f 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1555,6 +1555,36 @@ static const char *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);
@@ -2527,7 +2557,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

Re: [PATCH v3 6/7] x86/time: introduce command line option to select wallclock

[Jan Beulich]

On 03.09.2024 15:03, Roger Pau Monne wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2821,6 +2821,24 @@ 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.

Mention the CONFIG_* dependency, like we do elsewhere?

> + * `cmos` force usage of the CMOS RTC wallclock.
> +
> + * `efi` force usage of the EFI_GFET_TIME run-time method when booted from EFI
> +   firmware.

Nit: Stray F.

Jan

[PATCH v3 7/7] x86/time: probe the CMOS RTC by default

[Roger Pau Monne]

Probing for the CMOS RTC registers consist in reading IO ports, and we expect
those reads to have no side effects even when the CMOS RTC is not present.  Xen
already does a similar probing (reading of IO ports) by default when searching
for possible CMOS aliased locations.

Switch the default to probe for the CMOS RTC by default when ACPI FADT contains
the ACPI_FADT_NO_CMOS_RTC flag.  At the same time introduce a new option that
can be used to turn off the probing: `wallclock=no-cmos-probe`.  Deprecate the
previous `cmos-rtc-probe` option.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - New in this version.
---
 docs/misc/xen-command-line.pandoc | 12 ++++++++++--
 xen/arch/x86/time.c               |  9 ++++++---
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 23de922b9705..0d603b9521ae 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -326,11 +326,14 @@ Interrupts.  Specifying zero disables CMCI handling.
 ### cmos-rtc-probe (x86)
 > `= <boolean>`
 
-> Default: `false`
+> Default: `true`
 
 Flag to indicate whether to probe for a CMOS Real Time Clock irrespective of
 ACPI indicating none to be there.
 
+**WARNING: The `cmos-rtc-probe` option is deprecated and superseded by
+_wallclock=no-cmos-probe_ using both options in combination is undefined.**
+
 ### com1 (x86)
 ### com2 (x86)
 > `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>|msi][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
@@ -2822,7 +2825,7 @@ 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`
+> `= auto | xen | cmos | no-cmos-probe | efi`
 
 > Default: `auto`
 
@@ -2836,6 +2839,11 @@ Allow forcing the usage of a specific wallclock source.
 
  * `cmos` force usage of the CMOS RTC wallclock.
 
+ * `no-cmos-probe` do not probe for the CMOS RTC presence if the ACPI FADT
+   table signals there's no CMOS RTC.  Implies using the same heuristics as
+   the `auto` option.  By default Xen will probe for the CMOS RTC presence
+   even when ACPI FADT signals no CMOS RTC available.
+
  * `efi` force usage of the EFI_GFET_TIME run-time method when booted from EFI
    firmware.
 
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 6e19c666d13f..8e6ecbe5e964 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1292,7 +1292,7 @@ static bool __get_cmos_time(struct rtc_time *rtc)
     return t1 <= SECONDS(1) && t2 < MILLISECS(3);
 }
 
-static bool __initdata cmos_rtc_probe;
+static bool __initdata cmos_rtc_probe = true;
 boolean_param("cmos-rtc-probe", cmos_rtc_probe);
 
 static bool __init cmos_probe(void)
@@ -1560,6 +1560,8 @@ static int __init cf_check parse_wallclock(const char *arg)
     if ( !arg )
         return -EINVAL;
 
+    cmos_rtc_probe = true;
+
     if ( !strcmp("auto", arg) )
         wallclock_source = WALLCLOCK_UNSET;
     else if ( !strcmp("xen", arg) )
@@ -1571,6 +1573,8 @@ static int __init cf_check parse_wallclock(const char *arg)
     }
     else if ( !strcmp("cmos", arg) )
         wallclock_source = WALLCLOCK_CMOS;
+    else if ( !strcmp("no-cmos-probe", arg) )
+        cmos_rtc_probe = false;
     else if ( !strcmp("efi", arg) )
     {
         if ( !efi_enabled(EFI_RS) )
@@ -1609,8 +1613,7 @@ static void __init probe_wallclock(void)
           !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "",
           cmos_rtc_probe ? " CMOS" : "",
           efi_enabled(EFI_RS) ? " EFI" : "",
-          !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n"
-           : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n" : "");
+          !efi_enabled(EFI_RS) ? "System must be booted from EFI\n" : "");
 }
 
 static unsigned long get_wallclock_time(void)
-- 
2.46.0

Re: [PATCH v3 7/7] x86/time: probe the CMOS RTC by default

[Jan Beulich]

On 03.09.2024 15:03, Roger Pau Monne wrote:
> Probing for the CMOS RTC registers consist in reading IO ports, and we expect
> those reads to have no side effects even when the CMOS RTC is not present.

But what do we gain from this besides possible being slower to boot?

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -326,11 +326,14 @@ Interrupts.  Specifying zero disables CMCI handling.
>  ### cmos-rtc-probe (x86)
>  > `= <boolean>`
>  
> -> Default: `false`
> +> Default: `true`
>  
>  Flag to indicate whether to probe for a CMOS Real Time Clock irrespective of
>  ACPI indicating none to be there.
>  
> +**WARNING: The `cmos-rtc-probe` option is deprecated and superseded by
> +_wallclock=no-cmos-probe_ using both options in combination is undefined.**

Hmm, but then ...

> @@ -2822,7 +2825,7 @@ 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`
> +> `= auto | xen | cmos | no-cmos-probe | efi`

... this wants to be a boolean sub-option "cmos-probe", such that the flag
can still be set both ways (in particular for a later command line option
to override an earlier one).

> @@ -2836,6 +2839,11 @@ Allow forcing the usage of a specific wallclock source.
>  
>   * `cmos` force usage of the CMOS RTC wallclock.
>  
> + * `no-cmos-probe` do not probe for the CMOS RTC presence if the ACPI FADT
> +   table signals there's no CMOS RTC.  Implies using the same heuristics as
> +   the `auto` option.  By default Xen will probe for the CMOS RTC presence
> +   even when ACPI FADT signals no CMOS RTC available.

"By default ..." reads as if this would always occur, which I don't think
is the case.

> @@ -1560,6 +1560,8 @@ static int __init cf_check parse_wallclock(const char *arg)
>      if ( !arg )
>          return -EINVAL;
>  
> +    cmos_rtc_probe = true;
> +
>      if ( !strcmp("auto", arg) )
>          wallclock_source = WALLCLOCK_UNSET;
>      else if ( !strcmp("xen", arg) )
> @@ -1571,6 +1573,8 @@ static int __init cf_check parse_wallclock(const char *arg)
>      }
>      else if ( !strcmp("cmos", arg) )
>          wallclock_source = WALLCLOCK_CMOS;
> +    else if ( !strcmp("no-cmos-probe", arg) )
> +        cmos_rtc_probe = false;
>      else if ( !strcmp("efi", arg) )
>      {
>          if ( !efi_enabled(EFI_RS) )

And to request a particular wallclock _and_ control the probing one then
needs two wallclock= on the command line? And - because of the forcing to
true of cmos_rtc_probe - even in a particular order. Not very nice from a
usability pov.

Jan

Re: [PATCH v3 7/7] x86/time: probe the CMOS RTC by default

[Roger Pau Monné]

On Tue, Sep 03, 2024 at 05:48:09PM +0200, Jan Beulich wrote:
> On 03.09.2024 15:03, Roger Pau Monne wrote:
> > Probing for the CMOS RTC registers consist in reading IO ports, and we expect
> > those reads to have no side effects even when the CMOS RTC is not present.
> 
> But what do we gain from this besides possible being slower to boot?

The intent is that Xen can successfully boot on more systems without
having to pass specific command line options.

> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -326,11 +326,14 @@ Interrupts.  Specifying zero disables CMCI handling.
> >  ### cmos-rtc-probe (x86)
> >  > `= <boolean>`
> >  
> > -> Default: `false`
> > +> Default: `true`
> >  
> >  Flag to indicate whether to probe for a CMOS Real Time Clock irrespective of
> >  ACPI indicating none to be there.
> >  
> > +**WARNING: The `cmos-rtc-probe` option is deprecated and superseded by
> > +_wallclock=no-cmos-probe_ using both options in combination is undefined.**
> 
> Hmm, but then ...
> 
> > @@ -2822,7 +2825,7 @@ 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`
> > +> `= auto | xen | cmos | no-cmos-probe | efi`
> 
> ... this wants to be a boolean sub-option "cmos-probe", such that the flag
> can still be set both ways (in particular for a later command line option
> to override an earlier one).

What's the point in overriding?  Either the users selects a specific
wallclock to use, or it's left for Xen to decide which wallclock to
pick, either with (auto) or without (no-cmos-probe) possibly probing
the CMOS RTC.

Multiple different wallclock options being passed on the command line
will result in just the last one taking effect.

> > @@ -2836,6 +2839,11 @@ Allow forcing the usage of a specific wallclock source.
> >  
> >   * `cmos` force usage of the CMOS RTC wallclock.
> >  
> > + * `no-cmos-probe` do not probe for the CMOS RTC presence if the ACPI FADT
> > +   table signals there's no CMOS RTC.  Implies using the same heuristics as
> > +   the `auto` option.  By default Xen will probe for the CMOS RTC presence
> > +   even when ACPI FADT signals no CMOS RTC available.
> 
> "By default ..." reads as if this would always occur, which I don't think
> is the case.

Hm, not when using the Xen timer source indeed, there's no probing
then.

> > @@ -1560,6 +1560,8 @@ static int __init cf_check parse_wallclock(const char *arg)
> >      if ( !arg )
> >          return -EINVAL;
> >  
> > +    cmos_rtc_probe = true;
> > +
> >      if ( !strcmp("auto", arg) )
> >          wallclock_source = WALLCLOCK_UNSET;
> >      else if ( !strcmp("xen", arg) )
> > @@ -1571,6 +1573,8 @@ static int __init cf_check parse_wallclock(const char *arg)
> >      }
> >      else if ( !strcmp("cmos", arg) )
> >          wallclock_source = WALLCLOCK_CMOS;
> > +    else if ( !strcmp("no-cmos-probe", arg) )
> > +        cmos_rtc_probe = false;
> >      else if ( !strcmp("efi", arg) )
> >      {
> >          if ( !efi_enabled(EFI_RS) )
> 
> And to request a particular wallclock _and_ control the probing one then
> needs two wallclock= on the command line? And - because of the forcing to
> true of cmos_rtc_probe - even in a particular order. Not very nice from a
> usability pov.

If you request a specific wallclock then there's no probing, so
nothing to control.  I agree the interface is not great, but I
couldn't come up with anything better.

I'm kind of fine with not introducing an extra option to wallclock= to
control the CMOS RTC probing, but would you agree to switching
cmos-rtc-probe to true by default?

Thanks, Roger.

Re: [PATCH v3 7/7] x86/time: probe the CMOS RTC by default

[Jan Beulich]

On 04.09.2024 14:45, Roger Pau Monné wrote:
> On Tue, Sep 03, 2024 at 05:48:09PM +0200, Jan Beulich wrote:
>> On 03.09.2024 15:03, Roger Pau Monne wrote:
>>> Probing for the CMOS RTC registers consist in reading IO ports, and we expect
>>> those reads to have no side effects even when the CMOS RTC is not present.
>>
>> But what do we gain from this besides possible being slower to boot?
> 
> The intent is that Xen can successfully boot on more systems without
> having to pass specific command line options.

At the risk of breaking on others, in perhaps subtle ways?

>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -326,11 +326,14 @@ Interrupts.  Specifying zero disables CMCI handling.
>>>  ### cmos-rtc-probe (x86)
>>>  > `= <boolean>`
>>>  
>>> -> Default: `false`
>>> +> Default: `true`
>>>  
>>>  Flag to indicate whether to probe for a CMOS Real Time Clock irrespective of
>>>  ACPI indicating none to be there.
>>>  
>>> +**WARNING: The `cmos-rtc-probe` option is deprecated and superseded by
>>> +_wallclock=no-cmos-probe_ using both options in combination is undefined.**
>>
>> Hmm, but then ...
>>
>>> @@ -2822,7 +2825,7 @@ 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`
>>> +> `= auto | xen | cmos | no-cmos-probe | efi`
>>
>> ... this wants to be a boolean sub-option "cmos-probe", such that the flag
>> can still be set both ways (in particular for a later command line option
>> to override an earlier one).
> 
> What's the point in overriding?  Either the users selects a specific
> wallclock to use, or it's left for Xen to decide which wallclock to
> pick, either with (auto) or without (no-cmos-probe) possibly probing
> the CMOS RTC.

Overriding can be quite relevant, in particular with xen.efi. There you put
command line options in a config file. You may not want to edit that config
file every time you try something (you may not even have an editor, first
and foremost when there's no EFI shell offered by firmware), and xen.efi
intentionally also parses options from its command line (after the first --
separator). Those "manually" supplied options want to be able to override
whatever is in the config file.

> Multiple different wallclock options being passed on the command line
> will result in just the last one taking effect.

That's the goal, yes.

>>> @@ -1560,6 +1560,8 @@ static int __init cf_check parse_wallclock(const char *arg)
>>>      if ( !arg )
>>>          return -EINVAL;
>>>  
>>> +    cmos_rtc_probe = true;
>>> +
>>>      if ( !strcmp("auto", arg) )
>>>          wallclock_source = WALLCLOCK_UNSET;
>>>      else if ( !strcmp("xen", arg) )
>>> @@ -1571,6 +1573,8 @@ static int __init cf_check parse_wallclock(const char *arg)
>>>      }
>>>      else if ( !strcmp("cmos", arg) )
>>>          wallclock_source = WALLCLOCK_CMOS;
>>> +    else if ( !strcmp("no-cmos-probe", arg) )
>>> +        cmos_rtc_probe = false;
>>>      else if ( !strcmp("efi", arg) )
>>>      {
>>>          if ( !efi_enabled(EFI_RS) )
>>
>> And to request a particular wallclock _and_ control the probing one then
>> needs two wallclock= on the command line? And - because of the forcing to
>> true of cmos_rtc_probe - even in a particular order. Not very nice from a
>> usability pov.
> 
> If you request a specific wallclock then there's no probing, so
> nothing to control.  I agree the interface is not great, but I
> couldn't come up with anything better.
> 
> I'm kind of fine with not introducing an extra option to wallclock= to
> control the CMOS RTC probing, but would you agree to switching
> cmos-rtc-probe to true by default?

I remain to be convinced of this being a good idea.

Jan

Re: [PATCH v3 0/7] x86/time: improvements to wallclock logic

[Jan Beulich]

On 03.09.2024 15:02, Roger Pau Monne wrote:
> 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.
> First 3 patches should be non-functional changes, mostly chopping the
> current logic into smaller functions so that in patch 4 the probing vs
> runtime wallclock logic can be split.
> 
> Patch 5 changes the preference to use CMOS RTC even when booted from EFI
> firmware.
> 
> Finally patches 6 introduces a new command line option to bypass the
> probing an allow specifying which wallclock source to use on the command
> line.  Patch 7 enables CMOS RTC probing by default.
> 
> Thanks, Roger.
> 
> Roger Pau Monne (7):
>   x86/time: introduce helper to fetch Xen wallclock when running as a
>     guest
>   x86/time: move CMOS edge detection into read helper
>   x86/time: split CMOS read and probe logic into function
>   x86/time: introduce probing logic for the wallclock
>   x86/time: prefer CMOS over EFI_GET_TIME
>   x86/time: introduce command line option to select wallclock
>   x86/time: probe the CMOS RTC by default
> 
>  docs/misc/xen-command-line.pandoc |  28 +++-
>  xen/arch/x86/time.c               | 238 +++++++++++++++++++++++-------
>  2 files changed, 208 insertions(+), 58 deletions(-)

Having reached patch 6, it seems pretty clear that somewhere in the series
a CHANGELOG.md entry wants adding.

Jan