Skip to contentSkip to conversation

MCA/TLB recovery

PATCH In review Luck, Tony 3 participants 6 messages Oct 3, 2003 – Oct 8, 2003

lore
Share & tools
lore

Luck, Tony

author
#0 lore raw

PATCH 1/1 MCA/TLB recovery

This part is just a clean-up of the logging code to only print registers that are implemented (instead of blindly dumping CR[0] ... CR[127] and AR[0] ... AR[127].

-Tony

arch/ia64/kernel/mca.c +13 -5
diff -ru linux-2.6.0-test5/arch/ia64/kernel/mca.c temp/arch/ia64/kernel/mca.c
@@ -1498,6 +1498,12 @@
 			lh->timestamp.slh_second, lh->severity);
 }
 
+/* Bitmasks of implemented registers */
+static u64 arbits[2] = { 0x11117f2f00ffUL, 0x7UL };
+static u64 brbits[1] = { 0xffUL };
+static u64 crbits[2] = { 0x3fb0107UL, 0x307ffUL };
+static u64 rrbits[1] = { 0xffUL };
+
 /*
  * ia64_log_processor_regs_print
  *	Print the contents of the saved processor register(s) in the format
@@ -1513,6 +1519,7 @@
 void
 ia64_log_processor_regs_print(u64	*regs,
 			      int	reg_num,
+			      u64	*reg_bits,
 			      char	*reg_class,
 			      char	*reg_prefix,
 			      prfunc_t	prfunc)
@@ -1521,7 +1528,8 @@
 
 	prfunc("+%s Registers\n", reg_class);
 	for (i = 0; i < reg_num; i++)
-		prfunc("+ %s[%d] 0x%lx\n", reg_prefix, i, regs[i]);
+		if (reg_bits[i/64] & (1UL << (i%64)))
+			prfunc("+ %s[%d] 0x%lx\n", reg_prefix, i, regs[i]);
 }
 
 /*
@@ -2159,22 +2167,22 @@
 
 		/* Print branch register contents if valid */
 		if (spsi->valid.br)
-			ia64_log_processor_regs_print(spsi->br, 8, "Branch", "br",
+			ia64_log_processor_regs_print(spsi->br, 8, brbits, "Branch", "br",
 						      prfunc);
 
 		/* Print control register contents if valid */
 		if (spsi->valid.cr)
-			ia64_log_processor_regs_print(spsi->cr, 128, "Control", "cr",
+			ia64_log_processor_regs_print(spsi->cr, 128, crbits, "Control", "cr",
 						      prfunc);
 
 		/* Print application register contents if valid */
 		if (spsi->valid.ar)
-			ia64_log_processor_regs_print(spsi->ar, 128, "Application",
+			ia64_log_processor_regs_print(spsi->ar, 128, arbits, "Application",
 						      "ar", prfunc);
 
 		/* Print region register contents if valid */
 		if (spsi->valid.rr)
-			ia64_log_processor_regs_print(spsi->rr, 8, "Region", "rr",
+			ia64_log_processor_regs_print(spsi->rr, 8, rrbits, "Region", "rr",
 						      prfunc);
 
 		/* Print floating-point register contents if valid */
#1 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Tony

Tony> This part is just a clean-up of the logging code to only Tony> print registers that are implemented (instead of blindly Tony> dumping CR[0] ... CR[127] and AR[0] ... AR[127].

  Tony> +/* Bitmasks of implemented registers */
  Tony> +static u64 arbits[2] = { 0x11117f2f00ffUL, 0x7UL };
  Tony> +static u64 crbits[2] = { 0x3fb0107UL, 0x307ffUL };

Shouldn't these masks be obtained from PAL?

  Tony> +static u64 brbits[1] = { 0xffUL };
  Tony> +static u64 rrbits[1] = { 0xffUL };

These look a bit weird to me (since they're architected), but I guess it's your call...

	--david
author
#2 lore raw
In reply to Luck, Tony
Quoted 5 lines
  Tony> +/* Bitmasks of implemented registers */
  Tony> +static u64 arbits[2] = { 0x11117f2f00ffUL, 0x7UL };
  Tony> +static u64 crbits[2] = { 0x3fb0107UL, 0x307ffUL };

Shouldn't these masks be obtained from PAL?

You are right ... PAL_REGISTER_INFO returns this information (I had forgotten that it existed).

Quoted 5 lines
  Tony> +static u64 brbits[1] = { 0xffUL };
  Tony> +static u64 rrbits[1] = { 0xffUL };

These look a bit weird to me (since they're architected), but I guess it's your call...

This just fell out of sharing the same code to print the register values ... it might look less weird if I fix the arbits/crbits code to use PAL_REGISTER_INFO.

Overall this part of the patch is mostly fluff. Long term the right thing to do is remove all these printk()s from the kernel and leave paring of SAL error records to some user mode code (using some mechanism like Bjorn did in 2.4).

Any comments on parts 1 & 3???

-Tony

#3 lore raw
In reply to Luck, Tony
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Tony

Tony> Any comments on parts 1 & 3???

I'm still catching up with my mail backlog, so I didn't have time to look at it very carefully, but from what I saw, they looked fine to me. The big issue now, of course, is that Linus wants to be in bug-fix-only, so the cosmetic fixes would have to be removed from the patch before I can accept it.

	--david
#4 lore raw
In reply to Luck, Tony
Quoted 9 lines · David Mosberger
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Tony

Tony> Any comments on parts 1 & 3???

I'm still catching up with my mail backlog, so I didn't have time to look at it very carefully, but from what I saw, they looked fine to me. The big issue now, of course, is that Linus wants to be in bug-fix-only, so the cosmetic fixes would have to be removed from the patch before I can accept it.

Really, even for ia64 code? Does that mean all the changes I've recently pushed you will be dropped? What about the fix for CONFIG_DISCONTIGMEM + CONFIG_VIRTUAL_MEM_MAP that I've been working so hard to clean up?

Thanks,
Jesse

#5 lore raw
In reply to Luck, Tony
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · On Wed, 8 Oct 2003 15:45:16 -0700, [email protected] (Jesse Barnes) said:

Jesse> Really, even for ia64 code?

Yes, Linus was very clear that the bug-fix-only applies to the entire kernel, including all platform-specific code.

Jesse> Does that mean all the changes I've recently pushed you will Jesse> be dropped? What about the fix for CONFIG_DISCONTIGMEM + Jesse> CONFIG_VIRTUAL_MEM_MAP that I've been working so hard to Jesse> clean up?

If it's a bug fix, it should be OK (for some definition of "bug"). But we may have to expend some extra effort to make it _really_ a bug fix only.

	--david