| Message ID | 20240903130303.71334-8-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/time: improvements to wallclock logic | expand |
On 03.09.2024 15:03, Roger Pau Monne wrote: > Probing for the CMOS RTC registers consist in reading IO ports, and we expect > those reads to have no side effects even when the CMOS RTC is not present. But what do we gain from this besides possible being slower to boot? > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -326,11 +326,14 @@ Interrupts. Specifying zero disables CMCI handling. > ### cmos-rtc-probe (x86) > > `= <boolean>` > > -> Default: `false` > +> Default: `true` > > Flag to indicate whether to probe for a CMOS Real Time Clock irrespective of > ACPI indicating none to be there. > > +**WARNING: The `cmos-rtc-probe` option is deprecated and superseded by > +_wallclock=no-cmos-probe_ using both options in combination is undefined.** Hmm, but then ... > @@ -2822,7 +2825,7 @@ 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` > +> `= auto | xen | cmos | no-cmos-probe | efi` ... this wants to be a boolean sub-option "cmos-probe", such that the flag can still be set both ways (in particular for a later command line option to override an earlier one). > @@ -2836,6 +2839,11 @@ Allow forcing the usage of a specific wallclock source. > > * `cmos` force usage of the CMOS RTC wallclock. > > + * `no-cmos-probe` do not probe for the CMOS RTC presence if the ACPI FADT > + table signals there's no CMOS RTC. Implies using the same heuristics as > + the `auto` option. By default Xen will probe for the CMOS RTC presence > + even when ACPI FADT signals no CMOS RTC available. "By default ..." reads as if this would always occur, which I don't think is the case. > @@ -1560,6 +1560,8 @@ static int __init cf_check parse_wallclock(const char *arg) > if ( !arg ) > return -EINVAL; > > + cmos_rtc_probe = true; > + > if ( !strcmp("auto", arg) ) > wallclock_source = WALLCLOCK_UNSET; > else if ( !strcmp("xen", arg) ) > @@ -1571,6 +1573,8 @@ static int __init cf_check parse_wallclock(const char *arg) > } > else if ( !strcmp("cmos", arg) ) > wallclock_source = WALLCLOCK_CMOS; > + else if ( !strcmp("no-cmos-probe", arg) ) > + cmos_rtc_probe = false; > else if ( !strcmp("efi", arg) ) > { > if ( !efi_enabled(EFI_RS) ) And to request a particular wallclock _and_ control the probing one then needs two wallclock= on the command line? And - because of the forcing to true of cmos_rtc_probe - even in a particular order. Not very nice from a usability pov. Jan
On Tue, Sep 03, 2024 at 05:48:09PM +0200, Jan Beulich wrote: > On 03.09.2024 15:03, Roger Pau Monne wrote: > > Probing for the CMOS RTC registers consist in reading IO ports, and we expect > > those reads to have no side effects even when the CMOS RTC is not present. > > But what do we gain from this besides possible being slower to boot? The intent is that Xen can successfully boot on more systems without having to pass specific command line options. > > --- a/docs/misc/xen-command-line.pandoc > > +++ b/docs/misc/xen-command-line.pandoc > > @@ -326,11 +326,14 @@ Interrupts. Specifying zero disables CMCI handling. > > ### cmos-rtc-probe (x86) > > > `= <boolean>` > > > > -> Default: `false` > > +> Default: `true` > > > > Flag to indicate whether to probe for a CMOS Real Time Clock irrespective of > > ACPI indicating none to be there. > > > > +**WARNING: The `cmos-rtc-probe` option is deprecated and superseded by > > +_wallclock=no-cmos-probe_ using both options in combination is undefined.** > > Hmm, but then ... > > > @@ -2822,7 +2825,7 @@ 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` > > +> `= auto | xen | cmos | no-cmos-probe | efi` > > ... this wants to be a boolean sub-option "cmos-probe", such that the flag > can still be set both ways (in particular for a later command line option > to override an earlier one). What's the point in overriding? Either the users selects a specific wallclock to use, or it's left for Xen to decide which wallclock to pick, either with (auto) or without (no-cmos-probe) possibly probing the CMOS RTC. Multiple different wallclock options being passed on the command line will result in just the last one taking effect. > > @@ -2836,6 +2839,11 @@ Allow forcing the usage of a specific wallclock source. > > > > * `cmos` force usage of the CMOS RTC wallclock. > > > > + * `no-cmos-probe` do not probe for the CMOS RTC presence if the ACPI FADT > > + table signals there's no CMOS RTC. Implies using the same heuristics as > > + the `auto` option. By default Xen will probe for the CMOS RTC presence > > + even when ACPI FADT signals no CMOS RTC available. > > "By default ..." reads as if this would always occur, which I don't think > is the case. Hm, not when using the Xen timer source indeed, there's no probing then. > > @@ -1560,6 +1560,8 @@ static int __init cf_check parse_wallclock(const char *arg) > > if ( !arg ) > > return -EINVAL; > > > > + cmos_rtc_probe = true; > > + > > if ( !strcmp("auto", arg) ) > > wallclock_source = WALLCLOCK_UNSET; > > else if ( !strcmp("xen", arg) ) > > @@ -1571,6 +1573,8 @@ static int __init cf_check parse_wallclock(const char *arg) > > } > > else if ( !strcmp("cmos", arg) ) > > wallclock_source = WALLCLOCK_CMOS; > > + else if ( !strcmp("no-cmos-probe", arg) ) > > + cmos_rtc_probe = false; > > else if ( !strcmp("efi", arg) ) > > { > > if ( !efi_enabled(EFI_RS) ) > > And to request a particular wallclock _and_ control the probing one then > needs two wallclock= on the command line? And - because of the forcing to > true of cmos_rtc_probe - even in a particular order. Not very nice from a > usability pov. If you request a specific wallclock then there's no probing, so nothing to control. I agree the interface is not great, but I couldn't come up with anything better. I'm kind of fine with not introducing an extra option to wallclock= to control the CMOS RTC probing, but would you agree to switching cmos-rtc-probe to true by default? Thanks, Roger.
On 04.09.2024 14:45, Roger Pau Monné wrote: > On Tue, Sep 03, 2024 at 05:48:09PM +0200, Jan Beulich wrote: >> On 03.09.2024 15:03, Roger Pau Monne wrote: >>> Probing for the CMOS RTC registers consist in reading IO ports, and we expect >>> those reads to have no side effects even when the CMOS RTC is not present. >> >> But what do we gain from this besides possible being slower to boot? > > The intent is that Xen can successfully boot on more systems without > having to pass specific command line options. At the risk of breaking on others, in perhaps subtle ways? >>> --- a/docs/misc/xen-command-line.pandoc >>> +++ b/docs/misc/xen-command-line.pandoc >>> @@ -326,11 +326,14 @@ Interrupts. Specifying zero disables CMCI handling. >>> ### cmos-rtc-probe (x86) >>> > `= <boolean>` >>> >>> -> Default: `false` >>> +> Default: `true` >>> >>> Flag to indicate whether to probe for a CMOS Real Time Clock irrespective of >>> ACPI indicating none to be there. >>> >>> +**WARNING: The `cmos-rtc-probe` option is deprecated and superseded by >>> +_wallclock=no-cmos-probe_ using both options in combination is undefined.** >> >> Hmm, but then ... >> >>> @@ -2822,7 +2825,7 @@ 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` >>> +> `= auto | xen | cmos | no-cmos-probe | efi` >> >> ... this wants to be a boolean sub-option "cmos-probe", such that the flag >> can still be set both ways (in particular for a later command line option >> to override an earlier one). > > What's the point in overriding? Either the users selects a specific > wallclock to use, or it's left for Xen to decide which wallclock to > pick, either with (auto) or without (no-cmos-probe) possibly probing > the CMOS RTC. Overriding can be quite relevant, in particular with xen.efi. There you put command line options in a config file. You may not want to edit that config file every time you try something (you may not even have an editor, first and foremost when there's no EFI shell offered by firmware), and xen.efi intentionally also parses options from its command line (after the first -- separator). Those "manually" supplied options want to be able to override whatever is in the config file. > Multiple different wallclock options being passed on the command line > will result in just the last one taking effect. That's the goal, yes. >>> @@ -1560,6 +1560,8 @@ static int __init cf_check parse_wallclock(const char *arg) >>> if ( !arg ) >>> return -EINVAL; >>> >>> + cmos_rtc_probe = true; >>> + >>> if ( !strcmp("auto", arg) ) >>> wallclock_source = WALLCLOCK_UNSET; >>> else if ( !strcmp("xen", arg) ) >>> @@ -1571,6 +1573,8 @@ static int __init cf_check parse_wallclock(const char *arg) >>> } >>> else if ( !strcmp("cmos", arg) ) >>> wallclock_source = WALLCLOCK_CMOS; >>> + else if ( !strcmp("no-cmos-probe", arg) ) >>> + cmos_rtc_probe = false; >>> else if ( !strcmp("efi", arg) ) >>> { >>> if ( !efi_enabled(EFI_RS) ) >> >> And to request a particular wallclock _and_ control the probing one then >> needs two wallclock= on the command line? And - because of the forcing to >> true of cmos_rtc_probe - even in a particular order. Not very nice from a >> usability pov. > > If you request a specific wallclock then there's no probing, so > nothing to control. I agree the interface is not great, but I > couldn't come up with anything better. > > I'm kind of fine with not introducing an extra option to wallclock= to > control the CMOS RTC probing, but would you agree to switching > cmos-rtc-probe to true by default? I remain to be convinced of this being a good idea. Jan
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 23de922b9705..0d603b9521ae 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -326,11 +326,14 @@ Interrupts. Specifying zero disables CMCI handling. ### cmos-rtc-probe (x86) > `= <boolean>` -> Default: `false` +> Default: `true` Flag to indicate whether to probe for a CMOS Real Time Clock irrespective of ACPI indicating none to be there. +**WARNING: The `cmos-rtc-probe` option is deprecated and superseded by +_wallclock=no-cmos-probe_ using both options in combination is undefined.** + ### com1 (x86) ### com2 (x86) > `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>|msi][,[<port-bdf>][,[<bridge-bdf>]]]]]]` @@ -2822,7 +2825,7 @@ 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` +> `= auto | xen | cmos | no-cmos-probe | efi` > Default: `auto` @@ -2836,6 +2839,11 @@ Allow forcing the usage of a specific wallclock source. * `cmos` force usage of the CMOS RTC wallclock. + * `no-cmos-probe` do not probe for the CMOS RTC presence if the ACPI FADT + table signals there's no CMOS RTC. Implies using the same heuristics as + the `auto` option. By default Xen will probe for the CMOS RTC presence + even when ACPI FADT signals no CMOS RTC available. + * `efi` force usage of the EFI_GFET_TIME run-time method when booted from EFI firmware. diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index 6e19c666d13f..8e6ecbe5e964 100644 --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -1292,7 +1292,7 @@ static bool __get_cmos_time(struct rtc_time *rtc) return t1 <= SECONDS(1) && t2 < MILLISECS(3); } -static bool __initdata cmos_rtc_probe; +static bool __initdata cmos_rtc_probe = true; boolean_param("cmos-rtc-probe", cmos_rtc_probe); static bool __init cmos_probe(void) @@ -1560,6 +1560,8 @@ static int __init cf_check parse_wallclock(const char *arg) if ( !arg ) return -EINVAL; + cmos_rtc_probe = true; + if ( !strcmp("auto", arg) ) wallclock_source = WALLCLOCK_UNSET; else if ( !strcmp("xen", arg) ) @@ -1571,6 +1573,8 @@ static int __init cf_check parse_wallclock(const char *arg) } else if ( !strcmp("cmos", arg) ) wallclock_source = WALLCLOCK_CMOS; + else if ( !strcmp("no-cmos-probe", arg) ) + cmos_rtc_probe = false; else if ( !strcmp("efi", arg) ) { if ( !efi_enabled(EFI_RS) ) @@ -1609,8 +1613,7 @@ static void __init probe_wallclock(void) !cmos_rtc_probe && !efi_enabled(EFI_RS) ? " None" : "", cmos_rtc_probe ? " CMOS" : "", efi_enabled(EFI_RS) ? " EFI" : "", - !cmos_rtc_probe ? "Try with command line option \"cmos-rtc-probe\"\n" - : !efi_enabled(EFI_RS) ? "System must be booted from EFI\n" : ""); + !efi_enabled(EFI_RS) ? "System must be booted from EFI\n" : ""); } static unsigned long get_wallclock_time(void)
Probing for the CMOS RTC registers consist in reading IO ports, and we expect those reads to have no side effects even when the CMOS RTC is not present. Xen already does a similar probing (reading of IO ports) by default when searching for possible CMOS aliased locations. Switch the default to probe for the CMOS RTC by default when ACPI FADT contains the ACPI_FADT_NO_CMOS_RTC flag. At the same time introduce a new option that can be used to turn off the probing: `wallclock=no-cmos-probe`. Deprecate the previous `cmos-rtc-probe` option. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v2: - New in this version. --- docs/misc/xen-command-line.pandoc | 12 ++++++++++-- xen/arch/x86/time.c | 9 ++++++--- 2 files changed, 16 insertions(+), 5 deletions(-)