Re: [PATCH v6 1/2] x86/time: introduce command line option to select wallclock

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

On Thu, Sep 12, 2024 at 01:57:00PM +0200, Jan Beulich wrote:
> On 12.09.2024 13:15, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1552,6 +1552,35 @@ static const char *__init wallclock_type_to_string(void)
> >      return "";
> >  }
> >  
> > +static int __init cf_check parse_wallclock(const char *arg)
> > +{
> > +    if ( !arg )
> > +        return -EINVAL;
> > +
> > +    if ( !strcmp("auto", arg) )
> > +        wallclock_source = WALLCLOCK_UNSET;
> > +    else if ( !strcmp("xen", arg) )
> > +    {
> > +        if ( !xen_guest )
> > +            return -EINVAL;
> > +
> > +        wallclock_source = WALLCLOCK_XEN;
> > +    }
> > +    else if ( !strcmp("cmos", arg) )
> > +        wallclock_source = WALLCLOCK_CMOS;
> > +    else if ( !strcmp("efi", arg) )
> > +        /*
> > +         * Checking if run-time services are available must be done after
> > +         * command line parsing.
> > +         */
> > +        wallclock_source = WALLCLOCK_EFI;
> > +    else
> > +        return -EINVAL;
> > +
> > +    return 0;
> > +}
> > +custom_param("wallclock", parse_wallclock);
> > +
> >  static void __init probe_wallclock(void)
> >  {
> >      ASSERT(wallclock_source == WALLCLOCK_UNSET);
> > @@ -2527,7 +2556,15 @@ int __init init_xen_time(void)
> >  
> >      open_softirq(TIME_CALIBRATE_SOFTIRQ, local_time_calibration);
> >  
> > -    probe_wallclock();
> > +    /*
> > +     * EFI run time services can be disabled from the command line, hence the
> > +     * check for them cannot be done as part of the wallclock option parsing.
> > +     */
> > +    if ( wallclock_source == WALLCLOCK_EFI && !efi_enabled(EFI_RS) )
> > +        wallclock_source = WALLCLOCK_UNSET;
> > +
> > +    if ( wallclock_source == WALLCLOCK_UNSET )
> > +        probe_wallclock();
> 
> I don't want to stand in the way, and I can live with this form, so I'd like to
> ask that EFI folks or Andrew provide the necessary A-b / R-b. I'd like to note
> though that there continue to be quirks here. They may not be affecting the
> overall result as long as we have only three possible wallclocks, but there
> might be problems if a 4th one appeared.
> 
> With the EFI_RS check in the command line handler and assuming the default case
> of no "efi=no-rs" on the command line, EFI_RS may still end up being off by the
> time the command line is being parsed. With "wallclock=cmos wallclock=efi" this
> would result in no probing and "cmos" chosen if there was that check in place.
> With the logic as added here there will be probing in that case. Replace "cmos"
> by a hypothetical non-default 4th wallclock type (i.e. probing picking "cmos"),
> and I expect you can see how the result would then not necessarily be as
> expected.

My expectation would be that if "wallclock=cmos wallclock=efi" is used
the last option overrides any previous one, and hence if that last
option is not valid the logic will fallback to the default selection
(in this case to probing).

Thinking about this, it might make sense to unconditionally set
wallclock_source = WALLCLOCK_UNSET at the start of parse_wallclock()
to avoid previous instances carrying over if later ones are not valid.

Thanks, Roger.