[PATCH] ARM: put a valid "barebox" signature in the header on big-endian systems.

[PATCH] ARM: put a valid "barebox" signature in the header on big-endian systems.

[Krzysztof Halasa]

Signed-off-by: Krzysztof Hałasa <khc@pm.waw.pl>

diff --git a/arch/arm/include/asm/barebox-arm-head.h b/arch/arm/include/asm/barebox-arm-head.h
index 0dc3074..2c250e9 100644
--- a/arch/arm/include/asm/barebox-arm-head.h
+++ b/arch/arm/include/asm/barebox-arm-head.h
@@ -24,8 +24,7 @@ static inline void barebox_arm_head(void)
 		"1: b 1b\n"
 		"1: b 1b\n"
 #endif
-		".word 0x65726162\n"			/* 'bare' */
-		".word 0x00786f62\n"			/* 'box' */
+		".asciz \"barebox\"\n"
 		".word _text\n"				/* text base. If copied there,
 							 * barebox can skip relocation
 							 */

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: put a valid "barebox" signature in the header on big-endian systems.

[Sascha Hauer]

On Sat, May 05, 2012 at 11:47:19PM +0200, Krzysztof Halasa wrote:
> Signed-off-by: Krzysztof Hałasa <khc@pm.waw.pl>
> 
> diff --git a/arch/arm/include/asm/barebox-arm-head.h b/arch/arm/include/asm/barebox-arm-head.h
> index 0dc3074..2c250e9 100644
> --- a/arch/arm/include/asm/barebox-arm-head.h
> +++ b/arch/arm/include/asm/barebox-arm-head.h
> @@ -24,8 +24,7 @@ static inline void barebox_arm_head(void)
>  		"1: b 1b\n"
>  		"1: b 1b\n"
>  #endif
> -		".word 0x65726162\n"			/* 'bare' */
> -		".word 0x00786f62\n"			/* 'box' */
> +		".asciz \"barebox\"\n"
>  		".word _text\n"				/* text base. If copied there,
>  							 * barebox can skip relocation

Does the barebox filetype detection need a fix aswell?

enum filetype file_detect_type(void *_buf)
{
        u32 *buf = _buf;
        u8 *buf8 = _buf;

        if (strncmp(buf8, "#!/bin/sh", 9) == 0)
                return filetype_sh;
        if (buf[8] == 0x65726162 && buf[9] == 0x00786f62)
                return filetype_arm_barebox;

This seems wrong on big endian systems.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: put a valid "barebox" signature in the header on big-endian systems.

[Krzysztof Halasa]

Sascha Hauer <s.hauer@pengutronix.de> writes:

>> -		".word 0x65726162\n"			/* 'bare' */
>> -		".word 0x00786f62\n"			/* 'box' */
>> +		".asciz \"barebox\"\n"
>>  		".word _text\n"				/* text base. If copied there,
>>  							 * barebox can skip relocation
>
> Does the barebox filetype detection need a fix aswell?

I guess so.

> enum filetype file_detect_type(void *_buf)
> {
>         u32 *buf = _buf;
>         u8 *buf8 = _buf;
>
>         if (strncmp(buf8, "#!/bin/sh", 9) == 0)
>                 return filetype_sh;

BTW there can be a white space between "#!" part and the interpreter
name.

>         if (buf[8] == 0x65726162 && buf[9] == 0x00786f62)
>                 return filetype_arm_barebox;
>
> This seems wrong on big endian systems.

Definitely.
A memcmp() would be better (I don't know about the buffer length).
-- 
Krzysztof Halasa

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: put a valid "barebox" signature in the header on big-endian systems.

[Sascha Hauer]

On Sat, May 05, 2012 at 11:47:19PM +0200, Krzysztof Halasa wrote:
> Signed-off-by: Krzysztof Hałasa <khc@pm.waw.pl>

Applied, thanks

Sascha

> 
> diff --git a/arch/arm/include/asm/barebox-arm-head.h b/arch/arm/include/asm/barebox-arm-head.h
> index 0dc3074..2c250e9 100644
> --- a/arch/arm/include/asm/barebox-arm-head.h
> +++ b/arch/arm/include/asm/barebox-arm-head.h
> @@ -24,8 +24,7 @@ static inline void barebox_arm_head(void)
>  		"1: b 1b\n"
>  		"1: b 1b\n"
>  #endif
> -		".word 0x65726162\n"			/* 'bare' */
> -		".word 0x00786f62\n"			/* 'box' */
> +		".asciz \"barebox\"\n"
>  		".word _text\n"				/* text base. If copied there,
>  							 * barebox can skip relocation
>  							 */
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: put a valid "barebox" signature in the header on big-endian systems.

[Uwe Kleine-König]

Hello Krzysztof,

On Sat, May 05, 2012 at 11:47:19PM +0200, Krzysztof Halasa wrote:
> Signed-off-by: Krzysztof Hałasa <khc@pm.waw.pl>
> 
> diff --git a/arch/arm/include/asm/barebox-arm-head.h b/arch/arm/include/asm/barebox-arm-head.h
> index 0dc3074..2c250e9 100644
> --- a/arch/arm/include/asm/barebox-arm-head.h
> +++ b/arch/arm/include/asm/barebox-arm-head.h
> @@ -24,8 +24,7 @@ static inline void barebox_arm_head(void)
>  		"1: b 1b\n"
>  		"1: b 1b\n"
>  #endif
> -		".word 0x65726162\n"			/* 'bare' */
> -		".word 0x00786f62\n"			/* 'box' */
> +		".asciz \"barebox\"\n"
>  		".word _text\n"				/* text base. If copied there,
>  							 * barebox can skip relocation
>  							 */
Another downside of this patch (apart from the file type detection that
still needs adaption?) is that the magic is different depending on
endianess. I'm not sure if it really matters, but this change makes it
harder (or at least more error prone) for tools like file(1) to detect
the image type. (Well, it might be hard anyhow, as barebox does have a
machine specific header anyhow depending on the machine.)

Moreover I wonder what the motivation here is to change a magic. Was it
just "oh, this looks ugly"? If yes, is that enough?

Just my 0.02€ on this patch, I don't know how many big endian barebox
images are out there and I don't really care (neither about big endian
images nor about this change). So feel free to ignore my concerns if you
know better.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: put a valid "barebox" signature in the header on big-endian systems.

[Antony Pavlov]

On 13 May 2012 01:38, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> Hello Krzysztof,
>
> On Sat, May 05, 2012 at 11:47:19PM +0200, Krzysztof Halasa wrote:
>> Signed-off-by: Krzysztof Hałasa <khc@pm.waw.pl>
>>
>> diff --git a/arch/arm/include/asm/barebox-arm-head.h b/arch/arm/include/asm/barebox-arm-head.h
>> index 0dc3074..2c250e9 100644
>> --- a/arch/arm/include/asm/barebox-arm-head.h
>> +++ b/arch/arm/include/asm/barebox-arm-head.h
>> @@ -24,8 +24,7 @@ static inline void barebox_arm_head(void)
>>               "1: b 1b\n"
>>               "1: b 1b\n"
>>  #endif
>> -             ".word 0x65726162\n"                    /* 'bare' */
>> -             ".word 0x00786f62\n"                    /* 'box' */
>> +             ".asciz \"barebox\"\n"
>>               ".word _text\n"                         /* text base. If copied there,
>>                                                        * barebox can skip relocation
>>                                                        */
> Another downside of this patch (apart from the file type detection that
> still needs adaption?) is that the magic is different depending on
> endianess.

Can this addition to the patch fix the endianess issue?

--- a/common/filetype.c
+++ b/common/filetype.c
@@ -56,7 +56,7 @@ enum filetype file_detect_type(void *_buf)

        if (strncmp(buf8, "#!/bin/sh", 9) == 0)
                return filetype_sh;
-       if (buf[8] == 0x65726162 && buf[9] == 0x00786f62)
+       if (strncmp(buf8 + 0x20, "barebox", 7) == 0)
                return filetype_arm_barebox;
        if (buf[9] == 0x016f2818 || buf[9] == 0x18286f01)
                return filetype_arm_zimage;

-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: put a valid "barebox" signature in the header on big-endian systems.

[Uwe Kleine-König]

On Sun, May 13, 2012 at 03:56:48AM +0400, Antony Pavlov wrote:
> On 13 May 2012 01:38, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > Hello Krzysztof,
> >
> > On Sat, May 05, 2012 at 11:47:19PM +0200, Krzysztof Halasa wrote:
> >> Signed-off-by: Krzysztof Hałasa <khc@pm.waw.pl>
> >>
> >> diff --git a/arch/arm/include/asm/barebox-arm-head.h b/arch/arm/include/asm/barebox-arm-head.h
> >> index 0dc3074..2c250e9 100644
> >> --- a/arch/arm/include/asm/barebox-arm-head.h
> >> +++ b/arch/arm/include/asm/barebox-arm-head.h
> >> @@ -24,8 +24,7 @@ static inline void barebox_arm_head(void)
> >>               "1: b 1b\n"
> >>               "1: b 1b\n"
> >>  #endif
> >> -             ".word 0x65726162\n"                    /* 'bare' */
> >> -             ".word 0x00786f62\n"                    /* 'box' */
> >> +             ".asciz \"barebox\"\n"
> >>               ".word _text\n"                         /* text base. If copied there,
> >>                                                        * barebox can skip relocation
> >>                                                        */
> > Another downside of this patch (apart from the file type detection that
> > still needs adaption?) is that the magic is different depending on
> > endianess.
> 
> Can this addition to the patch fix the endianess issue?
> 
> --- a/common/filetype.c
> +++ b/common/filetype.c
> @@ -56,7 +56,7 @@ enum filetype file_detect_type(void *_buf)
> 
>         if (strncmp(buf8, "#!/bin/sh", 9) == 0)
>                 return filetype_sh;
> -       if (buf[8] == 0x65726162 && buf[9] == 0x00786f62)
> +       if (strncmp(buf8 + 0x20, "barebox", 7) == 0)
the original checked 8 bytes, you only check 7. Is this intended?

>                 return filetype_arm_barebox;
>         if (buf[9] == 0x016f2818 || buf[9] == 0x18286f01)
>                 return filetype_arm_zimage;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: put a valid "barebox" signature in the header on big-endian systems.

[Krzysztof Halasa]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

>> +++ b/arch/arm/include/asm/barebox-arm-head.h
>> @@ -24,8 +24,7 @@ static inline void barebox_arm_head(void)
>>  		"1: b 1b\n"
>>  		"1: b 1b\n"
>>  #endif
>> -		".word 0x65726162\n"			/* 'bare' */
>> -		".word 0x00786f62\n"			/* 'box' */
>> +		".asciz \"barebox\"\n"
>>  		".word _text\n"				/* text base. If copied there,
>>  							 * barebox can skip relocation
>>  							 */
> Another downside of this patch (apart from the file type detection that
> still needs adaption?) is that the magic is different depending on
> endianess.

Actually it's the opposite - i.e., the magic is now always "barebox\0"
and doesn't depend on endianness :-)

Without this patch, the magic changes with endianness (it becomes
"erab\0xob" on BE).

> Just my 0.02€ on this patch, I don't know how many big endian barebox
> images are out there

Guess I have most of these here :-)
Jean-Christophe may have some, too.
-- 
Krzysztof Halasa

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: put a valid "barebox" signature in the header on big-endian systems.

[Sascha Hauer]

On Sun, May 13, 2012 at 12:05:33PM +0200, Krzysztof Halasa wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> 
> >> +++ b/arch/arm/include/asm/barebox-arm-head.h
> >> @@ -24,8 +24,7 @@ static inline void barebox_arm_head(void)
> >>  		"1: b 1b\n"
> >>  		"1: b 1b\n"
> >>  #endif
> >> -		".word 0x65726162\n"			/* 'bare' */
> >> -		".word 0x00786f62\n"			/* 'box' */
> >> +		".asciz \"barebox\"\n"
> >>  		".word _text\n"				/* text base. If copied there,
> >>  							 * barebox can skip relocation
> >>  							 */
> > Another downside of this patch (apart from the file type detection that
> > still needs adaption?) is that the magic is different depending on
> > endianess.
> 
> Actually it's the opposite - i.e., the magic is now always "barebox\0"
> and doesn't depend on endianness :-)
> 
> Without this patch, the magic changes with endianness (it becomes
> "erab\0xob" on BE).

This may have the upside that with this we are able to detect foreign
endian images like done in the zImage support. Here we use this to know
that we have to swap byte endianess before jumping to the kernel.

I probably should have chosen some arbitrary number rather than a
string, then nobody would have wondered about this ;)

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox