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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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 ...

> +
>  ### 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.

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
    }

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

~Andrew