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

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

On Tue, Sep 10, 2024 at 04:29:43PM +0200, Jan Beulich wrote:
> On 10.09.2024 16:24, Roger Pau Monné wrote:
> > On Tue, Sep 10, 2024 at 03:49:52PM +0200, Jan Beulich wrote:
> >> On 10.09.2024 15:10, Roger Pau Monné wrote:
> >>>  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.
> 
> Hmm. Rejecting the option if we can is reasonable imo. "efi=rs" can imo only
> sensibly be used to override an earlier "efi=no-rs". Hence what we see while
> parsing the wallclock option gives us at least reasonable grounds to reject
> the option if EFI_RS is already clear. We then merely fail to reject the
> option if the flag is cleared later.

I won't strongly argue about it, but I think having a non-reliable
check in parse_wallclock() is confusing.  I would have to add a
comment there anyway to note that depending on the position of the efi
and wallclock parameters the check for EFI_RS might not be effective -
at which point I think it's best to unify the check so it's uniformly
performed in init_xen_time().

> Yet in the end I'd be happy to leave this particular aspect to you and the
> EFI maintainers.

Thanks again for the feedback.

Roger.