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

[PATCH v2 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 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 | 39 +++++++++++++++++++++------------------
 arch/mips/kernel/idle.c  |  1 -
 2 files changed, 21 insertions(+), 19 deletions(-)

-- 
2.48.1

[PATCH v2 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.

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

diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
index a572ce36a24f..9747b216648f 100644
--- a/arch/mips/kernel/genex.S
+++ b/arch/mips/kernel/genex.S
@@ -104,25 +104,27 @@ 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
 	.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) */
 1:
@@ -136,9 +138,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
+	/* 32 byte idle interrupt region */
+	ori 	k0, 0x1f
+	daddiu	k0, 1
 	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 v2 1/1] MIPS: Fix idle VS timer enqueue

[Thomas Bogendoerfer]

On Tue, Feb 18, 2025 at 10:02:03AM +0100, Marco Crivellari wrote:
> 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.
> 
> Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
> ---
>  arch/mips/kernel/genex.S | 39 +++++++++++++++++++++------------------
>  arch/mips/kernel/idle.c  |  1 -
>  2 files changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> index a572ce36a24f..9747b216648f 100644
> --- a/arch/mips/kernel/genex.S
> +++ b/arch/mips/kernel/genex.S
> @@ -104,25 +104,27 @@ 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

My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but
they are here for a purpose. I might still need them...

> +	/* start of idle interrupt region */
> +	MFC0	t0, CP0_STATUS
> +	/* Enable Interrput */
> +	ori 	t0, 0x1f
> +	xori	t0, 0x1e
> +	MTC0	t0, CP0_STATUS
> +	_ssnop
> +	_ssnop
> +	_ssnop

instead of handcoded hazard nops, use __irq_enable_hazard for that

>  	.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) */
>  1:
> @@ -136,9 +138,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
> +	/* 32 byte idle interrupt region */
> +	ori 	k0, 0x1f
> +	daddiu	k0, 1

/local/tbogendoerfer/korg/linux/arch/mips/kernel/genex.S: Assembler messages:
/local/tbogendoerfer/korg/linux/arch/mips/kernel/genex.S:151: Error: opcode not supported on this processor: mips32r2 (mips32r2) `daddiu $26,1'
/local/tbogendoerfer/korg/linux/arch/mips/kernel/genex.S:271: Error: opcode not supported on this processor: mips32r2 (mips32r2) `daddiu $26,1'

looks like you haven't compiled this code for 32bit. Use PTR_ADDIU, which
will use the correct instuction for 32 and 64bit.

But I doubt this works, because the wait instruction is not aligned to
a 32 byte boundary, but the code assuemes this, IMHO.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

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

[Huacai Chen]

Hi, Thomas,

On Tue, Feb 18, 2025 at 7:59 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Tue, Feb 18, 2025 at 10:02:03AM +0100, Marco Crivellari wrote:
> > 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.
> >
> > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
> > ---
> >  arch/mips/kernel/genex.S | 39 +++++++++++++++++++++------------------
> >  arch/mips/kernel/idle.c  |  1 -
> >  2 files changed, 21 insertions(+), 19 deletions(-)
> >
> > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > index a572ce36a24f..9747b216648f 100644
> > --- a/arch/mips/kernel/genex.S
> > +++ b/arch/mips/kernel/genex.S
> > @@ -104,25 +104,27 @@ 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
>
> My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but
> they are here for a purpose. I might still need them...
The original code needs #ifdef CONFIG_CPU_MICROMIPS because nop in
MICROMIPS is 2 bytes, so need another four nop to align. But _ssnop is
always 4 bytes, so we can remove #ifdefs.

>
> > +     /* start of idle interrupt region */
> > +     MFC0    t0, CP0_STATUS
> > +     /* Enable Interrput */
> > +     ori     t0, 0x1f
> > +     xori    t0, 0x1e
> > +     MTC0    t0, CP0_STATUS
> > +     _ssnop
> > +     _ssnop
> > +     _ssnop
>
> instead of handcoded hazard nops, use __irq_enable_hazard for that
No, I don't think so, this region should make sure be 32 bytes on each
platform, but __irq_enable_hazard is not consistent, 3 _ssnop is the
fallback implementation but available for all MIPS.

>
> >       .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) */
> >  1:
> > @@ -136,9 +138,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
> > +     /* 32 byte idle interrupt region */
> > +     ori     k0, 0x1f
> > +     daddiu  k0, 1
>
> /local/tbogendoerfer/korg/linux/arch/mips/kernel/genex.S: Assembler messages:
> /local/tbogendoerfer/korg/linux/arch/mips/kernel/genex.S:151: Error: opcode not supported on this processor: mips32r2 (mips32r2) `daddiu $26,1'
> /local/tbogendoerfer/korg/linux/arch/mips/kernel/genex.S:271: Error: opcode not supported on this processor: mips32r2 (mips32r2) `daddiu $26,1'
>
> looks like you haven't compiled this code for 32bit. Use PTR_ADDIU, which
> will use the correct instuction for 32 and 64bit.
Sorry, this is my mistake.

>
> But I doubt this works, because the wait instruction is not aligned to
> a 32 byte boundary, but the code assuemes this, IMHO.
Why? After this patch we only use 4 byte instructions.


Huacai
>
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

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

[Thomas Bogendoerfer]

On Tue, Feb 18, 2025 at 08:14:43PM +0800, Huacai Chen wrote:
> Hi, Thomas,
> 
> On Tue, Feb 18, 2025 at 7:59 PM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> >
> > On Tue, Feb 18, 2025 at 10:02:03AM +0100, Marco Crivellari wrote:
> > > 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.
> > >
> > > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
> > > ---
> > >  arch/mips/kernel/genex.S | 39 +++++++++++++++++++++------------------
> > >  arch/mips/kernel/idle.c  |  1 -
> > >  2 files changed, 21 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > > index a572ce36a24f..9747b216648f 100644
> > > --- a/arch/mips/kernel/genex.S
> > > +++ b/arch/mips/kernel/genex.S
> > > @@ -104,25 +104,27 @@ 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
> >
> > My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but
> > they are here for a purpose. I might still need them...
> The original code needs #ifdef CONFIG_CPU_MICROMIPS because nop in
> MICROMIPS is 2 bytes, so need another four nop to align. But _ssnop is
> always 4 bytes, so we can remove #ifdefs.

ic

> > > +     _ssnop
> > > +     _ssnop
> > > +     _ssnop
> >
> > instead of handcoded hazard nops, use __irq_enable_hazard for that
> No, I don't think so, this region should make sure be 32 bytes on each
> platform, but __irq_enable_hazard is not consistent, 3 _ssnop is the
> fallback implementation but available for all MIPS.

you are right for most cases, but there is one case

#elif (defined(CONFIG_CPU_MIPSR1) && !defined(CONFIG_MIPS_ALCHEMY)) || \
        defined(CONFIG_CPU_BMIPS)

which uses

#define __irq_enable_hazard                                             \
        ___ssnop;                                                       \
        ___ssnop;                                                       \
        ___ssnop;                                                       \
        ___ehb

if MIPSR1 || BMIPS needs "rollback" handler 3 ssnnop would be wrong as
irq enable hazard.

> > But I doubt this works, because the wait instruction is not aligned to
> > a 32 byte boundary, but the code assuemes this, IMHO.
> Why? After this patch we only use 4 byte instructions.

I've should have looked at the compiled output, sorry for the noise

Still this construct feels rather fragile.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

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

[Maciej W. Rozycki]

On Tue, 18 Feb 2025, Thomas Bogendoerfer wrote:

> > > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > > > index a572ce36a24f..9747b216648f 100644
> > > > --- a/arch/mips/kernel/genex.S
> > > > +++ b/arch/mips/kernel/genex.S
> > > > @@ -104,25 +104,27 @@ 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
> > >
> > > My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but
> > > they are here for a purpose. I might still need them...
> > The original code needs #ifdef CONFIG_CPU_MICROMIPS because nop in
> > MICROMIPS is 2 bytes, so need another four nop to align. But _ssnop is
> > always 4 bytes, so we can remove #ifdefs.
> 
> ic

 This was wrong anyway (and I had arguments about it with people back in 
the time, but it was a hopeless battle).  Instead `.align ...' or an 
equivalent pseudo-op (`.balign', etc.) should have been used, removing the 
fragility of this piece or the need for CONFIG_CPU_MICROMIPS conditional.  
Here and in other places.

> > > > +     _ssnop
> > > > +     _ssnop
> > > > +     _ssnop
> > >
> > > instead of handcoded hazard nops, use __irq_enable_hazard for that
> > No, I don't think so, this region should make sure be 32 bytes on each
> > platform, but __irq_enable_hazard is not consistent, 3 _ssnop is the
> > fallback implementation but available for all MIPS.
> 
> you are right for most cases, but there is one case
> 
> #elif (defined(CONFIG_CPU_MIPSR1) && !defined(CONFIG_MIPS_ALCHEMY)) || \
>         defined(CONFIG_CPU_BMIPS)
> 
> which uses
> 
> #define __irq_enable_hazard                                             \
>         ___ssnop;                                                       \
>         ___ssnop;                                                       \
>         ___ssnop;                                                       \
>         ___ehb
> 
> if MIPSR1 || BMIPS needs "rollback" handler 3 ssnnop would be wrong as
> irq enable hazard.

 Indeed, EHB must always be included, because for R2+ CPUs SSNOP doesn't 
clear the hazard (it never clears, but with earlier CPUs it just stalls 
the pipeline long enough for the hazard to eventually clear by itself).

> > > But I doubt this works, because the wait instruction is not aligned to
> > > a 32 byte boundary, but the code assuemes this, IMHO.
> > Why? After this patch we only use 4 byte instructions.
> 
> I've should have looked at the compiled output, sorry for the noise

 Umm, there's the commit description to document justification for such 
changes made and it's not the reviewer's duty to chase every such detail 
that has been omitted from a submission.

 FAOD this is a request to include this detail in v3.

> Still this construct feels rather fragile.

 I think `.align', etc. can still be used to ensure enough space has been 
consumed and if you want to make a code sequence's size check, then a 
piece such as:

0:
	nop
	nop
	nop
	nop
1:
	.ifne	1b - 0b - 32
	.error	"code consistency verification failure"
	.endif

should do even where alignment does not matter.  And you could possibly do 
smarter stuff with `.rept' if (1b - 0b) turns out below rather than above 
the threshold required, but I'm leaving it as an exercise for the reader.

  Maciej

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

[Huacai Chen]

On Tue, Feb 18, 2025 at 9:51 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Tue, Feb 18, 2025 at 08:14:43PM +0800, Huacai Chen wrote:
> > Hi, Thomas,
> >
> > On Tue, Feb 18, 2025 at 7:59 PM Thomas Bogendoerfer
> > <tsbogend@alpha.franken.de> wrote:
> > >
> > > On Tue, Feb 18, 2025 at 10:02:03AM +0100, Marco Crivellari wrote:
> > > > 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.
> > > >
> > > > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
> > > > ---
> > > >  arch/mips/kernel/genex.S | 39 +++++++++++++++++++++------------------
> > > >  arch/mips/kernel/idle.c  |  1 -
> > > >  2 files changed, 21 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > > > index a572ce36a24f..9747b216648f 100644
> > > > --- a/arch/mips/kernel/genex.S
> > > > +++ b/arch/mips/kernel/genex.S
> > > > @@ -104,25 +104,27 @@ 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
> > >
> > > My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but
> > > they are here for a purpose. I might still need them...
> > The original code needs #ifdef CONFIG_CPU_MICROMIPS because nop in
> > MICROMIPS is 2 bytes, so need another four nop to align. But _ssnop is
> > always 4 bytes, so we can remove #ifdefs.
>
> ic
>
> > > > +     _ssnop
> > > > +     _ssnop
> > > > +     _ssnop
> > >
> > > instead of handcoded hazard nops, use __irq_enable_hazard for that
> > No, I don't think so, this region should make sure be 32 bytes on each
> > platform, but __irq_enable_hazard is not consistent, 3 _ssnop is the
> > fallback implementation but available for all MIPS.
>
> you are right for most cases, but there is one case
>
> #elif (defined(CONFIG_CPU_MIPSR1) && !defined(CONFIG_MIPS_ALCHEMY)) || \
>         defined(CONFIG_CPU_BMIPS)
>
> which uses
>
> #define __irq_enable_hazard                                             \
>         ___ssnop;                                                       \
>         ___ssnop;                                                       \
>         ___ssnop;                                                       \
>         ___ehb
>
> if MIPSR1 || BMIPS needs "rollback" handler 3 ssnnop would be wrong as
> irq enable hazard.
Emm, this is a problem. I think we can add _ehb after 3 _ssnop. And
then change the below "daddiu k0, 1" to "PTR_ADDIU k0, 5".

Maybe there is a better solution, but I think this is the simplest.

Huacai

>
> > > But I doubt this works, because the wait instruction is not aligned to
> > > a 32 byte boundary, but the code assuemes this, IMHO.
> > Why? After this patch we only use 4 byte instructions.
>
> I've should have looked at the compiled output, sorry for the noise
>
> Still this construct feels rather fragile.
>
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

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

[Marco Crivellari]

Hi,

Sorry for the late reply.

> Umm, there's the commit description to document justification for such
>changes made and it's not the reviewer's duty to chase every such detail
>that has been omitted from a submission.
>
 >FAOD this is a request to include this detail in v3.

How does it sound integrate the v3 commit message with:

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.

something more accurate to suggest?

Thank you

On Tue, Feb 18, 2025 at 4:23 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Tue, 18 Feb 2025, Thomas Bogendoerfer wrote:
>
> > > > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > > > > index a572ce36a24f..9747b216648f 100644
> > > > > --- a/arch/mips/kernel/genex.S
> > > > > +++ b/arch/mips/kernel/genex.S
> > > > > @@ -104,25 +104,27 @@ 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
> > > >
> > > > My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but
> > > > they are here for a purpose. I might still need them...
> > > The original code needs #ifdef CONFIG_CPU_MICROMIPS because nop in
> > > MICROMIPS is 2 bytes, so need another four nop to align. But _ssnop is
> > > always 4 bytes, so we can remove #ifdefs.
> >
> > ic
>
>  This was wrong anyway (and I had arguments about it with people back in
> the time, but it was a hopeless battle).  Instead `.align ...' or an
> equivalent pseudo-op (`.balign', etc.) should have been used, removing the
> fragility of this piece or the need for CONFIG_CPU_MICROMIPS conditional.
> Here and in other places.
>
> > > > > +     _ssnop
> > > > > +     _ssnop
> > > > > +     _ssnop
> > > >
> > > > instead of handcoded hazard nops, use __irq_enable_hazard for that
> > > No, I don't think so, this region should make sure be 32 bytes on each
> > > platform, but __irq_enable_hazard is not consistent, 3 _ssnop is the
> > > fallback implementation but available for all MIPS.
> >
> > you are right for most cases, but there is one case
> >
> > #elif (defined(CONFIG_CPU_MIPSR1) && !defined(CONFIG_MIPS_ALCHEMY)) || \
> >         defined(CONFIG_CPU_BMIPS)
> >
> > which uses
> >
> > #define __irq_enable_hazard                                             \
> >         ___ssnop;                                                       \
> >         ___ssnop;                                                       \
> >         ___ssnop;                                                       \
> >         ___ehb
> >
> > if MIPSR1 || BMIPS needs "rollback" handler 3 ssnnop would be wrong as
> > irq enable hazard.
>
>  Indeed, EHB must always be included, because for R2+ CPUs SSNOP doesn't
> clear the hazard (it never clears, but with earlier CPUs it just stalls
> the pipeline long enough for the hazard to eventually clear by itself).
>
> > > > But I doubt this works, because the wait instruction is not aligned to
> > > > a 32 byte boundary, but the code assuemes this, IMHO.
> > > Why? After this patch we only use 4 byte instructions.
> >
> > I've should have looked at the compiled output, sorry for the noise
>
>  Umm, there's the commit description to document justification for such
> changes made and it's not the reviewer's duty to chase every such detail
> that has been omitted from a submission.
>
>  FAOD this is a request to include this detail in v3.
>
> > Still this construct feels rather fragile.
>
>  I think `.align', etc. can still be used to ensure enough space has been
> consumed and if you want to make a code sequence's size check, then a
> piece such as:
>
> 0:
>         nop
>         nop
>         nop
>         nop
> 1:
>         .ifne   1b - 0b - 32
>         .error  "code consistency verification failure"
>         .endif
>
> should do even where alignment does not matter.  And you could possibly do
> smarter stuff with `.rept' if (1b - 0b) turns out below rather than above
> the threshold required, but I'm leaving it as an exercise for the reader.
>
>   Maciej



-- 

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@suse.com