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