Re: [PATCH v1.1 2/2] x86/hpet: Don't enable legacy replacement mode unconditionally

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/03/2021 09:51, Jan Beulich wrote:
> On 25.03.2021 18:21, Andrew Cooper wrote:
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1274,9 +1274,42 @@ supported. See docs/misc/arm/big.LITTLE.txt for more 
>> information.
>>  When the hmp-unsafe option is disabled (default), CPUs that are not
>>  identical to the boot CPU will be parked and not used by Xen.
>>  
>> +### hpet (x86)
>> +    = List of [ <bool> | broadcast=<bool> | legacy-replacement=<bool> ]
>> +
>> +    Applicability: x86
> If this is the more modern form to express this information, then the
> (x86) I did put on the sub-title line should imo be dropped.

Oops yes.

>
>> +Controls Xen's use of the system's High Precision Event Timer.  By default,
>> +Xen will use an HPET when available and not subject to errata.  Use of the
>> +HPET can be disabled by specifying `hpet=0`.
>> +
>> + * The `broadcast` boolean is disabled by default, but forces Xen to keep
>> +   using the broadcast for CPUs in deep C-states even when an RTC interrupt 
>> is
>> +   enabled.  This then also affects raising of the RTC interrupt.
>> +
>> + * The `legacy-replacement` boolean allows for control over whether Legacy
>> +   Replacement mode is enabled.
>> +
>> +   Legacy Replacement mode is intended for hardware which does not have an
>> +   8025 PIT, and allows the HPET to be configured into a compatible mode.
> 8254 ?

I did spot and fix that...

>
>> @@ -1922,14 +1924,38 @@ static void __init check_timer(void)
>>             vector, apic1, pin1, apic2, pin2);
>>  
>>      if (pin1 != -1) {
>> +        bool hpet_changed = false;
>> +
>>          /*
>>           * Ok, does IRQ0 through the IOAPIC work?
>>           */
>>          unmask_IO_APIC_irq(irq_to_desc(0));
>> +    retry_ioapic_pin:
>>          if (timer_irq_works()) {
>>              local_irq_restore(flags);
>>              return;
>>          }
>> +
>> +        /*
>> +         * Intel chipsets from Skylake/ApolloLake onwards can statically 
>> clock
>> +         * gate the 8259 PIT.  This option is enabled by default in slightly
> 8254?

but failed to spot this one.  (It was an error from the original
patch).  Fixed.

>
>> +         * 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.
>> +         *
>> +         * If the user hasn't made an explicit option, attempt to 
>> reconfigure
> s/option/choice/ or s/made/given/?
>
>> +         * the HPET into legacy mode to re-establish the timer interrupt.
>> +         */
>> +        if ( opt_hpet_legacy_replacement < 0 &&
>> +             !hpet_changed && hpet_enable_legacy_replacement_mode() )
>> +        {
>> +            printk(XENLOG_ERR "..no 8254 timer found - trying HPET Legacy 
>> Replacement Mode\n");
>> +            hpet_changed = true;
>> +            goto retry_ioapic_pin;
>> +        }
>> +
>>          clear_IO_APIC_pin(apic1, pin1);
>>          printk(KERN_ERR "..MP-BIOS bug: 8254 timer not connected to "
>>                 "IO-APIC\n");
> As mentioned on irc already, I'm somewhat concerned by doing this change
> first (and also not undoing it if it didn't work). An AMD Turion based
> laptop I was using many years ago required one of the other fallbacks to
> be engaged, and hence I'd expect certain other (old?) systems to be
> similarly affected. Sadly (for the purposes here) I don't have this
> laptop anymore, so wouldn't be able to verify whether the above actually
> breaks there.

Turion is K8, so very obsolete these days.  If it doesn't have an
IO-APIC, its even less likely to have an HPET.

Even if it does have an HPET, there isn't anything to suggest that
legacy replacement mode is broken.

Would you prefer me to undo the change?  Its not easy - we have the boot
time config stashed, but if it was periodic before, the accumulator is
broken because we can never read that value back out.

> As a minor remark, I find the "goto" based approach not very nice (we've
> been generally saying we consider "goto" okay largely for simplification
> of error handling, to avoid having many [partly] redundant function exit
> paths), but I can see how using for() or while() or do/while() would
> make the code larger and require deeper indentation.

Actually there is rather less to repeat than I was expecting.  I think
it is reasonable to take out the goto.

I don't think we want to multiply this with all fallback modes.  The
issue we're fixing here (new systems don't have a PIT) is orthogonal to
the rest of the fallback chain here which is looking for PIC/APIC problems.

~Andrew