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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan