_stext is ill-defined / SysRq-T broken

_stext is ill-defined / SysRq-T broken

[Johannes Stezenbach]

I use the following patch to enable SysRq on serial console
(via Ctrl-A-F == "Break" in minicom):


--- linux-oss-2.4.19-rc1-20020715.base/drivers/char/dummy_keyb.c	Fri Oct 19 03:24:11 2001
+++ linux-oss-2.4.19-rc1-20020715/drivers/char/dummy_keyb.c	Tue Jul 16 18:57:19 2002
@@ -23,9 +23,15 @@
  * CONFIG_VT.
  *
  */
+#include <linux/config.h>
 #include <linux/sched.h>
 #include <linux/errno.h>
 #include <linux/init.h>
+
+#if (!defined(CONFIG_PC_KEYB) && defined (CONFIG_MAGIC_SYSRQ))
+unsigned char kbd_sysrq_xlate[128];
+unsigned char kbd_sysrq_key = 0;
+#endif
 
 void kbd_leds(unsigned char leds)
 {



SysRq-T (showTasks) gives me the following output:
-----------------
SysRq : Show State

                         free                        sibling
  task             PC    stack   pid father child younger older
init          S 00000000     0     1      0   179               (NOTLB)

Call Trace:keventd       S 00000000     0     2      1             3       (L-TLB)

Call Trace:ksoftirqd_CPU S 00000000     0     3      1             4     2 (L-TLB)
[snip]
-----------------

Two problems:
a) there is no call trace, and 
b) there is bad formatting when the call trace is empty.


I found that the cause for this is that _stext (defined in head.S) is in
.text.init instead of .text, so it's past _etext.

I suggest the patch below to fix the bad formatting, but I'm
not shure about _stext. Should kernel_entry be placed into the .text
section, or should _stext = _ftext in ld.script?

Another nit (I don't have ejtag, so I don't care much):
  head.S: Assembler messages:
  head.S:102: Warning: Macro instruction expanded into multiple instructions in a branch delay slot
Maybe someone has spare nop to put there.


Johannes


--- linux-oss-2.4.19-rc1-20020715.base/arch/mips/kernel/traps.c	Mon Jul 15 15:42:06 2002
+++ linux-oss-2.4.19-rc1-20020715/arch/mips/kernel/traps.c	Wed Jul 24 20:08:05 2002
@@ -230,7 +230,7 @@ void show_trace(unsigned int *sp)
 	module_start = VMALLOC_START;
 	module_end = module_start + MODULE_RANGE;
 
-	printk("\nCall Trace:");
+	printk("Call Trace:");
 
 	while ((unsigned long) stack & (PAGE_SIZE -1)) {
 		unsigned long addr;
@@ -250,21 +250,20 @@ void show_trace(unsigned int *sp)
 		 */
 
 		if ((addr >= kernel_start && addr < kernel_end) ||
-		    (addr >= module_start && addr < module_end)) { 
+		    (addr >= module_start && addr < module_end)) {
 
-			printk(" [<%08lx>]", addr);
-			if (column++ == 5) {
-				printk("\n");
-				column = 0;
-			}
 			if (++i > 40) {
 				printk(" ...");
 				break;
 			}
+			if (column++ > 5) {
+				printk("\n           ");
+				column = 1;
+			}
+			printk(" [<%08lx>]", addr);
 		}
 	}
-	if (column != 0)
-		printk("\n");
+	printk("\n");
 }
 
 void show_trace_task(struct task_struct *tsk)

Re: _stext is ill-defined / SysRq-T broken

[Maciej W. Rozycki]

On Wed, 24 Jul 2002, Johannes Stezenbach wrote:

> I found that the cause for this is that _stext (defined in head.S) is in
> .text.init instead of .text, so it's past _etext.
> 
> I suggest the patch below to fix the bad formatting, but I'm
> not shure about _stext. Should kernel_entry be placed into the .text
> section, or should _stext = _ftext in ld.script?

 I think the intent is to skip initial parts of .text, specifically the
exception handlers (if linked at KSEG0).  kernel_entry is in .text.init
deliberately, as it's not needed except early in the boot process.  I
propose the following change.  Does it work for you? 

> Another nit (I don't have ejtag, so I don't care much):
>   head.S: Assembler messages:
>   head.S:102: Warning: Macro instruction expanded into multiple instructions in a branch delay slot
> Maybe someone has spare nop to put there.

 The warning is spurious, but there seems no way to shut up gas in this
condition.  Thus an extra "nop" looks like the right workaround.

 I'll check your patch at run-time later -- I've noticed that the current
output is less than satisfying.

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

patch-mips-2.4.19-rc1-20020719-stext-0
diff -up --recursive --new-file linux-mips-2.4.19-rc1-20020719.macro/arch/mips/kernel/head.S linux-mips-2.4.19-rc1-20020719/arch/mips/kernel/head.S
--- linux-mips-2.4.19-rc1-20020719.macro/arch/mips/kernel/head.S	2002-06-04 03:04:12.000000000 +0000
+++ linux-mips-2.4.19-rc1-20020719/arch/mips/kernel/head.S	2002-07-25 10:03:16.000000000 +0000
@@ -35,6 +35,10 @@
 		 */
 		.fill	0x400
 
+		/* The following two symbols are used for kernel profiling. */
+		EXPORT(stext)
+		EXPORT(_stext)
+
 		__INIT
 
 		/* Cache Error */
@@ -144,9 +148,6 @@ ejtag_return:
 		 */
 		NESTED(kernel_entry, 16, sp)
 		.set	noreorder
-		/* The following two symbols are used for kernel profiling. */
-		EXPORT(stext)
-		EXPORT(_stext)
 
 		/*
 		 * Stack for kernel and init, current variable

Re: _stext is ill-defined / SysRq-T broken

[Johannes Stezenbach]

On Thu, Jul 25, 2002 at 12:08:15PM +0200, Maciej W. Rozycki wrote:
> 
>  I think the intent is to skip initial parts of .text, specifically the
> exception handlers (if linked at KSEG0).  kernel_entry is in .text.init
> deliberately, as it's not needed except early in the boot process.  I
> propose the following change.  Does it work for you? 
> 
> diff -up --recursive --new-file linux-mips-2.4.19-rc1-20020719.macro/arch/mips/kernel/head.S linux-mips-2.4.19-rc1-20020719/arch/mips/kernel/head.S
> --- linux-mips-2.4.19-rc1-20020719.macro/arch/mips/kernel/head.S	2002-06-04 03:04:12.000000000 +0000
> +++ linux-mips-2.4.19-rc1-20020719/arch/mips/kernel/head.S	2002-07-25 10:03:16.000000000 +0000
> @@ -35,6 +35,10 @@
>  		 */
>  		.fill	0x400
>  
> +		/* The following two symbols are used for kernel profiling. */
> +		EXPORT(stext)
> +		EXPORT(_stext)
> +
>  		__INIT
>  
>  		/* Cache Error */
> @@ -144,9 +148,6 @@ ejtag_return:
>  		 */
>  		NESTED(kernel_entry, 16, sp)
>  		.set	noreorder
> -		/* The following two symbols are used for kernel profiling. */
> -		EXPORT(stext)
> -		EXPORT(_stext)
>  
>  		/*
>  		 * Stack for kernel and init, current variable
> 

Yes, works.

Just one nit while we're at it:
On most systems the .fill 0x400 is unnecessary  and wastes 1KB (more
than the .text.init size of head.o). Wouldn't it be better to remove the
.fill and require the LOADADDR in arch/mips/Makefile to be >= 0x80000400?


Johannes

Re: _stext is ill-defined / SysRq-T broken

[Maciej W. Rozycki]

On Thu, 25 Jul 2002, Johannes Stezenbach wrote:

> On most systems the .fill 0x400 is unnecessary  and wastes 1KB (more
> than the .text.init size of head.o). Wouldn't it be better to remove the
> .fill and require the LOADADDR in arch/mips/Makefile to be >= 0x80000400?

 It probably would, but I'm afraid we are bound by various firmware's
limitations.  Not all systems seem to be able to load a system executable
at an arbitrary address.  Anyone can comment?

 Anyway a linker script magic seems to be possible to achieve a similar
result without hurting systems that set LOADADDR above 0x800003ff.  I'll
look into it.

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: _stext is ill-defined / SysRq-T broken

[Johannes Stezenbach]

On Thu, Jul 25, 2002 at 12:08:15PM +0200, Maciej W. Rozycki wrote:
> On Wed, 24 Jul 2002, Johannes Stezenbach wrote:
> 
> > I suggest the patch below to fix the bad formatting, but I'm
> > not shure about _stext. Should kernel_entry be placed into the .text
> > section, or should _stext = _ftext in ld.script?
> 
>  I'll check your patch at run-time later -- I've noticed that the current
> output is less than satisfying.

I just noticed that my patch garbles Oops output,
because I removed the initial "\n" from the printk("Call Trace").

It can be solved by adding a printk("\n") at the very end of show_stack().
I grepped the whole kernel for other uses of show_trace()
and show_stack(), and found nothing that would conflict with it.


Johannes

[patch] Oops and magic SysRq stack dump clean-ups

[Maciej W. Rozycki]

Hello,

 I've reviewed the stack dumping code more thoroughly and here is the
result.  Please check if it's OK for you.  Tested visually with oopses and
<SysRq>+<t> on MIPS/Linux and MIPS64/Linux.  The idea is to fit as much
data as possible in as little space as possible and at the same time lay
the numbers out visually nicely so that manual copying of output from a
terminal for ksymoops analysis is easier for a reader.  Tools ignore
spacing when processing such output anyway.

 Based somewhat on the i386 port.  Addresses cast to signed longs, since
they are such on MIPS (additionally, the code doesn't care anyway but the
resulting source is smaller).  Any objections? 

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

patch-mips-2.4.19-rc1-20020719-mips-show_trace-8
diff -up --recursive --new-file linux-mips-2.4.19-rc1-20020719.macro/arch/mips/kernel/traps.c linux-mips-2.4.19-rc1-20020719/arch/mips/kernel/traps.c
--- linux-mips-2.4.19-rc1-20020719.macro/arch/mips/kernel/traps.c	2002-07-08 16:46:04.000000000 +0000
+++ linux-mips-2.4.19-rc1-20020719/arch/mips/kernel/traps.c	2002-07-27 11:48:19.000000000 +0000
@@ -181,61 +181,95 @@ static inline void simulate_sc(struct pt
 }
 
 /*
+ * If the address is either in the .text section of the
+ * kernel, or in the vmalloc'ed module regions, it *may*
+ * be the address of a calling routine
+ */
+
+#ifdef CONFIG_MODULES
+
+extern struct module *module_list;
+extern struct module kernel_module;
+
+static inline int kernel_text_address(long addr)
+{
+	extern char _stext, _etext;
+	int retval = 0;
+	struct module *mod;
+
+	if (addr >= (long) &_stext && addr <= (long) &_etext)
+		return 1;
+
+	for (mod = module_list; mod != &kernel_module; mod = mod->next) {
+		/* mod_bound tests for addr being inside the vmalloc'ed
+		 * module area. Of course it'd be better to test only
+		 * for the .text subset... */
+		if (mod_bound(addr, 0, mod)) {
+			retval = 1;
+			break;
+		}
+	}
+
+	return retval;
+}
+
+#else
+
+static inline int kernel_text_address(long addr)
+{
+	extern char _stext, _etext;
+
+	return (addr >= (long) &_stext && addr <= (long) &_etext);
+}
+
+#endif
+
+/*
  * This routine abuses get_user()/put_user() to reference pointers
  * with at least a bit of error checking ...
  */
-void show_stack(unsigned int *sp)
+void show_stack(long *sp)
 {
 	int i;
-	unsigned int *stack;
-
-	stack = sp ? sp : (unsigned int *)&sp;
-	i = 0;
+	long stackdata;
 
-	printk("Stack:");
-	while ((unsigned long) stack & (PAGE_SIZE - 1)) {
-		unsigned long stackdata;
+	sp = sp ? sp : (long *)&sp;
 
-		if (__get_user(stackdata, stack++)) {
-			printk(" (Bad stack address)");
+	printk("Stack:   ");
+	i = 1;
+	while ((long) sp & (PAGE_SIZE - 1)) {
+		if (i && ((i % 8) == 0))
+			printk("\n");
+		if (i > 40) {
+			printk(" ...");
 			break;
 		}
 
-		printk(" %08lx", stackdata);
-
-		if (++i > 40) {
-			printk(" ...");
+		if (__get_user(stackdata, sp++)) {
+			printk(" (Bad stack address)");
 			break;
 		}
 
-		if (i % 8 == 0)
-			printk("\n      ");
+		printk(" %08lx", stackdata);
+		i++;
 	}
+	printk("\n");
 }
 
-void show_trace(unsigned int *sp)
+void show_trace(long *sp)
 {
 	int i;
-	int column = 0;
-	unsigned int *stack;
-	unsigned long kernel_start, kernel_end;
-	unsigned long module_start, module_end;
-	extern char _stext, _etext;
-
-	stack = sp ? sp : (unsigned int *) &sp;
-	i = 0;
-
-	kernel_start = (unsigned long) &_stext;
-	kernel_end = (unsigned long) &_etext;
-	module_start = VMALLOC_START;
-	module_end = module_start + MODULE_RANGE;
+	long addr;
 
-	printk("\nCall Trace:");
+	sp = sp ? sp : (long *) &sp;
 
-	while ((unsigned long) stack & (PAGE_SIZE -1)) {
-		unsigned long addr;
+	printk("Call Trace:  ");
+	i = 1;
+	while ((long) sp & (PAGE_SIZE - 1)) {
 
-		if (__get_user(addr, stack++)) {
+		if (__get_user(addr, sp++)) {
+			if (i && ((i % 6) == 0))
+				printk("\n");
 			printk(" (Bad stack address)\n");
 			break;
 		}
@@ -249,27 +283,24 @@ void show_trace(unsigned int *sp)
 		 * out the call path that was taken.
 		 */
 
-		if ((addr >= kernel_start && addr < kernel_end) ||
-		    (addr >= module_start && addr < module_end)) { 
-
-			printk(" [<%08lx>]", addr);
-			if (column++ == 5) {
+		if (kernel_text_address(addr)) {
+			if (i && ((i % 6) == 0))
 				printk("\n");
-				column = 0;
-			}
-			if (++i > 40) {
+			if (i > 40) {
 				printk(" ...");
 				break;
 			}
+
+			printk(" [<%08lx>]", addr);
+			i++;
 		}
 	}
-	if (column != 0)
-		printk("\n");
+	printk("\n");
 }
 
 void show_trace_task(struct task_struct *tsk)
 {
-	show_trace((unsigned int *)tsk->thread.reg29);
+	show_trace((long *)tsk->thread.reg29);
 }
 
 void show_code(unsigned int *pc)
@@ -321,8 +352,8 @@ void show_registers(struct pt_regs *regs
 	show_regs(regs);
 	printk("Process %s (pid: %d, stackpage=%08lx)\n",
 		current->comm, current->pid, (unsigned long) current);
-	show_stack((unsigned int *) regs->regs[29]);
-	show_trace((unsigned int *) regs->regs[29]);
+	show_stack((long *) regs->regs[29]);
+	show_trace((long *) regs->regs[29]);
 	show_code((unsigned int *) regs->cp0_epc);
 	printk("\n");
 }
diff -up --recursive --new-file linux-mips-2.4.19-rc1-20020719.macro/arch/mips64/kernel/traps.c linux-mips-2.4.19-rc1-20020719/arch/mips64/kernel/traps.c
--- linux-mips-2.4.19-rc1-20020719.macro/arch/mips64/kernel/traps.c	2002-07-08 16:46:08.000000000 +0000
+++ linux-mips-2.4.19-rc1-20020719/arch/mips64/kernel/traps.c	2002-07-27 11:43:15.000000000 +0000
@@ -70,60 +70,91 @@ int kstack_depth_to_print = 24;
 #define OPCODE 0xfc000000
 
 /*
+ * If the address is either in the .text section of the
+ * kernel, or in the vmalloc'ed module regions, it *may*
+ * be the address of a calling routine
+ */
+
+#ifdef CONFIG_MODULES
+
+extern struct module *module_list;
+extern struct module kernel_module;
+
+static inline int kernel_text_address(long addr)
+{
+	extern char _stext, _etext;
+	int retval = 0;
+	struct module *mod;
+
+	if (addr >= (long) &_stext && addr <= (long) &_etext)
+		return 1;
+
+	for (mod = module_list; mod != &kernel_module; mod = mod->next) {
+		/* mod_bound tests for addr being inside the vmalloc'ed
+		 * module area. Of course it'd be better to test only
+		 * for the .text subset... */
+		if (mod_bound(addr, 0, mod)) {
+			retval = 1;
+			break;
+		}
+	}
+
+	return retval;
+}
+
+#else
+
+static inline int kernel_text_address(long addr)
+{
+	extern char _stext, _etext;
+
+	return (addr >= (long) &_stext && addr <= (long) &_etext);
+}
+
+#endif
+
+/*
  * This routine abuses get_user()/put_user() to reference pointers
  * with at least a bit of error checking ...
  */
-void show_stack(unsigned long *sp)
+void show_stack(long *sp)
 {
 	int i;
-	unsigned long *stack;
-
-	stack = sp;
-	i = 0;
+	long stackdata;
 
 	printk("Stack:");
-	while ((unsigned long) stack & (PAGE_SIZE - 1)) {
-		unsigned long stackdata;
-
-		if (__get_user(stackdata, stack++)) {
-			printk(" (Bad stack address)");
+	i = 0;
+	while ((long) sp & (PAGE_SIZE - 1)) {
+		if (i && ((i % 4) == 0))
+			printk("\n      ");
+		if (i > 40) {
+			printk(" ...");
 			break;
 		}
 
-		printk(" %016lx", stackdata);
-
-		if (++i > 40) {
-			printk(" ...");
+		if (__get_user(stackdata, sp++)) {
+			printk(" (Bad stack address)");
 			break;
 		}
 
-		if (i % 4 == 0)
-			printk("\n      ");
+		printk(" %016lx", stackdata);
+		i++;
 	}
+	printk("\n");
 }
 
-void show_trace(unsigned long *sp)
+void show_trace(long *sp)
 {
 	int i;
-	unsigned long *stack;
-	unsigned long kernel_start, kernel_end;
-	unsigned long module_start, module_end;
-	extern char _stext, _etext;
+	long addr;
 
-	stack = sp;
+	printk("Call Trace:");
 	i = 0;
+	while ((long) sp & (PAGE_SIZE - 1)) {
 
-	kernel_start = (unsigned long) &_stext;
-	kernel_end = (unsigned long) &_etext;
-	module_start = VMALLOC_START;
-	module_end = module_start + MODULE_RANGE;
-
-	printk("\nCall Trace:");
-
-	while ((unsigned long) stack & (PAGE_SIZE - 1)) {
-		unsigned long addr;
-
-		if (__get_user(addr, stack++)) {
+		if (__get_user(addr, sp++)) {
+			if (i && ((i % 3) == 0))
+				printk("\n           ");
 			printk(" (Bad stack address)\n");
 			break;
 		}
@@ -137,25 +168,24 @@ void show_trace(unsigned long *sp)
 		 * out the call path that was taken.
 		 */
 
-		if ((addr >= kernel_start && addr < kernel_end) ||
-		    (addr >= module_start && addr < module_end)) { 
-
-			/* Since our kernel is still at KSEG0,
-			 * truncate the address so that ksymoops
-			 * understands it.
-			 */
-			printk(" [<%08x>]", (unsigned int) addr);
-			if (++i > 40) {
+		if (kernel_text_address(addr)) {
+			if (i && ((i % 3) == 0))
+				printk("\n           ");
+			if (i > 40) {
 				printk(" ...");
 				break;
 			}
+
+			printk(" [<%016lx>]", addr);
+			i++;
 		}
 	}
+	printk("\n");
 }
 
 void show_trace_task(struct task_struct *tsk)
 {
-	show_trace((unsigned long *)tsk->thread.reg29);
+	show_trace((long *)tsk->thread.reg29);
 }
 
 
@@ -224,8 +254,8 @@ void show_registers(struct pt_regs *regs
 	show_regs(regs);
 	printk("Process %s (pid: %d, stackpage=%016lx)\n",
 		current->comm, current->pid, (unsigned long) current);
-	show_stack((unsigned long *) regs->regs[29]);
-	show_trace((unsigned long *) regs->regs[29]);
+	show_stack((long *) regs->regs[29]);
+	show_trace((long *) regs->regs[29]);
 	show_code((unsigned int *) regs->cp0_epc);
 	printk("\n");
 }

Re: [patch] Oops and magic SysRq stack dump clean-ups

[Johannes Stezenbach]

On Mon, Jul 29, 2002 at 03:25:53PM +0200, Maciej W. Rozycki wrote:
>  I've reviewed the stack dumping code more thoroughly and here is the
> result.  Please check if it's OK for you.  Tested visually with oopses and
> <SysRq>+<t> on MIPS/Linux and MIPS64/Linux.  The idea is to fit as much
> data as possible in as little space as possible and at the same time lay
> the numbers out visually nicely so that manual copying of output from a
> terminal for ksymoops analysis is easier for a reader.  Tools ignore
> spacing when processing such output anyway.
> 
>  Based somewhat on the i386 port.  Addresses cast to signed longs, since
> they are such on MIPS (additionally, the code doesn't care anyway but the
> resulting source is smaller).  Any objections? 

Nope, patch and its output look good to me.


Johannes