sparc boot failure

sparc boot failure

[Robert Reif]

sparc has failed to boot recently and I bisected it down to this patch:

8dd9453737822469837d48d5da3785ce70fb2118 is first bad commit
commit 8dd9453737822469837d48d5da3785ce70fb2118
Author: Frederic Weisbecker <fweisbec@gmail.com>
Date:   Tue Oct 21 21:56:42 2008 -0700
 
    sparc: correct section of current_pc()
     
    Latest mainline gives this section mismatch on sparc:
     
    The function current_pc() references
    the variable __init no_sun4u_here.
    This is often because current_pc lacks a __init
    annotation or the annotation of no_sun4u_here is wrong.
     
    Since current_pc() is used only in early time, it is correct to
    put it in .init section.
     
    Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Reverting this patch gets current git booting again.
 

Re: sparc boot failure

[David Miller]

From: Robert Reif <reif@earthlink.net>
Date: Wed, 05 Nov 2008 22:03:52 -0500

> sparc has failed to boot recently and I bisected it down to this patch:
 ...
>  sparc: correct section of current_pc()
 ...
> Reverting this patch gets current git booting again.

Thanks for tracking this down.  I'll take a look and if I
can't figure it out I'll revert.

Re: sparc boot failure

[Frédéric Weisbecker]

2008/11/6 David Miller <davem@davemloft.net>:
> From: Robert Reif <reif@earthlink.net>
> Date: Wed, 05 Nov 2008 22:03:52 -0500
>
>> sparc has failed to boot recently and I bisected it down to this patch:
>  ...
>>  sparc: correct section of current_pc()
>  ...
>> Reverting this patch gets current git booting again.
>
> Thanks for tracking this down.  I'll take a look and if I
> can't figure it out I'll revert.


Sorry, I will not have the time to look at it. But this bug is weird...

Re: sparc boot failure

[Robert Reif]

Frédéric Weisbecker wrote:
> 2008/11/6 David Miller <davem@davemloft.net>:
>   
>> From: Robert Reif <reif@earthlink.net>
>> Date: Wed, 05 Nov 2008 22:03:52 -0500
>>
>>     
>>> sparc has failed to boot recently and I bisected it down to this patch:
>>>       
>>  ...
>>     
>>>  sparc: correct section of current_pc()
>>>       
>>  ...
>>     
>>> Reverting this patch gets current git booting again.
>>>       
>> Thanks for tracking this down.  I'll take a look and if I
>> can't figure it out I'll revert.
>>     
>
>
> Sorry, I will not have the time to look at it. But this bug is weird...
> --
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>   
You can't change sections in the middle of a chunk of code.  The code 
above current_pc: falls through to that label which is now in a 
different section.  Thats why you get an illegal instruction trap at bootup.

Re: sparc boot failure

[Frédéric Weisbecker]

2008/11/6 Robert Reif <reif@earthlink.net>:
> Frédéric Weisbecker wrote:
>>
>> 2008/11/6 David Miller <davem@davemloft.net>:
>>
>>>
>>> From: Robert Reif <reif@earthlink.net>
>>> Date: Wed, 05 Nov 2008 22:03:52 -0500
>>>
>>>
>>>>
>>>> sparc has failed to boot recently and I bisected it down to this patch:
>>>>
>>>
>>>  ...
>>>
>>>>
>>>>  sparc: correct section of current_pc()
>>>>
>>>
>>>  ...
>>>
>>>>
>>>> Reverting this patch gets current git booting again.
>>>>
>>>
>>> Thanks for tracking this down.  I'll take a look and if I
>>> can't figure it out I'll revert.
>>>
>>
>>
>> Sorry, I will not have the time to look at it. But this bug is weird...
>> --
>> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>
> You can't change sections in the middle of a chunk of code.  The code above
> current_pc: falls through to that label which is now in a different section.
>  Thats why you get an illegal instruction trap at bootup.
>

Oh ok, I understand...

Re: sparc boot failure

[Sam Ravnborg]

On Wed, Nov 05, 2008 at 08:10:30PM -0800, David Miller wrote:
> From: Robert Reif <reif@earthlink.net>
> Date: Wed, 05 Nov 2008 22:03:52 -0500
> 
> > sparc has failed to boot recently and I bisected it down to this patch:
>  ...
> >  sparc: correct section of current_pc()
>  ...
> > Reverting this patch gets current git booting again.
> 
> Thanks for tracking this down.  I'll take a look and if I
> can't figure it out I'll revert.

The commit looks like this:
--- a/arch/sparc/kernel/head.S
+++ b/arch/sparc/kernel/head.S
@@ -465,6 +465,7 @@ gokernel:
                mov     %o7, %g4                ! Save %o7

                /* Jump to it, and pray... */
+               __INIT
 current_pc:
                call    1f
                 nop

So the original code assumed that after executing "mov %o7, %g4"
it would execute "call 1f".
But chaning section breaks this assumption and the boot fails.

The code at the label gokernel: all looks like it belongs to __HEAD
but I do not know about the traptable just above it.

If we ignore the traptable I think the below is the more correct approach:

diff --git a/arch/sparc/kernel/head.S b/arch/sparc/kernel/head.S
index 2fe2c11..ae479a5 100644
--- a/arch/sparc/kernel/head.S
+++ b/arch/sparc/kernel/head.S
@@ -72,7 +72,7 @@ sun4e_notsup:
        .align 4

        /* The Sparc trap table, bootloader gives us control at _start. */
-       .text
+       __HEAD
        .globl  start, _stext, _start, __stext
        .globl  trapbase
 _start:   /* danger danger */
@@ -465,7 +465,6 @@ gokernel:
                mov     %o7, %g4                ! Save %o7

                /* Jump to it, and pray... */
-               __INIT
 current_pc:
                call    1f
                 nop

[Copy'n'paste so it will not apply - because I do not think it is enough]
But someone that knows this stuff should fix it.

I checked vmlinux.lds.S and I can see that sparc needs to have
.head.text added before we can use __HEAD in assembler.

I assume it is something like this:

diff --git a/arch/sparc/kernel/vmlinux.lds.S b/arch/sparc/kernel/vmlinux.lds.S
index b1002c6..12d1be0 100644
--- a/arch/sparc/kernel/vmlinux.lds.S
+++ b/arch/sparc/kernel/vmlinux.lds.S
@@ -10,8 +10,11 @@ jiffies = jiffies_64 + 4;
 SECTIONS
 {
 	. = 0x10000 + SIZEOF_HEADERS;
-	.text 0xf0004000 :
-	{
+	.text.head 0xf0004000 : {
+		_text = .;                      /* Text and read-only data */
+		*(.text.head)
+	} : text = 0
+	.text : {
 		_text = .;
 		TEXT_TEXT
 		SCHED_TEXT


With this change we are then less dependent on link orderr which is a good thing.

	Sam

Re: sparc boot failure

[Robert Reif]

[-- Attachment #1: Type: text/plain, Size: 3291 bytes --]

Sam Ravnborg wrote:
> On Wed, Nov 05, 2008 at 08:10:30PM -0800, David Miller wrote:
>   
>> From: Robert Reif <reif@earthlink.net>
>> Date: Wed, 05 Nov 2008 22:03:52 -0500
>>
>>     
>>> sparc has failed to boot recently and I bisected it down to this patch:
>>>       
>>  ...
>>     
>>>  sparc: correct section of current_pc()
>>>       
>>  ...
>>     
>>> Reverting this patch gets current git booting again.
>>>       
>> Thanks for tracking this down.  I'll take a look and if I
>> can't figure it out I'll revert.
>>     
>
> The commit looks like this:
> --- a/arch/sparc/kernel/head.S
> +++ b/arch/sparc/kernel/head.S
> @@ -465,6 +465,7 @@ gokernel:
>                 mov     %o7, %g4                ! Save %o7
>
>                 /* Jump to it, and pray... */
> +               __INIT
>  current_pc:
>                 call    1f
>                  nop
>
> So the original code assumed that after executing "mov %o7, %g4"
> it would execute "call 1f".
> But chaning section breaks this assumption and the boot fails.
>
> The code at the label gokernel: all looks like it belongs to __HEAD
> but I do not know about the traptable just above it.
>
> If we ignore the traptable I think the below is the more correct approach:
>
> diff --git a/arch/sparc/kernel/head.S b/arch/sparc/kernel/head.S
> index 2fe2c11..ae479a5 100644
> --- a/arch/sparc/kernel/head.S
> +++ b/arch/sparc/kernel/head.S
> @@ -72,7 +72,7 @@ sun4e_notsup:
>         .align 4
>
>         /* The Sparc trap table, bootloader gives us control at _start. */
> -       .text
> +       __HEAD
>         .globl  start, _stext, _start, __stext
>         .globl  trapbase
>  _start:   /* danger danger */
> @@ -465,7 +465,6 @@ gokernel:
>                 mov     %o7, %g4                ! Save %o7
>
>                 /* Jump to it, and pray... */
> -               __INIT
>  current_pc:
>                 call    1f
>                  nop
>
> [Copy'n'paste so it will not apply - because I do not think it is enough]
> But someone that knows this stuff should fix it.
>
> I checked vmlinux.lds.S and I can see that sparc needs to have
> .head.text added before we can use __HEAD in assembler.
>
> I assume it is something like this:
>
> diff --git a/arch/sparc/kernel/vmlinux.lds.S b/arch/sparc/kernel/vmlinux.lds.S
> index b1002c6..12d1be0 100644
> --- a/arch/sparc/kernel/vmlinux.lds.S
> +++ b/arch/sparc/kernel/vmlinux.lds.S
> @@ -10,8 +10,11 @@ jiffies = jiffies_64 + 4;
>  SECTIONS
>  {
>  	. = 0x10000 + SIZEOF_HEADERS;
> -	.text 0xf0004000 :
> -	{
> +	.text.head 0xf0004000 : {
> +		_text = .;                      /* Text and read-only data */
> +		*(.text.head)
> +	} : text = 0
> +	.text : {
>  		_text = .;
>  		TEXT_TEXT
>  		SCHED_TEXT
>
>
> With this change we are then less dependent on link orderr which is a good thing.
>
> 	Sam
> --
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>   
The second patch doesn't compile.  The attached patch does but it 
doesn't boot because the kernel is now too big to load.

The size of vmlinux went from 4632210 to 4946070.

Can we at least revert the original patch so sparc boots and add a 
better patch later?




[-- Attachment #2: head.diff.txt --]
[-- Type: text/plain, Size: 471 bytes --]

diff --git a/arch/sparc/kernel/vmlinux.lds.S b/arch/sparc/kernel/vmlinux.lds.S
index b1002c6..e7ff0b9 100644
--- a/arch/sparc/kernel/vmlinux.lds.S
+++ b/arch/sparc/kernel/vmlinux.lds.S
@@ -10,8 +10,11 @@ jiffies = jiffies_64 + 4;
 SECTIONS
 {
 	. = 0x10000 + SIZEOF_HEADERS;
-	.text 0xf0004000 :
-	{
+	.text.head 0xf0004000 : {
+		_text = .;                      /* Text and read-only data */
+		*(.text.head)
+	} = 0
+	.text : {
 		_text = .;
 		TEXT_TEXT
 		SCHED_TEXT

Re: sparc boot failure

[David Miller]

From: Robert Reif <reif@earthlink.net>
Date: Fri, 07 Nov 2008 21:41:12 -0500

> Can we at least revert the original patch so sparc boots and add a better patch later?

Sure, I'll do that later tonight.

Re: sparc boot failure

[Sam Ravnborg]

Hi Robert.

> The second patch doesn't compile.  The attached patch does but it 
> doesn't boot because the kernel is now too big to load.
> 
> The size of vmlinux went from 4632210 to 4946070.

Can you send me your config so I can see why this size increase happens.
defconfig did not build with my sparc toolchain.

Did you have any mods in head.S too?

[I know davem are planning to revert the offending patch, but I would like
to understand this size increase anyway]


	Sam