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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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