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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> +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 ?

> @@ -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?

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

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.

Jan