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

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

On Mon, Sep 16, 2024 at 02:11:08PM +0100, Andrew Cooper wrote:
> On 13/09/2024 8:59 am, Roger Pau Monne wrote:
> > diff --git a/docs/misc/xen-command-line.pandoc 
> > b/docs/misc/xen-command-line.pandoc
> > index 959cf45b55d9..2a9b3b9b8975 100644
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -2816,6 +2816,27 @@ vwfi to `native` reduces irq latency significantly. 
> > It can also lead to
> >  suboptimal scheduling decisions, but only when the system is
> >  oversubscribed (i.e., in total there are more vCPUs than pCPUs).
> >  
> > +### 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.
> 
> For both `xen` and `efi`, something should be said about "if selected
> and not satisfied, Xen falls back to other heuristics".
> 
> > +
> > +If the selected option is invalid or not available Xen will default to 
> > `auto`.
> 
> I'm afraid that I'm firmly of the opinion that "auto" on the cmdline is
> unnecessary complexity.  Auto is the default, and doesn't need
> specifying explicitly.  That in turn simplifies ...

What about overriding earlier choice? For example overriding a built-in
cmdline? That said, with the current code, the same can be achieved with
wallclock=foo, and living with the warning in boot log...

> > +
> >  ### watchdog (x86)
> >  > `= force | <boolean>`
> >  
> > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> > index 29b026735e5d..e4751684951e 100644
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1552,6 +1552,37 @@ static const char *__init 
> > wallclock_type_to_string(void)
> >      return "";
> >  }
> >  
> > +static int __init cf_check parse_wallclock(const char *arg)
> > +{
> > +    wallclock_source = WALLCLOCK_UNSET;
> > +
> > +    if ( !arg )
> > +        return -EINVAL;
> > +
> > +    if ( !strcmp("auto", arg) )
> > +        ASSERT(wallclock_source == WALLCLOCK_UNSET);
> 
> ... this.
> 
> Hitting this assert will manifest as a system reboot/hang with no
> information on serial/VGA, because all of this runs prior to getting up
> the console.  You only get to see it on a PVH boot because we bodge the
> Xen console into default-existence.

This assert is no-op as wallclock_source is unconditionally set to 
WALLCLOCK_UNSET few lines above.

> So, ASSERT()/etc really need avoiding wherever possible in cmdline parsing.
> 
> In this case, all it serves to do is break examples like `wallclock=xen
> wallclock=auto` case, which is unlike our latest-takes-precedence
> behaviour everywhere else.
> 
> > +    else if ( !strcmp("xen", arg) )
> > +    {
> > +        if ( !xen_guest )
> 
> We don't normally treat this path as an error when parsing (we know what
> it is, even if we can't action it).  Instead, there's no_config_param()
> to be more friendly (for PVH at least).
> 
> It's a bit awkward, but this should do:
> 
>     {
> #ifdef CONFIG_XEN_GUEST
>         wallclock_source = WALLCLOCK_XEN;
> #else
>         no_config_param("XEN_GUEST", "wallclock", s, ss);
> #endif
>     }

Can you boot the binary build with CONFIG_XEN_GUEST=y as native? If so,
the above will not be enough, a runtime check is needed anyway.

> There probably wants to be something similar for EFI, although it's not
> a plain CONFIG so it might be more tricky.

It needs to be runtime check here even more. Not only because of
different boot modes, but due to interaction with efi=no-rs (or any
other reason for not having runtime services). See the comment there.

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

Attachment: signature.asc
Description: PGP signature