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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxx>
> >> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> >> [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@xxxxxxxx>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> 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.