Re: [PATCH] x86/timer: Fix boot on Intel systems using ITSSPRC static PIT clock gating

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, Jan 6, 2021 at 8:06 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> Recent Intel client devices have disabled the legacy PIT for powersaving
> reasons, breaking compatibility with a traditional IBM PC.  Xen depends on a
> legacy timer interrupt to check that the IO-APIC/PIC routing is configured
> correctly, and fails to boot with:
>
>   (XEN) *******************************
>   (XEN) Panic on CPU 0:
>   (XEN) IO-APIC + timer doesn't work!  Boot with apic_verbosity=debug and 
> send report.  Then try booting with the `noapic` option
>   (XEN) *******************************
>
> While this setting can be undone by Xen, the details of how to differ by
> chipset, and would be very short sighted for battery based devices.  See bit 2
> "8254 Static Clock Gating Enable" in:
>
>   
> https://edc.intel.com/content/www/us/en/design/products-and-solutions/processors-and-chipsets/comet-lake-u/intel-400-series-chipset-on-package-platform-controller-hub-register-database/itss-power-reduction-control-itssprc-offset-3300/
>
> All impacted systems have an HPET, but there is no indication of the absence
> of PIT functionality, nor a suitable way to probe for its absence.  As a short
> term fix, reconfigure the HPET into legacy replacement mode.  A better
> longterm fix would be to avoid the reliance on the timer interrupt entirely.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Tested-by: Jason Andryuk <jandryuk@xxxxxxxxx>

On the Dell 7200 that couldn't boot without Xen modification, but with
a build fix below.

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Aaron Janse <aaron@xxxxxxxxx>
> CC: Jason Andryuk <jandryuk@xxxxxxxxx>
> CC: Ondrej Balaz <blami@xxxxxxxxx>
> CC: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> CC: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
>
> Slightly RFC.  On older platforms this does generate some spurious PIC
> interrupts during boot, but they're benign.  I was hoping to have time to fix
> those too, but I'm getting an increasing number of requests to post this
> patch.

Spurious being the timer interrupt is now running?  In my local
patches, I have a function clear HPET_CFG_LEGACY after check_timer():

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index e98e08e9c8..b62dea190a 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2047,6 +2048,7 @@ void __init setup_IO_APIC(void)
     setup_IO_APIC_irqs();
     init_IO_APIC_traps();
     check_timer();
+    hpet_disable_legacy();
     print_IO_APIC();
     ioapic_pm_state_alloc();

> Other followup actions:
>  * Overhaul our setup logic.  Large quantities of it is legacy for pre-64bit
>    systems, and not applicable to Xen these days.
>  * Have Xen turn the PIT off when it isn't being used as the timesource.  Its
>    a waste of time servicing useless interrupts.
>  * Make `clocksource=pit` not enter an infinite loop on these systems
>  * Drop all references to `noapic`.  These days, the only thing it will ever
>    do is make a bad situation worse.
> ---
>  xen/arch/x86/hpet.c | 67 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index e6fab8acd8..f9541af537 100644
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c

<snip>

> +        /*
> +         * 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;

hpet.c: In function ‘hpet_setup’:
hpet.c:828:70: error: expected ‘;’ before ‘)’ token
  828 |   ticks = (SECONDS(1) / HZ) * div_sc(hpet_rate, SECONDS(1), 32)) >> 32;

Missing an additional leading '('
ticks = ((SECONDS(1) / HZ) * div_sc(hpet_rate, SECONDS(1), 32)) >> 32;

Thanks,
Jason