[PATCH] MIPS: make local_irq_disable macro safe for non-mipsr2

[PATCH] MIPS: make local_irq_disable macro safe for non-mipsr2

[Jim Quinlan]

For non-mipsr2 processors, the local_irq_disable contains an mfc0-mtc0
pair with instructions inbetween.  With preemption enabled, this sequence
may get preempted and effect a stale value of CP0_STATUS when executing
the mtc0 instruction.  This commit avoids this scenario by incrementing
the preempt count before the mfc0 and decrementing it after the mtc9.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 arch/mips/include/asm/asmmacro.h |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/arch/mips/include/asm/asmmacro.h b/arch/mips/include/asm/asmmacro.h
index 6c8342a..3f809a4 100644
--- a/arch/mips/include/asm/asmmacro.h
+++ b/arch/mips/include/asm/asmmacro.h
@@ -9,6 +9,7 @@
 #define _ASM_ASMMACRO_H
 
 #include <asm/hazards.h>
+//#include <asm/asm-offsets.h>
 
 #ifdef CONFIG_32BIT
 #include <asm/asmmacro-32.h>
@@ -54,11 +55,21 @@
 	.endm
 
 	.macro	local_irq_disable reg=t0
+#ifdef CONFIG_PREEMPT
+	lw      \reg, TI_PRE_COUNT($28)
+	addi    \reg, \reg, 1
+	sw      \reg, TI_PRE_COUNT($28)
+#endif
 	mfc0	\reg, CP0_STATUS
 	ori	\reg, \reg, 1
 	xori	\reg, \reg, 1
 	mtc0	\reg, CP0_STATUS
 	irq_disable_hazard
+#ifdef CONFIG_PREEMPT
+	lw      \reg, TI_PRE_COUNT($28)
+	addi    \reg, \reg, -1
+	sw      \reg, TI_PRE_COUNT($28)
+#endif
 	.endm
 #endif /* CONFIG_MIPS_MT_SMTC */
 
-- 
1.7.6

Re: [PATCH] MIPS: make local_irq_disable macro safe for non-mipsr2

[David Daney]

On 11/27/2013 12:34 PM, Jim Quinlan wrote:
> For non-mipsr2 processors, the local_irq_disable contains an mfc0-mtc0
> pair with instructions inbetween.  With preemption enabled, this sequence
> may get preempted and effect a stale value of CP0_STATUS when executing
> the mtc0 instruction.  This commit avoids this scenario by incrementing
> the preempt count before the mfc0 and decrementing it after the mtc9.
>
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>   arch/mips/include/asm/asmmacro.h |   11 +++++++++++
>   1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/arch/mips/include/asm/asmmacro.h b/arch/mips/include/asm/asmmacro.h
> index 6c8342a..3f809a4 100644
> --- a/arch/mips/include/asm/asmmacro.h
> +++ b/arch/mips/include/asm/asmmacro.h
> @@ -9,6 +9,7 @@
>   #define _ASM_ASMMACRO_H
>
>   #include <asm/hazards.h>
> +//#include <asm/asm-offsets.h>

I'm guessing it didn't pass checkpatch.pl

>
>   #ifdef CONFIG_32BIT
>   #include <asm/asmmacro-32.h>
> @@ -54,11 +55,21 @@
>   	.endm
>
>   	.macro	local_irq_disable reg=t0
> +#ifdef CONFIG_PREEMPT
> +	lw      \reg, TI_PRE_COUNT($28)
> +	addi    \reg, \reg, 1
> +	sw      \reg, TI_PRE_COUNT($28)
> +#endif

Does this need to be atomic?

More importantly, how does that prevent the problem you describe?

An interrupt can still occur between the mfc0 and mtc0 causing Status 
bits to be changed.  What bits do we care about?  is it IM*, I doubt you 
would see CU* changing.

Which bits are getting clobbered that shouldn't be?


>   	mfc0	\reg, CP0_STATUS
>   	ori	\reg, \reg, 1
>   	xori	\reg, \reg, 1
>   	mtc0	\reg, CP0_STATUS
>   	irq_disable_hazard
> +#ifdef CONFIG_PREEMPT
> +	lw      \reg, TI_PRE_COUNT($28)
> +	addi    \reg, \reg, -1
> +	sw      \reg, TI_PRE_COUNT($28)
> +#endif
>   	.endm
>   #endif /* CONFIG_MIPS_MT_SMTC */
>
>

Re: [PATCH] MIPS: make local_irq_disable macro safe for non-mipsr2

[Ralf Baechle]

On Wed, Nov 27, 2013 at 12:58:26PM -0800, David Daney wrote:

> On 11/27/2013 12:34 PM, Jim Quinlan wrote:
> >For non-mipsr2 processors, the local_irq_disable contains an mfc0-mtc0
> >pair with instructions inbetween.  With preemption enabled, this sequence
> >may get preempted and effect a stale value of CP0_STATUS when executing
> >the mtc0 instruction.  This commit avoids this scenario by incrementing
> >the preempt count before the mfc0 and decrementing it after the mtc9.
> >
> >Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> >---
> >  arch/mips/include/asm/asmmacro.h |   11 +++++++++++
> >  1 files changed, 11 insertions(+), 0 deletions(-)
> >
> >diff --git a/arch/mips/include/asm/asmmacro.h b/arch/mips/include/asm/asmmacro.h
> >index 6c8342a..3f809a4 100644
> >--- a/arch/mips/include/asm/asmmacro.h
> >+++ b/arch/mips/include/asm/asmmacro.h
> >@@ -9,6 +9,7 @@
> >  #define _ASM_ASMMACRO_H
> >
> >  #include <asm/hazards.h>
> >+//#include <asm/asm-offsets.h>
> 
> I'm guessing it didn't pass checkpatch.pl
> 
> >
> >  #ifdef CONFIG_32BIT
> >  #include <asm/asmmacro-32.h>
> >@@ -54,11 +55,21 @@
> >  	.endm
> >
> >  	.macro	local_irq_disable reg=t0
> >+#ifdef CONFIG_PREEMPT
> >+	lw      \reg, TI_PRE_COUNT($28)
> >+	addi    \reg, \reg, 1
> >+	sw      \reg, TI_PRE_COUNT($28)
> >+#endif
> 
> Does this need to be atomic?
> 
> More importantly, how does that prevent the problem you describe?
> 
> An interrupt can still occur between the mfc0 and mtc0 causing
> Status bits to be changed.  What bits do we care about?  is it IM*,
> I doubt you would see CU* changing.
> 
> Which bits are getting clobbered that shouldn't be?
> 
> 
> >  	mfc0	\reg, CP0_STATUS
> >  	ori	\reg, \reg, 1
> >  	xori	\reg, \reg, 1
> >  	mtc0	\reg, CP0_STATUS
> >  	irq_disable_hazard
> >+#ifdef CONFIG_PREEMPT
> >+	lw      \reg, TI_PRE_COUNT($28)
> >+	addi    \reg, \reg, -1
> >+	sw      \reg, TI_PRE_COUNT($28)
> >+#endif
> >  	.endm
> >  #endif /* CONFIG_MIPS_MT_SMTC */

This patch is sorting out the part that were missed by e97c5b6098 [MIPS:
Make irqflags.h functions preempt-safe for non-mipsr2 cpus].

This races are easy to hit on systems that use irq-cpu.c, that is the
IM bits directly to process device interrupts.  This for example was
the reason for the '99 vintage patch 94f05bab9b [The CPO_STATUS interrupt
mask patch].  Preemption just made the hole lots bigger.

  Ralf