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

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>

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>

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