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é <[email protected]>
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
