| Message ID | 20240909145455.7517-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/time: improvements to wallclock logic | expand |
On 09.09.2024 16:54, Roger Pau Monne wrote: > Allow setting the used wallclock from the command line. When the option is set > to a value different than `auto` the probing is bypassed and the selected > implementation is used (as long as it's available). > > The `xen` and `efi` options require being booted as a Xen guest (with Xen guest > supported built-in) or from UEFI firmware. Perhaps add "respectively"? > --- 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.) Jan
On Tue, Sep 10, 2024 at 11:32:05AM +0200, Jan Beulich wrote: > On 09.09.2024 16:54, Roger Pau Monne wrote: > > Allow setting the used wallclock from the command line. When the option is set > > to a value different than `auto` the probing is bypassed and the selected > > implementation is used (as long as it's available). > > > > The `xen` and `efi` options require being booted as a Xen guest (with Xen guest > > supported built-in) or from UEFI firmware. > > Perhaps add "respectively"? Sure. > > > --- 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. 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(); Thanks, Roger.
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
On Tue, Sep 10, 2024 at 03:49:52PM +0200, Jan Beulich wrote: > 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 ... It's not too complicated to attempt to arrange for something half sane even if the user provided options are nonsense. I've seen people accumulate all kind of crap on the command line "just because I've read it online". > > 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). I didn't put in my previous reply, but I've removed the efi_enabled() check from the option parsing and instead added this comment: /* * Checking if run-time services are available must be done after * command line parsing. */ I don't think there's much point in doing the check in parse_wallclock() if it's not reliable, so your reference in the comment to "the check done as part of option parsing" is no longer valid. Thanks, Roger.
On 10.09.2024 16:24, Roger Pau Monné wrote: > On Tue, Sep 10, 2024 at 03:49:52PM +0200, Jan Beulich wrote: >> On 10.09.2024 15:10, Roger Pau Monné wrote: >>> 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). > > I didn't put in my previous reply, but I've removed the efi_enabled() > check from the option parsing and instead added this comment: > > /* > * Checking if run-time services are available must be done after > * command line parsing. > */ > > I don't think there's much point in doing the check in > parse_wallclock() if it's not reliable, so your reference in the > comment to "the check done as part of option parsing" is no longer > valid. Hmm. Rejecting the option if we can is reasonable imo. "efi=rs" can imo only sensibly be used to override an earlier "efi=no-rs". Hence what we see while parsing the wallclock option gives us at least reasonable grounds to reject the option if EFI_RS is already clear. We then merely fail to reject the option if the flag is cleared later. Yet in the end I'd be happy to leave this particular aspect to you and the EFI maintainers. Jan
On Tue, Sep 10, 2024 at 04:29:43PM +0200, Jan Beulich wrote: > On 10.09.2024 16:24, Roger Pau Monné wrote: > > On Tue, Sep 10, 2024 at 03:49:52PM +0200, Jan Beulich wrote: > >> On 10.09.2024 15:10, Roger Pau Monné wrote: > >>> 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). > > > > I didn't put in my previous reply, but I've removed the efi_enabled() > > check from the option parsing and instead added this comment: > > > > /* > > * Checking if run-time services are available must be done after > > * command line parsing. > > */ > > > > I don't think there's much point in doing the check in > > parse_wallclock() if it's not reliable, so your reference in the > > comment to "the check done as part of option parsing" is no longer > > valid. > > Hmm. Rejecting the option if we can is reasonable imo. "efi=rs" can imo only > sensibly be used to override an earlier "efi=no-rs". Hence what we see while > parsing the wallclock option gives us at least reasonable grounds to reject > the option if EFI_RS is already clear. We then merely fail to reject the > option if the flag is cleared later. I won't strongly argue about it, but I think having a non-reliable check in parse_wallclock() is confusing. I would have to add a comment there anyway to note that depending on the position of the efi and wallclock parameters the check for EFI_RS might not be effective - at which point I think it's best to unify the check so it's uniformly performed in init_xen_time(). > Yet in the end I'd be happy to leave this particular aspect to you and the > EFI maintainers. Thanks again for the feedback. Roger.
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 0a66e1ee2d7e..1944b9d8eb9d 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2821,6 +2821,25 @@ 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. + ### watchdog (x86) > `= force | <boolean>` diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index 1dcbd9f520f5..c6d3f19a56d1 100644 --- 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; + + 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); @@ -2525,7 +2555,8 @@ int __init init_xen_time(void) open_softirq(TIME_CALIBRATE_SOFTIRQ, local_time_calibration); - probe_wallclock(); + if ( wallclock_source == WALLCLOCK_UNSET ) + probe_wallclock(); printk(XENLOG_INFO "Wallclock source: %s\n", wallclock_type_to_string());
Allow setting the used wallclock from the command line. When the option is set to a value different than `auto` the probing is bypassed and the selected implementation is used (as long as it's available). The `xen` and `efi` options require being booted as a Xen guest (with Xen guest supported built-in) or from UEFI firmware. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v3: - Note xen option is only available if Xen guest support it built. - Fix typo. --- docs/misc/xen-command-line.pandoc | 19 ++++++++++++++++++ xen/arch/x86/time.c | 33 ++++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 1 deletion(-)