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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 5269 bytes --]

On Thu, Sep 12, 2024 at 03:47:53PM +0200, Roger Pau Monné wrote:
> 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`.

Looks good.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]