[PATCh 0/2] Add Intel Lunarlake to X86_BUG_MONITOR

[PATCh 0/2] Add Intel Lunarlake to X86_BUG_MONITOR

[Len Brown]

Early adopters of Intel Lunar Lake systems have noticed
unacceptable GUI delays.  This patch addresses those systems.

[PATCH] x86/cpu: Add INTEL_LUNARLAKE_M to X86_BUG_MONITOR

	Upstream patch, also applies to 6.11

[PATCH backport to 6.10] x86/cpu: Add INTEL_FAM6_LUNARLAKE_M to X86_BUG_MONITOR

	Backport to 6.10 for old model number scheme

[PATCH backport to 6.10] x86/cpu: Add INTEL_FAM6_LUNARLAKE_M to X86_BUG_MONITOR

[Len Brown]

From: Len Brown <len.brown@intel.com>

Under some conditions, MONITOR wakeups on Lunar Lake processors
can be lost, resulting in significant user-visible delays.

Add LunarLake to X86_BUG_MONITOR so that wake_up_idle_cpu()
always sends an IPI, avoiding this potential delay.
Update the X86_BUG_MONITOR workaround to handle
the new smp_kick_mwait_play_dead() path.

Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219364

Cc: stable@vger.kernel.org # 6.10
Signed-off-by: Len Brown <len.brown@intel.com>
---
This is a backport of the upstream patch to Linux-6.10 and earlier
---
 arch/x86/kernel/cpu/intel.c | 3 ++-
 arch/x86/kernel/smpboot.c   | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 3ef4e0137d21..e6f4c16c0267 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -583,7 +583,8 @@ static void init_intel(struct cpuinfo_x86 *c)
 		set_cpu_bug(c, X86_BUG_CLFLUSH_MONITOR);
 
 	if (c->x86 == 6 && boot_cpu_has(X86_FEATURE_MWAIT) &&
-		((c->x86_model == INTEL_FAM6_ATOM_GOLDMONT)))
+		((c->x86_model == INTEL_FAM6_ATOM_GOLDMONT) ||
+		(c->x86_model == INTEL_FAM6_LUNARLAKE_M)))
 		set_cpu_bug(c, X86_BUG_MONITOR);
 
 #ifdef CONFIG_X86_64
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 0c35207320cb..ca9358acc626 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1376,6 +1376,9 @@ void smp_kick_mwait_play_dead(void)
 		for (i = 0; READ_ONCE(md->status) != newstate && i < 1000; i++) {
 			/* Bring it out of mwait */
 			WRITE_ONCE(md->control, newstate);
+			/* If MWAIT unreliable, send IPI */
+			if (boot_cpu_has_bug(X86_BUG_MONITOR))
+				__apic_send_IPI(cpu, RESCHEDULE_VECTOR);
 			udelay(5);
 		}
 
-- 
2.43.0

[PATCH] x86/cpu: Add INTEL_LUNARLAKE_M to X86_BUG_MONITOR

[Len Brown]

From: Len Brown <len.brown@intel.com>

Under some conditions, MONITOR wakeups on Lunar Lake processors
can be lost, resulting in significant user-visible delays.

Add LunarLake to X86_BUG_MONITOR so that wake_up_idle_cpu()
always sends an IPI, avoiding this potential delay.

Also update the X86_BUG_MONITOR workaround to handle
the new smp_kick_mwait_play_dead() path.

Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219364

Cc: stable@vger.kernel.org # 6.11
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 3 ++-
 arch/x86/kernel/smpboot.c   | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index e7656cbef68d..aa63f5f780a0 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -586,7 +586,8 @@ static void init_intel(struct cpuinfo_x86 *c)
 	     c->x86_vfm == INTEL_WESTMERE_EX))
 		set_cpu_bug(c, X86_BUG_CLFLUSH_MONITOR);
 
-	if (boot_cpu_has(X86_FEATURE_MWAIT) && c->x86_vfm == INTEL_ATOM_GOLDMONT)
+	if (boot_cpu_has(X86_FEATURE_MWAIT) &&
+			(c->x86_vfm == INTEL_ATOM_GOLDMONT || c->x86_vfm == INTEL_LUNARLAKE_M))
 		set_cpu_bug(c, X86_BUG_MONITOR);
 
 #ifdef CONFIG_X86_64
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 766f092dab80..910cb2d72c13 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1377,6 +1377,9 @@ void smp_kick_mwait_play_dead(void)
 		for (i = 0; READ_ONCE(md->status) != newstate && i < 1000; i++) {
 			/* Bring it out of mwait */
 			WRITE_ONCE(md->control, newstate);
+			/* If MONITOR unreliable, send IPI */
+			if (boot_cpu_has_bug(X86_BUG_MONITOR))
+				__apic_send_IPI(cpu, RESCHEDULE_VECTOR);
 			udelay(5);
 		}
 
-- 
2.43.0

Re: [PATCH] x86/cpu: Add INTEL_LUNARLAKE_M to X86_BUG_MONITOR

[Rafael J. Wysocki]

On Fri, Nov 8, 2024 at 2:52 PM Len Brown <lenb@kernel.org> wrote:
>
> From: Len Brown <len.brown@intel.com>
>
> Under some conditions, MONITOR wakeups on Lunar Lake processors
> can be lost, resulting in significant user-visible delays.
>
> Add LunarLake to X86_BUG_MONITOR so that wake_up_idle_cpu()
> always sends an IPI, avoiding this potential delay.
>
> Also update the X86_BUG_MONITOR workaround to handle
> the new smp_kick_mwait_play_dead() path.
>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219364
>
> Cc: stable@vger.kernel.org # 6.11
> Signed-off-by: Len Brown <len.brown@intel.com>

Overall

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  arch/x86/kernel/cpu/intel.c | 3 ++-
>  arch/x86/kernel/smpboot.c   | 3 +++
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index e7656cbef68d..aa63f5f780a0 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -586,7 +586,8 @@ static void init_intel(struct cpuinfo_x86 *c)
>              c->x86_vfm == INTEL_WESTMERE_EX))
>                 set_cpu_bug(c, X86_BUG_CLFLUSH_MONITOR);
>
> -       if (boot_cpu_has(X86_FEATURE_MWAIT) && c->x86_vfm == INTEL_ATOM_GOLDMONT)
> +       if (boot_cpu_has(X86_FEATURE_MWAIT) &&
> +                       (c->x86_vfm == INTEL_ATOM_GOLDMONT || c->x86_vfm == INTEL_LUNARLAKE_M))
>                 set_cpu_bug(c, X86_BUG_MONITOR);
>
>  #ifdef CONFIG_X86_64
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 766f092dab80..910cb2d72c13 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1377,6 +1377,9 @@ void smp_kick_mwait_play_dead(void)
>                 for (i = 0; READ_ONCE(md->status) != newstate && i < 1000; i++) {
>                         /* Bring it out of mwait */
>                         WRITE_ONCE(md->control, newstate);
> +                       /* If MONITOR unreliable, send IPI */
> +                       if (boot_cpu_has_bug(X86_BUG_MONITOR))
> +                               __apic_send_IPI(cpu, RESCHEDULE_VECTOR);

The  __apic_send_IPI() call could be wrapped into something like
__native_smp_send_reschedule() to underline the analogy between this
and what happens in native_smp_send_reschedule().

It is still fine as is though IMV.

>                         udelay(5);
>                 }
>
> --

Re: [PATCH] x86/cpu: Add INTEL_LUNARLAKE_M to X86_BUG_MONITOR

[Peter Zijlstra]

On Fri, Nov 08, 2024 at 08:49:31AM -0500, Len Brown wrote:
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 766f092dab80..910cb2d72c13 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1377,6 +1377,9 @@ void smp_kick_mwait_play_dead(void)
>  		for (i = 0; READ_ONCE(md->status) != newstate && i < 1000; i++) {
>  			/* Bring it out of mwait */
>  			WRITE_ONCE(md->control, newstate);
> +			/* If MONITOR unreliable, send IPI */
> +			if (boot_cpu_has_bug(X86_BUG_MONITOR))
> +				__apic_send_IPI(cpu, RESCHEDULE_VECTOR);
>  			udelay(5);
>  		}

Going over that code again, mwait_play_dead() is doing __mwait(.exc=0)
with IRQs disabled.

So that IPI you're trying to send there won't do no nothing :-/

Now that comment there says MCE/NMI/SMI are still open (non-maskable
etc.) so perhaps prod it on the NMI vector?

This does seem to suggest the above code path wasn't actually tested.
Perhaps mark your local machine with BUG_MONITOR, remove the md->control
WRITE_ONCE() and try kexec to test it?

Thomas, any other thoughts?

Re: [PATCH] x86/cpu: Add INTEL_LUNARLAKE_M to X86_BUG_MONITOR

[Thomas Gleixner]

On Fri, Nov 08 2024 at 08:49, Len Brown wrote:
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index e7656cbef68d..aa63f5f780a0 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -586,7 +586,8 @@ static void init_intel(struct cpuinfo_x86 *c)
>  	     c->x86_vfm == INTEL_WESTMERE_EX))
>  		set_cpu_bug(c, X86_BUG_CLFLUSH_MONITOR);
>  
> -	if (boot_cpu_has(X86_FEATURE_MWAIT) && c->x86_vfm == INTEL_ATOM_GOLDMONT)
> +	if (boot_cpu_has(X86_FEATURE_MWAIT) &&
> +			(c->x86_vfm == INTEL_ATOM_GOLDMONT || c->x86_vfm == INTEL_LUNARLAKE_M))

This indentation is bogus.

>  		set_cpu_bug(c, X86_BUG_MONITOR);

> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 766f092dab80..910cb2d72c13 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1377,6 +1377,9 @@ void smp_kick_mwait_play_dead(void)
>  		for (i = 0; READ_ONCE(md->status) != newstate && i < 1000; i++) {
>  			/* Bring it out of mwait */
>  			WRITE_ONCE(md->control, newstate);
> +			/* If MONITOR unreliable, send IPI */
> +			if (boot_cpu_has_bug(X86_BUG_MONITOR))
> +				__apic_send_IPI(cpu, RESCHEDULE_VECTOR);

How is this supposed to work?

The local APIC of the offline CPU is shut down and only responds to
INIT, NMI, SMI, and SIPI.

Even if the APIC would react to the IPI, then the offline CPU would not
notice as is has interrupts disabled when it reaches mwait_play_dead().

Seriously?

Thanks,

        tglx

Re: [PATCH] x86/cpu: Add INTEL_LUNARLAKE_M to X86_BUG_MONITOR

[Thomas Gleixner]

On Mon, Nov 11 2024 at 17:23, Peter Zijlstra wrote:
> On Fri, Nov 08, 2024 at 08:49:31AM -0500, Len Brown wrote:
>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>> index 766f092dab80..910cb2d72c13 100644
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -1377,6 +1377,9 @@ void smp_kick_mwait_play_dead(void)
>>  		for (i = 0; READ_ONCE(md->status) != newstate && i < 1000; i++) {
>>  			/* Bring it out of mwait */
>>  			WRITE_ONCE(md->control, newstate);
>> +			/* If MONITOR unreliable, send IPI */
>> +			if (boot_cpu_has_bug(X86_BUG_MONITOR))
>> +				__apic_send_IPI(cpu, RESCHEDULE_VECTOR);
>>  			udelay(5);
>>  		}
>
> Going over that code again, mwait_play_dead() is doing __mwait(.exc=0)
> with IRQs disabled.

And the APIC is shut down. So it won't react on the IPI either.

> So that IPI you're trying to send there won't do no nothing :-/
>
> Now that comment there says MCE/NMI/SMI are still open (non-maskable
> etc.) so perhaps prod it on the NMI vector?
>
> This does seem to suggest the above code path wasn't actually tested.

I'm not sure whether that's just a suggestion :)

> Perhaps mark your local machine with BUG_MONITOR, remove the md->control
> WRITE_ONCE() and try kexec to test it?
>
> Thomas, any other thoughts?

NMI should work. See exc_nmi():

	if (arch_cpu_is_offline(smp_processor_id())) {
		if (microcode_nmi_handler_enabled())
			microcode_offline_nmi_handler();
		return;
	}

Thanks,

        tglx

Re: [PATCH backport to 6.10] x86/cpu: Add INTEL_FAM6_LUNARLAKE_M to X86_BUG_MONITOR

[Thomas Gleixner]

On Fri, Nov 08 2024 at 08:49, Len Brown wrote:
> From: Len Brown <len.brown@intel.com>
>
> Under some conditions, MONITOR wakeups on Lunar Lake processors
> can be lost, resulting in significant user-visible delays.
>
> Add LunarLake to X86_BUG_MONITOR so that wake_up_idle_cpu()
> always sends an IPI, avoiding this potential delay.
> Update the X86_BUG_MONITOR workaround to handle
> the new smp_kick_mwait_play_dead() path.
>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219364
>
> Cc: stable@vger.kernel.org # 6.10
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
> This is a backport of the upstream patch to Linux-6.10 and earlier

You either fail to understand or intentionally ignore the process for
stable backports, which is in place since more than a decade.

Documentation/process/* has plenty of information how that works and
you're around long enough to know that already.

I don't care about you wasting your time, but I very much care about you
wasting my time to deal with pointless emails. I get plenty enough of
them every day.

Thanks,

        tglx

[PATCH 0/1 v2] x86/cpu: Add INTEL_LUNARLAKE_M to X86_BUG_MONITOR

[Len Brown]

It was folly to respond to review feedback by attempting to add bits
to the kexec path to this simple patch.  So that misguided hunk is removed.

Here is the 1-line patch that addresses the problem that users see
in the field today.

If kexec proves to be an issue on LNL, that can be addressed
in a subsequent patch.

thanks,
-Len

[PATCH 1/1] x86/cpu: Add INTEL_LUNARLAKE_M to X86_BUG_MONITOR

[Len Brown]

From: Len Brown <len.brown@intel.com>

Under some conditions, MONITOR wakeups on Lunar Lake processors
can be lost, resulting in significant user-visible delays.

Add LunarLake to X86_BUG_MONITOR so that wake_up_idle_cpu()
always sends an IPI, avoiding this potential delay.

Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219364

Cc: stable@vger.kernel.org # 6.11
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index e7656cbef68d..284cd561499c 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -586,7 +586,9 @@ static void init_intel(struct cpuinfo_x86 *c)
 	     c->x86_vfm == INTEL_WESTMERE_EX))
 		set_cpu_bug(c, X86_BUG_CLFLUSH_MONITOR);
 
-	if (boot_cpu_has(X86_FEATURE_MWAIT) && c->x86_vfm == INTEL_ATOM_GOLDMONT)
+	if (boot_cpu_has(X86_FEATURE_MWAIT) &&
+	    (c->x86_vfm == INTEL_ATOM_GOLDMONT
+	     || c->x86_vfm == INTEL_LUNARLAKE_M))
 		set_cpu_bug(c, X86_BUG_MONITOR);
 
 #ifdef CONFIG_X86_64
-- 
2.43.0

Re: [PATCH 1/1] x86/cpu: Add INTEL_LUNARLAKE_M to X86_BUG_MONITOR

[Rafael J. Wysocki]

On Tue, Nov 12, 2024 at 6:37 AM Len Brown <lenb@kernel.org> wrote:
>
> From: Len Brown <len.brown@intel.com>
>
> Under some conditions, MONITOR wakeups on Lunar Lake processors
> can be lost, resulting in significant user-visible delays.
>
> Add LunarLake to X86_BUG_MONITOR so that wake_up_idle_cpu()
> always sends an IPI, avoiding this potential delay.
>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219364
>
> Cc: stable@vger.kernel.org # 6.11
> Signed-off-by: Len Brown <len.brown@intel.com>

So again

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

and see one super-minor nit below.

> ---
>  arch/x86/kernel/cpu/intel.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index e7656cbef68d..284cd561499c 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -586,7 +586,9 @@ static void init_intel(struct cpuinfo_x86 *c)
>              c->x86_vfm == INTEL_WESTMERE_EX))
>                 set_cpu_bug(c, X86_BUG_CLFLUSH_MONITOR);
>
> -       if (boot_cpu_has(X86_FEATURE_MWAIT) && c->x86_vfm == INTEL_ATOM_GOLDMONT)
> +       if (boot_cpu_has(X86_FEATURE_MWAIT) &&
> +           (c->x86_vfm == INTEL_ATOM_GOLDMONT
> +            || c->x86_vfm == INTEL_LUNARLAKE_M))

I would put the || at the end of the previous line, that is

> +           (c->x86_vfm == INTEL_ATOM_GOLDMONT ||
> +            c->x86_vfm == INTEL_LUNARLAKE_M))

>                 set_cpu_bug(c, X86_BUG_MONITOR);
>
>  #ifdef CONFIG_X86_64
> --
> 2.43.0
>

Re: [PATCH 1/1] x86/cpu: Add INTEL_LUNARLAKE_M to X86_BUG_MONITOR

[Rafael J. Wysocki]

On Tue, Nov 12, 2024 at 2:12 PM Len Brown <lenb@kernel.org> wrote:
>
> On Tue, Nov 12, 2024 at 6:44 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> > > -       if (boot_cpu_has(X86_FEATURE_MWAIT) && c->x86_vfm == INTEL_ATOM_GOLDMONT)
> > > +       if (boot_cpu_has(X86_FEATURE_MWAIT) &&
> > > +           (c->x86_vfm == INTEL_ATOM_GOLDMONT
> > > +            || c->x86_vfm == INTEL_LUNARLAKE_M))
> >
> > I would put the || at the end of the previous line, that is
>
>
> It isn't my personal preference for human readability either,
> but this is what scripts/Lindent does...

Well, it doesn't match the coding style of the first line ...

Re: [PATCH 1/1] x86/cpu: Add INTEL_LUNARLAKE_M to X86_BUG_MONITOR

[Rafael J. Wysocki]

On Tue, Nov 12, 2024 at 3:02 PM Len Brown <lenb@kernel.org> wrote:
>
> On Tue, Nov 12, 2024 at 8:14 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Nov 12, 2024 at 2:12 PM Len Brown <lenb@kernel.org> wrote:
> > >
> > > On Tue, Nov 12, 2024 at 6:44 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > > > -       if (boot_cpu_has(X86_FEATURE_MWAIT) && c->x86_vfm == INTEL_ATOM_GOLDMONT)
> > > > > +       if (boot_cpu_has(X86_FEATURE_MWAIT) &&
> > > > > +           (c->x86_vfm == INTEL_ATOM_GOLDMONT
> > > > > +            || c->x86_vfm == INTEL_LUNARLAKE_M))
> > > >
> > > > I would put the || at the end of the previous line, that is
> > >
> > >
> > > It isn't my personal preference for human readability either,
> > > but this is what scripts/Lindent does...
> >
> > Well, it doesn't match the coding style of the first line ...
>
> Fair observation.
>
> I'll bite.
>
> If you took the existing intel.c and added it as a patch to the kernel,
> the resulting checkpatch would have 6 errors and 33 warnings.
>
> If you ran Lindent on the existing intel.c, the resulting diff would be
> 408 lines --  1 file changed, 232 insertions(+), 176 deletions(-)
>
> This for a file that is only 1300 lines long.
>
> If whitespace nirvana is the goal, tools are the answer, not the valuable
> cycles of human reviewers.

Well, the advice always given is to follow the coding style of the
given fine in the first place.

checkpatch reflects the preferences of its author is this particular
respect and maintainers' preferences tend to differ from one to
another.