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 03:49:52PM +0200, Jan Beulich wrote:
> On 10.09.2024 15:10, Roger Pau Monné wrote:
> > On Tue, Sep 10, 2024 at 11:32:05AM +0200, Jan Beulich wrote:
> >> On 09.09.2024 16:54, Roger Pau Monne wrote:
> >>> --- 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.
> 
> Well, if you don't want to actually do this ;-) then ...

It's not too complicated to attempt to arrange for something half sane
even if the user provided options are nonsense.  I've seen people
accumulate all kind of crap on the command line "just because I've
read it online".

> >  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();
> 
> ... this is probably the best we can do (nit: s/form/from/ in the comment;
> maybe also "..., hence the check done as part of option parsing may not
> suffice" or some such).

I didn't put in my previous reply, but I've removed the efi_enabled()
check from the option parsing and instead added this comment:

        /*
         * Checking if run-time services are available must be done after
         * command line parsing.
         */

I don't think there's much point in doing the check in
parse_wallclock() if it's not reliable, so your reference in the
comment to "the check done as part of option parsing" is no longer
valid.

Thanks, Roger.