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.
Fixes: c65a5480ff29 ("[MIPS] Fix potential latency problem due to non-atomic cpu_wait.")
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Acked-by: Frederic Weisbecker <frederic@kernel.org>
---
arch/mips/include/asm/idle.h | 3 +-
arch/mips/kernel/genex.S | 62 +++++++++++++++++++++---------------
arch/mips/kernel/idle.c | 7 ----
3 files changed, 37 insertions(+), 35 deletions(-)
diff --git a/arch/mips/include/asm/idle.h b/arch/mips/include/asm/idle.h
index 0992cad9c632..2bc3678455ed 100644
--- a/arch/mips/include/asm/idle.h
+++ b/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)
diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
index a572ce36a24f..46d975d00298 100644
--- a/arch/mips/kernel/genex.S
+++ b/arch/mips/kernel/genex.S
@@ -104,42 +104,52 @@ handle_vcei:
__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
+ /* 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
diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c
index 5abc8b7340f8..80e8a04a642e 100644
--- a/arch/mips/kernel/idle.c
+++ b/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
--
2.49.0Hi, Marco and Thomas,
On Fri, Apr 4, 2025 at 12:11 AM Marco Crivellari
<marco.crivellari@suse.com> wrote:
>
> MIPS re-enables interrupts on its idle routine and performs
> a TIF_NEED_RESCHED check afterwards before putting the CPU to sleep.
>
> The IRQs firing between the check and the 'wait' instruction may set the
> TIF_NEED_RESCHED flag. In order to deal with this possible race, IRQs
> interrupting __r4k_wait() rollback their return address to the
> beginning of __r4k_wait() so that TIF_NEED_RESCHED is checked
> again before going back to sleep.
>
> However idle IRQs can also queue timers that may require a tick
> reprogramming through a new generic idle loop iteration but those timers
> would go unnoticed here because __r4k_wait() only checks
> TIF_NEED_RESCHED. It doesn't check for pending timers.
>
> Fix this with fast-forwarding idle IRQs return address to the end of the
> idle routine instead of the beginning, so that the generic idle loop
> handles both TIF_NEED_RESCHED and pending timers.
>
> 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.
>
> Fixes: c65a5480ff29 ("[MIPS] Fix potential latency problem due to non-atomic cpu_wait.")
> Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> Acked-by: Frederic Weisbecker <frederic@kernel.org>
> ---
> arch/mips/include/asm/idle.h | 3 +-
> arch/mips/kernel/genex.S | 62 +++++++++++++++++++++---------------
> arch/mips/kernel/idle.c | 7 ----
> 3 files changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/arch/mips/include/asm/idle.h b/arch/mips/include/asm/idle.h
> index 0992cad9c632..2bc3678455ed 100644
> --- a/arch/mips/include/asm/idle.h
> +++ b/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)
> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> index a572ce36a24f..46d975d00298 100644
> --- a/arch/mips/kernel/genex.S
> +++ b/arch/mips/kernel/genex.S
> @@ -104,42 +104,52 @@ handle_vcei:
>
> __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
> + /* 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
I'm very sorry for the late response, but I think ".previous" should
be moved to the second patch.
Huacai
>
> .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
> diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c
> index 5abc8b7340f8..80e8a04a642e 100644
> --- a/arch/mips/kernel/idle.c
> +++ b/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
> --
> 2.49.0
>
On Sun, 27 Apr 2025, Huacai Chen wrote: > > +r4k_wait_exit: > > + .set mips0 > > + local_irq_disable > > jr ra > > - nop > > - .set pop > > - END(__r4k_wait) > > + END(r4k_wait) > > + .previous > I'm very sorry for the late response, but I think ".previous" should > be moved to the second patch. Indeed; does it even assemble? Correctness aside I'd rather it didn't cause someone a problem with bisecting sometime. NB I had no opportunity either to look at this version earlier. Maciej
Hi everyone, Sorry, I have tested the code only with both the patches applied (when I also ran the tests with qemu) and I didn't notice my mistake. I will submit a new version with the correction and the changes suggested in the other patch, when we find the proper way to do it. Thank you. On Mon, Apr 28, 2025 at 3:32 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote: > > On Sun, 27 Apr 2025, Huacai Chen wrote: > > > > +r4k_wait_exit: > > > + .set mips0 > > > + local_irq_disable > > > jr ra > > > - nop > > > - .set pop > > > - END(__r4k_wait) > > > + END(r4k_wait) > > > + .previous > > I'm very sorry for the late response, but I think ".previous" should > > be moved to the second patch. > > Indeed; does it even assemble? Correctness aside I'd rather it didn't > cause someone a problem with bisecting sometime. NB I had no opportunity > either to look at this version earlier. > > Maciej -- Marco Crivellari L3 Support Engineer, Technology & Product marco.crivellari@suse.com
Gentle Ping. Thanks.
© 2016 - 2025 Red Hat, Inc.