| Message ID | 20240912111514.26934-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/time: improvements to wallclock logic | expand |
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
On Thu, Sep 12, 2024 at 01:57:00PM +0200, Jan Beulich wrote: > 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. My expectation would be that if "wallclock=cmos wallclock=efi" is used the last option overrides any previous one, and hence if that last option is not valid the logic will fallback to the default selection (in this case to probing). Thinking about this, it might make sense to unconditionally set wallclock_source = WALLCLOCK_UNSET at the start of parse_wallclock() to avoid previous instances carrying over if later ones are not valid. Thanks, Roger.
On Thu, Sep 12, 2024 at 02:56:55PM +0200, Roger Pau Monné wrote: > On Thu, Sep 12, 2024 at 01:57:00PM +0200, Jan Beulich wrote: > > 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. > > My expectation would be that if "wallclock=cmos wallclock=efi" is used > the last option overrides any previous one, and hence if that last > option is not valid the logic will fallback to the default selection > (in this case to probing). That would be my expectation too. If some kind of preference would be expected, it should looks like wallclock=efi,cmos, but I don't think we need that. > Thinking about this, it might make sense to unconditionally set > wallclock_source = WALLCLOCK_UNSET at the start of parse_wallclock() > to avoid previous instances carrying over if later ones are not valid. This may be a good idea. But more importantly, the behavior should be included in the option documentation (that if a selected value is not available, it fallback to auto). And maybe a log message when that happens (but I'm okay with skipping this one, as selected wallclock source is logged already)?
On Thu, Sep 12, 2024 at 03:30:29PM +0200, Marek Marczykowski-Górecki wrote: > On Thu, Sep 12, 2024 at 02:56:55PM +0200, Roger Pau Monné wrote: > > On Thu, Sep 12, 2024 at 01:57:00PM +0200, Jan Beulich wrote: > > > 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. > > > > My expectation would be that if "wallclock=cmos wallclock=efi" is used > > the last option overrides any previous one, and hence if that last > > option is not valid the logic will fallback to the default selection > > (in this case to probing). > > That would be my expectation too. If some kind of preference would be > expected, it should looks like wallclock=efi,cmos, but I don't think we > need that. > > > Thinking about this, it might make sense to unconditionally set > > wallclock_source = WALLCLOCK_UNSET at the start of parse_wallclock() > > to avoid previous instances carrying over if later ones are not valid. > > This may be a good idea. But more importantly, the behavior should be > included in the option documentation (that if a selected value is not > available, it fallback to auto). And maybe a log message when that > happens (but I'm okay with skipping this one, as selected wallclock > source is logged already)? Thanks, would you be fine with: ### 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. If the selected option is not available Xen will default to `auto`. Regards, Roger.
On Thu, Sep 12, 2024 at 03:47:53PM +0200, Roger Pau Monné wrote: > On Thu, Sep 12, 2024 at 03:30:29PM +0200, Marek Marczykowski-Górecki wrote: > > On Thu, Sep 12, 2024 at 02:56:55PM +0200, Roger Pau Monné wrote: > > > On Thu, Sep 12, 2024 at 01:57:00PM +0200, Jan Beulich wrote: > > > > 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. > > > > > > My expectation would be that if "wallclock=cmos wallclock=efi" is used > > > the last option overrides any previous one, and hence if that last > > > option is not valid the logic will fallback to the default selection > > > (in this case to probing). > > > > That would be my expectation too. If some kind of preference would be > > expected, it should looks like wallclock=efi,cmos, but I don't think we > > need that. > > > > > Thinking about this, it might make sense to unconditionally set > > > wallclock_source = WALLCLOCK_UNSET at the start of parse_wallclock() > > > to avoid previous instances carrying over if later ones are not valid. > > > > This may be a good idea. But more importantly, the behavior should be > > included in the option documentation (that if a selected value is not > > available, it fallback to auto). And maybe a log message when that > > happens (but I'm okay with skipping this one, as selected wallclock > > source is logged already)? > > Thanks, would you be fine with: > > ### 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. > > If the selected option is not available Xen will default to `auto`. Looks good.
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 959cf45b55d9..a4f916d0d33e 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2816,6 +2816,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 9588502f28e8..b97c81ebd3e9 100644 --- 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(); 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 respectively. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v5: - Do EFI run-time services checking after command line parsing. 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 | 39 ++++++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 1 deletion(-)