[PATCH v5 0/1] MIPS: Fix idle VS timer enqueue

[PATCH v5 0/1] MIPS: Fix idle VS timer enqueue

[Marco Crivellari]

This patch aims to fix idle routine while the CPU receive an interrupt,
because __r4k_wait() only checks if TIF_NEED_RESCHED is set before
going to sleep.
The same behavior has been changed in LoongArch [1].

Code (cross) compiled successfully and I manage to test it on a VM
emulating a malta board. I ran QEMU with:

qemu-system-mips64el -M malta -m 2G -kernel vmlinux -serial stdio -drive \
file=rootfs.ext2,format=raw -append "rootwait root=/dev/sda" -cpu 5Kc

rootfs generated using buildroot (malta default configuration).

- [1] https://github.com/chenhuacai/linux/commit/a8aa673ea46c03b3f62992ffa4ffe810ac84f6e3

---
Changes in v5:
 - comment: idle interrupt region, instead of rollback region

Changes in v4:
 - comments: 36 byte region

Changes in v3:
 - changed "daddiu k0, 1" with PTR_ADDIU k0, 5
 - replaced CONFIG_CPU_MICROMIPS with 3 _ssnop followed by _ehb
 - integrated the commit message with explanation about
   CONFIG_CPU_MICROMIPS replacement

Changes in v2:
 - Changes introduced by Huacai:
    https://lore.kernel.org/linux-mips/20250214105047.150835-1-marco.crivellari@suse.com/T/#m75d9c587829e15e0d7baec13078be4e65c936408

Marco Crivellari (1):
  MIPS: Fix idle VS timer enqueue

 arch/mips/kernel/genex.S | 42 ++++++++++++++++++++++------------------
 arch/mips/kernel/idle.c  |  1 -
 2 files changed, 23 insertions(+), 20 deletions(-)

-- 
2.48.1

[PATCH v5 1/1] MIPS: Fix idle VS timer enqueue

[Marco Crivellari]

MIPS re-enables interrupts on its idle routine and performs
a TIF_NEED_RESCHED check afterwards before putting the CPU to sleep.

The IRQs firing between the check and the 'wait' instruction may set the
TIF_NEED_RESCHED flag. In order to deal with this possible race, IRQs
interrupting __r4k_wait() rollback their return address to the
beginning of __r4k_wait() so that TIF_NEED_RESCHED is checked
again before going back to sleep.

However idle IRQs can also queue timers that may require a tick
reprogramming through a new generic idle loop iteration but those timers
would go unnoticed here because __r4k_wait() only checks
TIF_NEED_RESCHED. It doesn't check for pending timers.

Fix this with fast-forwarding idle IRQs return address to the end of the
idle routine instead of the beginning, so that the generic idle loop
handles both TIF_NEED_RESCHED and pending timers.

CONFIG_CPU_MICROMIPS has been removed along with the nop instructions.
There, NOPs are 2 byte in size, so change the code with 3 _ssnop which are
always 4 byte and remove the ifdef. Added ehb to make sure the hazard
is always cleared.

Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
---
 arch/mips/kernel/genex.S | 42 ++++++++++++++++++++++------------------
 arch/mips/kernel/idle.c  |  1 -
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
index a572ce36a24f..474738d9124e 100644
--- a/arch/mips/kernel/genex.S
+++ b/arch/mips/kernel/genex.S
@@ -104,27 +104,30 @@ handle_vcei:
 
 	__FINIT
 
-	.align	5	/* 32 byte rollback region */
+	.align	5
 LEAF(__r4k_wait)
 	.set	push
 	.set	noreorder
-	/* start of rollback region */
-	LONG_L	t0, TI_FLAGS($28)
-	nop
-	andi	t0, _TIF_NEED_RESCHED
-	bnez	t0, 1f
-	 nop
-	nop
-	nop
-#ifdef CONFIG_CPU_MICROMIPS
-	nop
-	nop
-	nop
-	nop
-#endif
+	/* start of idle interrupt region */
+	MFC0	t0, CP0_STATUS
+	/* Enable Interrput */
+	ori 	t0, 0x1f
+	xori	t0, 0x1e
+	MTC0	t0, CP0_STATUS
+	_ssnop
+	_ssnop
+	_ssnop
+	_ehb
 	.set	MIPS_ISA_ARCH_LEVEL_RAW
+	/*
+	 * If an interrupt lands here, between enabling interrupts above and
+	 * going idle on the next instruction, we must *NOT* go idle since the
+	 * interrupt could have set TIF_NEED_RESCHED or caused a timer to need
+	 * resched. Fall through -- see rollback_handler below -- and have
+	 * the idle loop take care of things.
+	 */
 	wait
-	/* end of rollback region (the region size must be power of two) */
+	/* end of idle interrupt region */
 1:
 	jr	ra
 	 nop
@@ -136,9 +139,10 @@ LEAF(__r4k_wait)
 	.set	push
 	.set	noat
 	MFC0	k0, CP0_EPC
-	PTR_LA	k1, __r4k_wait
-	ori	k0, 0x1f	/* 32 byte rollback region */
-	xori	k0, 0x1f
+	PTR_LA	k1, 1b
+	/* 36 byte idle interrupt region */
+	ori 	k0, 0x1f
+	PTR_ADDIU	k0, 5
 	bne	k0, k1, \handler
 	MTC0	k0, CP0_EPC
 	.set pop
diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c
index 5abc8b7340f8..1f74c0589f7e 100644
--- a/arch/mips/kernel/idle.c
+++ b/arch/mips/kernel/idle.c
@@ -37,7 +37,6 @@ static void __cpuidle r3081_wait(void)
 
 void __cpuidle r4k_wait(void)
 {
-	raw_local_irq_enable();
 	__r4k_wait();
 	raw_local_irq_disable();
 }
-- 
2.48.1

Re: [PATCH v5 1/1] MIPS: Fix idle VS timer enqueue

[Frederic Weisbecker]

Le Fri, Feb 28, 2025 at 11:05:09AM +0100, Marco Crivellari a écrit :
> MIPS re-enables interrupts on its idle routine and performs
> a TIF_NEED_RESCHED check afterwards before putting the CPU to sleep.
> 
> The IRQs firing between the check and the 'wait' instruction may set the
> TIF_NEED_RESCHED flag. In order to deal with this possible race, IRQs
> interrupting __r4k_wait() rollback their return address to the
> beginning of __r4k_wait() so that TIF_NEED_RESCHED is checked
> again before going back to sleep.
> 
> However idle IRQs can also queue timers that may require a tick
> reprogramming through a new generic idle loop iteration but those timers
> would go unnoticed here because __r4k_wait() only checks
> TIF_NEED_RESCHED. It doesn't check for pending timers.
> 
> Fix this with fast-forwarding idle IRQs return address to the end of the
> idle routine instead of the beginning, so that the generic idle loop
> handles both TIF_NEED_RESCHED and pending timers.
> 
> CONFIG_CPU_MICROMIPS has been removed along with the nop instructions.
> There, NOPs are 2 byte in size, so change the code with 3 _ssnop which are
> always 4 byte and remove the ifdef. Added ehb to make sure the hazard
> is always cleared.
> 
> Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>

Acked-by: Frederic Weisbecker <frederic@kernel.org>

Re: [PATCH v5 1/1] MIPS: Fix idle VS timer enqueue

[Maciej W. Rozycki]

On Fri, 28 Feb 2025, Marco Crivellari wrote:

> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> index a572ce36a24f..474738d9124e 100644
> --- a/arch/mips/kernel/genex.S
> +++ b/arch/mips/kernel/genex.S
> @@ -104,27 +104,30 @@ handle_vcei:
>  
>  	__FINIT
>  
> -	.align	5	/* 32 byte rollback region */
> +	.align	5
>  LEAF(__r4k_wait)
>  	.set	push
>  	.set	noreorder
> -	/* start of rollback region */
> -	LONG_L	t0, TI_FLAGS($28)
> -	nop
> -	andi	t0, _TIF_NEED_RESCHED
> -	bnez	t0, 1f
> -	 nop
> -	nop
> -	nop
> -#ifdef CONFIG_CPU_MICROMIPS
> -	nop
> -	nop
> -	nop
> -	nop
> -#endif
> +	/* start of idle interrupt region */
> +	MFC0	t0, CP0_STATUS
> +	/* Enable Interrput */
                        ^^
 Typo here; also please capitalise sentences and use full stops.

> +	ori 	t0, 0x1f

 No time for a thorough review here as I'm heading for a holiday right 
away, but I can see you still have a coprocessor move hazard here with 
MIPS III hardware.  The incoming value of $t0 to ORI is not what MFC0 has 
obtained.  It's the value from before MFC0.

> +	xori	t0, 0x1e

 And then it's only this XORI that sees the output from MFC0 (though 
there's actually a race between the two competing writes to $t0), so 
effectively you clobber the CP0.Status register...

> +	MTC0	t0, CP0_STATUS

 ... here.  You need to schedule your instructions differently.  But do 
you need `.set noreorder' in the first place?  Can you maybe find a way to 
avoid it, making all the hazard avoidance stuff much easier?

  Maciej

Re: [PATCH v5 1/1] MIPS: Fix idle VS timer enqueue

[Huacai Chen]

Hi, Maciej,

On Sun, Mar 2, 2025 at 8:54 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Fri, 28 Feb 2025, Marco Crivellari wrote:
>
> > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > index a572ce36a24f..474738d9124e 100644
> > --- a/arch/mips/kernel/genex.S
> > +++ b/arch/mips/kernel/genex.S
> > @@ -104,27 +104,30 @@ handle_vcei:
> >
> >       __FINIT
> >
> > -     .align  5       /* 32 byte rollback region */
> > +     .align  5
> >  LEAF(__r4k_wait)
> >       .set    push
> >       .set    noreorder
> > -     /* start of rollback region */
> > -     LONG_L  t0, TI_FLAGS($28)
> > -     nop
> > -     andi    t0, _TIF_NEED_RESCHED
> > -     bnez    t0, 1f
> > -      nop
> > -     nop
> > -     nop
> > -#ifdef CONFIG_CPU_MICROMIPS
> > -     nop
> > -     nop
> > -     nop
> > -     nop
> > -#endif
> > +     /* start of idle interrupt region */
> > +     MFC0    t0, CP0_STATUS
> > +     /* Enable Interrput */
>                         ^^
>  Typo here; also please capitalise sentences and use full stops.
>
> > +     ori     t0, 0x1f
>
>  No time for a thorough review here as I'm heading for a holiday right
> away, but I can see you still have a coprocessor move hazard here with
> MIPS III hardware.  The incoming value of $t0 to ORI is not what MFC0 has
> obtained.  It's the value from before MFC0.
If this is a problem, then the current local_irq_enable() is also
wrong for all MIPS III hardware, because this patch uses the same
instruction sequence of local_irq_enable().


Huacai

>
> > +     xori    t0, 0x1e
>
>  And then it's only this XORI that sees the output from MFC0 (though
> there's actually a race between the two competing writes to $t0), so
> effectively you clobber the CP0.Status register...
>
> > +     MTC0    t0, CP0_STATUS
>
>  ... here.  You need to schedule your instructions differently.  But do
> you need `.set noreorder' in the first place?  Can you maybe find a way to
> avoid it, making all the hazard avoidance stuff much easier?
>
>   Maciej

Re: [PATCH v5 1/1] MIPS: Fix idle VS timer enqueue

[Marco Crivellari]

Hi,

> +     /* Enable Interrput */
>                        ^^
> Typo here; also please capitalise sentences and use full stops.

Sorry, I probably forgot to run checkpatch this time.

>If this is a problem, then the current local_irq_enable() is also
>wrong for all MIPS III hardware, because this patch uses the same
>instruction sequence of local_irq_enable().

This is the doubt I have indeed.
Quoting from the manual (about M4K):

"The Spacing column shown in Table 2.6 and Table 2.7 indicates the
number of unrelated instructions (such as NOPs or SSNOPs) that,
prior to the capabilities of Release 2, would need to be placed
between the producer and consumer of the hazard in order to ensure
that the effects of the first instruction are seen by the second instruction."

The "Spacing column" value is 3, indeed.

"With the hazard elimination instructions available in Release 2, the
preferred method to eliminate hazards is to place one of the
instructions listed in Table 2.8 between the producer and consumer of the
hazard. Execution hazards can be removed by using the EHB [...]"

Not sure if I'm missing something here.

Thanks (to everyone)!

On Mon, Mar 3, 2025 at 9:13 AM Huacai Chen <chenhuacai@kernel.org> wrote:
>
> Hi, Maciej,
>
> On Sun, Mar 2, 2025 at 8:54 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
> >
> > On Fri, 28 Feb 2025, Marco Crivellari wrote:
> >
> > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > > index a572ce36a24f..474738d9124e 100644
> > > --- a/arch/mips/kernel/genex.S
> > > +++ b/arch/mips/kernel/genex.S
> > > @@ -104,27 +104,30 @@ handle_vcei:
> > >
> > >       __FINIT
> > >
> > > -     .align  5       /* 32 byte rollback region */
> > > +     .align  5
> > >  LEAF(__r4k_wait)
> > >       .set    push
> > >       .set    noreorder
> > > -     /* start of rollback region */
> > > -     LONG_L  t0, TI_FLAGS($28)
> > > -     nop
> > > -     andi    t0, _TIF_NEED_RESCHED
> > > -     bnez    t0, 1f
> > > -      nop
> > > -     nop
> > > -     nop
> > > -#ifdef CONFIG_CPU_MICROMIPS
> > > -     nop
> > > -     nop
> > > -     nop
> > > -     nop
> > > -#endif
> > > +     /* start of idle interrupt region */
> > > +     MFC0    t0, CP0_STATUS
> > > +     /* Enable Interrput */
> >                         ^^
> >  Typo here; also please capitalise sentences and use full stops.
> >
> > > +     ori     t0, 0x1f
> >
> >  No time for a thorough review here as I'm heading for a holiday right
> > away, but I can see you still have a coprocessor move hazard here with
> > MIPS III hardware.  The incoming value of $t0 to ORI is not what MFC0 has
> > obtained.  It's the value from before MFC0.
> If this is a problem, then the current local_irq_enable() is also
> wrong for all MIPS III hardware, because this patch uses the same
> instruction sequence of local_irq_enable().
>
>
> Huacai
>
> >
> > > +     xori    t0, 0x1e
> >
> >  And then it's only this XORI that sees the output from MFC0 (though
> > there's actually a race between the two competing writes to $t0), so
> > effectively you clobber the CP0.Status register...
> >
> > > +     MTC0    t0, CP0_STATUS
> >
> >  ... here.  You need to schedule your instructions differently.  But do
> > you need `.set noreorder' in the first place?  Can you maybe find a way to
> > avoid it, making all the hazard avoidance stuff much easier?
> >
> >   Maciej



--

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@suse.com