LinuxLists.cc - 2.6.33-rc1 Reboot right after bootloader

[permalink] [raw]

Subject: Re: 2.6.33-rc1 Reboot right after bootloader

Good evening,

I took me some time but I tracked down my reboot problem. The culprit is
commit

4a2ff67c88211026afcbdbc190c13f705dae1b59: kbuild: fix bzImage build for
x86

The system I am compiling the kernel on has dash as its defaults shell.
With the commit applied I get an instant reboot, reverting the patch
makes it work again.
If I switch my default shell to bash it works BOTH times.
I do not know why the shell makes a difference here though.
The version of dash is 0.5.5.1 from debian unstable.

Kind regards,
Michael

2009-12-20 08:48:14

[permalink] [raw]

Subject: Re: 2.6.33-rc1 Reboot right after bootloader

On Sun, Dec 20, 2009 at 12:34:57AM +0100, Michael Guntsche wrote:
> Good evening,
>
> I took me some time but I tracked down my reboot problem. The culprit is
> commit
>
> 4a2ff67c88211026afcbdbc190c13f705dae1b59: kbuild: fix bzImage build for
> x86
>
> The system I am compiling the kernel on has dash as its defaults shell.
> With the commit applied I get an instant reboot, reverting the patch
> makes it work again.
> If I switch my default shell to bash it works BOTH times.
> I do not know why the shell makes a difference here though.
> The version of dash is 0.5.5.1 from debian unstable.

Hi Michael & Oliver.

Just to check what is going wrong here could you try to execute the following two commands:
printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
/usr/bin/printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x

Please try as above and also with full path to printf (/usr/bin/printf)

On my system it looks like this:

$ printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 51a8 0037
0000004
$ /usr/bin/printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 51a8 0037
0000004
$ echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 51a8 0037
0000004

Sam

2009-12-20 09:09:47

by Johannes Stezenbach

[permalink] [raw]

Subject: Re: 2.6.33-rc1 Reboot right after bootloader

On Sun, Dec 20, 2009 at 09:46:56AM +0100, Sam Ravnborg wrote:
> On Sun, Dec 20, 2009 at 12:34:57AM +0100, Michael Guntsche wrote:
> >
> > I took me some time but I tracked down my reboot problem. The culprit is
> > commit
> >
> > 4a2ff67c88211026afcbdbc190c13f705dae1b59: kbuild: fix bzImage build for
> > x86
>
> Just to check what is going wrong here could you try to execute the following two commands:
> printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
> /usr/bin/printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
> echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x
>
> Please try as above and also with full path to printf (/usr/bin/printf)

Debian dash has a built-in printf which doesn't support \x.
/usr/bin/printf works fine.

$ printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 785c 3861 785c 3135 785c 3733 785c 3030
0000010
$ /usr/bin/printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 51a8 0037
0000004
$ echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 6e2d 2065 785c 3861 785c 3135 785c 3733
0000010 785c 3030 000a
0000015

Johannes

2009-12-20 09:11:49

[permalink] [raw]

Subject: Re: 2.6.33-rc1 Reboot right after bootloader

On 2009.12.20 09:46:56 , Sam Ravnborg wrote:
> Hi Michael & Oliver.
>
> Just to check what is going wrong here could you try to execute the following two commands:
> printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
> /usr/bin/printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
> echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x
>
> Please try as above and also with full path to printf (/usr/bin/printf)
>
> On my system it looks like this:
>
> $ printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
> 0000000 51a8 0037
> 0000004
Hi Sam,

With bash as shell I get the same results, dash is different though.

$ printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 785c 3861 785c 3135 785c 3733 785c 3030
0000010

$ /usr/bin/printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 51a8 0037
0000004

$ echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x
0000000 6e2d 2065 785c 3861 785c 3135 785c 3733
0000010 785c 3030 000a
0000015

Kind regards,
Michael

2009-12-20 10:05:07

[permalink] [raw]

Subject: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

We use ... printf \x ... when calculating the size of the
compressed kernel.
Unfortunately dash built-in printf does not support this notation
resulting in a non-bootable kernel.

Fix this by always using the external version of printf.

The commit that introduced this bug was:
4a2ff67c88211026afcbdbc190c13f705dae1b59: "kbuild: fix bzImage
build for x86"

Reported-by: Michael Guntsche <[email protected]>
Cc: Oliver Hartkopp <[email protected]>
Cc: Johannes Stezenbach <[email protected]>
Signed-off-by: Sam Ravnborg <[email protected]>
---

This should fix it but it would be great if it is tested.
Michal Marek, I think this needs to go upstream pretty quickly.

Sam

scripts/Makefile.lib | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index cd815ac..bd201d9 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -213,13 +213,14 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \

# Bzip2 and LZMA do not include size in file... so we have to fake that;
# append the size as a 32-bit littleendian number as gzip does.
+# Note: dash built-in printf does not support \x so use /usr/bin version
size_append = printf $(shell \
dec_size=0; \
for F in $1; do \
fsize=$$(stat -c "%s" $$F); \
dec_size=$$(expr $$dec_size + $$fsize); \
done; \
-printf "%08x" $$dec_size | \
+/usr/bin/printf "%08x" $$dec_size | \
sed 's/$..$$..$$..$$..$/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g' \
)

--
1.6.0.6

2009-12-20 10:20:43

by Oliver Hartkopp

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

2009-12-20 10:29:10

by Willy Tarreau

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

On Sun, Dec 20, 2009 at 11:03:44AM +0100, Sam Ravnborg wrote:
> We use ... printf \x ... when calculating the size of the
> compressed kernel.
> Unfortunately dash built-in printf does not support this notation
> resulting in a non-bootable kernel.
>
> Fix this by always using the external version of printf.

Do we really want to workaround shells bugs ? I mean, either
the shell correctly implements the printf function or it does
not at all so that the correct printf is found in the path.
"man printf" clearly states that \x is supported, so what
should be fixed is the shell's implementation of printf.

The more absolute paths we specify, the less portable the
build system. And if linking /bin/sh to whatever shell works
but linking it to dash breaks, it's a shell bug.

Willy

2009-12-20 10:38:51

[permalink] [raw]

Subject: Re: 2.6.33-rc1 Reboot right after bootloader

Johannes Stezenbach wrote:
> On Sun, Dec 20, 2009 at 09:46:56AM +0100, Sam Ravnborg wrote:
>> On Sun, Dec 20, 2009 at 12:34:57AM +0100, Michael Guntsche wrote:
>>> I took me some time but I tracked down my reboot problem. The culprit is
>>> commit
>>>
>>> 4a2ff67c88211026afcbdbc190c13f705dae1b59: kbuild: fix bzImage build for
>>> x86
>> Just to check what is going wrong here could you try to execute the following two commands:
>> printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
>> /usr/bin/printf \\xa8\\x51\\x37\\x00 > x ; hexdump x
>> echo -ne \\xa8\\x51\\x37\\x00 > x ; hexdump x
>>
>> Please try as above and also with full path to printf (/usr/bin/printf)
>
> Debian dash has a built-in printf which doesn't support \x.
> /usr/bin/printf works fine.

So it looks like I was wrong with the last patch here.
I changed `/bin/echo -ne' to printf. And while it worked
for me at that time, apparently it does not work for others.

Now I wonder why it worked for me. I can confirm that
in current debian testing (dash-0.5.5.1-3) and in debian
stable (dash-0.5.4-12) dash's built-in printf does not
interpret \x escape sequence. I sure verified the fix
I proposed, rebuilding kernels in a freshly-installed
debian testing with /bin/sh pointing to dash. I'll try
to investigate this. (Side note: since the time this
issue first hit me, I carry 4a2ff67c88211026afcbdbc190c13f705dae1b59
locally.)

It is even more interesting. I re-read POSIX description
of printf, see
http://www.opengroup.org/onlinepubs/000095399/utilities/printf.html
And this one, too, does NOT mention \x at all. Printf is
still "better" than echo because for echo _no_ interpretation
of escape sequences is mandated -- see
http://www.opengroup.org/onlinepubs/000095399/utilities/echo.html
but that does not help much since for printf, while \-sequences
are mandated, particular \x is not, so \x is a {GNU|common sense|...}
extension.

What the...

So now I don't know what to do. According to the standard, there's
no utility that will work here. ;) Maybe the attached (together
with 4a2ff67c88211026afcbdbc190c13f705dae1b59) will make everyone happy?

/mjt

Attachments:

use-printf-octal.diff (423.00 B)

2009-12-20 10:47:16

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

Willy Tarreau wrote:
> On Sun, Dec 20, 2009 at 11:03:44AM +0100, Sam Ravnborg wrote:
>> We use ... printf \x ... when calculating the size of the
>> compressed kernel.
>> Unfortunately dash built-in printf does not support this notation
>> resulting in a non-bootable kernel.
>>
>> Fix this by always using the external version of printf.
>
> Do we really want to workaround shells bugs ? I mean, either

There's no bugs in dash, as far as I can see. According to
POSIX, a) echo does not need to interpret _any_ escape sequences
at all, and b) printf is not required to interpret \x sequences.
Ref:

http://www.opengroup.org/onlinepubs/000095399/utilities/echo.html
...
string
A string to be written to standard output. If the first operand is -n,
or if any of the operands contain a backslash ( '\' ) character,
the results are implementation-defined.

[XSI] On XSI-conformant systems, [..] the following character sequences
shall be recognized on XSI-conformant systems within any of the arguments:

\a \b \c \f \n \r \t \v \\
\0num
Write an 8-bit value that is the zero, one, two, or three-digit octal
number num.

http://www.opengroup.org/onlinepubs/000095399/utilities/printf.html

In addition to the escape sequences shown in the Base Definitions volume of IEEE
Std 1003.1-2001, Chapter 5, File Format Notation ( '\\', '\a', '\b', '\f', '\n',
'\r', '\t', '\v' ), "\ddd", where ddd is a one, two, or three-digit octal number,
shall be written as a byte with the numeric value specified by the octal number.

[]
> The more absolute paths we specify, the less portable the
> build system. And if linking /bin/sh to whatever shell works
> but linking it to dash breaks, it's a shell bug.

Yes for absolute paths, and no for dash, see above.

/mjt

2009-12-20 11:10:42

[permalink] [raw]

Subject: Re: 2.6.33-rc1 Reboot right after bootloader

Michael Tokarev wrote:
[]
> So now I don't know what to do. According to the standard, there's
> no utility that will work here. ;) Maybe the attached (together
> with 4a2ff67c88211026afcbdbc190c13f705dae1b59) will make everyone happy?

--- a/scripts/Makefile.lib.orig 2009-12-19 18:36:01.944153109 +0300
+++ a/scripts/Makefile.lib 2009-12-20 13:36:04.014530573 +0300
@@ -215,6 +215,6 @@
dec_size=$$(expr $$dec_size + $$fsize); \
done; \
-printf "%08x" $$dec_size | \
- sed 's/$..$$..$$..$$..$/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g' \
+printf "%012o" $$dec_size | \
+ sed 's/$...$$...$$...$$...$/\\\\4\\\\3\\\\2\\\\1/g' \
)

Apparently that does not work with dash either. It produces \4\3\2\1:

$ tail -c 4 arch/x86/boot/compressed/vmlinux.bin.lzma | hexdump
0000000 0304 0102

That's insane.

How about removing the whole hack and keeping the uncompressed size in
another file? (I'll try to cook something today - it requires a bit
more changes than the 2-liner above).

/mjt

2009-12-20 11:19:45

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

Sam Ravnborg wrote:
> We use ... printf \x ... when calculating the size of the
> compressed kernel.
> Unfortunately dash built-in printf does not support this notation
> resulting in a non-bootable kernel.
>
> Fix this by always using the external version of printf.
>
> The commit that introduced this bug was:
> 4a2ff67c88211026afcbdbc190c13f705dae1b59: "kbuild: fix bzImage
> build for x86"

It's not that simple Sam. This commit is a part of the game.
It works neither with printf as after my commit above, nor with
original fix with `/bin/echo -ne'. In neither case the fix is
complete or robust. Because there are two places in the few lines,
around this, both affected and both are non-POSIX-conformant.

> Reported-by: Michael Guntsche <[email protected]>
> Cc: Oliver Hartkopp <[email protected]>
> Cc: Johannes Stezenbach <[email protected]>
> Signed-off-by: Sam Ravnborg <[email protected]>
> ---
>
> This should fix it but it would be great if it is tested.
> Michal Marek, I think this needs to go upstream pretty quickly.

This whole issue only affects debian and ubuntu for now.
But the bug is very difficult to track down, since the prob
happens on early boot.

For now, I think, the best is to ensure we're building with
bash not dash. Ie, SHELL=/bin/bash make ... will work.
Because even with this second fix, it still does not work
correctly with dash's echo and printf behaving differently
than bash's (but still conforming to POSIX). It produces
good kernel, or at least I don't see any obvious probs with
the resulting kernel, but at least `make V=1' output looks
all wrong in this area (since dash's echo interpret \nnn
by default and make uses echo to print commands as they're
executed).

The right fix is to eliminate all this hack altogether.
Imho anyway.

/mjt

> Sam
>
> scripts/Makefile.lib | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index cd815ac..bd201d9 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -213,13 +213,14 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
>
> # Bzip2 and LZMA do not include size in file... so we have to fake that;
> # append the size as a 32-bit littleendian number as gzip does.
> +# Note: dash built-in printf does not support \x so use /usr/bin version
> size_append = printf $(shell \
> dec_size=0; \
> for F in $1; do \
> fsize=$$(stat -c "%s" $$F); \
> dec_size=$$(expr $$dec_size + $$fsize); \
> done; \
> -printf "%08x" $$dec_size | \
> +/usr/bin/printf "%08x" $$dec_size | \
> sed 's/$..$$..$$..$$..$/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g' \
> )
>

2009-12-20 11:38:59

[permalink] [raw]

Subject: Re: 2.6.33-rc1 Reboot right after bootloader

Michael Tokarev ?????:
> Michael Tokarev wrote:
> []
>> So now I don't know what to do. According to the standard, there's
>> no utility that will work here. ;) Maybe the attached (together
>> with 4a2ff67c88211026afcbdbc190c13f705dae1b59) will make everyone happy?
>
> --- a/scripts/Makefile.lib.orig 2009-12-19 18:36:01.944153109 +0300
> +++ a/scripts/Makefile.lib 2009-12-20 13:36:04.014530573 +0300
> @@ -215,6 +215,6 @@
> dec_size=$$(expr $$dec_size + $$fsize); \
> done; \
> -printf "%08x" $$dec_size | \
> - sed 's/$..$$..$$..$$..$/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g' \
> +printf "%012o" $$dec_size | \
> + sed 's/$...$$...$$...$$...$/\\\\4\\\\3\\\\2\\\\1/g' \
> )
>
> Apparently that does not work with dash either. It produces \4\3\2\1:
..
..because one pair of backslashes are missing before each digit, should be:

+ sed 's/$...$$...$$...$$...$/\\\\\\4\\\\\\3\\\\\\2\\\\\\1/g' \

But that still does not work, because a byte does not correspond to 3
octal characters, that's why it was hexadecimal...

/mjt

2009-12-20 12:03:01

by Andreas Schwab

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

Michael Tokarev <[email protected]> writes:

> Sam Ravnborg wrote:
>> We use ... printf \x ... when calculating the size of the
>> compressed kernel.
>> Unfortunately dash built-in printf does not support this notation
>> resulting in a non-bootable kernel.
>>
>> Fix this by always using the external version of printf.

That changed the wrong printf. :-)

>> The commit that introduced this bug was:
>> 4a2ff67c88211026afcbdbc190c13f705dae1b59: "kbuild: fix bzImage
>> build for x86"
>
> It's not that simple Sam. This commit is a part of the game.
> It works neither with printf as after my commit above, nor with
> original fix with `/bin/echo -ne'. In neither case the fix is
> complete or robust. Because there are two places in the few lines,
> around this, both affected and both are non-POSIX-conformant.

Does dash's printf support %b? Then this should work.

Andreas.

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index cd815ac..340813d 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -213,14 +213,16 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \

# Bzip2 and LZMA do not include size in file... so we have to fake that;
# append the size as a 32-bit littleendian number as gzip does.
-size_append = printf $(shell \
+size_append = printf %b $$( \
dec_size=0; \
for F in $1; do \
fsize=$$(stat -c "%s" $$F); \
dec_size=$$(expr $$dec_size + $$fsize); \
done; \
-printf "%08x" $$dec_size | \
- sed 's/$..$$..$$..$$..$/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g' \
+for i in 1 2 3 4; do \
+ printf '\\%04o' $$(expr $$dec_size % 256); \
+ dec_size=$$(expr $$dec_size / 256); \
+done \
)

quiet_cmd_bzip2 = BZIP2 $@

--
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

2009-12-20 12:19:42

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

On Sun, Dec 20, 2009 at 01:02:55PM +0100, Andreas Schwab wrote:
> Michael Tokarev <[email protected]> writes:
>
> > Sam Ravnborg wrote:
> >> We use ... printf \x ... when calculating the size of the
> >> compressed kernel.
> >> Unfortunately dash built-in printf does not support this notation
> >> resulting in a non-bootable kernel.
> >>
> >> Fix this by always using the external version of printf.
>
> That changed the wrong printf. :-)

Right - I have it correct in my repo here but "git commit --amend" fooled me.

Sam

2009-12-20 13:10:18

by Willy Tarreau

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

On Sun, Dec 20, 2009 at 01:47:13PM +0300, Michael Tokarev wrote:
> Willy Tarreau wrote:
> > On Sun, Dec 20, 2009 at 11:03:44AM +0100, Sam Ravnborg wrote:
> >> We use ... printf \x ... when calculating the size of the
> >> compressed kernel.
> >> Unfortunately dash built-in printf does not support this notation
> >> resulting in a non-bootable kernel.
> >>
> >> Fix this by always using the external version of printf.
> >
> > Do we really want to workaround shells bugs ? I mean, either
>
> There's no bugs in dash, as far as I can see. According to
> POSIX, a) echo does not need to interpret _any_ escape sequences
> at all, and b) printf is not required to interpret \x sequences.

Interesting.

> Ref:
>
> http://www.opengroup.org/onlinepubs/000095399/utilities/echo.html
> ...
> string
> A string to be written to standard output. If the first operand is -n,
> or if any of the operands contain a backslash ( '\' ) character,
> the results are implementation-defined.
>
> [XSI] On XSI-conformant systems, [..] the following character sequences
> shall be recognized on XSI-conformant systems within any of the arguments:
>
> \a \b \c \f \n \r \t \v \\
> \0num
> Write an 8-bit value that is the zero, one, two, or three-digit octal
> number num.
>
> http://www.opengroup.org/onlinepubs/000095399/utilities/printf.html
>
> In addition to the escape sequences shown in the Base Definitions volume of IEEE
> Std 1003.1-2001, Chapter 5, File Format Notation ( '\\', '\a', '\b', '\f', '\n',
> '\r', '\t', '\v' ), "\ddd", where ddd is a one, two, or three-digit octal number,
> shall be written as a byte with the numeric value specified by the octal number.

OK so the bug will not be fixed by calling /usr/bin/printf. It
will still work by pure luck when a the proper printf utility
will be there, but not when we use a posix-compliant one. The
proper fix then consists in writing octal chars in the form
of "\ddd" instead of "\xhh". And if dash is posix-compliant,
no need for the absolute path.

Willy

2009-12-20 13:50:30

by Arkadiusz Miskiewicz

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

On Sunday 20 of December 2009, Sam Ravnborg wrote:
\
> -printf "%08x" $$dec_size | \
> +/usr/bin/printf "%08x" $$dec_size | \
> sed 's/$..$$..$$..$$..$/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g' \
> )
>

Here printf is in /bin/ and not in /usr/bin.

--
Arkadiusz Miśkiewicz PLD/Linux Team
arekm / maven.pl http://ftp.pld-linux.org/

2009-12-20 14:12:35

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

On Sun, Dec 20, 2009 at 02:19:41PM +0300, Michael Tokarev wrote:
> Sam Ravnborg wrote:
> > We use ... printf \x ... when calculating the size of the
> > compressed kernel.
> > Unfortunately dash built-in printf does not support this notation
> > resulting in a non-bootable kernel.
> >
> > Fix this by always using the external version of printf.
> >
> > The commit that introduced this bug was:
> > 4a2ff67c88211026afcbdbc190c13f705dae1b59: "kbuild: fix bzImage
> > build for x86"
>
> It's not that simple Sam.

The whole business using shell scripts to append the size seems broken.
How about moving this functionality to mkpiggy where we are
less shell script dependent.

Something like the following.
I have only tested it lightly (vmlinux.bin did not differ
before/after the patch.

It includes the length also in the gzip case - I dunno if that matters.
Also I dunno if ".long" is the same on 32 and 64 bit.

Sam

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index f8ed065..d39fe2e 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -54,7 +54,8 @@ suffix-$(CONFIG_KERNEL_BZIP2) := bz2
suffix-$(CONFIG_KERNEL_LZMA) := lzma

quiet_cmd_mkpiggy = MKPIGGY $@
- cmd_mkpiggy = $(obj)/mkpiggy $< > $@ || ( rm -f $@ ; false )
+ cmd_mkpiggy = $(obj)/mkpiggy $< $(vmlinux.bin.all-y) > $@ || \
+ ( rm -f $@ ; false )

targets += piggy.S
$(obj)/piggy.S: $(obj)/vmlinux.bin.$(suffix-y) $(obj)/mkpiggy FORCE
diff --git a/arch/x86/boot/compressed/mkpiggy.c b/arch/x86/boot/compressed/mkpiggy.c
index bcbd36c..f4a1a3f 100644
--- a/arch/x86/boot/compressed/mkpiggy.c
+++ b/arch/x86/boot/compressed/mkpiggy.c
@@ -29,6 +29,7 @@
#include <stdio.h>
#include <string.h>
#include <inttypes.h>
+#include <sys/stat.h>

static uint32_t getle32(const void *p)
{
@@ -38,12 +39,27 @@ static uint32_t getle32(const void *p)
((uint32_t)cp[2] << 16) + ((uint32_t)cp[3] << 24);
}

+static file_size(const char *filename)
+{
+ struct stat statbuf;
+ int res;
+
+ res = stat(filename, &statbuf);
+ if (res < 0) {
+ perror(filename);
+ exit(1);
+ }
+ return (size_t)statbuf.st_size;
+}
+
int main(int argc, char *argv[])
{
uint32_t olen;
long ilen;
unsigned long offs;
+ size_t size;
FILE *f;
+ int i;

if (argc < 2) {
fprintf(stderr, "Usage: %s compressed_file\n", argv[0]);
@@ -67,6 +83,11 @@ int main(int argc, char *argv[])
olen = getle32(&olen);
fclose(f);

+ i = 2;
+ size = 0;
+ while (i < argc)
+ size += file_size(argv[i++]);
+
/*
* Now we have the input (compressed) and output (uncompressed)
* sizes, compute the necessary decompression offset...
@@ -91,6 +112,14 @@ int main(int argc, char *argv[])
printf(".globl input_data, input_data_end\n");
printf("input_data:\n");
printf(".incbin \"%s\"\n", argv[1]);
+
+ /*
+ * Bzip2 and LZMA do not include size in file... so we have to fake that;
+ * append the size as a 32-bit littleendian number as gzip does.
+ */
+ if (size > 0)
+ printf(".long %d\n", size);
+
printf("input_data_end:\n");

return 0;
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index cd815ac..10bef1c 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -211,27 +211,14 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
# Bzip2
# ---------------------------------------------------------------------------

-# Bzip2 and LZMA do not include size in file... so we have to fake that;
-# append the size as a 32-bit littleendian number as gzip does.
-size_append = printf $(shell \
-dec_size=0; \
-for F in $1; do \
- fsize=$$(stat -c "%s" $$F); \
- dec_size=$$(expr $$dec_size + $$fsize); \
-done; \
-printf "%08x" $$dec_size | \
- sed 's/$..$$..$$..$$..$/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g' \
-)
-
quiet_cmd_bzip2 = BZIP2 $@
-cmd_bzip2 = (cat $(filter-out FORCE,$^) | \
- bzip2 -9 && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
- (rm -f $@ ; false)
+cmd_bzip2 = cat $(filter-out FORCE,$^) | \
+ bzip2 -9 > $@ || (rm -f $@ ; false)

# Lzma
# ---------------------------------------------------------------------------

quiet_cmd_lzma = LZMA $@
-cmd_lzma = (cat $(filter-out FORCE,$^) | \
- lzma -9 && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
- (rm -f $@ ; false)
+cmd_lzma = cat $(filter-out FORCE,$^) | \
+ lzma -9 > $@ || (rm -f $@ ; false)
+

2009-12-21 12:23:00

by Michal Marek

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

Andreas Schwab napsal(a):
> Does dash's printf support %b? Then this should work.

Good idea. %b is specified by posix, so there is hope :). Does Andreas'
patch work for others as well? I'd like to fix this upstream ASAP, OTOH
right now it's broken "only" for dash users, so let's not rush and make
it even worse.

Thanks,
Michal
>
> Andreas.
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index cd815ac..340813d 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -213,14 +213,16 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
>
> # Bzip2 and LZMA do not include size in file... so we have to fake that;
> # append the size as a 32-bit littleendian number as gzip does.
> -size_append = printf $(shell \
> +size_append = printf %b $$( \
> dec_size=0; \
> for F in $1; do \
> fsize=$$(stat -c "%s" $$F); \
> dec_size=$$(expr $$dec_size + $$fsize); \
> done; \
> -printf "%08x" $$dec_size | \
> - sed 's/$..$$..$$..$$..$/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g' \
> +for i in 1 2 3 4; do \
> + printf '\\%04o' $$(expr $$dec_size % 256); \
> + dec_size=$$(expr $$dec_size / 256); \
> +done \
> )

2009-12-21 13:50:14

by Andreas Schwab

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

Michal Marek <[email protected]> writes:

> Andreas Schwab napsal(a):
>> Does dash's printf support %b? Then this should work.
>
> Good idea. %b is specified by posix, so there is hope :).

Actually you don't need it anyway. (Still untested though.)

Andreas.

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index cd815ac..7173c0c 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -213,14 +213,16 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \

# Bzip2 and LZMA do not include size in file... so we have to fake that;
# append the size as a 32-bit littleendian number as gzip does.
-size_append = printf $(shell \
+size_append = printf $$( \
dec_size=0; \
for F in $1; do \
fsize=$$(stat -c "%s" $$F); \
dec_size=$$(expr $$dec_size + $$fsize); \
done; \
-printf "%08x" $$dec_size | \
- sed 's/$..$$..$$..$$..$/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g' \
+for i in 1 2 3 4; do \
+ printf '\\%03o' $$(expr $$dec_size % 256); \
+ dec_size=$$(expr $$dec_size / 256); \
+done \
)

quiet_cmd_bzip2 = BZIP2 $@

--
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

2009-12-21 14:17:08

by Michal Marek

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

Sam Ravnborg wrote:
> The whole business using shell scripts to append the size seems broken.
> How about moving this functionality to mkpiggy where we are
> less shell script dependent.
>
> Something like the following.
> I have only tested it lightly (vmlinux.bin did not differ
> before/after the patch.

I think that it's a good idea to move this out of the makefiles. But for
2.6.33, I think I prefer the patch from Andreas, provided he signs it
off and Michael G confirms that it fixes his issue.

> It includes the length also in the gzip case - I dunno if that matters.

I don't know either, but this is trivially fixable by an option to
mkpiggy that is added only if CONFIG_KERNEL_BZIP2=y or CONFIG_KERNEL_LZMA=y.

> Also I dunno if ".long" is the same on 32 and 64 bit.

I think we can work this out until the .34 merge window :).

Michal
>
> Sam
>
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index f8ed065..d39fe2e 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -54,7 +54,8 @@ suffix-$(CONFIG_KERNEL_BZIP2) := bz2
> suffix-$(CONFIG_KERNEL_LZMA) := lzma
>
> quiet_cmd_mkpiggy = MKPIGGY $@
> - cmd_mkpiggy = $(obj)/mkpiggy $< > $@ || ( rm -f $@ ; false )
> + cmd_mkpiggy = $(obj)/mkpiggy $< $(vmlinux.bin.all-y) > $@ || \
> + ( rm -f $@ ; false )
>
> targets += piggy.S
> $(obj)/piggy.S: $(obj)/vmlinux.bin.$(suffix-y) $(obj)/mkpiggy FORCE
> diff --git a/arch/x86/boot/compressed/mkpiggy.c b/arch/x86/boot/compressed/mkpiggy.c
> index bcbd36c..f4a1a3f 100644
> --- a/arch/x86/boot/compressed/mkpiggy.c
> +++ b/arch/x86/boot/compressed/mkpiggy.c
> @@ -29,6 +29,7 @@
> #include <stdio.h>
> #include <string.h>
> #include <inttypes.h>
> +#include <sys/stat.h>
>
> static uint32_t getle32(const void *p)
> {
> @@ -38,12 +39,27 @@ static uint32_t getle32(const void *p)
> ((uint32_t)cp[2] << 16) + ((uint32_t)cp[3] << 24);
> }
>
> +static file_size(const char *filename)
> +{
> + struct stat statbuf;
> + int res;
> +
> + res = stat(filename, &statbuf);
> + if (res < 0) {
> + perror(filename);
> + exit(1);
> + }
> + return (size_t)statbuf.st_size;
> +}
> +
> int main(int argc, char *argv[])
> {
> uint32_t olen;
> long ilen;
> unsigned long offs;
> + size_t size;
> FILE *f;
> + int i;
>
> if (argc < 2) {
> fprintf(stderr, "Usage: %s compressed_file\n", argv[0]);
> @@ -67,6 +83,11 @@ int main(int argc, char *argv[])
> olen = getle32(&olen);
> fclose(f);
>
> + i = 2;
> + size = 0;
> + while (i < argc)
> + size += file_size(argv[i++]);
> +
> /*
> * Now we have the input (compressed) and output (uncompressed)
> * sizes, compute the necessary decompression offset...
> @@ -91,6 +112,14 @@ int main(int argc, char *argv[])
> printf(".globl input_data, input_data_end\n");
> printf("input_data:\n");
> printf(".incbin \"%s\"\n", argv[1]);
> +
> + /*
> + * Bzip2 and LZMA do not include size in file... so we have to fake that;
> + * append the size as a 32-bit littleendian number as gzip does.
> + */
> + if (size > 0)
> + printf(".long %d\n", size);
> +
> printf("input_data_end:\n");
>
> return 0;
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index cd815ac..10bef1c 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -211,27 +211,14 @@ cmd_gzip = (cat $(filter-out FORCE,$^) | gzip -f -9 > $@) || \
> # Bzip2
> # ---------------------------------------------------------------------------
>
> -# Bzip2 and LZMA do not include size in file... so we have to fake that;
> -# append the size as a 32-bit littleendian number as gzip does.
> -size_append = printf $(shell \
> -dec_size=0; \
> -for F in $1; do \
> - fsize=$$(stat -c "%s" $$F); \
> - dec_size=$$(expr $$dec_size + $$fsize); \
> -done; \
> -printf "%08x" $$dec_size | \
> - sed 's/$..$$..$$..$$..$/\\\\x\4\\\\x\3\\\\x\2\\\\x\1/g' \
> -)
> -
> quiet_cmd_bzip2 = BZIP2 $@
> -cmd_bzip2 = (cat $(filter-out FORCE,$^) | \
> - bzip2 -9 && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
> - (rm -f $@ ; false)
> +cmd_bzip2 = cat $(filter-out FORCE,$^) | \
> + bzip2 -9 > $@ || (rm -f $@ ; false)
>
> # Lzma
> # ---------------------------------------------------------------------------
>
> quiet_cmd_lzma = LZMA $@
> -cmd_lzma = (cat $(filter-out FORCE,$^) | \
> - lzma -9 && $(call size_append, $(filter-out FORCE,$^))) > $@ || \
> - (rm -f $@ ; false)
> +cmd_lzma = cat $(filter-out FORCE,$^) | \
> + lzma -9 > $@ || (rm -f $@ ; false)
> +

2009-12-21 16:29:00

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

On 21 Dec 09 15:17, Michal Marek wrote:
> > How about moving this functionality to mkpiggy where we are
> > less shell script dependent.
> >
> > Something like the following.
> > I have only tested it lightly (vmlinux.bin did not differ
> > before/after the patch.
>
>
> I think that it's a good idea to move this out of the makefiles. But for
> 2.6.33, I think I prefer the patch from Andreas, provided he signs it
> off and Michael G confirms that it fixes his issue.

Just a quick update from my side. I tried the patch from Andreas and build a kernel
with dash as defaulti shell. I was able to boot the resulting kernel image
without any problems.

Kind regards,
Michael

2009-12-21 18:29:28

by H. Peter Anvin

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

On 12/20/2009 06:11 AM, Sam Ravnborg wrote:
>
> The whole business using shell scripts to append the size seems broken.
> How about moving this functionality to mkpiggy where we are
> less shell script dependent.
>

Seems a helluva lot saner to me...

> It includes the length also in the gzip case - I dunno if that matters.
> Also I dunno if ".long" is the same on 32 and 64 bit.

Well, gzip already has a length at the end... shouldn't really hurt
anything, I suppose. ".long" is 32 bits on both 32 and 64 bits.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2009-12-21 19:09:42

by H. Peter Anvin

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

On 12/21/2009 10:28 AM, H. Peter Anvin wrote:
> On 12/20/2009 06:11 AM, Sam Ravnborg wrote:
>>
>> The whole business using shell scripts to append the size seems broken.
>> How about moving this functionality to mkpiggy where we are
>> less shell script dependent.
>>
>
> Seems a helluva lot saner to me...
>
>> It includes the length also in the gzip case - I dunno if that matters.
>> Also I dunno if ".long" is the same on 32 and 64 bit.
>
> Well, gzip already has a length at the end... shouldn't really hurt
> anything, I suppose. ".long" is 32 bits on both 32 and 64 bits.
>

Note, though: mkpiggy is x86, but this also affects other
architectures... so probably additional changes are needed.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2009-12-21 19:53:59

[permalink] [raw]

Subject: Re: [PATCH] kbuild: correct size calculation of bzImgae / fix x86 boot

Michael Guntsche wrote:
> On 21 Dec 09 15:17, Michal Marek wrote:
>>> How about moving this functionality to mkpiggy where we are
>>> less shell script dependent.
>>>
>>> Something like the following.
>>> I have only tested it lightly (vmlinux.bin did not differ
>>> before/after the patch.
>>
>> I think that it's a good idea to move this out of the makefiles. But for
>> 2.6.33, I think I prefer the patch from Andreas, provided he signs it
>> off and Michael G confirms that it fixes his issue.
>
> Just a quick update from my side. I tried the patch from Andreas and build a kernel
> with dash as defaulti shell. I was able to boot the resulting kernel image
> without any problems.

That's not a good test case. Because even with bogus data the kernel might
boot: for example, allocating alot more memory for the unpacking than necessary -
it does not crash but the data is still bogus.

The better (and much faster) check is to verify that the z_output_len value
in arch/x86/boot/compressed/piggy.S matches the size of
arch/x86/boot/compressed/vmlinuz.bin.

/mjt

2009-12-21 20:48:17