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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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)?

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

Attachment: signature.asc
Description: PGP signature