* [PATCH v2 1/3] x86/hpet: Factor hpet_enable_legacy_replacement_mode() out of hpet_setup()
2021-03-26 18:59 [PATCH for-4.15 v2 0/3] x86/hpet: Try to unbreak Ryzen 1800X systems Andrew Cooper
@ 2021-03-26 18:59 ` Andrew Cooper
2021-03-26 18:59 ` [PATCH v2 2/3] x86/hpet: Don't enable legacy replacement mode unconditionally Andrew Cooper
2021-03-26 18:59 ` [PATCH v2 3/3] x86/hpet: Restore old configuration if Legacy Replacement mode doesn't help Andrew Cooper
2 siblings, 0 replies; 7+ messages in thread
From: Andrew Cooper @ 2021-03-26 18:59 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Wei Liu, Ian Jackson, Marek Marczykowski-Górecki,
Frédéric Pierret
... in preparation to introduce a second caller.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Ian Jackson <iwj@xenproject.org>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
CC: Frédéric Pierret <frederic.pierret@qubes-os.org>
For 4.15. Pre-req for trying to unbreak AMD Ryzen 1800X systems.
v2:
* s/u64/uint64_t/
* Drop id local variable
---
xen/arch/x86/hpet.c | 116 ++++++++++++++++++++++++---------------------
xen/include/asm-x86/hpet.h | 6 +++
2 files changed, 68 insertions(+), 54 deletions(-)
diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 1ff005fb4a..c1d04f184f 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -754,11 +754,70 @@ int hpet_legacy_irq_tick(void)
}
static u32 *hpet_boot_cfg;
+static uint64_t __initdata hpet_rate;
+
+bool __init hpet_enable_legacy_replacement_mode(void)
+{
+ unsigned int cfg, c0_cfg, ticks, count;
+
+ if ( !hpet_rate ||
+ !(hpet_read32(HPET_ID) & HPET_ID_LEGSUP) ||
+ ((cfg = hpet_read32(HPET_CFG)) & HPET_CFG_LEGACY) )
+ return false;
+
+ /* Stop the main counter. */
+ hpet_write32(cfg & ~HPET_CFG_ENABLE, HPET_CFG);
+
+ /* Reconfigure channel 0 to be 32bit periodic. */
+ c0_cfg = hpet_read32(HPET_Tn_CFG(0));
+ c0_cfg |= (HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
+ HPET_TN_32BIT);
+ hpet_write32(c0_cfg, HPET_Tn_CFG(0));
+
+ /*
+ * The exact period doesn't have to match a legacy PIT. All we need
+ * is an interrupt queued up via the IO-APIC to check routing.
+ *
+ * Use HZ as the frequency.
+ */
+ ticks = ((SECONDS(1) / HZ) * div_sc(hpet_rate, SECONDS(1), 32)) >> 32;
+
+ count = hpet_read32(HPET_COUNTER);
+
+ /*
+ * HPET_TN_SETVAL above is atrociously documented in the spec.
+ *
+ * Periodic HPET channels have a main comparator register, and
+ * separate "accumulator" register. Despite being named accumulator
+ * in the spec, this is not an accurate description of its behaviour
+ * or purpose.
+ *
+ * Each time an interrupt is generated, the "accumulator" register is
+ * re-added to the comparator set up the new period.
+ *
+ * Normally, writes to the CMP register update both registers.
+ * However, under these semantics, it is impossible to set up a
+ * periodic timer correctly without the main HPET counter being at 0.
+ *
+ * Instead, HPET_TN_SETVAL is a self-clearing control bit which we can
+ * use for periodic timers to mean that the second write to CMP
+ * updates the accumulator only, and not the absolute comparator
+ * value.
+ *
+ * This lets us set a period when the main counter isn't at 0.
+ */
+ hpet_write32(count + ticks, HPET_Tn_CMP(0));
+ hpet_write32(ticks, HPET_Tn_CMP(0));
+
+ /* Restart the main counter, and legacy mode. */
+ hpet_write32(cfg | HPET_CFG_ENABLE | HPET_CFG_LEGACY, HPET_CFG);
+
+ return true;
+}
u64 __init hpet_setup(void)
{
- static u64 __initdata hpet_rate;
- unsigned int hpet_id, hpet_period, hpet_cfg;
+ unsigned int hpet_id, hpet_period;
unsigned int last, rem;
if ( hpet_rate )
@@ -805,58 +864,7 @@ u64 __init hpet_setup(void)
* Reconfigure the HPET into legacy mode to re-establish the timer
* interrupt.
*/
- if ( hpet_id & HPET_ID_LEGSUP &&
- !((hpet_cfg = hpet_read32(HPET_CFG)) & HPET_CFG_LEGACY) )
- {
- unsigned int c0_cfg, ticks, count;
-
- /* Stop the main counter. */
- hpet_write32(hpet_cfg & ~HPET_CFG_ENABLE, HPET_CFG);
-
- /* Reconfigure channel 0 to be 32bit periodic. */
- c0_cfg = hpet_read32(HPET_Tn_CFG(0));
- c0_cfg |= (HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
- HPET_TN_32BIT);
- hpet_write32(c0_cfg, HPET_Tn_CFG(0));
-
- /*
- * The exact period doesn't have to match a legacy PIT. All we need
- * is an interrupt queued up via the IO-APIC to check routing.
- *
- * Use HZ as the frequency.
- */
- ticks = ((SECONDS(1) / HZ) * div_sc(hpet_rate, SECONDS(1), 32)) >> 32;
-
- count = hpet_read32(HPET_COUNTER);
-
- /*
- * HPET_TN_SETVAL above is atrociously documented in the spec.
- *
- * Periodic HPET channels have a main comparator register, and
- * separate "accumulator" register. Despite being named accumulator
- * in the spec, this is not an accurate description of its behaviour
- * or purpose.
- *
- * Each time an interrupt is generated, the "accumulator" register is
- * re-added to the comparator set up the new period.
- *
- * Normally, writes to the CMP register update both registers.
- * However, under these semantics, it is impossible to set up a
- * periodic timer correctly without the main HPET counter being at 0.
- *
- * Instead, HPET_TN_SETVAL is a self-clearing control bit which we can
- * use for periodic timers to mean that the second write to CMP
- * updates the accumulator only, and not the absolute comparator
- * value.
- *
- * This lets us set a period when the main counter isn't at 0.
- */
- hpet_write32(count + ticks, HPET_Tn_CMP(0));
- hpet_write32(ticks, HPET_Tn_CMP(0));
-
- /* Restart the main counter, and legacy mode. */
- hpet_write32(hpet_cfg | HPET_CFG_ENABLE | HPET_CFG_LEGACY, HPET_CFG);
- }
+ hpet_enable_legacy_replacement_mode();
return hpet_rate;
}
diff --git a/xen/include/asm-x86/hpet.h b/xen/include/asm-x86/hpet.h
index fb6bf05065..50176de3d2 100644
--- a/xen/include/asm-x86/hpet.h
+++ b/xen/include/asm-x86/hpet.h
@@ -73,6 +73,12 @@ void hpet_disable(void);
int hpet_legacy_irq_tick(void);
/*
+ * Try to enable HPET Legacy Replacement mode. Returns a boolean indicating
+ * whether the HPET configuration was changed.
+ */
+bool hpet_enable_legacy_replacement_mode(void);
+
+/*
* Temporarily use an HPET event counter for timer interrupt handling,
* rather than using the LAPIC timer. Used for Cx state entry.
*/
--
2.11.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 2/3] x86/hpet: Don't enable legacy replacement mode unconditionally
2021-03-26 18:59 [PATCH for-4.15 v2 0/3] x86/hpet: Try to unbreak Ryzen 1800X systems Andrew Cooper
2021-03-26 18:59 ` [PATCH v2 1/3] x86/hpet: Factor hpet_enable_legacy_replacement_mode() out of hpet_setup() Andrew Cooper
@ 2021-03-26 18:59 ` Andrew Cooper
2021-03-29 11:40 ` Roger Pau Monné
2021-03-26 18:59 ` [PATCH v2 3/3] x86/hpet: Restore old configuration if Legacy Replacement mode doesn't help Andrew Cooper
2 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2021-03-26 18:59 UTC (permalink / raw)
To: Xen-devel
Cc: Jan Beulich, Andrew Cooper, Jan Beulich, Roger Pau Monné,
Wei Liu, Ian Jackson, Marek Marczykowski-Górecki,
Frédéric Pierret
From: Jan Beulich <jbeulich@suse.com>
Commit e1de4c196a2e ("x86/timer: Fix boot on Intel systems using ITSSPRC
static PIT clock gating") was reported to cause boot failures on certain
AMD Ryzen systems.
Refine the fix to do nothing in the default case, and only attempt to
configure legacy replacement mode if IRQ0 is found to not be working.
In addition, introduce a "hpet" command line option so this heuristic
can be overridden. Since it makes little sense to introduce just
"hpet=legacy-replacement", also allow for a boolean argument as well as
"broadcast" to replace the separate "hpetbroadcast" option.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Ian Jackson <iwj@xenproject.org>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
CC: Frédéric Pierret <frederic.pierret@qubes-os.org>
v2:
* Reinstate missing hunk from Jan's original patch.
* Fix up "8254 PIT".
* Drop "goto retry".
For 4.15: Attempt to unbreak AMD Ryzen 1800X systems.
---
docs/misc/xen-command-line.pandoc | 33 +++++++++++++++++++++++++++
xen/arch/x86/hpet.c | 48 +++++++++++++++++++++++++--------------
xen/arch/x86/io_apic.c | 27 ++++++++++++++++++++++
xen/include/asm-x86/hpet.h | 1 +
4 files changed, 92 insertions(+), 17 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index a0601ff838..a4bd3f12c5 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1274,9 +1274,42 @@ supported. See docs/misc/arm/big.LITTLE.txt for more information.
When the hmp-unsafe option is disabled (default), CPUs that are not
identical to the boot CPU will be parked and not used by Xen.
+### hpet
+ = List of [ <bool> | broadcast=<bool> | legacy-replacement=<bool> ]
+
+ Applicability: x86
+
+Controls Xen's use of the system's High Precision Event Timer. By default,
+Xen will use an HPET when available and not subject to errata. Use of the
+HPET can be disabled by specifying `hpet=0`.
+
+ * The `broadcast` boolean is disabled by default, but forces Xen to keep
+ using the broadcast for CPUs in deep C-states even when an RTC interrupt is
+ enabled. This then also affects raising of the RTC interrupt.
+
+ * The `legacy-replacement` boolean allows for control over whether Legacy
+ Replacement mode is enabled.
+
+ Legacy Replacement mode is intended for hardware which does not have an
+ 8254 PIT, and allows the HPET to be configured into a compatible mode.
+ Intel chipsets from Skylake/ApolloLake onwards can turn the PIT off for
+ power saving reasons, and there is no platform-agnostic mechanism for
+ discovering this.
+
+ By default, Xen will not change hardware configuration, unless the PIT
+ appears to be absent, at which point Xen will try to enable Legacy
+ Replacement mode before falling back to pre-IO-APIC interrupt routing
+ options.
+
+ This behaviour can be inhibited by specifying `legacy-replacement=0`.
+ Alternatively, this mode can be enabled unconditionally (if available) by
+ specifying `legacy-replacement=1`.
+
### hpetbroadcast (x86)
> `= <boolean>`
+Deprecated alternative of `hpet=broadcast`.
+
### hvm_debug (x86)
> `= <integer>`
diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index c1d04f184f..bfa75f135a 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -52,6 +52,8 @@ static unsigned int __read_mostly num_hpets_used;
DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
unsigned long __initdata hpet_address;
+int8_t __initdata opt_hpet_legacy_replacement = -1;
+static bool __initdata opt_hpet = true;
u8 __initdata hpet_blockid;
u8 __initdata hpet_flags;
@@ -63,6 +65,32 @@ u8 __initdata hpet_flags;
static bool __initdata force_hpet_broadcast;
boolean_param("hpetbroadcast", force_hpet_broadcast);
+static int __init parse_hpet_param(const char *s)
+{
+ const char *ss;
+ int val, rc = 0;
+
+ do {
+ ss = strchr(s, ',');
+ if ( !ss )
+ ss = strchr(s, '\0');
+
+ if ( (val = parse_bool(s, ss)) >= 0 )
+ opt_hpet = val;
+ else if ( (val = parse_boolean("broadcast", s, ss)) >= 0 )
+ force_hpet_broadcast = val;
+ else if ( (val = parse_boolean("legacy-replacement", s, ss)) >= 0 )
+ opt_hpet_legacy_replacement = val;
+ else
+ rc = -EINVAL;
+
+ s = ss + 1;
+ } while ( *ss );
+
+ return rc;
+}
+custom_param("hpet", parse_hpet_param);
+
/*
* Calculate a multiplication factor for scaled math, which is used to convert
* nanoseconds based values to clock ticks:
@@ -820,12 +848,9 @@ u64 __init hpet_setup(void)
unsigned int hpet_id, hpet_period;
unsigned int last, rem;
- if ( hpet_rate )
+ if ( hpet_rate || !hpet_address || !opt_hpet )
return hpet_rate;
- if ( hpet_address == 0 )
- return 0;
-
set_fixmap_nocache(FIX_HPET_BASE, hpet_address);
hpet_id = hpet_read32(HPET_ID);
@@ -852,19 +877,8 @@ u64 __init hpet_setup(void)
if ( (rem * 2) > hpet_period )
hpet_rate++;
- /*
- * Intel chipsets from Skylake/ApolloLake onwards can statically clock
- * gate the 8259 PIT. This option is enabled by default in slightly later
- * systems, as turning the PIT off is a prerequisite to entering the C11
- * power saving state.
- *
- * Xen currently depends on the legacy timer interrupt being active while
- * IRQ routing is configured.
- *
- * Reconfigure the HPET into legacy mode to re-establish the timer
- * interrupt.
- */
- hpet_enable_legacy_replacement_mode();
+ if ( opt_hpet_legacy_replacement > 0 )
+ hpet_enable_legacy_replacement_mode();
return hpet_rate;
}
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index e93265f379..3f131a81fb 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -29,6 +29,8 @@
#include <xen/acpi.h>
#include <xen/keyhandler.h>
#include <xen/softirq.h>
+
+#include <asm/hpet.h>
#include <asm/mc146818rtc.h>
#include <asm/smp.h>
#include <asm/desc.h>
@@ -1930,6 +1932,31 @@ static void __init check_timer(void)
local_irq_restore(flags);
return;
}
+
+ /*
+ * Intel chipsets from Skylake/ApolloLake onwards can statically clock
+ * gate the 8254 PIT. This option is enabled by default in slightly
+ * later systems, as turning the PIT off is a prerequisite to entering
+ * the C11 power saving state.
+ *
+ * Xen currently depends on the legacy timer interrupt being active
+ * while IRQ routing is configured.
+ *
+ * If the user hasn't made an explicit choice, attempt to reconfigure
+ * the HPET into legacy mode to re-establish the timer interrupt.
+ */
+ if ( opt_hpet_legacy_replacement < 0 &&
+ hpet_enable_legacy_replacement_mode() )
+ {
+ printk(XENLOG_ERR "..no 8254 timer found - trying HPET Legacy Replacement Mode\n");
+
+ if ( timer_irq_works() )
+ {
+ local_irq_restore(flags);
+ return;
+ }
+ }
+
clear_IO_APIC_pin(apic1, pin1);
printk(KERN_ERR "..MP-BIOS bug: 8254 timer not connected to "
"IO-APIC\n");
diff --git a/xen/include/asm-x86/hpet.h b/xen/include/asm-x86/hpet.h
index 50176de3d2..07bc8d6079 100644
--- a/xen/include/asm-x86/hpet.h
+++ b/xen/include/asm-x86/hpet.h
@@ -53,6 +53,7 @@
extern unsigned long hpet_address;
extern u8 hpet_blockid;
extern u8 hpet_flags;
+extern int8_t opt_hpet_legacy_replacement;
/*
* Detect and initialise HPET hardware: return counter update frequency.
--
2.11.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 2/3] x86/hpet: Don't enable legacy replacement mode unconditionally
2021-03-26 18:59 ` [PATCH v2 2/3] x86/hpet: Don't enable legacy replacement mode unconditionally Andrew Cooper
@ 2021-03-29 11:40 ` Roger Pau Monné
0 siblings, 0 replies; 7+ messages in thread
From: Roger Pau Monné @ 2021-03-29 11:40 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Wei Liu, Ian Jackson,
Marek Marczykowski-Górecki, Frédéric Pierret
On Fri, Mar 26, 2021 at 06:59:46PM +0000, Andrew Cooper wrote:
> From: Jan Beulich <jbeulich@suse.com>
>
> Commit e1de4c196a2e ("x86/timer: Fix boot on Intel systems using ITSSPRC
> static PIT clock gating") was reported to cause boot failures on certain
> AMD Ryzen systems.
>
> Refine the fix to do nothing in the default case, and only attempt to
> configure legacy replacement mode if IRQ0 is found to not be working.
>
> In addition, introduce a "hpet" command line option so this heuristic
> can be overridden. Since it makes little sense to introduce just
> "hpet=legacy-replacement", also allow for a boolean argument as well as
> "broadcast" to replace the separate "hpetbroadcast" option.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Ian Jackson <iwj@xenproject.org>
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> CC: Frédéric Pierret <frederic.pierret@qubes-os.org>
>
> v2:
> * Reinstate missing hunk from Jan's original patch.
> * Fix up "8254 PIT".
> * Drop "goto retry".
>
> For 4.15: Attempt to unbreak AMD Ryzen 1800X systems.
> ---
> docs/misc/xen-command-line.pandoc | 33 +++++++++++++++++++++++++++
> xen/arch/x86/hpet.c | 48 +++++++++++++++++++++++++--------------
> xen/arch/x86/io_apic.c | 27 ++++++++++++++++++++++
> xen/include/asm-x86/hpet.h | 1 +
> 4 files changed, 92 insertions(+), 17 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index a0601ff838..a4bd3f12c5 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1274,9 +1274,42 @@ supported. See docs/misc/arm/big.LITTLE.txt for more information.
> When the hmp-unsafe option is disabled (default), CPUs that are not
> identical to the boot CPU will be parked and not used by Xen.
>
> +### hpet
> + = List of [ <bool> | broadcast=<bool> | legacy-replacement=<bool> ]
> +
> + Applicability: x86
> +
> +Controls Xen's use of the system's High Precision Event Timer. By default,
> +Xen will use an HPET when available and not subject to errata. Use of the
> +HPET can be disabled by specifying `hpet=0`.
> +
> + * The `broadcast` boolean is disabled by default, but forces Xen to keep
> + using the broadcast for CPUs in deep C-states even when an RTC interrupt is
> + enabled. This then also affects raising of the RTC interrupt.
> +
> + * The `legacy-replacement` boolean allows for control over whether Legacy
> + Replacement mode is enabled.
> +
> + Legacy Replacement mode is intended for hardware which does not have an
> + 8254 PIT, and allows the HPET to be configured into a compatible mode.
> + Intel chipsets from Skylake/ApolloLake onwards can turn the PIT off for
> + power saving reasons, and there is no platform-agnostic mechanism for
> + discovering this.
> +
> + By default, Xen will not change hardware configuration, unless the PIT
> + appears to be absent, at which point Xen will try to enable Legacy
> + Replacement mode before falling back to pre-IO-APIC interrupt routing
> + options.
> +
> + This behaviour can be inhibited by specifying `legacy-replacement=0`.
> + Alternatively, this mode can be enabled unconditionally (if available) by
> + specifying `legacy-replacement=1`.
> +
> ### hpetbroadcast (x86)
> > `= <boolean>`
>
> +Deprecated alternative of `hpet=broadcast`.
> +
> ### hvm_debug (x86)
> > `= <integer>`
>
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index c1d04f184f..bfa75f135a 100644
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -52,6 +52,8 @@ static unsigned int __read_mostly num_hpets_used;
> DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
>
> unsigned long __initdata hpet_address;
> +int8_t __initdata opt_hpet_legacy_replacement = -1;
> +static bool __initdata opt_hpet = true;
> u8 __initdata hpet_blockid;
> u8 __initdata hpet_flags;
>
> @@ -63,6 +65,32 @@ u8 __initdata hpet_flags;
> static bool __initdata force_hpet_broadcast;
> boolean_param("hpetbroadcast", force_hpet_broadcast);
>
> +static int __init parse_hpet_param(const char *s)
> +{
> + const char *ss;
> + int val, rc = 0;
> +
> + do {
> + ss = strchr(s, ',');
> + if ( !ss )
> + ss = strchr(s, '\0');
> +
> + if ( (val = parse_bool(s, ss)) >= 0 )
> + opt_hpet = val;
> + else if ( (val = parse_boolean("broadcast", s, ss)) >= 0 )
> + force_hpet_broadcast = val;
> + else if ( (val = parse_boolean("legacy-replacement", s, ss)) >= 0 )
> + opt_hpet_legacy_replacement = val;
> + else
> + rc = -EINVAL;
> +
> + s = ss + 1;
> + } while ( *ss );
> +
> + return rc;
> +}
> +custom_param("hpet", parse_hpet_param);
> +
> /*
> * Calculate a multiplication factor for scaled math, which is used to convert
> * nanoseconds based values to clock ticks:
> @@ -820,12 +848,9 @@ u64 __init hpet_setup(void)
> unsigned int hpet_id, hpet_period;
> unsigned int last, rem;
>
> - if ( hpet_rate )
> + if ( hpet_rate || !hpet_address || !opt_hpet )
> return hpet_rate;
>
> - if ( hpet_address == 0 )
> - return 0;
> -
> set_fixmap_nocache(FIX_HPET_BASE, hpet_address);
>
> hpet_id = hpet_read32(HPET_ID);
> @@ -852,19 +877,8 @@ u64 __init hpet_setup(void)
> if ( (rem * 2) > hpet_period )
> hpet_rate++;
>
> - /*
> - * Intel chipsets from Skylake/ApolloLake onwards can statically clock
> - * gate the 8259 PIT. This option is enabled by default in slightly later
> - * systems, as turning the PIT off is a prerequisite to entering the C11
> - * power saving state.
> - *
> - * Xen currently depends on the legacy timer interrupt being active while
> - * IRQ routing is configured.
> - *
> - * Reconfigure the HPET into legacy mode to re-establish the timer
> - * interrupt.
> - */
> - hpet_enable_legacy_replacement_mode();
> + if ( opt_hpet_legacy_replacement > 0 )
> + hpet_enable_legacy_replacement_mode();
>
> return hpet_rate;
> }
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index e93265f379..3f131a81fb 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -29,6 +29,8 @@
> #include <xen/acpi.h>
> #include <xen/keyhandler.h>
> #include <xen/softirq.h>
> +
> +#include <asm/hpet.h>
> #include <asm/mc146818rtc.h>
> #include <asm/smp.h>
> #include <asm/desc.h>
> @@ -1930,6 +1932,31 @@ static void __init check_timer(void)
> local_irq_restore(flags);
> return;
> }
> +
> + /*
> + * Intel chipsets from Skylake/ApolloLake onwards can statically clock
> + * gate the 8254 PIT. This option is enabled by default in slightly
> + * later systems, as turning the PIT off is a prerequisite to entering
> + * the C11 power saving state.
> + *
> + * Xen currently depends on the legacy timer interrupt being active
> + * while IRQ routing is configured.
> + *
> + * If the user hasn't made an explicit choice, attempt to reconfigure
> + * the HPET into legacy mode to re-establish the timer interrupt.
> + */
> + if ( opt_hpet_legacy_replacement < 0 &&
> + hpet_enable_legacy_replacement_mode() )
> + {
> + printk(XENLOG_ERR "..no 8254 timer found - trying HPET Legacy Replacement Mode\n");
> +
> + if ( timer_irq_works() )
> + {
> + local_irq_restore(flags);
Is there any point in undoing the legacy replacement here, as I
understand it it's only required for the routing check?
Should we also prevent any attempts from using the PIT as a
timecounter in x86/time.c as a result of having the HPET in legacy
replacement mode?
AFAICT init_pit will already assert whether the PIT counters work, so
maybe there's no need for adding an extra check on whether legacy
replacement is enabled.
Thanks, Roger.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] x86/hpet: Restore old configuration if Legacy Replacement mode doesn't help
2021-03-26 18:59 [PATCH for-4.15 v2 0/3] x86/hpet: Try to unbreak Ryzen 1800X systems Andrew Cooper
2021-03-26 18:59 ` [PATCH v2 1/3] x86/hpet: Factor hpet_enable_legacy_replacement_mode() out of hpet_setup() Andrew Cooper
2021-03-26 18:59 ` [PATCH v2 2/3] x86/hpet: Don't enable legacy replacement mode unconditionally Andrew Cooper
@ 2021-03-26 18:59 ` Andrew Cooper
2021-03-29 9:23 ` Jan Beulich
2021-03-29 12:37 ` Roger Pau Monné
2 siblings, 2 replies; 7+ messages in thread
From: Andrew Cooper @ 2021-03-26 18:59 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Wei Liu, Ian Jackson, Marek Marczykowski-Górecki,
Frédéric Pierret
If Legacy Replacement mode doesn't help in check_timer(), restore the old
configuration before falling back to other workarounds.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Ian Jackson <iwj@xenproject.org>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
CC: Frédéric Pierret <frederic.pierret@qubes-os.org>
v2:
* New.
For 4.15: Attempt to unbreak AMD Ryzen 1800X systems.
I'm tempted to fold this into the previous patch, but its presented here in
isolation for ease of review.
Tested by repositioning the timer_irq_works() calls on a system with a working
PIT.
(XEN) ENABLING IO-APIC IRQs
(XEN) -> Using old ACK method
(XEN) ..TIMER: vector=0xF0 apic1=0 pin1=2 apic2=0 pin2=0
(XEN) ..no 8254 timer found - trying HPET Legacy Replacement Mode
(XEN) ..no HPET timer found - reverting Legacy Replacement Mode
(XEN) TSC deadline timer enabled
---
xen/arch/x86/hpet.c | 27 ++++++++++++++++++++++++++-
xen/arch/x86/io_apic.c | 4 ++++
xen/include/asm-x86/hpet.h | 6 ++++++
3 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index bfa75f135a..afe104dc93 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -783,6 +783,9 @@ int hpet_legacy_irq_tick(void)
static u32 *hpet_boot_cfg;
static uint64_t __initdata hpet_rate;
+static __initdata struct {
+ uint32_t cmp, cfg;
+} pre_legacy_c0;
bool __init hpet_enable_legacy_replacement_mode(void)
{
@@ -796,8 +799,11 @@ bool __init hpet_enable_legacy_replacement_mode(void)
/* Stop the main counter. */
hpet_write32(cfg & ~HPET_CFG_ENABLE, HPET_CFG);
+ /* Stash channel 0's old CFG/CMP incase we need to undo. */
+ pre_legacy_c0.cfg = c0_cfg = hpet_read32(HPET_Tn_CFG(0));
+ pre_legacy_c0.cmp = hpet_read32(HPET_Tn_CMP(0));
+
/* Reconfigure channel 0 to be 32bit periodic. */
- c0_cfg = hpet_read32(HPET_Tn_CFG(0));
c0_cfg |= (HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
HPET_TN_32BIT);
hpet_write32(c0_cfg, HPET_Tn_CFG(0));
@@ -843,6 +849,25 @@ bool __init hpet_enable_legacy_replacement_mode(void)
return true;
}
+void __init hpet_disable_legacy_replacement_mode(void)
+{
+ unsigned int cfg = hpet_read32(HPET_CFG);
+
+ ASSERT(hpet_rate);
+
+ cfg &= ~(HPET_CFG_LEGACY | HPET_CFG_ENABLE);
+
+ /* Stop the main counter and disable legacy mode. */
+ hpet_write32(cfg, HPET_CFG);
+
+ /* Restore pre-Legacy Replacement Mode settings. */
+ hpet_write32(pre_legacy_c0.cfg, HPET_Tn_CFG(0));
+ hpet_write32(pre_legacy_c0.cmp, HPET_Tn_CMP(0));
+
+ /* Restart the main counter. */
+ hpet_write32(cfg | HPET_CFG_ENABLE, HPET_CFG);
+}
+
u64 __init hpet_setup(void)
{
unsigned int hpet_id, hpet_period;
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 3f131a81fb..58b26d962c 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1955,6 +1955,10 @@ static void __init check_timer(void)
local_irq_restore(flags);
return;
}
+
+ /* Legacy Replacement mode hasn't helped. Undo it. */
+ printk(XENLOG_ERR "..no HPET timer found - reverting Legacy Replacement Mode\n");
+ hpet_disable_legacy_replacement_mode();
}
clear_IO_APIC_pin(apic1, pin1);
diff --git a/xen/include/asm-x86/hpet.h b/xen/include/asm-x86/hpet.h
index 07bc8d6079..8f9725a95e 100644
--- a/xen/include/asm-x86/hpet.h
+++ b/xen/include/asm-x86/hpet.h
@@ -80,6 +80,12 @@ int hpet_legacy_irq_tick(void);
bool hpet_enable_legacy_replacement_mode(void);
/*
+ * Undo the effects of hpet_disable_legacy_replacement_mode(). Must not be
+ * called unless enable() returned true.
+ */
+void hpet_disable_legacy_replacement_mode(void);
+
+/*
* Temporarily use an HPET event counter for timer interrupt handling,
* rather than using the LAPIC timer. Used for Cx state entry.
*/
--
2.11.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 3/3] x86/hpet: Restore old configuration if Legacy Replacement mode doesn't help
2021-03-26 18:59 ` [PATCH v2 3/3] x86/hpet: Restore old configuration if Legacy Replacement mode doesn't help Andrew Cooper
@ 2021-03-29 9:23 ` Jan Beulich
2021-03-29 12:37 ` Roger Pau Monné
1 sibling, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2021-03-29 9:23 UTC (permalink / raw)
To: Andrew Cooper
Cc: Roger Pau Monné,
Wei Liu, Ian Jackson, Marek Marczykowski-Górecki,
Frédéric Pierret, Xen-devel
On 26.03.2021 19:59, Andrew Cooper wrote:
> If Legacy Replacement mode doesn't help in check_timer(), restore the old
> configuration before falling back to other workarounds.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Ian Jackson <iwj@xenproject.org>
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> CC: Frédéric Pierret <frederic.pierret@qubes-os.org>
>
> v2:
> * New.
>
> For 4.15: Attempt to unbreak AMD Ryzen 1800X systems.
>
> I'm tempted to fold this into the previous patch, but its presented here in
> isolation for ease of review.
Both combined
Reviewed-by: Jan Beulich <jbeulich@suse.com>
(i.e. I think they strictly should be folded).
Just as a (further) consideration: While I continue to think that
undoing immediately is what we want, I wonder whether every further
fallback attempt wouldn't want to also be accompanied by trying if
it _combined_ with legacy replacement mode actually helps if alone
it didn't. Perhaps we want to cross that bridge only if we get a
respective report ...
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/3] x86/hpet: Restore old configuration if Legacy Replacement mode doesn't help
2021-03-26 18:59 ` [PATCH v2 3/3] x86/hpet: Restore old configuration if Legacy Replacement mode doesn't help Andrew Cooper
2021-03-29 9:23 ` Jan Beulich
@ 2021-03-29 12:37 ` Roger Pau Monné
1 sibling, 0 replies; 7+ messages in thread
From: Roger Pau Monné @ 2021-03-29 12:37 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Wei Liu, Ian Jackson,
Marek Marczykowski-Górecki, Frédéric Pierret
On Fri, Mar 26, 2021 at 06:59:47PM +0000, Andrew Cooper wrote:
> If Legacy Replacement mode doesn't help in check_timer(), restore the old
> configuration before falling back to other workarounds.
Oh, I guess this answers my question from the previous patch.
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Ian Jackson <iwj@xenproject.org>
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> CC: Frédéric Pierret <frederic.pierret@qubes-os.org>
>
> v2:
> * New.
>
> For 4.15: Attempt to unbreak AMD Ryzen 1800X systems.
Is this really the fix for AMD 1800X? I assumed not setting the HPET
into legacy replacement mode unconditionally was the fix for those
systems?
>
> I'm tempted to fold this into the previous patch, but its presented here in
> isolation for ease of review.
>
> Tested by repositioning the timer_irq_works() calls on a system with a working
> PIT.
>
> (XEN) ENABLING IO-APIC IRQs
> (XEN) -> Using old ACK method
> (XEN) ..TIMER: vector=0xF0 apic1=0 pin1=2 apic2=0 pin2=0
> (XEN) ..no 8254 timer found - trying HPET Legacy Replacement Mode
> (XEN) ..no HPET timer found - reverting Legacy Replacement Mode
> (XEN) TSC deadline timer enabled
> ---
> xen/arch/x86/hpet.c | 27 ++++++++++++++++++++++++++-
> xen/arch/x86/io_apic.c | 4 ++++
> xen/include/asm-x86/hpet.h | 6 ++++++
> 3 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index bfa75f135a..afe104dc93 100644
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -783,6 +783,9 @@ int hpet_legacy_irq_tick(void)
>
> static u32 *hpet_boot_cfg;
> static uint64_t __initdata hpet_rate;
> +static __initdata struct {
> + uint32_t cmp, cfg;
> +} pre_legacy_c0;
>
> bool __init hpet_enable_legacy_replacement_mode(void)
> {
> @@ -796,8 +799,11 @@ bool __init hpet_enable_legacy_replacement_mode(void)
> /* Stop the main counter. */
> hpet_write32(cfg & ~HPET_CFG_ENABLE, HPET_CFG);
>
> + /* Stash channel 0's old CFG/CMP incase we need to undo. */
> + pre_legacy_c0.cfg = c0_cfg = hpet_read32(HPET_Tn_CFG(0));
> + pre_legacy_c0.cmp = hpet_read32(HPET_Tn_CMP(0));
> +
> /* Reconfigure channel 0 to be 32bit periodic. */
> - c0_cfg = hpet_read32(HPET_Tn_CFG(0));
> c0_cfg |= (HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
> HPET_TN_32BIT);
> hpet_write32(c0_cfg, HPET_Tn_CFG(0));
> @@ -843,6 +849,25 @@ bool __init hpet_enable_legacy_replacement_mode(void)
> return true;
> }
>
> +void __init hpet_disable_legacy_replacement_mode(void)
> +{
> + unsigned int cfg = hpet_read32(HPET_CFG);
> +
> + ASSERT(hpet_rate);
> +
> + cfg &= ~(HPET_CFG_LEGACY | HPET_CFG_ENABLE);
> +
> + /* Stop the main counter and disable legacy mode. */
> + hpet_write32(cfg, HPET_CFG);
> +
> + /* Restore pre-Legacy Replacement Mode settings. */
> + hpet_write32(pre_legacy_c0.cfg, HPET_Tn_CFG(0));
> + hpet_write32(pre_legacy_c0.cmp, HPET_Tn_CMP(0));
> +
> + /* Restart the main counter. */
> + hpet_write32(cfg | HPET_CFG_ENABLE, HPET_CFG);
> +}
> +
> u64 __init hpet_setup(void)
> {
> unsigned int hpet_id, hpet_period;
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index 3f131a81fb..58b26d962c 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1955,6 +1955,10 @@ static void __init check_timer(void)
> local_irq_restore(flags);
> return;
> }
> +
> + /* Legacy Replacement mode hasn't helped. Undo it. */
> + printk(XENLOG_ERR "..no HPET timer found - reverting Legacy Replacement Mode\n");
> + hpet_disable_legacy_replacement_mode();
I think you could also get away just calling hpet_disable and
hpet_resume? (bypassing the system_reset_counter check)
Thanks, Roger.
^ permalink raw reply [flat|nested] 7+ messages in thread