* [PATCH 0/1] MIPS: Fix idle VS timer enqueue @ 2025-02-14 10:50 Marco Crivellari 2025-02-14 10:50 ` [PATCH] " Marco Crivellari 0 siblings, 1 reply; 7+ messages in thread From: Marco Crivellari @ 2025-02-14 10:50 UTC (permalink / raw) To: linux-mips, linux-kernel Cc: Thomas Bogendoerfer, Marco Crivellari, Frederic Weisbecker 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 Marco Crivellari (1): MIPS: Fix idle VS timer enqueue arch/mips/kernel/genex.S | 36 ++++++++++++++++++++---------------- arch/mips/kernel/idle.c | 1 - 2 files changed, 20 insertions(+), 17 deletions(-) -- 2.48.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] MIPS: Fix idle VS timer enqueue 2025-02-14 10:50 [PATCH 0/1] MIPS: Fix idle VS timer enqueue Marco Crivellari @ 2025-02-14 10:50 ` Marco Crivellari 2025-02-14 12:32 ` Frederic Weisbecker ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Marco Crivellari @ 2025-02-14 10:50 UTC (permalink / raw) To: linux-mips, linux-kernel Cc: Thomas Bogendoerfer, Marco Crivellari, Frederic Weisbecker MIPS re-enables interrupts on its idle routine and performs a TIF_NEED_RESCHED check afterwards before putting the CPU to sleep. The IRQs firing between the check and the 'wait' instruction may set the TIF_NEED_RESCHED flag. In order to deal with this possible race, IRQs interrupting __r4k_wait() rollback their return address to the beginning of __r4k_wait() so that TIF_NEED_RESCHED is checked again before going back to sleep. However idle IRQs can also queue timers that may require a tick reprogramming through a new generic idle loop iteration but those timers would go unnoticed here because __r4k_wait() only checks TIF_NEED_RESCHED. It doesn't check for pending timers. Fix this with fast-forwarding idle IRQs return address to the end of the idle routine instead of the beginning, so that the generic idle loop handles both TIF_NEED_RESCHED and pending timers. Signed-off-by: Marco Crivellari <marco.crivellari@suse.com> --- arch/mips/kernel/genex.S | 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) 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 */ + 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] MIPS: Fix idle VS timer enqueue 2025-02-14 10:50 ` [PATCH] " Marco Crivellari @ 2025-02-14 12:32 ` Frederic Weisbecker 2025-02-14 14:46 ` Huacai Chen 2025-02-15 5:05 ` Maciej W. Rozycki 2 siblings, 0 replies; 7+ messages in thread From: Frederic Weisbecker @ 2025-02-14 12:32 UTC (permalink / raw) To: Marco Crivellari Cc: linux-mips, linux-kernel, Thomas Bogendoerfer, Anna-Maria Behnsen, Thomas Gleixner, Peter Zijlstra Le Fri, Feb 14, 2025 at 11:50:47AM +0100, Marco Crivellari a écrit : > MIPS re-enables interrupts on its idle routine and performs > a TIF_NEED_RESCHED check afterwards before putting the CPU to sleep. > > The IRQs firing between the check and the 'wait' instruction may set the > TIF_NEED_RESCHED flag. In order to deal with this possible race, IRQs > interrupting __r4k_wait() rollback their return address to the > beginning of __r4k_wait() so that TIF_NEED_RESCHED is checked > again before going back to sleep. > > However idle IRQs can also queue timers that may require a tick > reprogramming through a new generic idle loop iteration but those timers > would go unnoticed here because __r4k_wait() only checks > TIF_NEED_RESCHED. It doesn't check for pending timers. > > Fix this with fast-forwarding idle IRQs return address to the end of the > idle routine instead of the beginning, so that the generic idle loop > handles both TIF_NEED_RESCHED and pending timers. > > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com> Acked-by: Frederic Weisbecker <frederic@kernel.org> Thanks a lot! ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] MIPS: Fix idle VS timer enqueue 2025-02-14 10:50 ` [PATCH] " Marco Crivellari 2025-02-14 12:32 ` Frederic Weisbecker @ 2025-02-14 14:46 ` Huacai Chen 2025-02-14 15:57 ` Marco Crivellari 2025-02-15 5:05 ` Maciej W. Rozycki 2 siblings, 1 reply; 7+ messages in thread From: Huacai Chen @ 2025-02-14 14:46 UTC (permalink / raw) To: Marco Crivellari Cc: linux-mips, linux-kernel, Thomas Bogendoerfer, Frederic Weisbecker Hi, Marco, On Fri, Feb 14, 2025 at 6:51 PM 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. > > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com> > --- > 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] MIPS: Fix idle VS timer enqueue 2025-02-14 14:46 ` Huacai Chen @ 2025-02-14 15:57 ` Marco Crivellari 2025-02-15 14:41 ` Huacai Chen 0 siblings, 1 reply; 7+ messages in thread From: Marco Crivellari @ 2025-02-14 15:57 UTC (permalink / raw) To: Huacai Chen Cc: linux-mips, linux-kernel, Thomas Bogendoerfer, Frederic Weisbecker, Anna-Maria Behnsen, Thomas Gleixner, Peter Zijlstra Hi, thanks for the replies! Huacai: I used the '1' label when I compiled the first time, but I noticed this warning, so I decided to use the label to avoid this: "WARNING: modpost: vmlinux: section mismatch in reference: rollback_except_vec_vi+0x4 (section: .text) -> except_vec4 (section: .init.text)" By the way, I will use the 1 label like you suggested. > 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. About this, does it look better if i don't change this part of the code (except for PTR_LA k1, 1b) ? eg. @@ -136,7 +144,7 @@ LEAF(__r4k_wait) .set push .set noat MFC0 k0, CP0_EPC - PTR_LA k1, __r4k_wait + PTR_LA k1, 1b ori k0, 0x1f /* 32 byte rollback region */ xori k0, 0x1f bne k0, k1, \handler So the real change would be: - /* start of rollback region */ - LONG_L t0, TI_FLAGS($28) + /* start of idle interrupt region */ + MFC0 k0, CP0_STATUS nop - andi t0, _TIF_NEED_RESCHED - bnez t0, 1f - nop + /* Enable Interrupt */ + ori k0, 0x1f + xori k0, 0x1e + MTC0 k0, CP0_STATUS nop nop Maybe what I'm going to say is silly... But considering the region should be a power of 2, maybe can also be changed with: ori k0, 0x0f addi k0, 1 Thanks for your time! On Fri, Feb 14, 2025 at 3:47 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > Hi, Marco, > > On Fri, Feb 14, 2025 at 6:51 PM 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. > > > > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com> > > --- > > 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 > > > > -- Marco Crivellari L3 Support Engineer, Technology & Product marco.crivellari@suse.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] MIPS: Fix idle VS timer enqueue 2025-02-14 15:57 ` Marco Crivellari @ 2025-02-15 14:41 ` Huacai Chen 0 siblings, 0 replies; 7+ messages in thread From: Huacai Chen @ 2025-02-15 14:41 UTC (permalink / raw) To: Marco Crivellari Cc: linux-mips, linux-kernel, Thomas Bogendoerfer, Frederic Weisbecker, Anna-Maria Behnsen, Thomas Gleixner, Peter Zijlstra On Fri, Feb 14, 2025 at 11:57 PM Marco Crivellari <marco.crivellari@suse.com> wrote: > > Hi, thanks for the replies! > > Huacai: I used the '1' label when I compiled the first time, but I > noticed this warning, so I decided to use the label to avoid this: > > "WARNING: modpost: vmlinux: section mismatch in reference: > rollback_except_vec_vi+0x4 (section: .text) -> except_vec4 (section: > .init.text)" > > By the way, I will use the 1 label like you suggested. > > > 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. > > About this, does it look better if i don't change this part of the > code (except for PTR_LA k1, 1b) ? > eg. > > @@ -136,7 +144,7 @@ LEAF(__r4k_wait) > .set push > .set noat > MFC0 k0, CP0_EPC > - PTR_LA k1, __r4k_wait > + PTR_LA k1, 1b > ori k0, 0x1f /* 32 byte rollback region */ > xori k0, 0x1f > bne k0, k1, \handler > > So the real change would be: > > - /* start of rollback region */ > - LONG_L t0, TI_FLAGS($28) > + /* start of idle interrupt region */ > + MFC0 k0, CP0_STATUS > nop > - andi t0, _TIF_NEED_RESCHED > - bnez t0, 1f > - nop > + /* Enable Interrupt */ > + ori k0, 0x1f > + xori k0, 0x1e > + MTC0 k0, CP0_STATUS > nop > nop > > Maybe what I'm going to say is silly... > But considering the region should be a power of 2, maybe can also be > changed with: > ori k0, 0x0f > addi k0, 1 > > Thanks for your time! I think you didn't get my key point, and maybe also didn't get Maciej's point, I paste my whole changes here which is tested on real hardware. diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S index b6de8e88c1bd..4b55115160f0 100644 --- a/arch/mips/kernel/genex.S +++ b/arch/mips/kernel/genex.S @@ -104,25 +104,27 @@ handle_vcei: __FINIT - .align 5 /* 32 byte rollback region */ + .align 5 LEAF(__r4k_wait) .set push .set noreorder - /* start of rollback region */ - LONG_L t0, TI_FLAGS($28) - nop - andi t0, _TIF_NEED_RESCHED - bnez t0, 1f - nop - nop - nop -#ifdef CONFIG_CPU_MICROMIPS - nop - nop - nop - nop -#endif + /* start of idle interrupt region */ + MFC0 t0, CP0_STATUS + /* Enable Interrupt */ + ori t0, 0x1f + xori t0, 0x1e + MTC0 t0, CP0_STATUS + _ssnop + _ssnop + _ssnop .set MIPS_ISA_ARCH_LEVEL_RAW + /* + * If an interrupt lands here, between enabling interrupts above and + * going idle on the next instruction, we must *NOT* go idle since the + * interrupt could have set TIF_NEED_RESCHED or caused a timer to need + * resched. Fall through -- see rollback_handler below -- and have + * the idle loop take care of things. + */ wait /* end of rollback region (the region size must be power of two) */ 1: @@ -136,9 +138,10 @@ LEAF(__r4k_wait) .set push .set noat MFC0 k0, CP0_EPC - PTR_LA k1, __r4k_wait - ori k0, 0x1f /* 32 byte rollback region */ - xori k0, 0x1f + PTR_LA k1, 1b + /* 32 byte idle interrupt region */ + ori k0, 0x1f + daddiu k0, 1 bne k0, k1, \handler MTC0 k0, CP0_EPC .set pop diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c index 5abc8b7340f8..1f74c0589f7e 100644 --- a/arch/mips/kernel/idle.c +++ b/arch/mips/kernel/idle.c @@ -37,7 +37,6 @@ static void __cpuidle r3081_wait(void) void __cpuidle r4k_wait(void) { - raw_local_irq_enable(); __r4k_wait(); raw_local_irq_disable(); } Huacai > > On Fri, Feb 14, 2025 at 3:47 PM Huacai Chen <chenhuacai@kernel.org> wrote: > > > > Hi, Marco, > > > > On Fri, Feb 14, 2025 at 6:51 PM 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. > > > > > > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com> > > > --- > > > 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 > > > > > > > > > > -- > > Marco Crivellari > > L3 Support Engineer, Technology & Product > > > > > marco.crivellari@suse.com ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] MIPS: Fix idle VS timer enqueue 2025-02-14 10:50 ` [PATCH] " Marco Crivellari 2025-02-14 12:32 ` Frederic Weisbecker 2025-02-14 14:46 ` Huacai Chen @ 2025-02-15 5:05 ` Maciej W. Rozycki 2 siblings, 0 replies; 7+ messages in thread From: Maciej W. Rozycki @ 2025-02-15 5:05 UTC (permalink / raw) To: Marco Crivellari Cc: linux-mips, linux-kernel, Thomas Bogendoerfer, Frederic Weisbecker On Fri, 14 Feb 2025, Marco Crivellari wrote: > 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 Data from CP0 will be delivered late to $k0 on MIPS III, you need to fill the coprocessor move delay slot here. Existing code takes care about it. I bet you won't trigger this with QEMU, you need to verify with real hw. For CONFIG_CPU_HAS_DIEI you want EI here instead. > + xori k0, 0x1e > + MTC0 k0, CP0_STATUS You need to clear the CP0 hazard here, just as `raw_local_irq_enable' does, because WAIT halts the pipeline and the interrupt enable state may not have been propagated beforehand otherwise. Again, not triggerable with QEMU. > @@ -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) */ Have you verified this still stands with CONFIG_CPU_MICROMIPS after your change? > +SYM_INNER_LABEL(__r4k_wait_exit, SYM_L_LOCAL) > jr ra > - nop > .set pop > END(__r4k_wait) You're dropping the delay slot NOP here, meaning that whatever comes next gets there, possibly "mfc0 $k0, $c0_epc" (depending on alignment), why? > @@ -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 */ > + ori k0, k0, 0x0c > + bne k0, k1, \handler How is `__r4k_wait_exit' guaranteed to have bits 3:2 set and bit 1 (for CONFIG_CPU_MICROMIPS) clear? Also EPC may or may not have bit 1 set. NB not a full review and certainly not for the change of semantics, but for some issues spotted with the low-level correctness of new code. Maciej ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-02-15 14:41 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-02-14 10:50 [PATCH 0/1] MIPS: Fix idle VS timer enqueue Marco Crivellari 2025-02-14 10:50 ` [PATCH] " Marco Crivellari 2025-02-14 12:32 ` Frederic Weisbecker 2025-02-14 14:46 ` Huacai Chen 2025-02-14 15:57 ` Marco Crivellari 2025-02-15 14:41 ` Huacai Chen 2025-02-15 5:05 ` Maciej W. Rozycki
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.