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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 12, 2024 at 03:30:29PM +0200, Marek Marczykowski-Górecki wrote:
> On Thu, Sep 12, 2024 at 02:56:55PM +0200, Roger Pau Monné wrote:
> > 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).
> 
> That would be my expectation too. If some kind of preference would be
> expected, it should looks like wallclock=efi,cmos, but I don't think we
> need that.
> 
> > 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.
> 
> This may be a good idea. But more importantly, the behavior should be
> included in the option documentation (that if a selected value is not
> available, it fallback to auto). And maybe a log message when that
> happens (but I'm okay with skipping this one, as selected wallclock
> source is logged already)?

Thanks, would you be fine with:

### wallclock (x86)
> `= auto | xen | cmos | efi`

> Default: `auto`

Allow forcing the usage of a specific wallclock source.

 * `auto` let the hypervisor select the clocksource based on internal
   heuristics.

 * `xen` force usage of the Xen shared_info wallclock when booted as a Xen
   guest.  This option is only available if the hypervisor was compiled with
   `CONFIG_XEN_GUEST` enabled.

 * `cmos` force usage of the CMOS RTC wallclock.

 * `efi` force usage of the EFI_GET_TIME run-time method when booted from EFI
   firmware.

If the selected option is not available Xen will default to `auto`.

Regards, Roger.