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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2024 15:10, Roger Pau Monné wrote:
> On Tue, Sep 10, 2024 at 11:32:05AM +0200, Jan Beulich wrote:
>> On 09.09.2024 16:54, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/time.c
>>> +++ b/xen/arch/x86/time.c
>>> @@ -1550,6 +1550,36 @@ 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) )
>>> +    {
>>> +        if ( !efi_enabled(EFI_RS) )
>>> +            return -EINVAL;
>>
>> I'm afraid there's a problem here, and I'm sorry for not paying attention
>> earlier: EFI_RS is possibly affected by "efi=" (and hence may change after
>> this code ran). (It can also be cleared if ->SetVirtualAddressMap() fails,
>> but I think that's strictly ahead of command line parsing.)
> 
> Hm, I see, thanks for noticing.  Anyone using 'efi=no-rs
> wallclock=efi' likely deserves to be punished.

Well, if you don't want to actually do this ;-) then ...

>  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). The subsequent log message should be sufficient to
tell them what's going on.

Jan