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

["Roger Pau Monné" <roger.pau@citrix.com>]

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@citrix.com>
> 
> 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.