Correctly dereference ia64_mca_data

arch/ia64/kernel/mca_asm.S is treating per_cpu__ia64_mca_data as the start of the mca data, instead of as a pointer to the mca data. It ends up overwriting the rest of the per cpu area with the MCA stack and bspstore. Since we dereference ia64_mca_data several times, make it a macro.

Index: linux/arch/ia64/kernel/mca_asm.S
=================================--- linux.orig/arch/ia64/kernel/mca_asm.S 2005-02-08 18:02:43.000000000 +1100
+++ linux/arch/ia64/kernel/mca_asm.S 2005-02-08 19:08:25.000000000 +1100
@@ -101,6 +101,11 @@
ld8 tmp=[sal_to_os_handoff];; \
st8 [os_to_sal_handoff]=tmp;;

+#define GET_IA64_MCA_DATA(reg) \
+ GET_THIS_PADDR(reg, ia64_mca_data) \
+ ;; \
+ ld8 reg=[reg]
+
.global ia64_os_mca_dispatch
.global ia64_os_mca_dispatch_end
.global ia64_sal_to_os_handoff_state
@@ -309,14 +314,14 @@ err:
done_tlb_purge_and_reload:

// Setup new stack frame for OS_MCA handling
- GET_THIS_PADDR(r2, ia64_mca_data)
+ GET_IA64_MCA_DATA(r2)
;;
add r3 = IA64_MCA_CPU_STACKFRAME_OFFSET, r2 add r2 = IA64_MCA_CPU_RBSTORE_OFFSET, r2 ;;
rse_switch_context(r6,r3,r2);; // RSC management in this new context

- GET_THIS_PADDR(r2, ia64_mca_data)
+ GET_IA64_MCA_DATA(r2)
;;
add r2 = IA64_MCA_CPU_STACK_OFFSET+IA64_MCA_STACK_SIZE-16, r2 ;;
@@ -336,7 +341,7 @@ ia64_os_mca_virtual_begin:
ia64_os_mca_virtual_end:

// restore the original stack frame here
- GET_THIS_PADDR(r2, ia64_mca_data)
+ GET_IA64_MCA_DATA(r2)
;;
add r2 = IA64_MCA_CPU_STACKFRAME_OFFSET, r2
;;
@@ -380,7 +385,7 @@ ia64_os_mca_dispatch_end:
ia64_os_mca_proc_state_dump:
// Save bank 1 GRs 16-31 which will be used by c-language code when we switch // to virtual addressing mode.
- GET_THIS_PADDR(r2, ia64_mca_data)
+ GET_IA64_MCA_DATA(r2)
;;
add r2 = IA64_MCA_CPU_PROC_STATE_DUMP_OFFSET, r2
;;
@@ -613,7 +618,7 @@ end_os_mca_dump:
ia64_os_mca_proc_state_restore:

// Restore bank1 GR16-31
- GET_THIS_PADDR(r2, ia64_mca_data)
+ GET_IA64_MCA_DATA(r2)
;;
add r2 = IA64_MCA_CPU_PROC_STATE_DUMP_OFFSET, r2

That's the combination of my patch to (almost) correctly do the dereference, and Russ's patch (which fixes the spot where I managed to deref after adding the IA64_MCA_CPU_STACKFRAME_OFFSET). The macro is cleaner, and avoids the possibility of making a dumb mistake (like mine :-( )

Applied, and pushed.

This just survived me inserting a TLB error ... the resulting MCA was fixed and logged. The system is still ticking on all four cpus.

But Russ's last report of running with effectively the same patch wasn't positive:

Russ: Any detail on why the system died?

-Tony

Tony> Russ: Any detail on why the system died?

I assume you already checked that I didn't introduce any other stupid dereferencing bugs? I'd do it myself, but I'm still catching up with my email backlog...

	--david

I haven't spotted any others yet. Still looking.

-Tony

Russ is out in the woods for the next couple days. He should be back on Thursday.

Robin

Whatever bugs are left are subtle, not stupid. I've been injecting errors every 20 seconds[1]. I've seen 2 bad things happen so far:

1) My ethernet connection goes down within a dozen errors consistently. But I think this is an artifact of the error injection technology ... the same issue happens with 2.6.9

2) I saw one user process failure ... while stressing the system with a "make -j16" kernel build, one of the complilations error'd out with a bizarre internal gcc error. So possibly a register was corrupted???

-Tony

[1] I haven't been running the injector continuously all day at that rate ... just on and off with different tests running. But I have seen a few hundred MCAs recovered in this time.

Likely yes.
That's one of two errors I see on parisc-linux when a register is corrupted (segfaults are the symptom). And unfortunately the context switching has a corner case the tickles that bug on a regular basis. :^(

hth,
grant

I wrote a test program that loads up random values into registers (just r1-r31, a bunch of stacked registers, and f2-f127 for now) and then checks that all the registers haven't changed value a few thousand times, before reloading with a new set of random values.

I'm running that along with an MCA every 15 seconds.

It just went bang, saying that "f6" was corrupted ... but I'm not doing the right things to tickle the corner case on a regular basis ... so I only have the one data point so far.

-Tony

Does anyone have an interest in writing such a program for parisc?

The above comes from the ia64-linux mailing list.

We can run multiple instances on one cpu to test
the basic context switching is working right.

If it fails consistently with the same register getting corrupted, (gr26 is a suspect now), a code audit would be useful to confirm that the particular register is always getting properly saved/restored in a context switch.

If this test does NOT fail, then I'll suspect something with our syscall/glibc interface is fishy.

thanks,
grant
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Hmmm ... I'm not seeing anyplace where f6 is saved in the MCA path before we call up to ia64_mca_ucmc_handler() ... which is bad as f6 is designated as a scratch register.

-Tony

Tony> Hmmm ... I'm not seeing anyplace where f6 is saved in the MCA Tony> path before we call up to ia64_mca_ucmc_handler() ... which is Tony> bad as f6 is designated as a scratch register.

That certainly would do the trick.

Am I seeing this right: the path doesn't save practically nothing other than what is saved in the PAL min-state area? The path presumably also ought to switch the register-backing store (I think Keith alluded to this previously).

	--david

No ... we jump down to ia64_os_mca_proc_state_dump that saves a lot of stuff ... but not apparently the right stuff.

-Tony

Tony> No ... we jump down to ia64_os_mca_proc_state_dump that saves Tony> a lot of stuff ... but not apparently the right stuff.

Ah, yes, that looks (slightly) better. That code is really hard to follow and could use serious cleanup.

BTW: just as a heads-up: as I'm working on integrating libunwind, I'm changing the way INIT stack-dumps are done. The idea is to do things in such a way that we avoid having to create a (dummy) switch-stack structure before invoking the INIT-handler. Basically, it'll just simplify the assembly code. Hopefully, this will also make it easier to create a stack trace from an MCA-handler, but I'm not there yet.

	--david

No real additional information. It still fails with the latest release-2.6.11 tree. It sometimes recovers once or twice, but not more than that. Something is getting trashed or not restored correctly. At least it is easily reproduceable, which makes problems much easier to track down.

Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          [email protected]

I've established that the scratch FP registers need to be saved, but nobody is doing that.

Attached (untested) patch does that (not paticularly elegantly, but should be functional).

-Tony

===== arch/ia64/kernel/mca_asm.S 1.20 vs edited =====

@@ -599,6 +599,15 @@
 	add		r4=1,r4
 	br.cloop.sptk.few	cStRR
 	;;
+
+// save scratch FP regs
+	stf.spill [r2]=f6,16;;
+	stf.spill [r2]=f7,16;;
+	stf.spill [r2]=f8,16;;
+	stf.spill [r2]=f9,16;;
+	stf.spill [r2]=f10,16;;
+	stf.spill [r2]=f11,16;;
+
 end_os_mca_dump:
 	br	ia64_os_mca_done_dump;;
 

@@ -834,6 +843,15 @@
 	;;
 	mov		ar.lc=r5
 	;;
+
+// restore scratch FP regs
+	ldf.fill	f6=[r2],16;;
+	ldf.fill	f7=[r2],16;;
+	ldf.fill	f8=[r2],16;;
+	ldf.fill	f9=[r2],16;;
+	ldf.fill	f10=[r2],16;;
+	ldf.fill	f11=[r2],16;;
+
 end_os_mca_restore:
 	br	ia64_os_mca_done_restore;;
 

As davidm has pointed out, if the OS MCA handler saves the scratch and preserved registers before calling C and restores the values before returning to SAL then we have no problems. By definition, the only registers that matter are those in struct pt_regs. Think of an MCA as just another type of interrupt, with exactly the same requirements for saving registers.

I am completely dropping the proc_state_dump data area and 90% of the code in os_mca_dump/restore. Instead of saving everything in sight, I create a struct pt_regs from the current registers plus some data from the min_state_save area. That has three benefits - it gets rid of the special case RSE stack frame, it gives a real pt_regs for unwinding through the MCA handler and it guarantees that we save the required set of registers.

Work in progress, I should have a patch by Monday.

I should have added that my ugly fp-saving band-aid patch was just intended for temporary use (small enough to take in this late stage of 2.6.11-rcN development). Treating MCA as just another interrupt sounds to be *much* cleaner. I look forward to seeing the patch.

-Tony

That sounds like the right approach.

Thanks, Keith.

Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          [email protected]

A few people asked whether I could post the program ... it took a while to get sign-off ... but that gave me time to add "branch", "predicate" and half a dozen "application" registers to the mix, plus make it print the name of the register that was nuked (instead of a number that required manual translation).

I've tested it by using a debugger to zap one of each class of register that is being monitored to check that it works.

http://www.kernel.org/pub/linux/kernel/people/aegl/ia64regcheck.tgz

Usage ... compile, and run a few copies. If they all "exit(0)" (which may take a couple of days) the test passed. Otherwise you should see the name of the register printed to stderr, and exit code 1.

Apart from the MCA case, I haven't seen it report a problem yet ... but I've only run a few hours.

-Tony

I've started running multiple copies of checker with the error injection code and so far none have indicated any errors on Altix systems.

Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          [email protected]

When I try that patch, the system wedges in the MCA code (on Altix).

Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          [email protected]

That's a promising start. Which errors are you injecting ... I saw the "f6" problem on a TLB error (which could well have been detected and reported in the context of the checker program).

If you are still doing 2xECC memory errors, then they'll either be in the context of some other process, which will be killed. Or if you are in the context of one of the "checker" programs, it will be killed before it can see whether all the registers had the right value. So this might not be telling us much.

:-( Well I'll wait for Keith to do it properly with pt_regs etc.

-Tony