[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 4/7] x86/time: introduce probing logic for the wallclock
On Tue, Sep 03, 2024 at 05:32:27PM +0200, Jan Beulich wrote: > 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. My reasoning was that on a debug build we want to spot any such issues (as it's likely a symptom the RTC is misbehaving?) but on a release build we should rather return an incorrect wallclock time rather than panicking. I can remove the ASSERT and local variable altogether if you prefer. > > > @@ -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. I had it that way originally, but then it seemed the extra indentation made it less readable. Will see how can I adjust it, my preference would be for: 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" : ""); But that exceeds the 80 columns limit. Thanks, Roger.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |