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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.03.2021 17:32, Andrew Cooper wrote:
> On 26/03/2021 09:51, Jan Beulich wrote:
>> On 25.03.2021 18:21, Andrew Cooper wrote:
>>> @@ -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
>>> +         * 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
>>> +         * 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.

It did have an IO-APIC, but required one of the virtual-wire modes to
be enabled iirc.

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

With one firmware flaw there is about as much chance for another one
as there is for HPET to be working, I'd say. Iirc (very vaguely) it
did have a HPET, but no ACPI table entry for it, so we wouldn't have
used it.

> 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.

I didn't think the accumulator change would matter. I did think though
not having been in legacy replacement mode before might be better to
also not be in after, if its enabling didn't help anyway.

Jan