Hi, Marco,
On Fri, Feb 14, 2025 at 6:51 PM Marco Crivellari
<marco.crivellari@xxxxxxxx> wrote:
>
> MIPS re-enables interrupts on its idle routine and performs
> a TIF_NEED_RESCHED check afterwards before putting the CPU to sleep.
>
> The IRQs firing between the check and the 'wait' instruction may set the
> TIF_NEED_RESCHED flag. In order to deal with this possible race, IRQs
> interrupting __r4k_wait() rollback their return address to the
> beginning of __r4k_wait() so that TIF_NEED_RESCHED is checked
> again before going back to sleep.
>
> However idle IRQs can also queue timers that may require a tick
> reprogramming through a new generic idle loop iteration but those timers
> would go unnoticed here because __r4k_wait() only checks
> TIF_NEED_RESCHED. It doesn't check for pending timers.
>
> Fix this with fast-forwarding idle IRQs return address to the end of the
> idle routine instead of the beginning, so that the generic idle loop
> handles both TIF_NEED_RESCHED and pending timers.
>
> Signed-off-by: Marco Crivellari <marco.crivellari@xxxxxxxx>
> ---
> arch/mips/kernel/genex.S | 36 ++++++++++++++++++++----------------
> arch/mips/kernel/idle.c | 1 -
> 2 files changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> index a572ce36a24f..a78d5132c940 100644
> --- a/arch/mips/kernel/genex.S
> +++ b/arch/mips/kernel/genex.S
> @@ -104,18 +104,16 @@ 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
> + /* start of idle interrupt region */
> + MFC0 k0, CP0_STATUS
> + /* Enable Interrupt */
> + ori k0, 0x1f
> + xori k0, 0x1e
> + MTC0 k0, CP0_STATUS
> #ifdef CONFIG_CPU_MICROMIPS
> nop
> nop
> @@ -123,11 +121,17 @@ LEAF(__r4k_wait)
> nop
> #endif
> .set MIPS_ISA_ARCH_LEVEL_RAW
> + /*
> + * If an interrupt lands here, between enabling interrupts above and
> + * going idle on the next instruction, we must *NOT* go idle since the
> + * interrupt could have set TIF_NEED_RESCHED or caused a timer to need
> + * resched. Fall through -- see rollback_handler below -- and have
> + * the idle loop take care of things.
> + */
> wait
> - /* end of rollback region (the region size must be power of two) */
> -1:
> + /* end of idle interrupt region (the region size must be power of two) */
> +SYM_INNER_LABEL(__r4k_wait_exit, SYM_L_LOCAL)
You can also use label 1 as the LoongArch version.
> jr ra
> - nop
> .set pop
> END(__r4k_wait)
>
> @@ -136,10 +140,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
> - bne k0, k1, \handler
> + PTR_LA k1, __r4k_wait_exit
> + /* 3 instructions rollback region */
For MIPS the rollback region is not 3 instructions, and so you cannot
use 0xc below. I think there is no chance for the wait instruction
after this patch.
Huacai
> + ori k0, k0, 0x0c
> + bne k0, k1, \handler
> MTC0 k0, CP0_EPC
> .set pop
> .endm
> 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
>
>
