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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2021 02:06, Andrew Cooper wrote:
> 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.

We still will want to try to suppress those by the release of 4.15,
and ideally even before we backport this one.

> @@ -793,6 +793,71 @@ 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.
> +     */
> +    if ( hpet_id & HPET_ID_LEGSUP &&

Add parentheses here?

> +         !((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 cache
> +         * the "last write to cmp", as a hidden register.
> +         *
> +         * The semantics on generating a periodic interrupt is:
> +         *   cmp += "last value written to the cmp register"
> +         * which will reload a new period.
> +         *
> +         * Normally, writes to cmp update the main comparator as well as 
> being
> +         * cached for the reload value.  However, under these semantics, the
> +         * HPET main counter needs resetting to 0 to be able to configure the
> +         * period correctly.
> +         *
> +         * Instead, HPET_TN_SETVAL is a self-clearing control bit which we 
> can
> +         * use for periodic timers to mean that the second write to the
> +         * comparator updates only the "last written" cache, 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));

As you say, documentation is poor here. In fact the wording in the
HPET spec talks about updating of the "accumulator" instead;
perhaps just an unhelpful use of a different term. (Respective
Linux code has a comment indicating this is needed on a specific
AMD chipset only.)

It would seem more natural to me if things were explained a little
differently: Writes to the comparator with SETVAL clear always
update the "last written" value, i.e. the increment to be used
once the main counter equals the comparator. Writes with SETVAL set
update the comparator itself. (Assuming that's how it really is, of
course, but that's what I infer from the doc available.)

Linux additionally puts udelay(1) between the two writes. Do you
think we're fine without such, on all platforms?

> +        /* Restart the main counter, and legacy mode. */
> +        hpet_write32(hpet_cfg | HPET_CFG_ENABLE | HPET_CFG_LEGACY, HPET_CFG);

This isn't necessarily "restart" - you may start the counter for
the first time. Hence in the comment maybe "(Re)start ..."?

As to the spurious IRQs, does it perhaps matter at which point
CFG_LEGACY gets set? We could try setting it when clearing
CFG_ENABLE further up, or we could also try two separate writes
each setting just one of the bits. (At least I can't deduce
from the spec that we ought to be prepared to observe spurious
IRQs here.)

Jan