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, 94 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 1e3c13fc7360..9ebeee61b987 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1291,14 +1291,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;
@@ -1328,28 +1337,12 @@ 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);
 
-    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");
+    __get_cmos_time(&rtc);
 
     return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
 }
@@ -1532,12 +1525,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_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;
 }
 
 /***************************************************************************
@@ -2462,6 +2528,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

On 04.09.2024 17:31, Roger Pau Monne wrote:
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1291,14 +1291,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)

I'm sorry for not paying attention to this earlier, but "cmos" alone
is misleading here and perhaps even more so for cmos_read(). These
aren't about the CMOS (storage) but the CMOS RTC. May I suggest
cmos_rtc_{probe,read}() instead?

>  {
>      unsigned int seconds = 60;
>  
> +    if ( !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) )
> +        return true;
> +
> +    if ( !cmos_rtc_probe )
> +        return false;

With this I think ...

>      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;

... this ends up being dead code.

Jan

On Thu, Sep 05, 2024 at 05:58:47PM +0200, Jan Beulich wrote:
> On 04.09.2024 17:31, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1291,14 +1291,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)
> 
> I'm sorry for not paying attention to this earlier, but "cmos" alone
> is misleading here and perhaps even more so for cmos_read(). These
> aren't about the CMOS (storage) but the CMOS RTC. May I suggest
> cmos_rtc_{probe,read}() instead?

I've assumed that those living in time.c would be clear enough it's
the CMOS RTC, but I'm fine with renaming to cmos_rtc_{probe,read}().

> 
> >  {
> >      unsigned int seconds = 60;
> >  
> > +    if ( !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) )
> > +        return true;
> > +
> > +    if ( !cmos_rtc_probe )
> > +        return false;
> 
> With this I think ...
> 
> >      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;
> 
> ... this ends up being dead code.

Indeed, I've missed to remove that one when moving the check outside
of the for loop.

Thanks, Roger.