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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>

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