Hi, Marco,
On Thu, Feb 27, 2025 at 7:41 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.
>
> 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@xxxxxxxx>
> ---
> arch/mips/kernel/genex.S | 40 ++++++++++++++++++++++------------------
> arch/mips/kernel/idle.c | 1 -
> 2 files changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> index a572ce36a24f..7b66ab239049 100644
> --- a/arch/mips/kernel/genex.S
> +++ b/arch/mips/kernel/genex.S
> @@ -104,25 +104,28 @@ handle_vcei:
>
> __FINIT
>
> - .align 5 /* 32 byte rollback region */
> + .align 5
> LEAF(__r4k_wait)
> .set push
> .set noreorder
> - /* start of rollback region */
> - LONG_L t0, TI_FLAGS($28)
> - nop
> - andi t0, _TIF_NEED_RESCHED
> - bnez t0, 1f
> - nop
> - nop
> - nop
> -#ifdef CONFIG_CPU_MICROMIPS
> - nop
> - nop
> - nop
> - nop
> -#endif
> + /* start of idle interrupt region */
> + MFC0 t0, CP0_STATUS
> + /* Enable Interrput */
> + ori t0, 0x1f
> + xori t0, 0x1e
> + MTC0 t0, CP0_STATUS
> + _ssnop
> + _ssnop
> + _ssnop
> + _ehb
> .set MIPS_ISA_ARCH_LEVEL_RAW
> + /*
> + * If an interrupt lands here, between enabling interrupts above and
> + * going idle on the next instruction, we must *NOT* go idle since the
> + * interrupt could have set TIF_NEED_RESCHED or caused a timer to need
> + * resched. Fall through -- see rollback_handler below -- and have
> + * the idle loop take care of things.
> + */
> wait
> /* end of rollback region (the region size must be power of two) */
Now you add a _ehb in this version, so "(the region size must be power
of two)" should be removed.
> 1:
> @@ -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
> + /* 32 byte idle interrupt region */
And now the region is 36 bytes. :)
Huacai
> + 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
>
