Re: [PATCH v5 3/4] x86/time: introduce command line option to select wallclock

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

On Tue, Sep 10, 2024 at 11:32:05AM +0200, Jan Beulich wrote:
> On 09.09.2024 16:54, Roger Pau Monne wrote:
> > Allow setting the used wallclock from the command line.  When the option is set
> > to a value different than `auto` the probing is bypassed and the selected
> > implementation is used (as long as it's available).
> > 
> > The `xen` and `efi` options require being booted as a Xen guest (with Xen guest
> > supported built-in) or from UEFI firmware.
> 
> Perhaps add "respectively"?

Sure.

> 
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1550,6 +1550,36 @@ 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) )
> > +    {
> > +        if ( !efi_enabled(EFI_RS) )
> > +            return -EINVAL;
> 
> I'm afraid there's a problem here, and I'm sorry for not paying attention
> earlier: EFI_RS is possibly affected by "efi=" (and hence may change after
> this code ran). (It can also be cleared if ->SetVirtualAddressMap() fails,
> but I think that's strictly ahead of command line parsing.)

Hm, I see, thanks for noticing.  Anyone using 'efi=no-rs
wallclock=efi' likely deserves to be punished.  Would you be fine with
adding the following in init_xen_time():

    /*
     * EFI run time services can be disabled form 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();

Thanks, Roger.