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

[PATCH v6 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 v6:
 - typo in comment, capitalise sentences and full stops

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 v6 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..4e012421d00f 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 interrupt. */
+	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 v6 1/1] MIPS: Fix idle VS timer enqueue

[Thomas Bogendoerfer]

On Sat, Mar 15, 2025 at 08:40:02PM +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.

can you give a commit ID, when this change got introduced ?

> 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..4e012421d00f 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 interrupt. */
> +	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:

please give this label a name for example __r4k_wait_exit and do a
runtime check that it really has 36 bytes offset to __r4k_wait

>  	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

this is part of a macro, so I don't think using a commonly used label name
is a safe thing, that's why I want a named label here.

> +	/* 36 byte idle interrupt region. */
> +	ori 	k0, 0x1f
> +	PTR_ADDIU	k0, 5
>  	bne	k0, k1, \handler
>  	MTC0	k0, CP0_EPC
>  	.set pop

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

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

[Frederic Weisbecker]

Le Wed, Mar 19, 2025 at 12:07:07PM +0100, Thomas Bogendoerfer a écrit :
> On Sat, Mar 15, 2025 at 08:40:02PM +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.
> 
> can you give a commit ID, when this change got introduced ?

That would be:

     Fixes: c65a5480ff29 ("[MIPS] Fix potential latency problem due to non-atomic cpu_wait.")

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

[Thomas Bogendoerfer]

On Wed, Mar 19, 2025 at 03:06:12PM +0100, Frederic Weisbecker wrote:
> Le Wed, Mar 19, 2025 at 12:07:07PM +0100, Thomas Bogendoerfer a écrit :
> > On Sat, Mar 15, 2025 at 08:40:02PM +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.
> > 
> > can you give a commit ID, when this change got introduced ?
> 
> That would be:
> 
>      Fixes: c65a5480ff29 ("[MIPS] Fix potential latency problem due to non-atomic cpu_wait.")

hmm, so even then checking TIF_NEED_RESCHED wasn't enough  (we are talking
about 2.6.27) ?

Thomas.

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

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

[Frederic Weisbecker]

Le Wed, Mar 19, 2025 at 03:42:16PM +0100, Thomas Bogendoerfer a écrit :
> On Wed, Mar 19, 2025 at 03:06:12PM +0100, Frederic Weisbecker wrote:
> > Le Wed, Mar 19, 2025 at 12:07:07PM +0100, Thomas Bogendoerfer a écrit :
> > > On Sat, Mar 15, 2025 at 08:40:02PM +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.
> > > 
> > > can you give a commit ID, when this change got introduced ?
> > 
> > That would be:
> > 
> >      Fixes: c65a5480ff29 ("[MIPS] Fix potential latency problem due to non-atomic cpu_wait.")
> 
> hmm, so even then checking TIF_NEED_RESCHED wasn't enough  (we are talking
> about 2.6.27) ?

Right, the real issue dates even back earlier because the aforementioned fix only
handled the TIF_NEED_RESCHED part of the problem but the timer part of the problem
was there before.

However nohz was introduced only earlier the same year (2.6.21). We probably
don't need to dig much further...

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

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

[Frederic Weisbecker]

Le Wed, Mar 19, 2025 at 12:07:07PM +0100, Thomas Bogendoerfer a écrit :
> On Sat, Mar 15, 2025 at 08:40:02PM +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.
> 
> can you give a commit ID, when this change got introduced ?
> 
> > 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..4e012421d00f 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 interrupt. */
> > +	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:
> 
> please give this label a name for example __r4k_wait_exit and do a
> runtime check that it really has 36 bytes offset to __r4k_wait

Where would be the best place for that?

arch/mips/kernel/setup.c:setup_arch() maybe?

Thanks.

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

[Thomas Bogendoerfer]

On Wed, Mar 19, 2025 at 03:43:13PM +0100, Frederic Weisbecker wrote:
> > >  	.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:
> > 
> > please give this label a name for example __r4k_wait_exit and do a
> > runtime check that it really has 36 bytes offset to __r4k_wait
> 
> Where would be the best place for that?
> 
> arch/mips/kernel/setup.c:setup_arch() maybe?

scratch runtime check, a compile check is what I wanted to write...

something like

.if ((__r4k_wait_exit - __r4k_wait) != 36)
.err
.endif

Thomas.

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

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

[Marco Crivellari]

Hi,

yesterday I made this changes:

@@ -128,7 +128,11 @@ LEAF(__r4k_wait)
        */
       wait
       /* End of idle interrupt region. */
-1:
+SYM_INNER_LABEL(__r4k_wait_exit, SYM_L_LOCAL)
+       /* Check idle interrupt region size. */
+       .ifne   __r4k_wait_exit - __r4k_wait - 36
+       .error  "Idle interrupt region size mismatch: expected 36 bytes."
+       .endif
       jr      ra
        nop
       .set    pop
@@ -139,7 +143,7 @@ LEAF(__r4k_wait)
       .set    push
       .set    noat
       MFC0    k0, CP0_EPC
-       PTR_LA  k1, 1b
+       PTR_LA  k1, __r4k_wait_exit
       /* 36 byte idle interrupt region. */
       ori     k0, 0x1f
       PTR_ADDIU       k0, 5


Would it be ok to have the check done in this way?

Thanks


On Wed, Mar 19, 2025 at 11:09 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Wed, Mar 19, 2025 at 03:43:13PM +0100, Frederic Weisbecker wrote:
> > > >   .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:
> > >
> > > please give this label a name for example __r4k_wait_exit and do a
> > > runtime check that it really has 36 bytes offset to __r4k_wait
> >
> > Where would be the best place for that?
> >
> > arch/mips/kernel/setup.c:setup_arch() maybe?
>
> scratch runtime check, a compile check is what I wanted to write...
>
> something like
>
> .if ((__r4k_wait_exit - __r4k_wait) != 36)
> .err
> .endif
>
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]



-- 

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@suse.com

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

[Thomas Bogendoerfer]

On Thu, Mar 20, 2025 at 09:44:23AM +0100, Marco Crivellari wrote:
> Hi,
> 
> yesterday I made this changes:
> 
> @@ -128,7 +128,11 @@ LEAF(__r4k_wait)
>         */
>        wait
>        /* End of idle interrupt region. */
> -1:
> +SYM_INNER_LABEL(__r4k_wait_exit, SYM_L_LOCAL)

do we really need that for a symbol local to that file ?

> +       /* Check idle interrupt region size. */
> +       .ifne   __r4k_wait_exit - __r4k_wait - 36

I have to say, that I prefer my .if statement, since this clearly spells out
the comparision in the expression. Is there a reason for your version ?

Thomas.

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

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

[Marco Crivellari]

On Fri, Mar 21, 2025 at 10:38 AM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
> do we really need that for a symbol local to that file ?

Not really, I can use the label directly.

On Fri, Mar 21, 2025 at 10:38 AM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> > +       /* Check idle interrupt region size. */
> > +       .ifne   __r4k_wait_exit - __r4k_wait - 36
>
> I have to say, that I prefer my .if statement, since this clearly spells out
> the comparision in the expression. Is there a reason for your version ?

Sure, let's keep your version.
Can we use the "error" message above? (""Idle interrupt region size
mismatch: expected 36 bytes."").
Or at least something similar, I think it is easier to understand the
reason for the error. What do you think?

Thanks!

-- 

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@suse.com

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

[Thomas Bogendoerfer]

On Fri, Mar 21, 2025 at 11:44:10AM +0100, Marco Crivellari wrote:
> On Fri, Mar 21, 2025 at 10:38 AM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> > do we really need that for a symbol local to that file ?
> 
> Not really, I can use the label directly.
> 
> On Fri, Mar 21, 2025 at 10:38 AM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> >
> > > +       /* Check idle interrupt region size. */
> > > +       .ifne   __r4k_wait_exit - __r4k_wait - 36
> >
> > I have to say, that I prefer my .if statement, since this clearly spells out
> > the comparision in the expression. Is there a reason for your version ?
> 
> Sure, let's keep your version.
> Can we use the "error" message above? (""Idle interrupt region size
> mismatch: expected 36 bytes."").
> Or at least something similar, I think it is easier to understand the
> reason for the error. What do you think?

the error message works for me :-)

Thomas.

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

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

[Maciej W. Rozycki]

On Fri, 21 Mar 2025, Thomas Bogendoerfer wrote:

> > > > +       /* Check idle interrupt region size. */
> > > > +       .ifne   __r4k_wait_exit - __r4k_wait - 36
> > >
> > > I have to say, that I prefer my .if statement, since this clearly spells out
> > > the comparision in the expression. Is there a reason for your version ?
> > 
> > Sure, let's keep your version.
> > Can we use the "error" message above? (""Idle interrupt region size
> > mismatch: expected 36 bytes."").
> > Or at least something similar, I think it is easier to understand the
> > reason for the error. What do you think?
> 
> the error message works for me :-)

 I'm glad seeing this being sorted properly, thank you!

  Maciej

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

[Maciej W. Rozycki]

On Sat, 15 Mar 2025, Marco Crivellari wrote:

> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> index a572ce36a24f..4e012421d00f 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 interrupt. */
> +	ori 	t0, 0x1f

 This instruction sequence still suffers from the coprocessor move delay 
hazard.  How many times do I need to request to get it fixed (counting 
three so far)?

  Maciej

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

[Marco Crivellari]

Hi Maciej,

On Fri, Mar 21, 2025 at 12:53 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>  This instruction sequence still suffers from the coprocessor move delay
> hazard.  How many times do I need to request to get it fixed (counting
> three so far)?

Can I have more details about this?

I can see it is the same code present also in local_irq_enable()
(arch_local_irq_enable()),
and from the manual I've seen:

"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 [...]"

What am I missing?

Thanks in advance


--

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@suse.com

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

[Maciej W. Rozycki]

On Fri, 21 Mar 2025, Marco Crivellari wrote:

> >  This instruction sequence still suffers from the coprocessor move delay
> > hazard.  How many times do I need to request to get it fixed (counting
> > three so far)?
> 
> Can I have more details about this?
> 
> I can see it is the same code present also in local_irq_enable()
> (arch_local_irq_enable()),

 Unlike `__r4k_wait' that code is not in a `.set noreorder' region and 
the assembler will therefore resolve the hazard by inserting a NOP where 
required by the architecture level requested (with `-march=...', etc.).  
Try compiling that function for a MIPS III configuration such as 
decstation_r4k_defconfig or just by hand with `-march=mips3' and see 
what machine code is produced.

> and from the manual I've seen:
> 
> "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 [...]"

 Whatever manual you quote it refers to MIPS Release 2, which is only 
dated 2003 (following Release 1 from 2001), but `__r4k_wait' has to 
continue handling older architecture revisions going back to MIPS III 
ISA from 1991.  We also support MIPS I ISA from 1985 which has further 
instruction scheduling requirements, but `__r4k_wait' is for newer 
processors only, because older ones had no WAIT instruction, so we can 
ignore them (but note that the MIPS I load delay slot is regardless 
observed in current code in the form of a NOP inserted after LONG_L).

> What am I missing?

 This is common MIPS knowledge really, encoded in the GNU toolchain and 
especially GAS since forever.  While I can't cite a canonical reference, 
the hazard is listed e.g. in Table 13.1 "Instructions with scheduling 
implications" and Table 13.3 "R4xxx/R5000 Coprocessor 0 Hazards" from 
"IDT MIPS Microprocessor Family Software Reference Manual," Version 2.0, 
from October 1996.  I do believe the document is available online.

 I'm fairly sure the hazard is also listed there in Dominic Sweetman's 
"See MIPS Run Linux," but I don't have my copy handy right now.

 Best is to avoid using a `.set noreorder' region in the first place.  
But is it really needed here?  Does the rollback area have to be of a 
hardcoded size rather than one calculated by the assembler based on 
actual machine code produced?  It seems to me having it calculated would 
reduce complexity here and let us use the EI instruction where available 
as well.

 HTH,

  Maciej

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

[Marco Crivellari]

Hi Maciej,

Thanks a lot for all the information.

>  Unlike `__r4k_wait' that code is not in a `.set noreorder' region and
> the assembler will therefore resolve the hazard by inserting a NOP where
> required by the architecture level requested (with `-march=...', etc.).
> Try compiling that function for a MIPS III configuration such as
> decstation_r4k_defconfig or just by hand with `-march=mips3' and see
> what machine code is produced.

I tried with the configuration you suggested, and indeed I can see a NOP.

>  Whatever manual you quote it refers to MIPS Release 2, which is only
> dated 2003

About the MIPS manual, anyhow, it is "MIPS32 M4 Processor Core" (year 2008).
Maybe I've also picked the wrong manual.

I've also found the manual you mentioned online, thanks.

>  Best is to avoid using a `.set noreorder' region in the first place.
> But is it really needed here?  Does the rollback area have to be of a
> hardcoded size rather than one calculated by the assembler based on
> actual machine code produced?  It seems to me having it calculated would
> reduce complexity here and let us use the EI instruction where available
> as well.

Well, considering the complexity and how the code looks fragile even with
a small change, yes, that's likely better to avoid noreorder.

I think I'm going to need some guidance here.
Please, correct me where something is wrong.

1)
When you say "let us use the EI instruction where available" are you
referring to do
something like below?

#if defined(CONFIG_CPU_HAS_DIEI)
ei
#else
MFC0    t0, CP0_STATUS
ori     t0, 0x1f
xori    t0, 0x1e
MTC0    t0, CP0_STATUS
#endif

2)
Removing "noreorder" would let the compiler add "nops" where they are needed.
But that still means the 3 ssnop and ehb are still needed, right?

My subsequent dumb question is: there is the guarantee that the
compiler will not
reorder / change something we did?
This question also came after reading the manual you quoted (paragraph
"Coprocessor Hazards"):

"For example, after an mtc0 to the Status register which changes an
interrupt mask bit,
there will be two further instructions before the interrupt is
actually enabled or disabled.
[...]
To cope with these situations usually requires the programmer to take explicit
action to prevent the assembler from scheduling inappropriate
instructions after a
dangerous mtc0. This is done by using the .set noreorder directive,
discussed below"

3)
Considering the size is determined by the compiler, the check about
the idle interrupt
size region should not be needed, correct?

4)
ori and PTR_ADDIU should be removed of course from the rollback handler macro.
Can I have some hints about the needed change?
Using QEMU is always working, so I'm not sure if what I will change is
also correct.


Many thanks in advance, also for your time!




On Fri, Mar 21, 2025 at 9:11 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Fri, 21 Mar 2025, Marco Crivellari wrote:
>
> > >  This instruction sequence still suffers from the coprocessor move delay
> > > hazard.  How many times do I need to request to get it fixed (counting
> > > three so far)?
> >
> > Can I have more details about this?
> >
> > I can see it is the same code present also in local_irq_enable()
> > (arch_local_irq_enable()),
>
>  Unlike `__r4k_wait' that code is not in a `.set noreorder' region and
> the assembler will therefore resolve the hazard by inserting a NOP where
> required by the architecture level requested (with `-march=...', etc.).
> Try compiling that function for a MIPS III configuration such as
> decstation_r4k_defconfig or just by hand with `-march=mips3' and see
> what machine code is produced.
>
> > and from the manual I've seen:
> >
> > "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 [...]"
>
>  Whatever manual you quote it refers to MIPS Release 2, which is only
> dated 2003 (following Release 1 from 2001), but `__r4k_wait' has to
> continue handling older architecture revisions going back to MIPS III
> ISA from 1991.  We also support MIPS I ISA from 1985 which has further
> instruction scheduling requirements, but `__r4k_wait' is for newer
> processors only, because older ones had no WAIT instruction, so we can
> ignore them (but note that the MIPS I load delay slot is regardless
> observed in current code in the form of a NOP inserted after LONG_L).
>
> > What am I missing?
>
>  This is common MIPS knowledge really, encoded in the GNU toolchain and
> especially GAS since forever.  While I can't cite a canonical reference,
> the hazard is listed e.g. in Table 13.1 "Instructions with scheduling
> implications" and Table 13.3 "R4xxx/R5000 Coprocessor 0 Hazards" from
> "IDT MIPS Microprocessor Family Software Reference Manual," Version 2.0,
> from October 1996.  I do believe the document is available online.
>
>  I'm fairly sure the hazard is also listed there in Dominic Sweetman's
> "See MIPS Run Linux," but I don't have my copy handy right now.
>
>  Best is to avoid using a `.set noreorder' region in the first place.
> But is it really needed here?  Does the rollback area have to be of a
> hardcoded size rather than one calculated by the assembler based on
> actual machine code produced?  It seems to me having it calculated would
> reduce complexity here and let us use the EI instruction where available
> as well.
>
>  HTH,
>
>   Maciej



--

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@suse.com

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

[Huacai Chen]

On Tue, Mar 25, 2025 at 10:09 PM Marco Crivellari
<marco.crivellari@suse.com> wrote:
>
> Hi Maciej,
>
> Thanks a lot for all the information.
>
> >  Unlike `__r4k_wait' that code is not in a `.set noreorder' region and
> > the assembler will therefore resolve the hazard by inserting a NOP where
> > required by the architecture level requested (with `-march=...', etc.).
> > Try compiling that function for a MIPS III configuration such as
> > decstation_r4k_defconfig or just by hand with `-march=mips3' and see
> > what machine code is produced.
>
> I tried with the configuration you suggested, and indeed I can see a NOP.
>
> >  Whatever manual you quote it refers to MIPS Release 2, which is only
> > dated 2003
>
> About the MIPS manual, anyhow, it is "MIPS32 M4 Processor Core" (year 2008).
> Maybe I've also picked the wrong manual.
>
> I've also found the manual you mentioned online, thanks.
>
> >  Best is to avoid using a `.set noreorder' region in the first place.
> > But is it really needed here?  Does the rollback area have to be of a
> > hardcoded size rather than one calculated by the assembler based on
> > actual machine code produced?  It seems to me having it calculated would
> > reduce complexity here and let us use the EI instruction where available
> > as well.
>
> Well, considering the complexity and how the code looks fragile even with
> a small change, yes, that's likely better to avoid noreorder.
In my opinion keeping "noreorder" is the simplest, which means just
add an "nop" after MFC0 in the current version.

Huacai

>
> I think I'm going to need some guidance here.
> Please, correct me where something is wrong.
>
> 1)
> When you say "let us use the EI instruction where available" are you
> referring to do
> something like below?
>
> #if defined(CONFIG_CPU_HAS_DIEI)
> ei
> #else
> MFC0    t0, CP0_STATUS
> ori     t0, 0x1f
> xori    t0, 0x1e
> MTC0    t0, CP0_STATUS
> #endif
>
> 2)
> Removing "noreorder" would let the compiler add "nops" where they are needed.
> But that still means the 3 ssnop and ehb are still needed, right?
>
> My subsequent dumb question is: there is the guarantee that the
> compiler will not
> reorder / change something we did?
> This question also came after reading the manual you quoted (paragraph
> "Coprocessor Hazards"):
>
> "For example, after an mtc0 to the Status register which changes an
> interrupt mask bit,
> there will be two further instructions before the interrupt is
> actually enabled or disabled.
> [...]
> To cope with these situations usually requires the programmer to take explicit
> action to prevent the assembler from scheduling inappropriate
> instructions after a
> dangerous mtc0. This is done by using the .set noreorder directive,
> discussed below"
>
> 3)
> Considering the size is determined by the compiler, the check about
> the idle interrupt
> size region should not be needed, correct?
>
> 4)
> ori and PTR_ADDIU should be removed of course from the rollback handler macro.
> Can I have some hints about the needed change?
> Using QEMU is always working, so I'm not sure if what I will change is
> also correct.
>
>
> Many thanks in advance, also for your time!
>
>
>
>
> On Fri, Mar 21, 2025 at 9:11 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
> >
> > On Fri, 21 Mar 2025, Marco Crivellari wrote:
> >
> > > >  This instruction sequence still suffers from the coprocessor move delay
> > > > hazard.  How many times do I need to request to get it fixed (counting
> > > > three so far)?
> > >
> > > Can I have more details about this?
> > >
> > > I can see it is the same code present also in local_irq_enable()
> > > (arch_local_irq_enable()),
> >
> >  Unlike `__r4k_wait' that code is not in a `.set noreorder' region and
> > the assembler will therefore resolve the hazard by inserting a NOP where
> > required by the architecture level requested (with `-march=...', etc.).
> > Try compiling that function for a MIPS III configuration such as
> > decstation_r4k_defconfig or just by hand with `-march=mips3' and see
> > what machine code is produced.
> >
> > > and from the manual I've seen:
> > >
> > > "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 [...]"
> >
> >  Whatever manual you quote it refers to MIPS Release 2, which is only
> > dated 2003 (following Release 1 from 2001), but `__r4k_wait' has to
> > continue handling older architecture revisions going back to MIPS III
> > ISA from 1991.  We also support MIPS I ISA from 1985 which has further
> > instruction scheduling requirements, but `__r4k_wait' is for newer
> > processors only, because older ones had no WAIT instruction, so we can
> > ignore them (but note that the MIPS I load delay slot is regardless
> > observed in current code in the form of a NOP inserted after LONG_L).
> >
> > > What am I missing?
> >
> >  This is common MIPS knowledge really, encoded in the GNU toolchain and
> > especially GAS since forever.  While I can't cite a canonical reference,
> > the hazard is listed e.g. in Table 13.1 "Instructions with scheduling
> > implications" and Table 13.3 "R4xxx/R5000 Coprocessor 0 Hazards" from
> > "IDT MIPS Microprocessor Family Software Reference Manual," Version 2.0,
> > from October 1996.  I do believe the document is available online.
> >
> >  I'm fairly sure the hazard is also listed there in Dominic Sweetman's
> > "See MIPS Run Linux," but I don't have my copy handy right now.
> >
> >  Best is to avoid using a `.set noreorder' region in the first place.
> > But is it really needed here?  Does the rollback area have to be of a
> > hardcoded size rather than one calculated by the assembler based on
> > actual machine code produced?  It seems to me having it calculated would
> > reduce complexity here and let us use the EI instruction where available
> > as well.
> >
> >  HTH,
> >
> >   Maciej
>
>
>
> --
>
> Marco Crivellari
>
> L3 Support Engineer, Technology & Product
>
>
>
>
> marco.crivellari@suse.com

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

[Marco Crivellari]

I'm mostly thinking about future changes in this part of the code.
But if it is ok what we have now, and future changes are not a
problem, let's keep this version.

Would this be ok with you @Maciej?

If so, the region is now 40 bytes. This is what I did yesterday:

@@ -110,6 +110,7 @@ LEAF(__r4k_wait)
       .set    noreorder
       /* Start of idle interrupt region. */
       MFC0    t0, CP0_STATUS
+       nop
       /* Enable interrupt. */
       ori     t0, 0x1f
       xori    t0, 0x1e
@@ -128,7 +129,11 @@ LEAF(__r4k_wait)
        */
       wait
       /* End of idle interrupt region. */
-1:
+__r4k_wait_exit:
+       /* Check idle interrupt region size. */
+       .if ((__r4k_wait_exit - __r4k_wait) != 40)
+       .error  "Idle interrupt region size mismatch: expected 40 bytes."
+       .endif
       jr      ra
        nop
       .set    pop
@@ -139,10 +144,10 @@ LEAF(__r4k_wait)
       .set    push
       .set    noat
       MFC0    k0, CP0_EPC
-       PTR_LA  k1, 1b
-       /* 36 byte idle interrupt region. */
+       PTR_LA  k1, __r4k_wait_exit
+       /* 40 byte idle interrupt region. */
       ori     k0, 0x1f
-       PTR_ADDIU       k0, 5
+       PTR_ADDIU       k0, 9
       bne     k0, k1, \handler
       MTC0    k0, CP0_EPC
       .set pop

Under QEMU is working, but I'm not so sure the latest part is correct.

Thanks!



On Wed, Mar 26, 2025 at 2:20 AM Huacai Chen <chenhuacai@kernel.org> wrote:
>
> On Tue, Mar 25, 2025 at 10:09 PM Marco Crivellari
> <marco.crivellari@suse.com> wrote:
> >
> > Hi Maciej,
> >
> > Thanks a lot for all the information.
> >
> > >  Unlike `__r4k_wait' that code is not in a `.set noreorder' region and
> > > the assembler will therefore resolve the hazard by inserting a NOP where
> > > required by the architecture level requested (with `-march=...', etc.).
> > > Try compiling that function for a MIPS III configuration such as
> > > decstation_r4k_defconfig or just by hand with `-march=mips3' and see
> > > what machine code is produced.
> >
> > I tried with the configuration you suggested, and indeed I can see a NOP.
> >
> > >  Whatever manual you quote it refers to MIPS Release 2, which is only
> > > dated 2003
> >
> > About the MIPS manual, anyhow, it is "MIPS32 M4 Processor Core" (year 2008).
> > Maybe I've also picked the wrong manual.
> >
> > I've also found the manual you mentioned online, thanks.
> >
> > >  Best is to avoid using a `.set noreorder' region in the first place.
> > > But is it really needed here?  Does the rollback area have to be of a
> > > hardcoded size rather than one calculated by the assembler based on
> > > actual machine code produced?  It seems to me having it calculated would
> > > reduce complexity here and let us use the EI instruction where available
> > > as well.
> >
> > Well, considering the complexity and how the code looks fragile even with
> > a small change, yes, that's likely better to avoid noreorder.
> In my opinion keeping "noreorder" is the simplest, which means just
> add an "nop" after MFC0 in the current version.
>
> Huacai
>
> >
> > I think I'm going to need some guidance here.
> > Please, correct me where something is wrong.
> >
> > 1)
> > When you say "let us use the EI instruction where available" are you
> > referring to do
> > something like below?
> >
> > #if defined(CONFIG_CPU_HAS_DIEI)
> > ei
> > #else
> > MFC0    t0, CP0_STATUS
> > ori     t0, 0x1f
> > xori    t0, 0x1e
> > MTC0    t0, CP0_STATUS
> > #endif
> >
> > 2)
> > Removing "noreorder" would let the compiler add "nops" where they are needed.
> > But that still means the 3 ssnop and ehb are still needed, right?
> >
> > My subsequent dumb question is: there is the guarantee that the
> > compiler will not
> > reorder / change something we did?
> > This question also came after reading the manual you quoted (paragraph
> > "Coprocessor Hazards"):
> >
> > "For example, after an mtc0 to the Status register which changes an
> > interrupt mask bit,
> > there will be two further instructions before the interrupt is
> > actually enabled or disabled.
> > [...]
> > To cope with these situations usually requires the programmer to take explicit
> > action to prevent the assembler from scheduling inappropriate
> > instructions after a
> > dangerous mtc0. This is done by using the .set noreorder directive,
> > discussed below"
> >
> > 3)
> > Considering the size is determined by the compiler, the check about
> > the idle interrupt
> > size region should not be needed, correct?
> >
> > 4)
> > ori and PTR_ADDIU should be removed of course from the rollback handler macro.
> > Can I have some hints about the needed change?
> > Using QEMU is always working, so I'm not sure if what I will change is
> > also correct.
> >
> >
> > Many thanks in advance, also for your time!
> >
> >
> >
> >
> > On Fri, Mar 21, 2025 at 9:11 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
> > >
> > > On Fri, 21 Mar 2025, Marco Crivellari wrote:
> > >
> > > > >  This instruction sequence still suffers from the coprocessor move delay
> > > > > hazard.  How many times do I need to request to get it fixed (counting
> > > > > three so far)?
> > > >
> > > > Can I have more details about this?
> > > >
> > > > I can see it is the same code present also in local_irq_enable()
> > > > (arch_local_irq_enable()),
> > >
> > >  Unlike `__r4k_wait' that code is not in a `.set noreorder' region and
> > > the assembler will therefore resolve the hazard by inserting a NOP where
> > > required by the architecture level requested (with `-march=...', etc.).
> > > Try compiling that function for a MIPS III configuration such as
> > > decstation_r4k_defconfig or just by hand with `-march=mips3' and see
> > > what machine code is produced.
> > >
> > > > and from the manual I've seen:
> > > >
> > > > "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 [...]"
> > >
> > >  Whatever manual you quote it refers to MIPS Release 2, which is only
> > > dated 2003 (following Release 1 from 2001), but `__r4k_wait' has to
> > > continue handling older architecture revisions going back to MIPS III
> > > ISA from 1991.  We also support MIPS I ISA from 1985 which has further
> > > instruction scheduling requirements, but `__r4k_wait' is for newer
> > > processors only, because older ones had no WAIT instruction, so we can
> > > ignore them (but note that the MIPS I load delay slot is regardless
> > > observed in current code in the form of a NOP inserted after LONG_L).
> > >
> > > > What am I missing?
> > >
> > >  This is common MIPS knowledge really, encoded in the GNU toolchain and
> > > especially GAS since forever.  While I can't cite a canonical reference,
> > > the hazard is listed e.g. in Table 13.1 "Instructions with scheduling
> > > implications" and Table 13.3 "R4xxx/R5000 Coprocessor 0 Hazards" from
> > > "IDT MIPS Microprocessor Family Software Reference Manual," Version 2.0,
> > > from October 1996.  I do believe the document is available online.
> > >
> > >  I'm fairly sure the hazard is also listed there in Dominic Sweetman's
> > > "See MIPS Run Linux," but I don't have my copy handy right now.
> > >
> > >  Best is to avoid using a `.set noreorder' region in the first place.
> > > But is it really needed here?  Does the rollback area have to be of a
> > > hardcoded size rather than one calculated by the assembler based on
> > > actual machine code produced?  It seems to me having it calculated would
> > > reduce complexity here and let us use the EI instruction where available
> > > as well.
> > >
> > >  HTH,
> > >
> > >   Maciej
> >
> >
> >
> > --
> >
> > Marco Crivellari
> >
> > L3 Support Engineer, Technology & Product
> >
> >
> >
> >
> > marco.crivellari@suse.com



--

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@suse.com

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

[Thomas Bogendoerfer]

On Wed, Mar 26, 2025 at 10:46:08AM +0100, Marco Crivellari wrote:
> I'm mostly thinking about future changes in this part of the code.
> But if it is ok what we have now, and future changes are not a
> problem, let's keep this version.
> 
> Would this be ok with you @Maciej?
> 
> If so, the region is now 40 bytes. This is what I did yesterday:
> 
> @@ -110,6 +110,7 @@ LEAF(__r4k_wait)
>        .set    noreorder
>        /* Start of idle interrupt region. */
>        MFC0    t0, CP0_STATUS
> +       nop
>        /* Enable interrupt. */
>        ori     t0, 0x1f
>        xori    t0, 0x1e
> @@ -128,7 +129,11 @@ LEAF(__r4k_wait)
>         */
>        wait
>        /* End of idle interrupt region. */
> -1:
> +__r4k_wait_exit:
> +       /* Check idle interrupt region size. */
> +       .if ((__r4k_wait_exit - __r4k_wait) != 40)
> +       .error  "Idle interrupt region size mismatch: expected 40 bytes."
> +       .endif
>        jr      ra
>         nop
>        .set    pop
> @@ -139,10 +144,10 @@ LEAF(__r4k_wait)
>        .set    push
>        .set    noat
>        MFC0    k0, CP0_EPC
> -       PTR_LA  k1, 1b
> -       /* 36 byte idle interrupt region. */
> +       PTR_LA  k1, __r4k_wait_exit
> +       /* 40 byte idle interrupt region. */

IMHO, we can't extend this above 36 bytes, because the interrupt could
hit at the instruction before the wait, which is then in the
next 32byte block.

I was thinking about aligning __r4k_wait 16 byte earlier and place
the wait at the end of the 

Something like:

	.align 5
	.align 4
LEAF(__r4k_wait)
	// irq enable sequence
	wait // align to end of the 32byte block
_r4k_wait_exit:

>        ori     k0, 0x1f
> -       PTR_ADDIU       k0, 5
> +       PTR_ADDIU       k0, 9

this needs to
	PTR_ADDIU       k0, 1

then. and __r4k_wait_exit - __r4k_wait is 48

But there might better ways...

Thomas.

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

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

[Maciej W. Rozycki]

On Wed, 26 Mar 2025, Marco Crivellari wrote:

> I'm mostly thinking about future changes in this part of the code.
> But if it is ok what we have now, and future changes are not a
> problem, let's keep this version.
> 
> Would this be ok with you @Maciej?

 Nope, I think it's the wrong direction and I have a longer reply in the 
queue, but I yet need some time to complete it.  I just got off the plane 
having been away from home for a month and I have other stuff to do first.  
If it was broken for ~30 years, it can wait a couple days yet.

  Maciej

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

[Maciej W. Rozycki]

Hi Marco,

> >  Whatever manual you quote it refers to MIPS Release 2, which is only
> > dated 2003
> 
> About the MIPS manual, anyhow, it is "MIPS32 M4 Processor Core" (year 2008).
> Maybe I've also picked the wrong manual.

 It has to be M4K there, and that isn't the most fortunate choice, because 
it's a manual for the specific microarchitecture and then one that doesn't 
usually run Linux, because it has no TLB.  For MIPS Release 1 onwards the 
architecture manuals are a better choice, but still they don't necessarily 
get the details right for older legacy MIPS ISA revisions.

 For your reference the architecture manuals are:

- "MIPS32 Architecture For Programmers, Volume I: Introduction to the 
   MIPS32 Architecture",

- "MIPS32 Architecture For Programmers, Volume II: The MIPS32 Instruction 
   Set",

- "MIPS32 Architecture For Programmers, Volume III: The MIPS32 Privileged 
   Resource Architecture",

and their MIPS64 counterparts, released repeatedly as the architecture 
evolved with the document's major revision number matching the respective 
architecture release.  I do believe the most recent revisions continue 
being available from the MIPS web site.  But as the titles imply these 
manuals document the architecture and not the intricacies of the MIPS 
assembly language dialect or the various aspects of programming for the 
architecture.  For that you need another book.

> >  Best is to avoid using a `.set noreorder' region in the first place.
> > But is it really needed here?  Does the rollback area have to be of a
> > hardcoded size rather than one calculated by the assembler based on
> > actual machine code produced?  It seems to me having it calculated would
> > reduce complexity here and let us use the EI instruction where available
> > as well.
> 
> Well, considering the complexity and how the code looks fragile even with
> a small change, yes, that's likely better to avoid noreorder.

 FAOD this is my general observation, regardless of its applicability 
here.

> I think I'm going to need some guidance here.
> Please, correct me where something is wrong.
> 
> 1)
> When you say "let us use the EI instruction where available" are you
> referring to do
> something like below?
> 
> #if defined(CONFIG_CPU_HAS_DIEI)
> ei
> #else
> MFC0    t0, CP0_STATUS
> ori     t0, 0x1f
> xori    t0, 0x1e
> MTC0    t0, CP0_STATUS
> #endif

 I think let's just simplify this stuff a lot and make use of the existing 
code infrastructure.  Since we move interrupt enabling into `__r4k_wait', 
we can just get rid of the C wrapper the existence of which makes no sense 
anymore along with the extra nesting of function calls, and have something 
along the lines of:

	.section .cpuidle.text,"ax"
	/* Align to 32 bytes for the maximum idle interrupt region size.  */
	.align	5
LEAF(r4k_wait)
	/* Keep the ISA bit clear for calculations on local labels here.  */
0:	.fill	0
	/* Start of idle interrupt region.  */
	local_irq_enable
	/*
	 * If an interrupt lands here, before 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.
	 */
1:	.fill	0
	/* The R2 EI/EHB sequence takes 8 bytes, otherwise pad up.  */
	.if	1b - 0b > 32
	.error	"overlong idle interrupt region"
	.elseif	1b - 0b > 8
	.align	4
	.endif
2:	.fill	0
	.equ	r4k_wait_idle_size, 2b - 0b
	/* End of idle interrupt region; size has to be a power of 2.  */
	.set	MIPS_ISA_ARCH_LEVEL_RAW
r4k_wait_insn:
	wait
	.set	mips0
	local_irq_disable
	jr	ra
	END(r4k_wait)
	.previous

	.macro	BUILD_ROLLBACK_PROLOGUE handler
	FEXPORT(rollback_\handler)
	.set	push
	.set	noat
	MFC0	k0, CP0_EPC
	/* Subtract 2 to let the ISA bit propagate through the mask.  */
	PTR_LA	k1, r4k_wait_insn - 2
	ori	k0, r4k_wait_idle_size - 2
	bne	k0, k1, \handler
	MTC0	k0, CP0_EPC
	.set pop
	.endm

There's some complication here coming from the need to factor in the ISA 
bit in the microMIPS mode; something that hasn't been discussed so far.  
The `.fill 0' approach is a hack and it has struck me that we need to add 
a `.noinsn' pseudo-op to GAS for this purpose, complementing `.insn', but 
we need to stick with the hack for now anyway as it will take years until 
we can rely on a new feature in the assembler.

 NB don't refer to a local label from a macro as such a reference may end 
up pointing not where you want it to from the place the macro is pasted 
at.  For example with your v6 code BUILD_ROLLBACK_PROLOGUE refers to the 
intended label in `__r4k_wait' when pasted for `handle_int', but then to 
the label in `except_vec4' instead when pasted for `except_vec_vi' further 
down.

> 2)
> Removing "noreorder" would let the compiler add "nops" where they are needed.

 The assembler, not the compiler.

> But that still means the 3 ssnop and ehb are still needed, right?

 Yes, in the pessimistic case, which the code above avoids where not 
needed.  E.g. for R2 we now have:

807f1700 <r4k_wait>:
807f1700:	41606020 	ei
807f1704:	000000c0 	ehb

807f1708 <r4k_wait_insn>:
807f1708:	42000020 	wait
807f170c:	41606000 	di
807f1710:	000000c0 	ehb
807f1714:	03e00008 	jr	ra
807f1718:	00000000 	nop
	...

-- nice and sweet, and for the R10k likewise:

a80000000084e540 <r4k_wait>:
a80000000084e540:	400c6000 	mfc0	t0,$12
a80000000084e544:	358c0001 	ori	t0,t0,0x1
a80000000084e548:	408c6000 	mtc0	t0,$12
a80000000084e54c:	00000000 	nop

a80000000084e550 <r4k_wait_insn>:
a80000000084e550:	42000020 	wait
a80000000084e554: 	400c6000 	mfc0	t0,$12
a80000000084e558: 	358c0001 	ori	t0,t0,0x1
a80000000084e55c: 	398c0001 	xori	t0,t0,0x1
a80000000084e560: 	408c6000 	mtc0	t0,$12
a80000000084e564: 	03e00008 	jr	ra
a80000000084e568: 	00000000 	nop
	...

-- because it's fully interlocked, so no extra NOPs other than to pad the 
idle interrupt region to a power-of-two size.

> My subsequent dumb question is: there is the guarantee that the
> compiler will not
> reorder / change something we did?

 Not in this case; the assembler isn't that smart (which is why compiled 
code is usually a better choice where feasible) and except for one case 
all it can do is adding extra NOPs between instructions to avoid pipeline 
hazards (and then ones coming from data dependencies in register use 
only), including ones between non-adjacent instruction pairs.

 The only exception are jumps/branches where the assembler will schedule 
their delay slot where possible by swapping the jump/branch with the 
immediately preceding instruction where no data dependency exists between 
the two instructions.  Otherwise the delay slot will be filled with a NOP.

 For example with the code sequences above the last instruction produced 
by `local_irq_disable' could be scheduled into the delay slot of `jr ra'.  
It wouldn't be an issue here however and it doesn't actually happen with 
GAS, likely because `local_irq_disable' is a user macro.  For built-in 
instruction macros such as `la' the swapping of the final instruction does 
happen.

> This question also came after reading the manual you quoted (paragraph
> "Coprocessor Hazards"):
> 
> "For example, after an mtc0 to the Status register which changes an
> interrupt mask bit,
> there will be two further instructions before the interrupt is
> actually enabled or disabled.
> [...]
> To cope with these situations usually requires the programmer to take explicit
> action to prevent the assembler from scheduling inappropriate
> instructions after a
> dangerous mtc0. This is done by using the .set noreorder directive,
> discussed below"

 Technically it is correct and likely the original MIPSCO assembler from 
1985 or one supplied with IRIX were smarter, but GAS won't itself ever 
reorder instructions other than to fill branch delay slots, so we don't 
have to be worried.  Here's the relevant comment from GAS sources:

      /* There are a lot of optimizations we could do that we don't.
	 In particular, we do not, in general, reorder instructions.
	 If you use gcc with optimization, it will reorder
	 instructions and generally do much more optimization then we
	 do here; repeating all that work in the assembler would only
	 benefit hand written assembly code, and does not seem worth
	 it.  */

And in cases like this where we just don't want a bunch of instructions to 
be moved across a certain point we don't actually have to schedule any 
code by hand as a lone:

	.set	noreorder
	.set	reorder

barrier will do (or actually just a label should as well).

> 3)
> Considering the size is determined by the compiler, the check about
> the idle interrupt
> size region should not be needed, correct?

 I gave it some thought and concluded that the interrupt handling path has 
to be optimised for performance and the idle routine does not.  Therefore 
I think we need to stick to the power-of-two size for the idle interrupt 
region, because in that case the check for an interrupt arriving within 
`r4k_wait' but ahead of the WAIT instruction can be done with a pair of 
ALU operations and a single branch.  Anything more complex would require 
more operations in the interrupt handling path.

> 4)
> ori and PTR_ADDIU should be removed of course from the rollback handler macro.

 I can't imagine how we'd advance past WAIT without these instructions, 
what do you have in mind?

> Can I have some hints about the needed change?
> Using QEMU is always working, so I'm not sure if what I will change is
> also correct.

 Any verification with real hardware can only be based on the probability 
of an interrupt arriving at the right time and the right conditions for a 
pending timer.  I'd expect such an event to be relatively rare, so for the 
large part I think we need to rely on correct code generation rather than 
run-time verification.  Making use of the existing infrastructure rather 
than having an ad-hoc handcoded variant improves robustness here and also 
means no change will likely be needed should any further platform variant 
of the various hazard resolution macros be added in the future.

 NB how do you actually verify this stuff with QEMU?  Is it by injecting 
an interrupt by hand at a chosen code location via GDB attached to QEMU's 
built-in debug stub?

> Many thanks in advance, also for your time!

 You're welcome.

 Below I've included a complete change based on the outline above.  It 
seems to do the right thing for a couple of my configurations, but I've 
only eyeballed the resulting code and haven't tried running it.  Most of 
my hardware doesn't implement the WAIT instruction anyway.

 The missing `.cpuidle.text' section assignment is a separate preexisting 
problem and the fix might be worth splitting off into a preparatory patch, 
for backporting or documentation purposes.

 You can add my:

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>

on top of yours when using this code, since it's still ultimately yours, 
just massaged a little.  FWIW I went ahead and chose to cook this piece up 
myself since I realised how complex this issue actually is and that it 
would take us forever to get all the individual aspects nailed over e-mail 
communication.

 Let me know if you find anything here unclear or have any questions or 
comments.

  Maciej

---
 arch/mips/include/asm/idle.h |    3 --
 arch/mips/kernel/genex.S     |   59 ++++++++++++++++++++++++-------------------
 arch/mips/kernel/idle.c      |    7 -----
 3 files changed, 34 insertions(+), 35 deletions(-)

linux-mips-idle-vs-timer.diff
Index: linux-macro/arch/mips/include/asm/idle.h
===================================================================
--- linux-macro.orig/arch/mips/include/asm/idle.h
+++ linux-macro/arch/mips/include/asm/idle.h
@@ -6,8 +6,7 @@
 #include <linux/linkage.h>
 
 extern void (*cpu_wait)(void);
-extern void r4k_wait(void);
-extern asmlinkage void __r4k_wait(void);
+extern asmlinkage void r4k_wait(void);
 extern void r4k_wait_irqoff(void);
 
 static inline int using_rollback_handler(void)
Index: linux-macro/arch/mips/kernel/genex.S
===================================================================
--- linux-macro.orig/arch/mips/kernel/genex.S
+++ linux-macro/arch/mips/kernel/genex.S
@@ -104,41 +104,48 @@ NESTED(except_vec3_r4000, 0, sp)
 
 	__FINIT
 
-	.align	5	/* 32 byte rollback region */
-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
+	.section .cpuidle.text,"ax"
+	/* Align to 32 bytes for the maximum idle interrupt region size.  */
+	.align	5
+LEAF(r4k_wait)
+	/* Keep the ISA bit clear for calculations on local labels here.  */
+0:	.fill	0
+	/* Start of idle interrupt region.  */
+	local_irq_enable
+	/*
+	 * If an interrupt lands here, before 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.
+	 */
+1:	.fill	0
+	/* The R2 EI/EHB sequence takes 8 bytes, otherwise pad up.  */
+	.if	1b - 0b > 32
+	.error	"overlong idle interrupt region"
+	.elseif	1b - 0b > 8
+	.align	4
+	.endif
+2:	.fill	0
+	.equ	r4k_wait_idle_size, 2b - 0b
+	/* End of idle interrupt region; size has to be a power of 2.  */
 	.set	MIPS_ISA_ARCH_LEVEL_RAW
+r4k_wait_insn:
 	wait
-	/* end of rollback region (the region size must be power of two) */
-1:
+	.set	mips0
+	local_irq_disable
 	jr	ra
-	 nop
-	.set	pop
-	END(__r4k_wait)
+	END(r4k_wait)
+	.previous
 
 	.macro	BUILD_ROLLBACK_PROLOGUE handler
 	FEXPORT(rollback_\handler)
 	.set	push
 	.set	noat
 	MFC0	k0, CP0_EPC
-	PTR_LA	k1, __r4k_wait
-	ori	k0, 0x1f	/* 32 byte rollback region */
-	xori	k0, 0x1f
+	/* Subtract 2 to let the ISA bit propagate through the mask.  */
+	PTR_LA	k1, r4k_wait_insn - 2
+	ori	k0, r4k_wait_idle_size - 2
 	bne	k0, k1, \handler
 	MTC0	k0, CP0_EPC
 	.set pop
Index: linux-macro/arch/mips/kernel/idle.c
===================================================================
--- linux-macro.orig/arch/mips/kernel/idle.c
+++ linux-macro/arch/mips/kernel/idle.c
@@ -35,13 +35,6 @@ static void __cpuidle r3081_wait(void)
 	write_c0_conf(cfg | R30XX_CONF_HALT);
 }
 
-void __cpuidle r4k_wait(void)
-{
-	raw_local_irq_enable();
-	__r4k_wait();
-	raw_local_irq_disable();
-}
-
 /*
  * This variant is preferable as it allows testing need_resched and going to
  * sleep depending on the outcome atomically.  Unfortunately the "It is

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

[Maciej W. Rozycki]

On Fri, 28 Mar 2025, Maciej W. Rozycki wrote:

> just massaged a little.  FWIW I went ahead and chose to cook this piece up 
> myself since I realised how complex this issue actually is and that it 
> would take us forever to get all the individual aspects nailed over e-mail 
> communication.

 And yet after this many of internal iterations I did manage to miss one 
bit.  In the optimised version proposed we need to explicitly skip over 
the WAIT instruction like this:

r4k_wait_insn:
	wait
r4k_wait_exit:

and then:

	.set	noreorder
	bne	k0, k1, \handler
	 PTR_ADDIU	k0, r4k_wait_exit - r4k_wait_insn + 2
	.set	reorder

(and here we have a legitimate use for `.set noreorder' to avoid wasting a 
NOP for the branch delay slot due to a data antidependency on $k0; it's 
fine to clobber $k0 in the branch-taken case as by definition the register 
is dead at the exit from this macro).

 Updated patch follows.  I think we also need to replace "rollback" with 
another name as with new code we don't roll back anymore.

  Maciej

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
 arch/mips/include/asm/idle.h |    3 --
 arch/mips/kernel/genex.S     |   63 +++++++++++++++++++++++++------------------
 arch/mips/kernel/idle.c      |    7 ----
 3 files changed, 38 insertions(+), 35 deletions(-)

linux-mips-idle-vs-timer.diff
Index: linux-macro/arch/mips/include/asm/idle.h
===================================================================
--- linux-macro.orig/arch/mips/include/asm/idle.h
+++ linux-macro/arch/mips/include/asm/idle.h
@@ -6,8 +6,7 @@
 #include <linux/linkage.h>
 
 extern void (*cpu_wait)(void);
-extern void r4k_wait(void);
-extern asmlinkage void __r4k_wait(void);
+extern asmlinkage void r4k_wait(void);
 extern void r4k_wait_irqoff(void);
 
 static inline int using_rollback_handler(void)
Index: linux-macro/arch/mips/kernel/genex.S
===================================================================
--- linux-macro.orig/arch/mips/kernel/genex.S
+++ linux-macro/arch/mips/kernel/genex.S
@@ -104,42 +104,53 @@ NESTED(except_vec3_r4000, 0, sp)
 
 	__FINIT
 
-	.align	5	/* 32 byte rollback region */
-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
+	.section .cpuidle.text,"ax"
+	/* Align to 32 bytes for the maximum idle interrupt region size.  */
+	.align	5
+LEAF(r4k_wait)
+	/* Keep the ISA bit clear for calculations on local labels here.  */
+0:	.fill	0
+	/* Start of idle interrupt region.  */
+	local_irq_enable
+	/*
+	 * If an interrupt lands here, before 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.
+	 */
+1:	.fill	0
+	/* The R2 EI/EHB sequence takes 8 bytes, otherwise pad up.  */
+	.if	1b - 0b > 32
+	.error	"overlong idle interrupt region"
+	.elseif	1b - 0b > 8
+	.align	4
+	.endif
+2:	.fill	0
+	.equ	r4k_wait_idle_size, 2b - 0b
+	/* End of idle interrupt region; size has to be a power of 2.  */
 	.set	MIPS_ISA_ARCH_LEVEL_RAW
+r4k_wait_insn:
 	wait
-	/* end of rollback region (the region size must be power of two) */
-1:
+r4k_wait_exit:
+	.set	mips0
+	local_irq_disable
 	jr	ra
-	 nop
-	.set	pop
-	END(__r4k_wait)
+	END(r4k_wait)
+	.previous
 
 	.macro	BUILD_ROLLBACK_PROLOGUE handler
 	FEXPORT(rollback_\handler)
 	.set	push
 	.set	noat
 	MFC0	k0, CP0_EPC
-	PTR_LA	k1, __r4k_wait
-	ori	k0, 0x1f	/* 32 byte rollback region */
-	xori	k0, 0x1f
+	/* Subtract/add 2 to let the ISA bit propagate through the mask.  */
+	PTR_LA	k1, r4k_wait_insn - 2
+	ori	k0, r4k_wait_idle_size - 2
+	.set	noreorder
 	bne	k0, k1, \handler
+	 PTR_ADDIU	k0, r4k_wait_exit - r4k_wait_insn + 2
+	.set	reorder
 	MTC0	k0, CP0_EPC
 	.set pop
 	.endm
Index: linux-macro/arch/mips/kernel/idle.c
===================================================================
--- linux-macro.orig/arch/mips/kernel/idle.c
+++ linux-macro/arch/mips/kernel/idle.c
@@ -35,13 +35,6 @@ static void __cpuidle r3081_wait(void)
 	write_c0_conf(cfg | R30XX_CONF_HALT);
 }
 
-void __cpuidle r4k_wait(void)
-{
-	raw_local_irq_enable();
-	__r4k_wait();
-	raw_local_irq_disable();
-}
-
 /*
  * This variant is preferable as it allows testing need_resched and going to
  * sleep depending on the outcome atomically.  Unfortunately the "It is

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

[Marco Crivellari]

Hi Maciej,

>It has to be M4K there, and that isn't the most fortunate choice, because
>it's a manual for the specific microarchitecture and then one that doesn't
>usually run Linux, because it has no TLB.

Ah... my bad. I've also searched the manuals name you gave me.
I had one of them, but a specific volume:

"MIPS Architecture For Programmers Volume II-A: The MIPS64 Instruction
Set Reference Manual"

> There's some complication here coming from the need to factor in the ISA
> bit in the microMIPS mode; something that hasn't been discussed so far.
> The `.fill 0' approach is a hack and it has struck me that we need to add
> a `.noinsn' pseudo-op to GAS for this purpose, complementing `.insn', but
> we need to stick with the hack for now anyway as it will take years until
> we can rely on a new feature in the assembler.

Ah, interesting. So de facto having the "repeat" to 0, will generate nothing.

> NB don't refer to a local label from a macro as such a reference may end
> up pointing not where you want it to from the place the macro is pasted
> at.  For example with your v6 code BUILD_ROLLBACK_PROLOGUE refers to the
> intended label in `__r4k_wait' when pasted for `handle_int', but then to
> the label in `except_vec4' instead when pasted for `except_vec_vi' further
> down.

> Technically it is correct and likely the original MIPSCO assembler from
> 1985 or one supplied with IRIX were smarter, but GAS won't itself ever
> reorder instructions other than to fill branch delay slots, so we don't
> have to be worried.

Thank you!

> I gave it some thought and concluded that the interrupt handling path has
> to be optimised for performance and the idle routine does not.  Therefore
> I think we need to stick to the power-of-two size for the idle interrupt
> region, because in that case the check for an interrupt arriving within
>` r4k_wait' but ahead of the WAIT instruction can be done with a pair of
> ALU operations and a single branch.  Anything more complex would require
> more operations in the interrupt handling path.

Clear, thanks.

> I can't imagine how we'd advance past WAIT without these instructions,
> what do you have in mind?

I've not been precise, sorry.
I meant to remove the instructions like they are now because the
region would have
been different, then.
So, those instructions would have needed a change, in practice.

> NB how do you actually verify this stuff with QEMU?  Is it by injecting
> an interrupt by hand at a chosen code location via GDB attached to QEMU's
> built-in debug stub?

Short answer: I am not able to fully test this, I can only boot.

The reason is that gdb-multiarch is not working as expected.
The binary in my distribution has the python support broken. So when I try to
inject the interrupt, I'm receiving a python error (actually, I receive the same
error after the "target remote" command).
I've also tried to compile the binary from source, but again,
I understand why in OBS the build I found is broken...

> Below I've included a complete change based on the outline above.

Well, thanks a lot for that.

> Below I've included a complete change based on the outline above.  It
> seems to do the right thing for a couple of my configurations, but I've
> only eyeballed the resulting code and haven't tried running it.  Most of
> my hardware doesn't implement the WAIT instruction anyway.

It means it would be better to have someone else try the resulting
patch, I guess.

Once I have your final thoughts about the questions / observations below,
I will submit the V7 with all the changes.

> Let me know if you find anything here unclear or have any questions or
> comments.

1)

> /* Keep the ISA bit clear for calculations on local labels here. */

The ISA bit should be the bit 0, correct?
So, also in the macro code, it's done to preserve that bit.

2)
.section .cpuidle.text,"ax"

This should be a single patch, right?
Based on what I understood, 'a' should be the allocation, and 'x' the
executable attribute.
This should be in order to mark those symbols like "cpuidle text":

$ nm -n vmlinux | grep -A3 "cpuidle_text"
ffffffff80a127e0 T __cpuidle_text_start
ffffffff80a127e0 T r4k_wait
ffffffff80a12800 t r4k_wait_insn
ffffffff80a12804 t r4k_wait_exit
--
ffffffff80a12c00 T __cpuidle_text_end
ffffffff80a12c00 T __noinstr_text_end
ffffffff80a12c00 t rest_init
ffffffff80a12ccc t kernel_init

I guess it is used in order to understand if the instruction pointer is inside
one of these functions / labels.

How does a commit description sound like this?

"
mips: Add dedicated .cpuidle.text section for CPU idle routines

Add a dedicated executable section for CPU idle code to properly organize
and identify idle-related functions inside the .text section.
"

3)

> I think we also need to replace "rollback" with
> another name as with new code we don't roll back anymore.

Should be changed with "region", instead of rollback, maybe?
Do you have something better to suggest?

Thanks a lot!


On Fri, Mar 28, 2025 at 3:18 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Fri, 28 Mar 2025, Maciej W. Rozycki wrote:
>
> > just massaged a little.  FWIW I went ahead and chose to cook this piece up
> > myself since I realised how complex this issue actually is and that it
> > would take us forever to get all the individual aspects nailed over e-mail
> > communication.
>
>  And yet after this many of internal iterations I did manage to miss one
> bit.  In the optimised version proposed we need to explicitly skip over
> the WAIT instruction like this:
>
> r4k_wait_insn:
>         wait
> r4k_wait_exit:
>
> and then:
>
>         .set    noreorder
>         bne     k0, k1, \handler
>          PTR_ADDIU      k0, r4k_wait_exit - r4k_wait_insn + 2
>         .set    reorder
>
> (and here we have a legitimate use for `.set noreorder' to avoid wasting a
> NOP for the branch delay slot due to a data antidependency on $k0; it's
> fine to clobber $k0 in the branch-taken case as by definition the register
> is dead at the exit from this macro).
>
>  Updated patch follows.  I think we also need to replace "rollback" with
> another name as with new code we don't roll back anymore.
>
>   Maciej
>
> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> ---
>  arch/mips/include/asm/idle.h |    3 --
>  arch/mips/kernel/genex.S     |   63 +++++++++++++++++++++++++------------------
>  arch/mips/kernel/idle.c      |    7 ----
>  3 files changed, 38 insertions(+), 35 deletions(-)
>
> linux-mips-idle-vs-timer.diff
> Index: linux-macro/arch/mips/include/asm/idle.h
> ===================================================================
> --- linux-macro.orig/arch/mips/include/asm/idle.h
> +++ linux-macro/arch/mips/include/asm/idle.h
> @@ -6,8 +6,7 @@
>  #include <linux/linkage.h>
>
>  extern void (*cpu_wait)(void);
> -extern void r4k_wait(void);
> -extern asmlinkage void __r4k_wait(void);
> +extern asmlinkage void r4k_wait(void);
>  extern void r4k_wait_irqoff(void);
>
>  static inline int using_rollback_handler(void)
> Index: linux-macro/arch/mips/kernel/genex.S
> ===================================================================
> --- linux-macro.orig/arch/mips/kernel/genex.S
> +++ linux-macro/arch/mips/kernel/genex.S
> @@ -104,42 +104,53 @@ NESTED(except_vec3_r4000, 0, sp)
>
>         __FINIT
>
> -       .align  5       /* 32 byte rollback region */
> -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
> +       .section .cpuidle.text,"ax"
> +       /* Align to 32 bytes for the maximum idle interrupt region size.  */
> +       .align  5
> +LEAF(r4k_wait)
> +       /* Keep the ISA bit clear for calculations on local labels here.  */
> +0:     .fill   0
> +       /* Start of idle interrupt region.  */
> +       local_irq_enable
> +       /*
> +        * If an interrupt lands here, before 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.
> +        */
> +1:     .fill   0
> +       /* The R2 EI/EHB sequence takes 8 bytes, otherwise pad up.  */
> +       .if     1b - 0b > 32
> +       .error  "overlong idle interrupt region"
> +       .elseif 1b - 0b > 8
> +       .align  4
> +       .endif
> +2:     .fill   0
> +       .equ    r4k_wait_idle_size, 2b - 0b
> +       /* End of idle interrupt region; size has to be a power of 2.  */
>         .set    MIPS_ISA_ARCH_LEVEL_RAW
> +r4k_wait_insn:
>         wait
> -       /* end of rollback region (the region size must be power of two) */
> -1:
> +r4k_wait_exit:
> +       .set    mips0
> +       local_irq_disable
>         jr      ra
> -        nop
> -       .set    pop
> -       END(__r4k_wait)
> +       END(r4k_wait)
> +       .previous
>
>         .macro  BUILD_ROLLBACK_PROLOGUE handler
>         FEXPORT(rollback_\handler)
>         .set    push
>         .set    noat
>         MFC0    k0, CP0_EPC
> -       PTR_LA  k1, __r4k_wait
> -       ori     k0, 0x1f        /* 32 byte rollback region */
> -       xori    k0, 0x1f
> +       /* Subtract/add 2 to let the ISA bit propagate through the mask.  */
> +       PTR_LA  k1, r4k_wait_insn - 2
> +       ori     k0, r4k_wait_idle_size - 2
> +       .set    noreorder
>         bne     k0, k1, \handler
> +        PTR_ADDIU      k0, r4k_wait_exit - r4k_wait_insn + 2
> +       .set    reorder
>         MTC0    k0, CP0_EPC
>         .set pop
>         .endm
> Index: linux-macro/arch/mips/kernel/idle.c
> ===================================================================
> --- linux-macro.orig/arch/mips/kernel/idle.c
> +++ linux-macro/arch/mips/kernel/idle.c
> @@ -35,13 +35,6 @@ static void __cpuidle r3081_wait(void)
>         write_c0_conf(cfg | R30XX_CONF_HALT);
>  }
>
> -void __cpuidle r4k_wait(void)
> -{
> -       raw_local_irq_enable();
> -       __r4k_wait();
> -       raw_local_irq_disable();
> -}
> -
>  /*
>   * This variant is preferable as it allows testing need_resched and going to
>   * sleep depending on the outcome atomically.  Unfortunately the "It is



--

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@suse.com

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

[Maciej W. Rozycki]

On Mon, 31 Mar 2025, Marco Crivellari wrote:

> > There's some complication here coming from the need to factor in the ISA
> > bit in the microMIPS mode; something that hasn't been discussed so far.
> > The `.fill 0' approach is a hack and it has struck me that we need to add
> > a `.noinsn' pseudo-op to GAS for this purpose, complementing `.insn', but
> > we need to stick with the hack for now anyway as it will take years until
> > we can rely on a new feature in the assembler.
> 
> Ah, interesting. So de facto having the "repeat" to 0, will generate nothing.

 Correct, and as a side effect this pseudo-op clears the ISA bit on any 
label attached, as it is not an instruction.

> > I can't imagine how we'd advance past WAIT without these instructions,
> > what do you have in mind?
> 
> I've not been precise, sorry.
> I meant to remove the instructions like they are now because the
> region would have
> been different, then.
> So, those instructions would have needed a change, in practice.

 OK, so this is precisely what happened here with my proposal.  Thanks for 
clarifying.

> > NB how do you actually verify this stuff with QEMU?  Is it by injecting
> > an interrupt by hand at a chosen code location via GDB attached to QEMU's
> > built-in debug stub?
> 
> Short answer: I am not able to fully test this, I can only boot.
> 
> The reason is that gdb-multiarch is not working as expected.
> The binary in my distribution has the python support broken. So when I try to
> inject the interrupt, I'm receiving a python error (actually, I receive the same
> error after the "target remote" command).
> I've also tried to compile the binary from source, but again,
> I understand why in OBS the build I found is broken...

 Well, you should be able to set a breakpoint at `rollback_handle_int' and 
fiddle with $epc by hand to see if the code sequence correctly skips over 
WAIT.  Though I reckon QEMU used to have an issue with presenting the MIPS 
privileged context over its debug stub.  Has the issue been fixed?  Either 
way you should be able to just operate on the copy in $k0 retrieved with 
(D)MFC0.

> > Below I've included a complete change based on the outline above.  It
> > seems to do the right thing for a couple of my configurations, but I've
> > only eyeballed the resulting code and haven't tried running it.  Most of
> > my hardware doesn't implement the WAIT instruction anyway.
> 
> It means it would be better to have someone else try the resulting
> patch, I guess.

 Exhaustive run-time verification is not always necessary if you can 
demonstrate that your code is correct via other means, including 
proofreading.

 FAOD I have one MIPS32r2 system wired for testing, but that might not be 
the most interesting configuration to verify as it'll now just use EI/EHB 
to enable interrupts ahead of WAIT.  I could try an R1 kernel instead, but 
I'm not sure if it can be made to work owing to the differences in the FPU 
between R1 and R2 for the MIPS32 ISA.  I used to have a MIPS64 (R1) system 
there, but the CPU daughtercard sadly stopped working 3 years ago and I 
wasn't able to repair it, owing to the lack of available spare parts (it's 
most likely a dead CPU).

> > Let me know if you find anything here unclear or have any questions or
> > comments.
> 
> 1)
> 
> > /* Keep the ISA bit clear for calculations on local labels here. */
> 
> The ISA bit should be the bit 0, correct?
> So, also in the macro code, it's done to preserve that bit.

 Correct, the bit will be set according to the ISA mode at the time the 
originating machine instruction is executed, in EPC or any other register 
the PC is copied to, e.g. $ra.  Likewise any instruction setting the PC 
such as JR or ERET will set the ISA mode from the ISA bit of the source 
register (the ISA bit for exception entry is set from CP0 Config3.ISAOnExc 
register bit).

 All code labels in microMIPS code will have the bit set, so that 
relocations correctly calculate immediates used to make register jumps.  

 The ISA mode can be switched explicitly with the JALX instruction (you 
can mix regular MIPS and microMIPS code as long as hardware supports the 
other ISA mode; either or both can be implemented in a given piece of 
silicon).

 Other immediate jumps and branches preserve the current ISA mode, but the 
assembler and linker verify you don't attempt to use these instructions to 
pass control to code in the other mode; this is an assembly or link error 
depending at what stage the label reference is resolved.

 FYI the documents for the microMIPS mode of operation are respectively:

- "MIPS Architecture For Programmers Volume I-B: Introduction to the
   microMIPS32 Architecture"

- "MIPS Architecture for Programmers Volume II-B: The microMIPS32 
   Instruction Set"

and their microMIPS64 counterparts (although the first document is almost 
an exact copy of its regular MIPS variant).  Be sure to download revision 
5.xx, because revision 6.xx describes an entirely different ISA which we 
currently have no support for (downstream patches were never submitted; 
also microMIPSr6 removed branch delay slots, which caused all sorts of 
portability issues).  See Section 3.5 of the latter manual for further 
discussion of the ISA bit.

> 2)
> .section .cpuidle.text,"ax"
> 
> This should be a single patch, right?
> Based on what I understood, 'a' should be the allocation, and 'x' the
> executable attribute.

 Both correct (see the GAS manual for the section flags); this just 
matches the __cpuidle macro from <linux/compiler_types.h>.

> This should be in order to mark those symbols like "cpuidle text":
> 
> $ nm -n vmlinux | grep -A3 "cpuidle_text"
> ffffffff80a127e0 T __cpuidle_text_start
> ffffffff80a127e0 T r4k_wait
> ffffffff80a12800 t r4k_wait_insn
> ffffffff80a12804 t r4k_wait_exit
> --
> ffffffff80a12c00 T __cpuidle_text_end
> ffffffff80a12c00 T __noinstr_text_end
> ffffffff80a12c00 t rest_init
> ffffffff80a12ccc t kernel_init
> 
> I guess it is used in order to understand if the instruction pointer is inside
> one of these functions / labels.

 See the comment at said macro in <linux/compiler_types.h> and also commit 
6727ad9e206c ("nmi_backtrace: generate one-line reports for idle cpus").

> How does a commit description sound like this?
> 
> "
> mips: Add dedicated .cpuidle.text section for CPU idle routines
> 
> Add a dedicated executable section for CPU idle code to properly organize
> and identify idle-related functions inside the .text section.
> "

 How about:

"
MIPS: Move __r4k_wait() to .cpuidle.text section

Fix missing .cpuidle.text section assignment for __r4k_wait() to correct 
backtracing with nmi_backtrace().

Fixes: 97c8580e85cf ("MIPS: Annotate cpu_wait implementations with __cpuidle")
"

or suchlike (based on the commit referred)?

 This probably does not itself qualify for linux-stable as the issue is 
only annoying rather than fatal, but I think the annotation should be 
there in case someone wants to backport it downstream.

> 3)
> 
> > I think we also need to replace "rollback" with
> > another name as with new code we don't roll back anymore.
> 
> Should be changed with "region", instead of rollback, maybe?
> Do you have something better to suggest?

 Hmm, "skipover" maybe?

  Maciej

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

[Jiaxun Yang]

在2025年3月31日星期周一 下午9:09，Maciej W. Rozycki写道：
[...]
>
>  FAOD I have one MIPS32r2 system wired for testing, but that might not be 
> the most interesting configuration to verify as it'll now just use EI/EHB 
> to enable interrupts ahead of WAIT.  I could try an R1 kernel instead, but 
> I'm not sure if it can be made to work owing to the differences in the FPU 
> between R1 and R2 for the MIPS32 ISA.  I used to have a MIPS64 (R1) system 
> there, but the CPU daughtercard sadly stopped working 3 years ago and I 
> wasn't able to repair it, owing to the lack of available spare parts (it's 
> most likely a dead CPU).

I can test on legacy (R1 version) 4Kc RTL simulator if you wish. Is there any
thing specific you want to test? I think I can try interrupt flood and see if
there is any deadlock.

The simulation is painfully slow, so I'd wish to minimize test vector.

Thanks


-- 
- Jiaxun

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

[Marco Crivellari]

Hi Maciej,

> Well, you should be able to set a breakpoint at `rollback_handle_int' and
> fiddle with $epc by hand to see if the code sequence correctly skips over
> WAIT.  Though I reckon QEMU used to have an issue with presenting the MIPS
> privileged context over its debug stub.  Has the issue been fixed?  Either
> way you should be able to just operate on the copy in $k0 retrieved with
> (D)MFC0.

Nope, seems not fixed so far. But yes, changing $k0 is working fine.
With the cpu in idle code (executing "wait"), ctrl+c, then placed a bp
in rollback_handle_init+4. Then "c" to hit the bp.
When the bp is hit, I can see $k0 = r4k_wait_exit.

I changed $k0 with an address inside the region, and setting a bp on "bne",
 the value is equal to $k1. I'm assuming the value is also saved
correctly in $epc,
considering it points correctly to r4k_wait_exit.

> See Section 3.5 of the latter manual for further
> discussion of the ISA bit.

Thank you, I found the manuals for rev. 5,
I still didn't have enough time to take a look anyhow.

> See the comment at said macro in <linux/compiler_types.h> and also commit
> 6727ad9e206c ("nmi_backtrace: generate one-line reports for idle cpus").

Aha, thanks!

 > Hmm, "skipover" maybe?

Now that I'm looking at the code, shouldn't it be better to address this in a
separate patch or another time?

I can see the rollback_handler* is exported also in arch/mips/kernel/traps.c
And there are a few parts that makes use of the "rollback" name; I'm
wondering if also
the code should be refactored a bit then,
eg arch/mips /include/asm/idle.h:using_rollback_handler()


Thank you!


On Mon, Mar 31, 2025 at 10:09 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Mon, 31 Mar 2025, Marco Crivellari wrote:
>
> > > There's some complication here coming from the need to factor in the ISA
> > > bit in the microMIPS mode; something that hasn't been discussed so far.
> > > The `.fill 0' approach is a hack and it has struck me that we need to add
> > > a `.noinsn' pseudo-op to GAS for this purpose, complementing `.insn', but
> > > we need to stick with the hack for now anyway as it will take years until
> > > we can rely on a new feature in the assembler.
> >
> > Ah, interesting. So de facto having the "repeat" to 0, will generate nothing.
>
>  Correct, and as a side effect this pseudo-op clears the ISA bit on any
> label attached, as it is not an instruction.
>
> > > I can't imagine how we'd advance past WAIT without these instructions,
> > > what do you have in mind?
> >
> > I've not been precise, sorry.
> > I meant to remove the instructions like they are now because the
> > region would have
> > been different, then.
> > So, those instructions would have needed a change, in practice.
>
>  OK, so this is precisely what happened here with my proposal.  Thanks for
> clarifying.
>
> > > NB how do you actually verify this stuff with QEMU?  Is it by injecting
> > > an interrupt by hand at a chosen code location via GDB attached to QEMU's
> > > built-in debug stub?
> >
> > Short answer: I am not able to fully test this, I can only boot.
> >
> > The reason is that gdb-multiarch is not working as expected.
> > The binary in my distribution has the python support broken. So when I try to
> > inject the interrupt, I'm receiving a python error (actually, I receive the same
> > error after the "target remote" command).
> > I've also tried to compile the binary from source, but again,
> > I understand why in OBS the build I found is broken...
>
>  Well, you should be able to set a breakpoint at `rollback_handle_int' and
> fiddle with $epc by hand to see if the code sequence correctly skips over
> WAIT.  Though I reckon QEMU used to have an issue with presenting the MIPS
> privileged context over its debug stub.  Has the issue been fixed?  Either
> way you should be able to just operate on the copy in $k0 retrieved with
> (D)MFC0.
>
> > > Below I've included a complete change based on the outline above.  It
> > > seems to do the right thing for a couple of my configurations, but I've
> > > only eyeballed the resulting code and haven't tried running it.  Most of
> > > my hardware doesn't implement the WAIT instruction anyway.
> >
> > It means it would be better to have someone else try the resulting
> > patch, I guess.
>
>  Exhaustive run-time verification is not always necessary if you can
> demonstrate that your code is correct via other means, including
> proofreading.
>
>  FAOD I have one MIPS32r2 system wired for testing, but that might not be
> the most interesting configuration to verify as it'll now just use EI/EHB
> to enable interrupts ahead of WAIT.  I could try an R1 kernel instead, but
> I'm not sure if it can be made to work owing to the differences in the FPU
> between R1 and R2 for the MIPS32 ISA.  I used to have a MIPS64 (R1) system
> there, but the CPU daughtercard sadly stopped working 3 years ago and I
> wasn't able to repair it, owing to the lack of available spare parts (it's
> most likely a dead CPU).
>
> > > Let me know if you find anything here unclear or have any questions or
> > > comments.
> >
> > 1)
> >
> > > /* Keep the ISA bit clear for calculations on local labels here. */
> >
> > The ISA bit should be the bit 0, correct?
> > So, also in the macro code, it's done to preserve that bit.
>
>  Correct, the bit will be set according to the ISA mode at the time the
> originating machine instruction is executed, in EPC or any other register
> the PC is copied to, e.g. $ra.  Likewise any instruction setting the PC
> such as JR or ERET will set the ISA mode from the ISA bit of the source
> register (the ISA bit for exception entry is set from CP0 Config3.ISAOnExc
> register bit).
>
>  All code labels in microMIPS code will have the bit set, so that
> relocations correctly calculate immediates used to make register jumps.
>
>  The ISA mode can be switched explicitly with the JALX instruction (you
> can mix regular MIPS and microMIPS code as long as hardware supports the
> other ISA mode; either or both can be implemented in a given piece of
> silicon).
>
>  Other immediate jumps and branches preserve the current ISA mode, but the
> assembler and linker verify you don't attempt to use these instructions to
> pass control to code in the other mode; this is an assembly or link error
> depending at what stage the label reference is resolved.
>
>  FYI the documents for the microMIPS mode of operation are respectively:
>
> - "MIPS Architecture For Programmers Volume I-B: Introduction to the
>    microMIPS32 Architecture"
>
> - "MIPS Architecture for Programmers Volume II-B: The microMIPS32
>    Instruction Set"
>
> and their microMIPS64 counterparts (although the first document is almost
> an exact copy of its regular MIPS variant).  Be sure to download revision
> 5.xx, because revision 6.xx describes an entirely different ISA which we
> currently have no support for (downstream patches were never submitted;
> also microMIPSr6 removed branch delay slots, which caused all sorts of
> portability issues).  See Section 3.5 of the latter manual for further
> discussion of the ISA bit.
>
> > 2)
> > .section .cpuidle.text,"ax"
> >
> > This should be a single patch, right?
> > Based on what I understood, 'a' should be the allocation, and 'x' the
> > executable attribute.
>
>  Both correct (see the GAS manual for the section flags); this just
> matches the __cpuidle macro from <linux/compiler_types.h>.
>
> > This should be in order to mark those symbols like "cpuidle text":
> >
> > $ nm -n vmlinux | grep -A3 "cpuidle_text"
> > ffffffff80a127e0 T __cpuidle_text_start
> > ffffffff80a127e0 T r4k_wait
> > ffffffff80a12800 t r4k_wait_insn
> > ffffffff80a12804 t r4k_wait_exit
> > --
> > ffffffff80a12c00 T __cpuidle_text_end
> > ffffffff80a12c00 T __noinstr_text_end
> > ffffffff80a12c00 t rest_init
> > ffffffff80a12ccc t kernel_init
> >
> > I guess it is used in order to understand if the instruction pointer is inside
> > one of these functions / labels.
>
>  See the comment at said macro in <linux/compiler_types.h> and also commit
> 6727ad9e206c ("nmi_backtrace: generate one-line reports for idle cpus").
>
> > How does a commit description sound like this?
> >
> > "
> > mips: Add dedicated .cpuidle.text section for CPU idle routines
> >
> > Add a dedicated executable section for CPU idle code to properly organize
> > and identify idle-related functions inside the .text section.
> > "
>
>  How about:
>
> "
> MIPS: Move __r4k_wait() to .cpuidle.text section
>
> Fix missing .cpuidle.text section assignment for __r4k_wait() to correct
> backtracing with nmi_backtrace().
>
> Fixes: 97c8580e85cf ("MIPS: Annotate cpu_wait implementations with __cpuidle")
> "
>
> or suchlike (based on the commit referred)?
>
>  This probably does not itself qualify for linux-stable as the issue is
> only annoying rather than fatal, but I think the annotation should be
> there in case someone wants to backport it downstream.
>
> > 3)
> >
> > > I think we also need to replace "rollback" with
> > > another name as with new code we don't roll back anymore.
> >
> > Should be changed with "region", instead of rollback, maybe?
> > Do you have something better to suggest?
>
>  Hmm, "skipover" maybe?
>
>   Maciej



--

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@suse.com

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

[Maciej W. Rozycki]

On Wed, 2 Apr 2025, Marco Crivellari wrote:

> > Well, you should be able to set a breakpoint at `rollback_handle_int' and
> > fiddle with $epc by hand to see if the code sequence correctly skips over
> > WAIT.  Though I reckon QEMU used to have an issue with presenting the MIPS
> > privileged context over its debug stub.  Has the issue been fixed?  Either
> > way you should be able to just operate on the copy in $k0 retrieved with
> > (D)MFC0.
> 
> Nope, seems not fixed so far. But yes, changing $k0 is working fine.
> With the cpu in idle code (executing "wait"), ctrl+c, then placed a bp
> in rollback_handle_init+4. Then "c" to hit the bp.
> When the bp is hit, I can see $k0 = r4k_wait_exit.
> 
> I changed $k0 with an address inside the region, and setting a bp on "bne",
>  the value is equal to $k1. I'm assuming the value is also saved
> correctly in $epc,
> considering it points correctly to r4k_wait_exit.

 I think it's enough evidence to prove the code works as expected.

>  > Hmm, "skipover" maybe?
> 
> Now that I'm looking at the code, shouldn't it be better to address this in a
> separate patch or another time?
> 
> I can see the rollback_handler* is exported also in arch/mips/kernel/traps.c
> And there are a few parts that makes use of the "rollback" name; I'm
> wondering if also
> the code should be refactored a bit then,
> eg arch/mips /include/asm/idle.h:using_rollback_handler()

 Yeah, I agree cluttering the semantics change with the rename is not the 
best idea, so a follow-up patch approach will be better.

 Also I wonder if we could come up with yet another name that does not 
have the exact semantics implied, so that if in 20 years' time we change 
our minds again and decide: "Oh, let's do the rollback thing after all," 
we don't have to do the rename again.  Though it's like I'm diverging into 
the bikeshedding area here, so perhaps please just use whatever name you 
have found most reasonable at the time you're about to wrap up with this 
effort.

  Maciej

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

[Marco Crivellari]

Great, so I think that I will send the v7 without the "rename" part,
so we can address this in the future, if it sounds reasonable,
and find an appropriate name.

Thank you!


On Thu, Apr 3, 2025 at 2:00 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Wed, 2 Apr 2025, Marco Crivellari wrote:
>
> > > Well, you should be able to set a breakpoint at `rollback_handle_int' and
> > > fiddle with $epc by hand to see if the code sequence correctly skips over
> > > WAIT.  Though I reckon QEMU used to have an issue with presenting the MIPS
> > > privileged context over its debug stub.  Has the issue been fixed?  Either
> > > way you should be able to just operate on the copy in $k0 retrieved with
> > > (D)MFC0.
> >
> > Nope, seems not fixed so far. But yes, changing $k0 is working fine.
> > With the cpu in idle code (executing "wait"), ctrl+c, then placed a bp
> > in rollback_handle_init+4. Then "c" to hit the bp.
> > When the bp is hit, I can see $k0 = r4k_wait_exit.
> >
> > I changed $k0 with an address inside the region, and setting a bp on "bne",
> >  the value is equal to $k1. I'm assuming the value is also saved
> > correctly in $epc,
> > considering it points correctly to r4k_wait_exit.
>
>  I think it's enough evidence to prove the code works as expected.
>
> >  > Hmm, "skipover" maybe?
> >
> > Now that I'm looking at the code, shouldn't it be better to address this in a
> > separate patch or another time?
> >
> > I can see the rollback_handler* is exported also in arch/mips/kernel/traps.c
> > And there are a few parts that makes use of the "rollback" name; I'm
> > wondering if also
> > the code should be refactored a bit then,
> > eg arch/mips /include/asm/idle.h:using_rollback_handler()
>
>  Yeah, I agree cluttering the semantics change with the rename is not the
> best idea, so a follow-up patch approach will be better.
>
>  Also I wonder if we could come up with yet another name that does not
> have the exact semantics implied, so that if in 20 years' time we change
> our minds again and decide: "Oh, let's do the rollback thing after all,"
> we don't have to do the rename again.  Though it's like I'm diverging into
> the bikeshedding area here, so perhaps please just use whatever name you
> have found most reasonable at the time you're about to wrap up with this
> effort.
>
>   Maciej



-- 

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@suse.com

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

[Maciej W. Rozycki]

On Thu, 3 Apr 2025, Marco Crivellari wrote:

> Great, so I think that I will send the v7 without the "rename" part,
> so we can address this in the future, if it sounds reasonable,
> and find an appropriate name.

 Well, from experience "future" sounds like "never" to me, but if Thomas 
is fine with that, I'm not going to fight a battle here.  I've got enough 
stuff to do now, including some rather urgent discovered last month, as 
far as the MIPS port is concerned, let alone other things.  Thank you for 
your perseverance with this effort anyway.

  Maciej

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

[Marco Crivellari]

Hi,

No, I didn't wanna sound like "never".
I really meant the future patch series.

Depending on the load my full time job will have, it could be in the next
week. or a week later. :-)

Thanks!


On Thu, Apr 3, 2025 at 10:17 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Thu, 3 Apr 2025, Marco Crivellari wrote:
>
> > Great, so I think that I will send the v7 without the "rename" part,
> > so we can address this in the future, if it sounds reasonable,
> > and find an appropriate name.
>
>  Well, from experience "future" sounds like "never" to me, but if Thomas
> is fine with that, I'm not going to fight a battle here.  I've got enough
> stuff to do now, including some rather urgent discovered last month, as
> far as the MIPS port is concerned, let alone other things.  Thank you for
> your perseverance with this effort anyway.
>
>   Maciej



-- 

Marco Crivellari

L3 Support Engineer, Technology & Product




marco.crivellari@suse.com

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

[Frederic Weisbecker]

Le Fri, Mar 21, 2025 at 11:53:54AM +0000, Maciej W. Rozycki a écrit :
> On Sat, 15 Mar 2025, Marco Crivellari wrote:
> 
> > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > index a572ce36a24f..4e012421d00f 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 interrupt. */
> > +	ori 	t0, 0x1f
> 
>  This instruction sequence still suffers from the coprocessor move delay 
> hazard.  How many times do I need to request to get it fixed (counting 
> three so far)?

This is because your request had follow-ups from Huacai and Marco that
were left unanswered:

     https://lore.kernel.org/all/CAAhV-H5ptAzHTPAr1bxrgByZrnFmMK8zJ68Z++RwC=NHWjqZmw@mail.gmail.com/
     https://lore.kernel.org/all/CAAofZF4HAczyRmuRe-JmQ2wcZatevLwGTOMLf1V1okGbj7q5Wg@mail.gmail.com/

We have detected this longstanding architecture specific timer handling bug on
loongson and MIPS and we could have just dropped a report and let you guys deal with
it. Instead we decided to spend time ourselves (especially Marco) working on
fixes for these architectures we don't run and which we are not familiar with,
alongway taking reviews seriously and patiently re-iterating accordingly.

So please be gentle with us.

Thanks.

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

[Maciej W. Rozycki]

On Fri, 21 Mar 2025, Frederic Weisbecker wrote:

> > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > > index a572ce36a24f..4e012421d00f 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 interrupt. */
> > > +	ori 	t0, 0x1f
> > 
> >  This instruction sequence still suffers from the coprocessor move delay 
> > hazard.  How many times do I need to request to get it fixed (counting 
> > three so far)?
> 
> This is because your request had follow-ups from Huacai and Marco that
> were left unanswered:
> 
>      https://lore.kernel.org/all/CAAhV-H5ptAzHTPAr1bxrgByZrnFmMK8zJ68Z++RwC=NHWjqZmw@mail.gmail.com/

 The conclusion made there is however wrong: `local_irq_enable' code 
plays no tricks with instruction scheduling and lets the toolchain 
resolve any pipeline hazards automatically, while `__r4k_wait' arranges 
for instructions to be scheduled by hand and any hazards resolved by the 
human writer of the code.  There's even explicit `.set reorder' in 
`local_irq_enable', which is redundant, because it's the default mode 
for inline assembly.

 And I can't emphasise it enough: manual instruction scheduling is tough
and ought to be restricted to cases where there is no other way really, 
such as for placing an instruction in a branch delay slot where there is 
a data antidependency between the branch and the delay-slot instruction.  
Yet this approach has often been used by code authors for other reasons 
(or I daresay no reason at all), leaving it up to the maintainers to 
keep the code working in the changing conditions while the submitter has 
long gone.  I converted some of such code in the past, but it also takes 
time and effort that does not come for free.

>      https://lore.kernel.org/all/CAAofZF4HAczyRmuRe-JmQ2wcZatevLwGTOMLf1V1okGbj7q5Wg@mail.gmail.com/

 I missed that one, sorry.  A ping would have helped, and I never have 
an issue with being pinged.  I do hope I have now addressed that concern 
with my other reply.

 Thank you for the pointers.

> We have detected this longstanding architecture specific timer handling bug on
> loongson and MIPS and we could have just dropped a report and let you guys deal with
> it. Instead we decided to spend time ourselves (especially Marco) working on
> fixes for these architectures we don't run and which we are not familiar with,
> alongway taking reviews seriously and patiently re-iterating accordingly.

 Thank you for your effort, really appreciated.  Any fixes need to be 
technically correct however, it makes no sense to get one bug replaced 
with another one.  We've got enough technical debt accumulated already 
with a platform that no longer has any commercial support and relies 
solely on voluteers keeping it alive in their limited spare time.  I do 
have a long list of outstanding issues to address and ever so little 
time to take care of them, with hardware problems additionally kicking 
in and distracting every so often too.

> So please be gentle with us.

 As always, but also emphatic on this occasion.  We're in the same boat 
really, striving against the lack of resources and issues piling, and 
now we've made some progress.  Thank you for your understanding.

  Maciej

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

[Frederic Weisbecker]

Le Sat, Mar 22, 2025 at 04:08:31PM +0000, Maciej W. Rozycki a écrit :
> On Fri, 21 Mar 2025, Frederic Weisbecker wrote:
> 
> > > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> > > > index a572ce36a24f..4e012421d00f 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 interrupt. */
> > > > +	ori 	t0, 0x1f
> > > 
> > >  This instruction sequence still suffers from the coprocessor move delay 
> > > hazard.  How many times do I need to request to get it fixed (counting 
> > > three so far)?
> > 
> > This is because your request had follow-ups from Huacai and Marco that
> > were left unanswered:
> > 
> >      https://lore.kernel.org/all/CAAhV-H5ptAzHTPAr1bxrgByZrnFmMK8zJ68Z++RwC=NHWjqZmw@mail.gmail.com/
> 
>  The conclusion made there is however wrong: `local_irq_enable' code 
> plays no tricks with instruction scheduling and lets the toolchain 
> resolve any pipeline hazards automatically, while `__r4k_wait' arranges 
> for instructions to be scheduled by hand and any hazards resolved by the 
> human writer of the code.  There's even explicit `.set reorder' in 
> `local_irq_enable', which is redundant, because it's the default mode 
> for inline assembly.
> 
>  And I can't emphasise it enough: manual instruction scheduling is tough
> and ought to be restricted to cases where there is no other way really, 
> such as for placing an instruction in a branch delay slot where there is 
> a data antidependency between the branch and the delay-slot instruction.  
> Yet this approach has often been used by code authors for other reasons 
> (or I daresay no reason at all), leaving it up to the maintainers to 
> keep the code working in the changing conditions while the submitter has 
> long gone.  I converted some of such code in the past, but it also takes 
> time and effort that does not come for free.
> 
> >      https://lore.kernel.org/all/CAAofZF4HAczyRmuRe-JmQ2wcZatevLwGTOMLf1V1okGbj7q5Wg@mail.gmail.com/
> 
>  I missed that one, sorry.  A ping would have helped, and I never have 
> an issue with being pinged.  I do hope I have now addressed that concern 
> with my other reply.

Hopefully, I'll let Marco follow-up on that as I must confess I'm lost
with these details. But your help has been very valuable!

> 
> > We have detected this longstanding architecture specific timer handling bug on
> > loongson and MIPS and we could have just dropped a report and let you guys deal with
> > it. Instead we decided to spend time ourselves (especially Marco) working on
> > fixes for these architectures we don't run and which we are not familiar with,
> > alongway taking reviews seriously and patiently re-iterating accordingly.
> 
>  Thank you for your effort, really appreciated.  Any fixes need to be 
> technically correct however, it makes no sense to get one bug replaced 
> with another one.  We've got enough technical debt accumulated already 
> with a platform that no longer has any commercial support and relies 
> solely on voluteers keeping it alive in their limited spare time.  I do 
> have a long list of outstanding issues to address and ever so little 
> time to take care of them, with hardware problems additionally kicking 
> in and distracting every so often too.

Yeah I totally understand that!

> 
> > So please be gentle with us.
> 
>  As always, but also emphatic on this occasion.  We're in the same boat 
> really, striving against the lack of resources and issues piling, and 
> now we've made some progress.  Thank you for your understanding.

Heh I know... Thanks a lot!

> 
>   Maciej