[PATCH 1/2] Use structs instead of hardcoded offsets in x86 boot decompressor.

[PATCH 1/2] Use structs instead of hardcoded offsets in x86 boot decompressor.

[Kristian Høgsberg]

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 arch/x86/boot/compressed/misc.c |   26 +++++++++++---------------
 1 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 90456ce..0d03579 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -30,6 +30,7 @@
 #include <asm/io.h>
 #include <asm/page.h>
 #include <asm/boot.h>
+#include <asm/bootparam.h>
 
 /* WARNING!!
  * This code is compiled with -fPIC and it is relocated dynamically
@@ -187,13 +188,8 @@ static void gzip_release(void **);
 /*
  * This is set up by the setup-routine at boot-time
  */
-static unsigned char *real_mode; /* Pointer to real-mode data */
-
-#define RM_EXT_MEM_K   (*(unsigned short *)(real_mode + 0x2))
-#ifndef STANDARD_MEMORY_BIOS_CALL
-#define RM_ALT_MEM_K   (*(unsigned long *)(real_mode + 0x1e0))
-#endif
-#define RM_SCREEN_INFO (*(struct screen_info *)(real_mode+0))
+static struct boot_params *real_mode;		/* Pointer to real-mode data */
+static struct screen_info *rm_screen_info;	/* Pointer to real-mode data */
 
 extern unsigned char input_data[];
 extern int input_len;
@@ -276,12 +272,12 @@ static void putstr(const char *s)
 	char c;
 
 #ifdef CONFIG_X86_32
-	if (RM_SCREEN_INFO.orig_video_mode == 0 && lines == 0 && cols == 0)
+	if (rm_screen_info->orig_video_mode == 0 && lines == 0 && cols == 0)
 		return;
 #endif
 
-	x = RM_SCREEN_INFO.orig_x;
-	y = RM_SCREEN_INFO.orig_y;
+	x = rm_screen_info->orig_x;
+	y = rm_screen_info->orig_y;
 
 	while ((c = *s++) != '\0') {
 		if (c == '\n') {
@@ -302,8 +298,8 @@ static void putstr(const char *s)
 		}
 	}
 
-	RM_SCREEN_INFO.orig_x = x;
-	RM_SCREEN_INFO.orig_y = y;
+	rm_screen_info->orig_x = x;
+	rm_screen_info->orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
 	outb(14, vidport);
@@ -430,7 +426,7 @@ asmlinkage void decompress_kernel(void *rmode, memptr heap,
 {
 	real_mode = rmode;
 
-	if (RM_SCREEN_INFO.orig_video_mode == 7) {
+	if (rm_screen_info->orig_video_mode == 7) {
 		vidmem = (char *) 0xb0000;
 		vidport = 0x3b4;
 	} else {
@@ -438,8 +434,8 @@ asmlinkage void decompress_kernel(void *rmode, memptr heap,
 		vidport = 0x3d4;
 	}
 
-	lines = RM_SCREEN_INFO.orig_video_lines;
-	cols = RM_SCREEN_INFO.orig_video_cols;
+	lines = rm_screen_info->orig_video_lines;
+	cols = rm_screen_info->orig_video_cols;
 
 	window = output;		/* Output buffer (Normally at 1M) */
 	free_mem_ptr     = heap;	/* Heap */
-- 
1.5.5.1

[PATCH 2/2] Only print "Decompressing Linux" etc when 'noisy' is passed.

[Kristian Høgsberg]

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 arch/x86/boot/compressed/misc.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 0d03579..b7b5c1f 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -190,6 +190,7 @@ static void gzip_release(void **);
  */
 static struct boot_params *real_mode;		/* Pointer to real-mode data */
 static struct screen_info *rm_screen_info;	/* Pointer to real-mode data */
+static int noisy;
 
 extern unsigned char input_data[];
 extern int input_len;
@@ -391,7 +392,8 @@ static void parse_elf(void *output)
 		return;
 	}
 
-	putstr("Parsing ELF... ");
+	if (noisy)
+		putstr("Parsing ELF... ");
 
 	phdrs = malloc(sizeof(*phdrs) * ehdr.e_phnum);
 	if (!phdrs)
@@ -419,11 +421,27 @@ static void parse_elf(void *output)
 	}
 }
 
+static const char *strnstr(const char *string, int len, const char *s)
+{
+	int i, j;
+
+	for (i = 0; i < len; i++) {
+		for (j = 0; i + j < len && s[j]; j++)
+			if (string[i + j] != s[j])
+				break;
+		if (s[j] == '\0')
+			return string + i;
+	}
+
+	return NULL;
+}
 asmlinkage void decompress_kernel(void *rmode, memptr heap,
 				  unsigned char *input_data,
 				  unsigned long input_len,
 				  unsigned char *output)
 {
+	char *cmdline;
+
 	real_mode = rmode;
 
 	if (rm_screen_info->orig_video_mode == 7) {
@@ -437,6 +455,10 @@ asmlinkage void decompress_kernel(void *rmode, memptr heap,
 	lines = rm_screen_info->orig_video_lines;
 	cols = rm_screen_info->orig_video_cols;
 
+	cmdline = (char *) real_mode->hdr.cmd_line_ptr;
+	if (strnstr(cmdline, real_mode->hdr.cmdline_size, "noisy"))
+		noisy = 1;
+
 	window = output;		/* Output buffer (Normally at 1M) */
 	free_mem_ptr     = heap;	/* Heap */
 	free_mem_end_ptr = heap + BOOT_HEAP_SIZE;
@@ -461,9 +483,11 @@ asmlinkage void decompress_kernel(void *rmode, memptr heap,
 #endif
 
 	makecrc();
-	putstr("\nDecompressing Linux... ");
+	if (noisy)
+		putstr("\nDecompressing Linux... ");
 	gunzip();
 	parse_elf(output);
-	putstr("done.\nBooting the kernel.\n");
+	if (noisy)
+		putstr("done.\nBooting the kernel.\n");
 	return;
 }
-- 
1.5.5.1

Re: [PATCH 2/2] Only print "Decompressing Linux" etc when 'noisy' is passed.

[Johannes Weiner]

Hi,

Kristian Høgsberg <krh@redhat.com> writes:

> Signed-off-by: Kristian Høgsberg <krh@redhat.com>
> ---
>  arch/x86/boot/compressed/misc.c |   30 +++++++++++++++++++++++++++---
>  1 files changed, 27 insertions(+), 3 deletions(-)

Isn't that a bit much code to potentially suppress three lines of
output?

	Hannes

Re: [PATCH 1/2] Use structs instead of hardcoded offsets in x86 boot decompressor.

[Rik van Riel]

On Fri, 23 May 2008 17:59:27 -0400
Kristian Høgsberg <krh@redhat.com> wrote:

> Signed-off-by: Kristian Høgsberg <krh@redhat.com>

Acked-by: Rik van Riel <riel@redhat.com>


> +static struct boot_params *real_mode;		/* Pointer to real-mode data */
> +static struct screen_info *rm_screen_info;	/* Pointer to real-mode data */

People who wonder why these pointers never get initialized: they
point to the zeropage, which of course lives at address zero.

/* The so-called "zeropage" */
struct boot_params {
	struct screen_info screen_info;                 /* 0x000 */
        struct apm_bios_info apm_bios_info;             /* 0x040 */
        __u8  _pad2[12];                                /* 0x054 */
        struct ist_info ist_info;                       /* 0x060 */
	...

-- 
All rights reversed.

Re: [PATCH 1/2] Use structs instead of hardcoded offsets in x86 boot decompressor.

[H. Peter Anvin]

Rik van Riel wrote:
> 
> People who wonder why these pointers never get initialized: they
> point to the zeropage, which of course lives at address zero.
> 
> /* The so-called "zeropage" */
> struct boot_params {
> 	struct screen_info screen_info;                 /* 0x000 */
>         struct apm_bios_info apm_bios_info;             /* 0x040 */
>         __u8  _pad2[12];                                /* 0x054 */
>         struct ist_info ist_info;                       /* 0x060 */
> 	...
> 

Uhm... except it doesn't live at address zero, at all.

It's called "zeropage" because we used to recycle it into 
empty_zero_page, a long long time ago.

The bootparms structure is pointed to by %esi being passed from the 
setup code to the decompressor to the kernel.

	-hpa

Re: [PATCH 2/2] Only print "Decompressing Linux" etc when 'noisy' is passed.

[Mikael Pettersson]

=?utf-8?q?Kristian=20H=C3=B8gsberg?= writes:
 > Signed-off-by: Kristian Høgsberg <krh@redhat.com>
 > ---
 >  arch/x86/boot/compressed/misc.c |   30 +++++++++++++++++++++++++++---
 >  1 files changed, 27 insertions(+), 3 deletions(-)

How is this an improvement? What bug does it fix?
Is there any evidence that these messages are harmful
for end-users?

All this accomplishes is to make early boot failures harder
to debug.

Re: [PATCH 2/2] Only print "Decompressing Linux" etc when 'noisy' is passed.

[Diego Calleja]

El Sat, 24 May 2008 11:25:38 +0200, Mikael Pettersson <mikpe@it.uu.se> escribió:

> All this accomplishes is to make early boot failures harder
> to debug.

And there's already a parameter to switch the verbosity of printk - 'quiet'

Re: [PATCH 2/2] Only print "Decompressing Linux" etc when 'noisy' is passed.

[devzero]

what about using "quiet" here instead ?

quiet is already the parameter to suppress boot messages, so introducing another one doesn`t make sense, imho.

the early boot commandline parser was extended some time ago to support boolean options, see:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=32d0b9898029b7b3c7f161d31f57c4831d9049eb

this may shrink your patch , see example in arch/x86/boot/edd.c 


be_quiet = cmdline_find_option_bool("quiet");
....
if (!be_quiet)
    printf("Probing EDD (edd=off to disable)... ");


roland



List:       linux-kernel
Subject:    [PATCH 2/2] Only print "Decompressing Linux" etc when 'noisy' is passed.
From:       Kristian HÃ&#382;gsberg <krh () redhat ! com>
Date:       2008-05-23 21:59:28
Message-ID: 1211579968-15227-2-git-send-email-krh () redhat ! com
[Download message RAW]

Signed-off-by: Kristian HÃ&#382;gsberg <krh@redhat.com>
---
 arch/x86/boot/compressed/misc.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 0d03579..b7b5c1f 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -190,6 +190,7 @@ static void gzip_release(void **);
  */
 static struct boot_params *real_mode;          /* Pointer to real-mode data */
 static struct screen_info *rm_screen_info;     /* Pointer to real-mode data */
+static int noisy;
 
 extern unsigned char input_data[];
 extern int input_len;
@@ -391,7 +392,8 @@ static void parse_elf(void *output)
                return;
        }
 
-       putstr("Parsing ELF... ");
+       if (noisy)
+               putstr("Parsing ELF... ");
 
        phdrs = malloc(sizeof(*phdrs) * ehdr.e_phnum);
        if (!phdrs)
@@ -419,11 +421,27 @@ static void parse_elf(void *output)
        }
 }
 
+static const char *strnstr(const char *string, int len, const char *s)
+{
+       int i, j;
+
+       for (i = 0; i < len; i++) {
+               for (j = 0; i + j < len && s[j]; j++)
+                       if (string[i + j] != s[j])
+                               break;
+               if (s[j] == '\0')
+                       return string + i;
+       }
+
+       return NULL;
+}
 asmlinkage void decompress_kernel(void *rmode, memptr heap,
                                  unsigned char *input_data,
                                  unsigned long input_len,
                                  unsigned char *output)
 {
+       char *cmdline;
+
        real_mode = rmode;
 
        if (rm_screen_info->orig_video_mode == 7) {
@@ -437,6 +455,10 @@ asmlinkage void decompress_kernel(void *rmode, memptr heap,
        lines = rm_screen_info->orig_video_lines;
        cols = rm_screen_info->orig_video_cols;
 
+       cmdline = (char *) real_mode->hdr.cmd_line_ptr;
+       if (strnstr(cmdline, real_mode->hdr.cmdline_size, "noisy"))
+               noisy = 1;
+
        window = output;                /* Output buffer (Normally at 1M) */
        free_mem_ptr     = heap;        /* Heap */
        free_mem_end_ptr = heap + BOOT_HEAP_SIZE;
@@ -461,9 +483,11 @@ asmlinkage void decompress_kernel(void *rmode, memptr heap,
 #endif
 
        makecrc();
-       putstr("\nDecompressing Linux... ");
+       if (noisy)
+               putstr("\nDecompressing Linux... ");
        gunzip();
        parse_elf(output);
-       putstr("done.\nBooting the kernel.\n");
+       if (noisy)
+               putstr("done.\nBooting the kernel.\n");
        return;
 }
-- 
1.5.5.1
_______________________________________________________________________
EINE FÜR ALLE: die kostenlose WEB.DE-Plattform für Freunde und Deine
Homepage mit eigenem Namen. Jetzt starten! http://unddu.de/?kid=kid@mf2

Re: [PATCH 2/2] Only print "Decompressing Linux" etc when 'noisy' is passed.

[Kristian Høgsberg]

On Sat, May 24, 2008 at 09:28:21PM +0200, devzero@web.de wrote:
> what about using "quiet" here instead ?
> 
> quiet is already the parameter to suppress boot messages, so introducing another one doesn`t make sense, imho.

I agree, and I wasn't looking to introduce a new parameter, I meant to
reuse 'debug'.  That is already used for increasing the log level
(see init/main.c).  Also, as it is, the decompressor already prints
error messages if something goes wrong, so there's not much point in
printing the debug spew.  I think it makes sense to default to not
print it.

> the early boot commandline parser was extended some time ago to support boolean options, see:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=32d0b9898029b7b3c7f161d31f57c4831d9049eb
> 
> this may shrink your patch , see example in arch/x86/boot/edd.c 

Unfortunately, that doesn't help me.  This code is run before the kernel
is decompressed and the only functions available are the ones in misc.c.
And I know the strnstr approach isn't quite smart enough the handle quoted
parameters and similar, but I think it strikes a good balance.

I'll send an updated patch that looks for 'debug' instead of 'noisy'.

thanks,
Kristian

Re: [PATCH 1/2] Use structs instead of hardcoded offsets in x86 boot decompressor.

[Rik van Riel]

On Fri, 23 May 2008 23:17:01 -0700
"H. Peter Anvin" <hpa@zytor.com> wrote:

> Uhm... except it doesn't live at address zero, at all.
> 
> It's called "zeropage" because we used to recycle it into 
> empty_zero_page, a long long time ago.
> 
> The bootparms structure is pointed to by %esi being passed from the 
> setup code to the decompressor to the kernel.

Then why does dereferencing the uninitialized
pointer work, both in the code before and after
the patch?

What am I misssing?

Re: [PATCH 1/2] Use structs instead of hardcoded offsets in x86 boot decompressor.

[H. Peter Anvin]

Rik van Riel wrote:
> On Fri, 23 May 2008 23:17:01 -0700
> "H. Peter Anvin" <hpa@zytor.com> wrote:
> 
>> Uhm... except it doesn't live at address zero, at all.
>>
>> It's called "zeropage" because we used to recycle it into 
>> empty_zero_page, a long long time ago.
>>
>> The bootparms structure is pointed to by %esi being passed from the 
>> setup code to the decompressor to the kernel.
> 
> Then why does dereferencing the uninitialized
> pointer work, both in the code before and after
> the patch?
> 
> What am I misssing?

Heck if I know.  It definitely *shouldn't* work...

	-hpa

Re: [PATCH 1/2] Use structs instead of hardcoded offsets in x86 boot decompressor.

[Kristian Høgsberg]

On Sat, May 24, 2008 at 09:06:44PM -0700, H. Peter Anvin wrote:
> Rik van Riel wrote:
>> On Fri, 23 May 2008 23:17:01 -0700
>> "H. Peter Anvin" <hpa@zytor.com> wrote:
>>
>>> Uhm... except it doesn't live at address zero, at all.
>>>
>>> It's called "zeropage" because we used to recycle it into 
>>> empty_zero_page, a long long time ago.
>>>
>>> The bootparms structure is pointed to by %esi being passed from the setup 
>>> code to the decompressor to the kernel.
>>
>> Then why does dereferencing the uninitialized
>> pointer work, both in the code before and after
>> the patch?
>>
>> What am I misssing?
>
> Heck if I know.  It definitely *shouldn't* work...

Damn, I lost a chunk of the patch.  There's supposed to be a

        rm_screen_info = &real_mode->screen_info;

after the real_mode = rmode assignment.  In the code before,
RM_SCREEN_INFO was a macro that just referenced the real_mode pointer:

	#define RM_SCREEN_INFO (*(struct screen_info *)(real_mode+0))

Thanks, wil resend.
Kristian