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

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

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.