| Message ID | 20240830095220.49473-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/time: prefer CMOS over EFI_GET_TIME | expand |
On 30/08/2024 10:52 am, Roger Pau Monne wrote: > Split the logic that deals with probing and fetching the CMOS time into a > separate helper. While moving the code also take the opportunity to reduce the > scope of some local variables. > > No functional change intended. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> This does look like a straight rearrangement, so Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, although... > +static unsigned long get_cmos_time(void) > +{ > + struct rtc_time rtc; > + static bool __read_mostly cmos_rtc_probe; > + boolean_param("cmos-rtc-probe", cmos_rtc_probe); > + > + if ( efi_enabled(EFI_RS) ) > + { > + unsigned long 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 ( unlikely(!read_cmos_time(&rtc, cmos_rtc_probe)) ) > panic("No CMOS RTC found - system must be booted from EFI\n"); ... this really does show some (preexisting) tangled logic. All of this should live in an init function, and not be re-evaluated each time we call get_wallclock_time(), not that we call it very often. cmos_rtc_probe should be __initdata or at least __ro_after_init, but it gets written on every pass through the function on most systems. Lets focus on not crashing. Cleanup can come later. ~Andrew
On 30.08.2024 11:52, Roger Pau Monne wrote: > @@ -1285,33 +1270,56 @@ static unsigned long get_cmos_time(void) > } while ( (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) && > t2 < MILLISECS(3) ); > > - __get_cmos_time(&rtc); > + __get_cmos_time(rtc); > > spin_unlock_irqrestore(&rtc_lock, flags); > > - if ( likely(!cmos_rtc_probe) || > - t1 > SECONDS(1) || t2 >= MILLISECS(3) || > - rtc.sec >= 60 || rtc.min >= 60 || rtc.hour >= 24 || > - !rtc.day || rtc.day > 31 || > - !rtc.mon || rtc.mon > 12 ) > - break; > + if ( likely(!cmos_rtc_probe) ) > + return true; > + > + if ( t1 > SECONDS(1) || t2 >= MILLISECS(3) || > + rtc->sec >= 60 || rtc->min >= 60 || rtc->hour >= 24 || > + !rtc->day || rtc->day > 31 || > + !rtc->mon || rtc->mon > 12 ) > + return false; > > if ( seconds < 60 ) > { > - if ( rtc.sec != seconds ) > - { > - cmos_rtc_probe = false; This clearing of the variable is lost, which looks wrong to me. Jan > + if ( rtc->sec != seconds ) > acpi_gbl_FADT.boot_flags &= ~ACPI_FADT_NO_CMOS_RTC; > - } > - break; > + return true; > } > > process_pending_softirqs(); > > - seconds = rtc.sec; > + seconds = rtc->sec; > } > > - if ( unlikely(cmos_rtc_probe) ) > + ASSERT_UNREACHABLE(); > + return false; > +} > + > +static unsigned long get_cmos_time(void) > +{ > + struct rtc_time rtc; > + static bool __read_mostly cmos_rtc_probe; > + boolean_param("cmos-rtc-probe", cmos_rtc_probe); > + > + if ( efi_enabled(EFI_RS) ) > + { > + unsigned long 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 ( unlikely(!read_cmos_time(&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);
On Tue, Sep 03, 2024 at 08:24:18AM +0200, Jan Beulich wrote: > On 30.08.2024 11:52, Roger Pau Monne wrote: > > @@ -1285,33 +1270,56 @@ static unsigned long get_cmos_time(void) > > } while ( (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) && > > t2 < MILLISECS(3) ); > > > > - __get_cmos_time(&rtc); > > + __get_cmos_time(rtc); > > > > spin_unlock_irqrestore(&rtc_lock, flags); > > > > - if ( likely(!cmos_rtc_probe) || > > - t1 > SECONDS(1) || t2 >= MILLISECS(3) || > > - rtc.sec >= 60 || rtc.min >= 60 || rtc.hour >= 24 || > > - !rtc.day || rtc.day > 31 || > > - !rtc.mon || rtc.mon > 12 ) > > - break; > > + if ( likely(!cmos_rtc_probe) ) > > + return true; > > + > > + if ( t1 > SECONDS(1) || t2 >= MILLISECS(3) || > > + rtc->sec >= 60 || rtc->min >= 60 || rtc->hour >= 24 || > > + !rtc->day || rtc->day > 31 || > > + !rtc->mon || rtc->mon > 12 ) > > + return false; > > > > if ( seconds < 60 ) > > { > > - if ( rtc.sec != seconds ) > > - { > > - cmos_rtc_probe = false; > > This clearing of the variable is lost, which looks wrong to me. Note the code in get_cmos_time() is modified, so the variable is no longer used past the call to read_cmos_time(). Instead the signaling of whether the CMOS is functional or not is done using the return value of the newly introduced read_cmos_time() function. Thanks, Roger.
On 03.09.2024 09:35, Roger Pau Monné wrote: > On Tue, Sep 03, 2024 at 08:24:18AM +0200, Jan Beulich wrote: >> On 30.08.2024 11:52, Roger Pau Monne wrote: >>> @@ -1285,33 +1270,56 @@ static unsigned long get_cmos_time(void) >>> } while ( (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) && >>> t2 < MILLISECS(3) ); >>> >>> - __get_cmos_time(&rtc); >>> + __get_cmos_time(rtc); >>> >>> spin_unlock_irqrestore(&rtc_lock, flags); >>> >>> - if ( likely(!cmos_rtc_probe) || >>> - t1 > SECONDS(1) || t2 >= MILLISECS(3) || >>> - rtc.sec >= 60 || rtc.min >= 60 || rtc.hour >= 24 || >>> - !rtc.day || rtc.day > 31 || >>> - !rtc.mon || rtc.mon > 12 ) >>> - break; >>> + if ( likely(!cmos_rtc_probe) ) >>> + return true; >>> + >>> + if ( t1 > SECONDS(1) || t2 >= MILLISECS(3) || >>> + rtc->sec >= 60 || rtc->min >= 60 || rtc->hour >= 24 || >>> + !rtc->day || rtc->day > 31 || >>> + !rtc->mon || rtc->mon > 12 ) >>> + return false; >>> >>> if ( seconds < 60 ) >>> { >>> - if ( rtc.sec != seconds ) >>> - { >>> - cmos_rtc_probe = false; >> >> This clearing of the variable is lost, which looks wrong to me. > > Note the code in get_cmos_time() is modified, so the variable is no > longer used past the call to read_cmos_time(). Instead the signaling > of whether the CMOS is functional or not is done using the return > value of the newly introduced read_cmos_time() function. I wasn't concerned of the further processing on the 1st invocation, but of the behavior of the 2nd invocation. But yes, there the flag will end up being cleared because of the FADT flag also having been cleared. Not easily visible, though. Could minimally do with a remark in the description. Jan
On Tue, Sep 03, 2024 at 09:53:47AM +0200, Jan Beulich wrote: > On 03.09.2024 09:35, Roger Pau Monné wrote: > > On Tue, Sep 03, 2024 at 08:24:18AM +0200, Jan Beulich wrote: > >> On 30.08.2024 11:52, Roger Pau Monne wrote: > >>> @@ -1285,33 +1270,56 @@ static unsigned long get_cmos_time(void) > >>> } while ( (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) && > >>> t2 < MILLISECS(3) ); > >>> > >>> - __get_cmos_time(&rtc); > >>> + __get_cmos_time(rtc); > >>> > >>> spin_unlock_irqrestore(&rtc_lock, flags); > >>> > >>> - if ( likely(!cmos_rtc_probe) || > >>> - t1 > SECONDS(1) || t2 >= MILLISECS(3) || > >>> - rtc.sec >= 60 || rtc.min >= 60 || rtc.hour >= 24 || > >>> - !rtc.day || rtc.day > 31 || > >>> - !rtc.mon || rtc.mon > 12 ) > >>> - break; > >>> + if ( likely(!cmos_rtc_probe) ) > >>> + return true; > >>> + > >>> + if ( t1 > SECONDS(1) || t2 >= MILLISECS(3) || > >>> + rtc->sec >= 60 || rtc->min >= 60 || rtc->hour >= 24 || > >>> + !rtc->day || rtc->day > 31 || > >>> + !rtc->mon || rtc->mon > 12 ) > >>> + return false; > >>> > >>> if ( seconds < 60 ) > >>> { > >>> - if ( rtc.sec != seconds ) > >>> - { > >>> - cmos_rtc_probe = false; > >> > >> This clearing of the variable is lost, which looks wrong to me. > > > > Note the code in get_cmos_time() is modified, so the variable is no > > longer used past the call to read_cmos_time(). Instead the signaling > > of whether the CMOS is functional or not is done using the return > > value of the newly introduced read_cmos_time() function. > > I wasn't concerned of the further processing on the 1st invocation, but > of the behavior of the 2nd invocation. But yes, there the flag will end > up being cleared because of the FADT flag also having been cleared. Not > easily visible, though. Could minimally do with a remark in the > description. Indeed, the variable gets clearer on further invocations, as the adjustment to ACPI_FADT_NO_CMOS_RTC gets propagated. Given Andrew comments, I've reworked all of this and I think it ended up being clearer. Thanks, Roger.
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index a97d78484105..272ca2468ea6 100644 --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -1245,29 +1245,14 @@ static void __get_cmos_time(struct rtc_time *rtc) rtc->year += 100; } -static unsigned long get_cmos_time(void) +/* Returns true when fetching time from CMOS is successful. */ +static bool read_cmos_time(struct rtc_time *rtc, bool cmos_rtc_probe) { - unsigned long res, flags; - 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 ( ; ; ) { + unsigned long flags; s_time_t start, t1, t2; spin_lock_irqsave(&rtc_lock, flags); @@ -1285,33 +1270,56 @@ static unsigned long get_cmos_time(void) } while ( (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) && t2 < MILLISECS(3) ); - __get_cmos_time(&rtc); + __get_cmos_time(rtc); spin_unlock_irqrestore(&rtc_lock, flags); - if ( likely(!cmos_rtc_probe) || - t1 > SECONDS(1) || t2 >= MILLISECS(3) || - rtc.sec >= 60 || rtc.min >= 60 || rtc.hour >= 24 || - !rtc.day || rtc.day > 31 || - !rtc.mon || rtc.mon > 12 ) - break; + if ( likely(!cmos_rtc_probe) ) + return true; + + if ( t1 > SECONDS(1) || t2 >= MILLISECS(3) || + rtc->sec >= 60 || rtc->min >= 60 || rtc->hour >= 24 || + !rtc->day || rtc->day > 31 || + !rtc->mon || rtc->mon > 12 ) + return false; if ( seconds < 60 ) { - if ( rtc.sec != seconds ) - { - cmos_rtc_probe = false; + if ( rtc->sec != seconds ) acpi_gbl_FADT.boot_flags &= ~ACPI_FADT_NO_CMOS_RTC; - } - break; + return true; } process_pending_softirqs(); - seconds = rtc.sec; + seconds = rtc->sec; } - if ( unlikely(cmos_rtc_probe) ) + ASSERT_UNREACHABLE(); + return false; +} + +static unsigned long get_cmos_time(void) +{ + struct rtc_time rtc; + static bool __read_mostly cmos_rtc_probe; + boolean_param("cmos-rtc-probe", cmos_rtc_probe); + + if ( efi_enabled(EFI_RS) ) + { + unsigned long 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 ( unlikely(!read_cmos_time(&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);
Split the logic that deals with probing and fetching the CMOS time into a separate helper. While moving the code also take the opportunity to reduce the scope of some local variables. No functional change intended. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. --- xen/arch/x86/time.c | 72 +++++++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 32 deletions(-)