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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.04.2025 13:32, Roger Pau Monne wrote:
> There are several errata on Intel regarding the usage of the MONITOR/MWAIT
> instructions, all having in common that stores to the monitored region
> might not wake up the CPU.
> 
> Fix them by forcing the sending of an IPI for the affected models.
> 
> The Ice Lake issue has been reproduced internally on XenServer hardware,
> and the fix does seem to prevent it.  The symptom was APs getting stuck in
> the idle loop immediately after bring up, which in turn prevented the BSP
> from making progress.  This would happen before the watchdog was
> initialized, and hence the whole system would get stuck.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
with a nit and an entirely optional suggestion:

> @@ -380,6 +380,38 @@ 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.
> + *
> + * Force the sending of an IPI in those cases.
> + */
> +static void __init probe_mwait_errata(void)
> +{
> +    static const struct x86_cpu_id models[] = {

__initconst

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

You could simplify the condition by using just system_state < SYS_STATE_smp_boot
(without any &&), I think.

Jan