Re: SMP/preempt bug in matt-nb5-mips64

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Sun, Sep 05, 2010 at 11:35:41PM +0200, Manuel Bouyer wrote:
> Hello,
> I found a problem in matt-nb5-mips64's mips/mips/spl.S, regarding
> curcpu() and preemption. In both _splraise and _splsw_splhigh,
> curcpu() is loaded from L_CPU(MIPS_CURLWP) in a register early,
> especially before disabling interrupt. If the current IPL is 0,
> the current thread can be preempted and rescheduled on another CPU,
> and the new SPL is written back to the wrong cpu_info.
> >From there, bad things happens (what I've seen is an infinite loop
> from the interrupt handler on the victim CPU, because _splsw_splhigh
> thinks we're already at splhigh and do nothing, when interrupts are
> really enabled).
> The attached patch seems to fix it for me: it's enough to reload
> curcpu() before writing back the new IPL, as for the above senario to
> happen the old IPL of both CPUs has to be 0.

Well, unfortunably this is not enough.
If we get preempted just after loading L_CPU(MIPS_CURLWP) and before
checking the spl, we may be moved to another CPU and run at a later
time. When we get back running we check the spl of the previous CPU
which may not be 0 any more.
I couldn't find a better way than disabling the interrupts before
checking the spl (well, we could disable preemption but I suspect
it cost more than just disabling interrupts). The attached patch now seems to
DTRT for me.

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Index: spl.S
===================================================================
--- spl.S       (revision 100)
+++ spl.S       (working copy)
@@ -66,27 +66,32 @@
         * a1 = this IPL (IPL_*)
         * Can only use a0-a3 and v0-v1
         */
+       /*
+        * disable interrupt before messing with spl, or we could be
+        * preempted and moved to a different CPU
+        */
+       mfc0    v1, MIPS_COP_0_STATUS           # fetch status register
+       mtc0    zero, MIPS_COP_0_STATUS         ## disable interrupts
+       COP0_SYNC
        PTR_L   a3, L_CPU(MIPS_CURLWP)
        INT_L   v0, CPU_INFO_CPL(a3)            # get current IPL from cpu_info
-       sltu    v1, a1, v0                      # newipl < curipl
-       bnez    v1, 1f                          # yes, don't change.
-       mfc0    v1, MIPS_COP_0_STATUS           # fetch status register
+       sltu    a2, a1, v0                      # newipl < curipl
+       bnez    a2, 1f                          # yes, don't change.
+       nop
        or      v1, MIPS_INT_MASK               # enable all interrupts
        xor     a0, v1                          # disable ipl's masked bits
-       DYNAMIC_STATUS_MASK(a0,v0)              # machine dependent masking
-       mtc0    zero, MIPS_COP_0_STATUS         ## disable interrupts
-       COP0_SYNC
+       DYNAMIC_STATUS_MASK(a0,a2)              # machine dependent masking
        INT_S   a1, CPU_INFO_CPL(a3)            ## save IPL in cpu_info
        mtc0    a0, MIPS_COP_0_STATUS           ## store back
        COP0_SYNC
-#ifdef PARANOIA
        j       ra
         nop
-#endif /* PARANOIA */
 1:
+       mtc0    v1, MIPS_COP_0_STATUS           # restore status
+       COP0_SYNC
 #ifdef PARANOIA
        mfc0    v1, MIPS_COP_0_STATUS
-       and     a0, v1                          # a1 contains bit that MBZ
+       and     a0, v1                          # a0 contains bit that MBZ
 3:     bnez    a0, 3b                          # loop forever
         nop
 #endif /* PARANOIA */
@@ -98,7 +103,7 @@
        PTR_L   a3, L_CPU(MIPS_CURLWP)          # get cpu_info
        INT_L   a2, CPU_INFO_CPL(a3)            # get IPL from cpu_info
        beq     a0, a2, 2f                      # if same, nothing to do
-        nop
+        sync
 #ifdef PARANOIA
        sltu    v0, a0, a2                      # v0 = a0 < a2
 99:    beqz    v0, 99b                         # loop forever if false
@@ -210,30 +215,22 @@
 
 STATIC_LEAF(_splsw_splhigh)
 STATIC_XLEAF(_splsw_splhigh_noprof)
-       PTR_L   a3, L_CPU(MIPS_CURLWP)
-       INT_L   v0, CPU_INFO_CPL(a3)            # get current IPL from cpu_info
-       li      a1, IPL_HIGH                    # 
-       beq     v0, a1, 1f                      # don't do anything if IPL_HIGH
+       /*
+        * disable interrupt before messing with spl, or we could be
+        * preempted and moved to a different CPU
+        */
        mfc0    v1, MIPS_COP_0_STATUS           # fetch status register
        and     a0, v1, MIPS_INT_MASK           # select all interrupts
        xor     a0, v1                          # clear all interrupts
-       DYNAMIC_STATUS_MASK(a0,a2)              # machine dependent masking
+       DYNAMIC_STATUS_MASK(a0,a1)              # machine dependent masking
        mtc0    a0, MIPS_COP_0_STATUS           ## store back
        COP0_SYNC
+       PTR_L   a3, L_CPU(MIPS_CURLWP)
+       INT_L   v0, CPU_INFO_CPL(a3)            # get current IPL from cpu_info
+       li      a1, IPL_HIGH                    # 
        INT_S   a1, CPU_INFO_CPL(a3)            ## save IPL in cpu_info
-#ifdef PARANOIA
        j       ra                              ## return
         nop
-#endif /* PARANOIA */
-1:
-#ifdef PARANOIA
-       mfc0    v1, MIPS_COP_0_STATUS           # fetch status register
-       and     v1, MIPS_INT_MASK               # any int bits set?
-2:     bnez    v1, 2b                          # loop forever.
-        nop
-#endif /* PARANOIA */
-       j       ra                              ## return
-        nop
 END(_splsw_splhigh)
 
 STATIC_LEAF(_splsw_splddb)