Re: [PATCH v2 2/3] x86/hpet: Don't enable legacy replacement mode unconditionally

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

On Fri, Mar 26, 2021 at 06:59:46PM +0000, Andrew Cooper wrote:
> From: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Commit e1de4c196a2e ("x86/timer: Fix boot on Intel systems using ITSSPRC
> static PIT clock gating") was reported to cause boot failures on certain
> AMD Ryzen systems.
> 
> Refine the fix to do nothing in the default case, and only attempt to
> configure legacy replacement mode if IRQ0 is found to not be working.
> 
> In addition, introduce a "hpet" command line option so this heuristic
> can be overridden.  Since it makes little sense to introduce just
> "hpet=legacy-replacement", also allow for a boolean argument as well as
> "broadcast" to replace the separate "hpetbroadcast" option.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> CC: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> CC: Frédéric Pierret <frederic.pierret@xxxxxxxxxxxx>
> 
> v2:
>  * Reinstate missing hunk from Jan's original patch.
>  * Fix up "8254 PIT".
>  * Drop "goto retry".
> 
> For 4.15: Attempt to unbreak AMD Ryzen 1800X systems.
> ---
>  docs/misc/xen-command-line.pandoc | 33 +++++++++++++++++++++++++++
>  xen/arch/x86/hpet.c               | 48 
> +++++++++++++++++++++++++--------------
>  xen/arch/x86/io_apic.c            | 27 ++++++++++++++++++++++
>  xen/include/asm-x86/hpet.h        |  1 +
>  4 files changed, 92 insertions(+), 17 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc 
> b/docs/misc/xen-command-line.pandoc
> index a0601ff838..a4bd3f12c5 100644
> --- 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
> +    = List of [ <bool> | broadcast=<bool> | legacy-replacement=<bool> ]
> +
> +    Applicability: x86
> +
> +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
> +   8254 PIT, and allows the HPET to be configured into a compatible mode.
> +   Intel chipsets from Skylake/ApolloLake onwards can turn the PIT off for
> +   power saving reasons, and there is no platform-agnostic mechanism for
> +   discovering this.
> +
> +   By default, Xen will not change hardware configuration, unless the PIT
> +   appears to be absent, at which point Xen will try to enable Legacy
> +   Replacement mode before falling back to pre-IO-APIC interrupt routing
> +   options.
> +
> +   This behaviour can be inhibited by specifying `legacy-replacement=0`.
> +   Alternatively, this mode can be enabled unconditionally (if available) by
> +   specifying `legacy-replacement=1`.
> +
>  ### hpetbroadcast (x86)
>  > `= <boolean>`
>  
> +Deprecated alternative of `hpet=broadcast`.
> +
>  ### hvm_debug (x86)
>  > `= <integer>`
>  
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index c1d04f184f..bfa75f135a 100644
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -52,6 +52,8 @@ static unsigned int __read_mostly num_hpets_used;
>  DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
>  
>  unsigned long __initdata hpet_address;
> +int8_t __initdata opt_hpet_legacy_replacement = -1;
> +static bool __initdata opt_hpet = true;
>  u8 __initdata hpet_blockid;
>  u8 __initdata hpet_flags;
>  
> @@ -63,6 +65,32 @@ u8 __initdata hpet_flags;
>  static bool __initdata force_hpet_broadcast;
>  boolean_param("hpetbroadcast", force_hpet_broadcast);
>  
> +static int __init parse_hpet_param(const char *s)
> +{
> +    const char *ss;
> +    int val, rc = 0;
> +
> +    do {
> +        ss = strchr(s, ',');
> +        if ( !ss )
> +            ss = strchr(s, '\0');
> +
> +        if ( (val = parse_bool(s, ss)) >= 0 )
> +            opt_hpet = val;
> +        else if ( (val = parse_boolean("broadcast", s, ss)) >= 0 )
> +            force_hpet_broadcast = val;
> +        else if ( (val = parse_boolean("legacy-replacement", s, ss)) >= 0 )
> +            opt_hpet_legacy_replacement = val;
> +        else
> +            rc = -EINVAL;
> +
> +        s = ss + 1;
> +    } while ( *ss );
> +
> +    return rc;
> +}
> +custom_param("hpet", parse_hpet_param);
> +
>  /*
>   * Calculate a multiplication factor for scaled math, which is used to 
> convert
>   * nanoseconds based values to clock ticks:
> @@ -820,12 +848,9 @@ u64 __init hpet_setup(void)
>      unsigned int hpet_id, hpet_period;
>      unsigned int last, rem;
>  
> -    if ( hpet_rate )
> +    if ( hpet_rate || !hpet_address || !opt_hpet )
>          return hpet_rate;
>  
> -    if ( hpet_address == 0 )
> -        return 0;
> -
>      set_fixmap_nocache(FIX_HPET_BASE, hpet_address);
>  
>      hpet_id = hpet_read32(HPET_ID);
> @@ -852,19 +877,8 @@ 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.
> -     */
> -    hpet_enable_legacy_replacement_mode();
> +    if ( opt_hpet_legacy_replacement > 0 )
> +        hpet_enable_legacy_replacement_mode();
>  
>      return hpet_rate;
>  }
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index e93265f379..3f131a81fb 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -29,6 +29,8 @@
>  #include <xen/acpi.h>
>  #include <xen/keyhandler.h>
>  #include <xen/softirq.h>
> +
> +#include <asm/hpet.h>
>  #include <asm/mc146818rtc.h>
>  #include <asm/smp.h>
>  #include <asm/desc.h>
> @@ -1930,6 +1932,31 @@ static void __init check_timer(void)
>              local_irq_restore(flags);
>              return;
>          }
> +
> +        /*
> +         * Intel chipsets from Skylake/ApolloLake onwards can statically 
> clock
> +         * gate the 8254 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 choice, attempt to reconfigure
> +         * the HPET into legacy mode to re-establish the timer interrupt.
> +         */
> +        if ( opt_hpet_legacy_replacement < 0 &&
> +             hpet_enable_legacy_replacement_mode() )
> +        {
> +            printk(XENLOG_ERR "..no 8254 timer found - trying HPET Legacy 
> Replacement Mode\n");
> +
> +            if ( timer_irq_works() )
> +            {
> +                local_irq_restore(flags);

Is there any point in undoing the legacy replacement here, as I
understand it it's only required for the routing check?

Should we also prevent any attempts from using the PIT as a
timecounter in x86/time.c as a result of having the HPET in legacy
replacement mode?

AFAICT init_pit will already assert whether the PIT counters work, so
maybe there's no need for adding an extra check on whether legacy
replacement is enabled.

Thanks, Roger.