[PATCH 0/2] x86: HPET PC10 quirk workaround and some cleanup

[PATCH 0/2] x86: HPET PC10 quirk workaround and some cleanup

[Jan Beulich]

The workaround is a port of a recent Linux change, replacing earlier,
never accepted attempts to follow Linux in their PCI ID based working
around of the issue. The other patch is long due cleanup that I
happened to notice along the way.

1: x86/hpet: Use another crystalball to evaluate HPET usability
2: x86: de-duplicate MONITOR/MWAIT CPUID-related definitions

Jan

[PATCH 1/2] x86/hpet: Use another crystalball to evaluate HPET usability

[Jan Beulich]

From: Thomas Gleixner <tglx@linutronix.de>

On recent Intel systems the HPET stops working when the system reaches PC10
idle state.

The approach of adding PCI ids to the early quirks to disable HPET on
these systems is a whack a mole game which makes no sense.

Check for PC10 instead and force disable HPET if supported. The check is
overbroad as it does not take ACPI, mwait-idle enablement and command
line parameters into account. That's fine as long as there is at least
PMTIMER available to calibrate the TSC frequency. The decision can be
overruled by adding "clocksource=hpet" on the Xen command line.

Remove the related PCI quirks for affected Coffee Lake systems as they
are not longer required. That should also cover all other systems, i.e.
Ice Lake, Tiger Lake, and newer generations, which are most likely
affected by this as well.

Fixes: Yet another hardware trainwreck
Reported-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
[Linux commit: 6e3cd95234dc1eda488f4f487c281bac8fef4d9b]

I have to admit that the purpose of checking CPUID5_ECX_INTERRUPT_BREAK
is unclear to me, but I didn't want to diverge in technical aspects from
the Linux commit.

In mwait_pc10_supported(), besides some cosmetic adjustments, avoid UB
from shifting left a signed 4-bit constant by 28 bits.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Fully different replacement of "x86: avoid HPET use also on certain
    Coffee Lake H".

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -34,6 +34,7 @@
 #include <asm/fixmap.h>
 #include <asm/guest.h>
 #include <asm/mc146818rtc.h>
+#include <asm/mwait.h>
 #include <asm/div64.h>
 #include <asm/acpi.h>
 #include <asm/hpet.h>
@@ -395,14 +396,43 @@ static int64_t __init init_hpet(struct p
             }
 
         /*
-         * Some Coffee Lake platforms have a skewed HPET timer once the SoCs
-         * entered PC10.
+         * Some Coffee Lake and later platforms have a skewed HPET timer once
+         * they entered PC10.
+         *
+         * Check whether the system supports PC10. If so force disable HPET as
+         * that stops counting in PC10. This check is overbroad as it does not
+         * take any of the following into account:
+         *
+         *	- ACPI tables
+         *	- Enablement of mwait-idle
+         *	- Command line arguments which limit mwait-idle C-state support
+         *
+         * That's perfectly fine. HPET is a piece of hardware designed by
+         * committee and the only reasons why it is still in use on modern
+         * systems is the fact that it is impossible to reliably query TSC and
+         * CPU frequency via CPUID or firmware.
+         *
+         * If HPET is functional it is useful for calibrating TSC, but this can
+         * be done via PMTIMER as well which seems to be the last remaining
+         * timer on X86/INTEL platforms that has not been completely wreckaged
+         * by feature creep.
+         *
+         * In theory HPET support should be removed altogether, but there are
+         * older systems out there which depend on it because TSC and APIC timer
+         * are dysfunctional in deeper C-states.
          */
-        if ( pci_conf_read16(PCI_SBDF(0, 0, 0, 0),
-                             PCI_VENDOR_ID) == PCI_VENDOR_ID_INTEL &&
-             pci_conf_read16(PCI_SBDF(0, 0, 0, 0),
-                             PCI_DEVICE_ID) == 0x3ec4 )
-            hpet_address = 0;
+        if ( mwait_pc10_supported() )
+        {
+            uint64_t pcfg;
+
+            rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, pcfg);
+            if ( (pcfg & 0xf) < 8 )
+                /* nothing */;
+            else if ( !strcmp(opt_clocksource, pts->id) )
+                printk("HPET use requested via command line, but dysfunctional in PC10\n");
+            else
+                hpet_address = 0;
+        }
 
         if ( !hpet_address )
             printk("Disabling HPET for being unreliable\n");
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -1308,3 +1308,20 @@ int __init mwait_idle_init(struct notifi
 
 	return err;
 }
+
+/* Helper function for HPET. */
+bool __init mwait_pc10_supported(void)
+{
+	unsigned int ecx, edx, dummy;
+
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
+	    !cpu_has_monitor ||
+	    boot_cpu_data.cpuid_level < CPUID_MWAIT_LEAF)
+		return false;
+
+	cpuid(CPUID_MWAIT_LEAF, &dummy, &dummy, &ecx, &edx);
+
+	return (ecx & CPUID5_ECX_EXTENSIONS_SUPPORTED) &&
+	       (ecx & CPUID5_ECX_INTERRUPT_BREAK) &&
+	       (edx >> 28);
+}
--- a/xen/include/asm-x86/mwait.h
+++ b/xen/include/asm-x86/mwait.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_X86_MWAIT_H__
 #define __ASM_X86_MWAIT_H__
 
+#include <xen/types.h>
+
 #define MWAIT_SUBSTATE_MASK		0xf
 #define MWAIT_CSTATE_MASK		0xf
 #define MWAIT_SUBSTATE_SIZE		4
@@ -12,5 +14,6 @@
 #define MWAIT_ECX_INTERRUPT_BREAK	0x1
 
 void mwait_idle_with_hints(unsigned int eax, unsigned int ecx);
+bool mwait_pc10_supported(void);
 
 #endif /* __ASM_X86_MWAIT_H__ */

[PATCH 2/2] x86: de-duplicate MONITOR/MWAIT CPUID-related definitions

[Jan Beulich]

As of 724b55f48a6c ("x86: introduce MWAIT-based, ACPI-less CPU idle
driver") they (also) live in asm/mwait.h; no idea how I missed the
duplicates back at the time.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/acpi/lib.c
+++ b/xen/arch/x86/acpi/lib.c
@@ -24,6 +24,7 @@
 #include <xen/acpi.h>
 #include <asm/apic.h>
 #include <asm/fixmap.h>
+#include <asm/mwait.h>
 
 u32 __read_mostly acpi_smi_cmd;
 u8 __read_mostly acpi_enable_value;
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -22,10 +22,6 @@
 #define cpu_has(c, bit)		test_bit(bit, (c)->x86_capability)
 #define boot_cpu_has(bit)	test_bit(bit, boot_cpu_data.x86_capability)
 
-#define CPUID_MWAIT_LEAF                5
-#define CPUID5_ECX_EXTENSIONS_SUPPORTED 0x1
-#define CPUID5_ECX_INTERRUPT_BREAK      0x2
-
 #define CPUID_PM_LEAF                    6
 #define CPUID6_ECX_APERFMPERF_CAPABILITY 0x1
 

Re: [PATCH 1/2] x86/hpet: Use another crystalball to evaluate HPET usability

[Roger Pau Monné]

On Tue, Oct 19, 2021 at 09:07:39AM +0200, Jan Beulich wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> On recent Intel systems the HPET stops working when the system reaches PC10
> idle state.
> 
> The approach of adding PCI ids to the early quirks to disable HPET on
> these systems is a whack a mole game which makes no sense.
> 
> Check for PC10 instead and force disable HPET if supported. The check is
> overbroad as it does not take ACPI, mwait-idle enablement and command
> line parameters into account. That's fine as long as there is at least
> PMTIMER available to calibrate the TSC frequency. The decision can be
> overruled by adding "clocksource=hpet" on the Xen command line.
> 
> Remove the related PCI quirks for affected Coffee Lake systems as they
> are not longer required. That should also cover all other systems, i.e.
> Ice Lake, Tiger Lake, and newer generations, which are most likely
> affected by this as well.
> 
> Fixes: Yet another hardware trainwreck
> Reported-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> [Linux commit: 6e3cd95234dc1eda488f4f487c281bac8fef4d9b]
> 
> I have to admit that the purpose of checking CPUID5_ECX_INTERRUPT_BREAK
> is unclear to me, but I didn't want to diverge in technical aspects from
> the Linux commit.
> 
> In mwait_pc10_supported(), besides some cosmetic adjustments, avoid UB
> from shifting left a signed 4-bit constant by 28 bits.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Just one comment below.

> ---
> v2: Fully different replacement of "x86: avoid HPET use also on certain
>     Coffee Lake H".
> 
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -34,6 +34,7 @@
>  #include <asm/fixmap.h>
>  #include <asm/guest.h>
>  #include <asm/mc146818rtc.h>
> +#include <asm/mwait.h>
>  #include <asm/div64.h>
>  #include <asm/acpi.h>
>  #include <asm/hpet.h>
> @@ -395,14 +396,43 @@ static int64_t __init init_hpet(struct p
>              }
>  
>          /*
> -         * Some Coffee Lake platforms have a skewed HPET timer once the SoCs
> -         * entered PC10.
> +         * Some Coffee Lake and later platforms have a skewed HPET timer once
> +         * they entered PC10.
> +         *
> +         * Check whether the system supports PC10. If so force disable HPET as
> +         * that stops counting in PC10. This check is overbroad as it does not
> +         * take any of the following into account:
> +         *
> +         *	- ACPI tables
> +         *	- Enablement of mwait-idle
> +         *	- Command line arguments which limit mwait-idle C-state support
> +         *
> +         * That's perfectly fine. HPET is a piece of hardware designed by
> +         * committee and the only reasons why it is still in use on modern
> +         * systems is the fact that it is impossible to reliably query TSC and
> +         * CPU frequency via CPUID or firmware.
> +         *
> +         * If HPET is functional it is useful for calibrating TSC, but this can
> +         * be done via PMTIMER as well which seems to be the last remaining
> +         * timer on X86/INTEL platforms that has not been completely wreckaged
> +         * by feature creep.
> +         *
> +         * In theory HPET support should be removed altogether, but there are
> +         * older systems out there which depend on it because TSC and APIC timer
> +         * are dysfunctional in deeper C-states.
>           */
> -        if ( pci_conf_read16(PCI_SBDF(0, 0, 0, 0),
> -                             PCI_VENDOR_ID) == PCI_VENDOR_ID_INTEL &&
> -             pci_conf_read16(PCI_SBDF(0, 0, 0, 0),
> -                             PCI_DEVICE_ID) == 0x3ec4 )
> -            hpet_address = 0;
> +        if ( mwait_pc10_supported() )
> +        {
> +            uint64_t pcfg;
> +
> +            rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, pcfg);
> +            if ( (pcfg & 0xf) < 8 )
> +                /* nothing */;
> +            else if ( !strcmp(opt_clocksource, pts->id) )
> +                printk("HPET use requested via command line, but dysfunctional in PC10\n");
> +            else
> +                hpet_address = 0;

Should we print a message that HPET is being disabled?

Thanks, Roger.

Re: [PATCH 2/2] x86: de-duplicate MONITOR/MWAIT CPUID-related definitions

[Roger Pau Monné]

On Tue, Oct 19, 2021 at 09:08:22AM +0200, Jan Beulich wrote:
> As of 724b55f48a6c ("x86: introduce MWAIT-based, ACPI-less CPU idle
> driver") they (also) live in asm/mwait.h; no idea how I missed the
> duplicates back at the time.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 1/2] x86/hpet: Use another crystalball to evaluate HPET usability

[Jan Beulich]

On 19.10.2021 13:30, Roger Pau Monné wrote:
> On Tue, Oct 19, 2021 at 09:07:39AM +0200, Jan Beulich wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>>
>> On recent Intel systems the HPET stops working when the system reaches PC10
>> idle state.
>>
>> The approach of adding PCI ids to the early quirks to disable HPET on
>> these systems is a whack a mole game which makes no sense.
>>
>> Check for PC10 instead and force disable HPET if supported. The check is
>> overbroad as it does not take ACPI, mwait-idle enablement and command
>> line parameters into account. That's fine as long as there is at least
>> PMTIMER available to calibrate the TSC frequency. The decision can be
>> overruled by adding "clocksource=hpet" on the Xen command line.
>>
>> Remove the related PCI quirks for affected Coffee Lake systems as they
>> are not longer required. That should also cover all other systems, i.e.
>> Ice Lake, Tiger Lake, and newer generations, which are most likely
>> affected by this as well.
>>
>> Fixes: Yet another hardware trainwreck
>> Reported-by: Jakub Kicinski <kuba@kernel.org>
>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>> [Linux commit: 6e3cd95234dc1eda488f4f487c281bac8fef4d9b]
>>
>> I have to admit that the purpose of checking CPUID5_ECX_INTERRUPT_BREAK
>> is unclear to me, but I didn't want to diverge in technical aspects from
>> the Linux commit.
>>
>> In mwait_pc10_supported(), besides some cosmetic adjustments, avoid UB
>> from shifting left a signed 4-bit constant by 28 bits.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> @@ -395,14 +396,43 @@ static int64_t __init init_hpet(struct p
>>              }
>>  
>>          /*
>> -         * Some Coffee Lake platforms have a skewed HPET timer once the SoCs
>> -         * entered PC10.
>> +         * Some Coffee Lake and later platforms have a skewed HPET timer once
>> +         * they entered PC10.
>> +         *
>> +         * Check whether the system supports PC10. If so force disable HPET as
>> +         * that stops counting in PC10. This check is overbroad as it does not
>> +         * take any of the following into account:
>> +         *
>> +         *	- ACPI tables
>> +         *	- Enablement of mwait-idle
>> +         *	- Command line arguments which limit mwait-idle C-state support
>> +         *
>> +         * That's perfectly fine. HPET is a piece of hardware designed by
>> +         * committee and the only reasons why it is still in use on modern
>> +         * systems is the fact that it is impossible to reliably query TSC and
>> +         * CPU frequency via CPUID or firmware.
>> +         *
>> +         * If HPET is functional it is useful for calibrating TSC, but this can
>> +         * be done via PMTIMER as well which seems to be the last remaining
>> +         * timer on X86/INTEL platforms that has not been completely wreckaged
>> +         * by feature creep.
>> +         *
>> +         * In theory HPET support should be removed altogether, but there are
>> +         * older systems out there which depend on it because TSC and APIC timer
>> +         * are dysfunctional in deeper C-states.
>>           */
>> -        if ( pci_conf_read16(PCI_SBDF(0, 0, 0, 0),
>> -                             PCI_VENDOR_ID) == PCI_VENDOR_ID_INTEL &&
>> -             pci_conf_read16(PCI_SBDF(0, 0, 0, 0),
>> -                             PCI_DEVICE_ID) == 0x3ec4 )
>> -            hpet_address = 0;
>> +        if ( mwait_pc10_supported() )
>> +        {
>> +            uint64_t pcfg;
>> +
>> +            rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, pcfg);
>> +            if ( (pcfg & 0xf) < 8 )
>> +                /* nothing */;
>> +            else if ( !strcmp(opt_clocksource, pts->id) )
>> +                printk("HPET use requested via command line, but dysfunctional in PC10\n");
>> +            else
>> +                hpet_address = 0;
> 
> Should we print a message that HPET is being disabled?

There is one, and it was even visible in patch context that you
did strip from your reply:

         if ( !hpet_address )
             printk("Disabling HPET for being unreliable\n");

Jan

Re: [PATCH 1/2] x86/hpet: Use another crystalball to evaluate HPET usability

[Roger Pau Monné]

On Tue, Oct 19, 2021 at 02:08:38PM +0200, Jan Beulich wrote:
> On 19.10.2021 13:30, Roger Pau Monné wrote:
> > On Tue, Oct 19, 2021 at 09:07:39AM +0200, Jan Beulich wrote:
> >> From: Thomas Gleixner <tglx@linutronix.de>
> >>
> >> On recent Intel systems the HPET stops working when the system reaches PC10
> >> idle state.
> >>
> >> The approach of adding PCI ids to the early quirks to disable HPET on
> >> these systems is a whack a mole game which makes no sense.
> >>
> >> Check for PC10 instead and force disable HPET if supported. The check is
> >> overbroad as it does not take ACPI, mwait-idle enablement and command
> >> line parameters into account. That's fine as long as there is at least
> >> PMTIMER available to calibrate the TSC frequency. The decision can be
> >> overruled by adding "clocksource=hpet" on the Xen command line.
> >>
> >> Remove the related PCI quirks for affected Coffee Lake systems as they
> >> are not longer required. That should also cover all other systems, i.e.
> >> Ice Lake, Tiger Lake, and newer generations, which are most likely
> >> affected by this as well.
> >>
> >> Fixes: Yet another hardware trainwreck
> >> Reported-by: Jakub Kicinski <kuba@kernel.org>
> >> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> >> [Linux commit: 6e3cd95234dc1eda488f4f487c281bac8fef4d9b]
> >>
> >> I have to admit that the purpose of checking CPUID5_ECX_INTERRUPT_BREAK
> >> is unclear to me, but I didn't want to diverge in technical aspects from
> >> the Linux commit.
> >>
> >> In mwait_pc10_supported(), besides some cosmetic adjustments, avoid UB
> >> from shifting left a signed 4-bit constant by 28 bits.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> >> @@ -395,14 +396,43 @@ static int64_t __init init_hpet(struct p
> >>              }
> >>  
> >>          /*
> >> -         * Some Coffee Lake platforms have a skewed HPET timer once the SoCs
> >> -         * entered PC10.
> >> +         * Some Coffee Lake and later platforms have a skewed HPET timer once
> >> +         * they entered PC10.
> >> +         *
> >> +         * Check whether the system supports PC10. If so force disable HPET as
> >> +         * that stops counting in PC10. This check is overbroad as it does not
> >> +         * take any of the following into account:
> >> +         *
> >> +         *	- ACPI tables
> >> +         *	- Enablement of mwait-idle
> >> +         *	- Command line arguments which limit mwait-idle C-state support
> >> +         *
> >> +         * That's perfectly fine. HPET is a piece of hardware designed by
> >> +         * committee and the only reasons why it is still in use on modern
> >> +         * systems is the fact that it is impossible to reliably query TSC and
> >> +         * CPU frequency via CPUID or firmware.
> >> +         *
> >> +         * If HPET is functional it is useful for calibrating TSC, but this can
> >> +         * be done via PMTIMER as well which seems to be the last remaining
> >> +         * timer on X86/INTEL platforms that has not been completely wreckaged
> >> +         * by feature creep.
> >> +         *
> >> +         * In theory HPET support should be removed altogether, but there are
> >> +         * older systems out there which depend on it because TSC and APIC timer
> >> +         * are dysfunctional in deeper C-states.
> >>           */
> >> -        if ( pci_conf_read16(PCI_SBDF(0, 0, 0, 0),
> >> -                             PCI_VENDOR_ID) == PCI_VENDOR_ID_INTEL &&
> >> -             pci_conf_read16(PCI_SBDF(0, 0, 0, 0),
> >> -                             PCI_DEVICE_ID) == 0x3ec4 )
> >> -            hpet_address = 0;
> >> +        if ( mwait_pc10_supported() )
> >> +        {
> >> +            uint64_t pcfg;
> >> +
> >> +            rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, pcfg);
> >> +            if ( (pcfg & 0xf) < 8 )
> >> +                /* nothing */;
> >> +            else if ( !strcmp(opt_clocksource, pts->id) )
> >> +                printk("HPET use requested via command line, but dysfunctional in PC10\n");
> >> +            else
> >> +                hpet_address = 0;
> > 
> > Should we print a message that HPET is being disabled?
> 
> There is one, and it was even visible in patch context that you
> did strip from your reply:

I meant something about being disabled for PC10, but I think the
generic one is fine enough.

Thanks, Roger.

Re: [REGRESSION] [PATCH 1/2] x86/hpet: Use another crystalball to evaluate HPET usability

[Andrew Cooper]

On 19/10/2021 08:07, Jan Beulich wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> On recent Intel systems the HPET stops working when the system reaches PC10
> idle state.
>
> The approach of adding PCI ids to the early quirks to disable HPET on
> these systems is a whack a mole game which makes no sense.
>
> Check for PC10 instead and force disable HPET if supported. The check is
> overbroad as it does not take ACPI, mwait-idle enablement and command
> line parameters into account. That's fine as long as there is at least
> PMTIMER available to calibrate the TSC frequency. The decision can be
> overruled by adding "clocksource=hpet" on the Xen command line.
>
> Remove the related PCI quirks for affected Coffee Lake systems as they
> are not longer required. That should also cover all other systems, i.e.
> Ice Lake, Tiger Lake, and newer generations, which are most likely
> affected by this as well.
>
> Fixes: Yet another hardware trainwreck
> Reported-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> [Linux commit: 6e3cd95234dc1eda488f4f487c281bac8fef4d9b]
>
> I have to admit that the purpose of checking CPUID5_ECX_INTERRUPT_BREAK
> is unclear to me, but I didn't want to diverge in technical aspects from
> the Linux commit.
>
> In mwait_pc10_supported(), besides some cosmetic adjustments, avoid UB
> from shifting left a signed 4-bit constant by 28 bits.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This breaks booting on recent Intel platforms.

Ian: Complete blocker for the release.

~Andrew

[PATCH] x86/hpet: setup HPET even when disabled due to stopping in deep C states

[Roger Pau Monne]

Always allow the HPET to be setup, but don't report a frequency back
to the platform time source probe in order to avoid it from being
selected as a valid timer if it's not usable.

Doing the setup even when not intended to be used as a platform timer
is required so that is can be used in legacy replacement mode in order
to assert the IO-APIC is capable of receiving interrupts.

Fixes: c12731493a ('x86/hpet: Use another crystalball to evaluate HPET usability')
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/time.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index cbadaa036f..a290aba3e8 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -379,6 +379,12 @@ static int64_t __init init_hpet(struct platform_timesource *pts)
 {
     uint64_t hpet_rate, start;
     uint32_t count, target;
+    /*
+     * Allow HPET to be setup, but report a frequency of 0 so it's not selected
+     * as a timer source. This is required so it can be used in legacy
+     * replacement mode in check_timer.
+     */
+    bool disable_hpet = false;
 
     if ( hpet_address && strcmp(opt_clocksource, pts->id) &&
          cpuidle_using_deep_cstate() )
@@ -391,7 +397,7 @@ static int64_t __init init_hpet(struct platform_timesource *pts)
             case 0x0f1c:
             /* HPET on Cherry Trail platforms will halt in deep C states. */
             case 0x229c:
-                hpet_address = 0;
+                disable_hpet = true;
                 break;
             }
 
@@ -431,14 +437,14 @@ static int64_t __init init_hpet(struct platform_timesource *pts)
             else if ( !strcmp(opt_clocksource, pts->id) )
                 printk("HPET use requested via command line, but dysfunctional in PC10\n");
             else
-                hpet_address = 0;
+                disable_hpet = true;
         }
 
-        if ( !hpet_address )
+        if ( disable_hpet )
             printk("Disabling HPET for being unreliable\n");
     }
 
-    if ( (hpet_rate = hpet_setup()) == 0 )
+    if ( (hpet_rate = hpet_setup()) == 0 || disable_hpet )
         return 0;
 
     pts->frequency = hpet_rate;
-- 
2.33.0

Re: [PATCH] x86/hpet: setup HPET even when disabled due to stopping in deep C states

[Andrew Cooper]

On 26/10/2021 16:12, Roger Pau Monne wrote:
> Always allow the HPET to be setup, but don't report a frequency back
> to the platform time source probe in order to avoid it from being
> selected as a valid timer if it's not usable.
>
> Doing the setup even when not intended to be used as a platform timer
> is required so that is can be used in legacy replacement mode in order
> to assert the IO-APIC is capable of receiving interrupts.
>
> Fixes: c12731493a ('x86/hpet: Use another crystalball to evaluate HPET usability')
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Yup - does fix the regression.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH] x86/hpet: setup HPET even when disabled due to stopping in deep C states

[Ian Jackson]

Roger Pau Monne writes ("[PATCH] x86/hpet: setup HPET even when disabled due to stopping in deep C states"):
> Always allow the HPET to be setup, but don't report a frequency back
> to the platform time source probe in order to avoid it from being
> selected as a valid timer if it's not usable.
> 
> Doing the setup even when not intended to be used as a platform timer
> is required so that is can be used in legacy replacement mode in order
> to assert the IO-APIC is capable of receiving interrupts.
> 
> Fixes: c12731493a ('x86/hpet: Use another crystalball to evaluate HPET usability')
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Re: [PATCH 2/2] x86: de-duplicate MONITOR/MWAIT CPUID-related definitions

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH 2/2] x86: de-duplicate MONITOR/MWAIT CPUID-related definitions"):
> On Tue, Oct 19, 2021 at 09:08:22AM +0200, Jan Beulich wrote:
> > As of 724b55f48a6c ("x86: introduce MWAIT-based, ACPI-less CPU idle
> > driver") they (also) live in asm/mwait.h; no idea how I missed the
> > duplicates back at the time.
> > 
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Re: [PATCH] x86/hpet: setup HPET even when disabled due to stopping in deep C states

[Jan Beulich]

On 26.10.2021 17:12, Roger Pau Monne wrote:
> Always allow the HPET to be setup, but don't report a frequency back
> to the platform time source probe in order to avoid it from being
> selected as a valid timer if it's not usable.
> 
> Doing the setup even when not intended to be used as a platform timer
> is required so that is can be used in legacy replacement mode in order
> to assert the IO-APIC is capable of receiving interrupts.
> 
> Fixes: c12731493a ('x86/hpet: Use another crystalball to evaluate HPET usability')

I realize this has gone in already, but imo pointing at this commit is
only part of the truth (maybe the larger one). e1de4c196a2e ("x86/timer:
Fix boot on Intel systems using ITSSPRC static PIT clock gating") post-
dates d5294a302c84 ("x86: avoid HPET use on certain Intel platforms")
by over a year, yet it's that patch'es logic which the referenced
commit merely extended. I am of the opinion that e1de4c196a2e should
have made the adjustment you make now, and hence should (also) have
been tagged.

Jan