Re: [PATCH] x86/intel: workaround several MONITOR/MWAIT errata

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.04.2025 18:19, Roger Pau Monne wrote:
> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -441,8 +441,14 @@ void cpuidle_wakeup_mwait(cpumask_t *mask)
>      cpumask_andnot(mask, mask, &target);
>  }
>  
> +/* Force sending of a wakeup IPI regardless of mwait usage. */
> +bool force_mwait_ipi_wakeup __read_mostly;

Nit: Annotation between type and identifier, please.

> @@ -380,6 +380,43 @@ static void probe_c3_errata(const struct cpuinfo_x86 *c)
>      }
>  }
>  
> +/*
> + * APL30: One use of the MONITOR/MWAIT instruction pair is to allow a logical
> + * processor to wait in a sleep state until a store to the armed address 
> range
> + * occurs. Due to this erratum, stores to the armed address range may not
> + * trigger MWAIT to resume execution.
> + *
> + * ICX143: Under complex microarchitectural conditions, a monitor that is 
> armed
> + * with the MWAIT instruction may not be triggered, leading to a processor
> + * hang.
> + *
> + * LNL030: Problem P-cores may not exit power state Core C6 on monitor hit.

I didn't manage to spot all three spec updates; none of these have a ucode fix,
hence permitting the workaround to be avoided?

Since CPX is 3rd Gen Xeon Scalable just like ICX is, I'm surprised that one's
unaffected. The most recent spec update there is a year old than ICX'es, so
may simply be too old to include the erratum?

Sunny Cove is used by further Icelake models - they're known to be unaffected?

> + * Force the sending of an IPI in those cases.
> + */
> +static void probe_mwait_errata(void)
> +{
> +    static const struct x86_cpu_id models[] = {
> +        /* Apollo Lake */
> +        INTEL_FAM6_MODEL(0x5C),
> +        /* Ice Lake */
> +        INTEL_FAM6_MODEL(0x6A),
> +        INTEL_FAM6_MODEL(0x6C),
> +        /* Lunar Lake */
> +        INTEL_FAM6_MODEL(0xBD),

Use identifiers from intel-family.h here?

> +        { }
> +    };
> +#undef INTEL_FAM6_MODEL
> +
> +    if ( boot_cpu_has(X86_FEATURE_MONITOR) && !force_mwait_ipi_wakeup &&
> +         x86_match_cpu(models) )
> +    {
> +        printk(XENLOG_WARNING
> +               "Forcing IPI MWAIT wakeup due to CPU erratum\n");
> +        force_mwait_ipi_wakeup = true;
> +    }
> +}

Do we really need to cater for asymmetric systems? IOW can't we do this once
on the BSP? Otherwise - why the use of boot_cpu_has() here? Oh, wait ...

> @@ -406,6 +443,8 @@ static void Intel_errata_workarounds(struct cpuinfo_x86 
> *c)
>               __set_bit(X86_FEATURE_CLFLUSH_MONITOR, c->x86_capability);
>  
>       probe_c3_errata(c);
> +     if (c == &boot_cpu_data)
> +             probe_mwait_errata();
>  }

..., you do this for the BSP only. Then why's the function not __init and
the global variable not __ro_after_init (and models[] __initconst)?

(Later) Except that this path is also taken for S3 resume, from
recheck_cpu_features(). This shouldn't alter the variable value anymore,
though. A disagreement ought to result in recheck_cpu_features() to
report failure. (Imo perhaps better to avoid the call above during resume.)

Jan