[PATCH] memtest86+ fix

[PATCH] memtest86+ fix

[Robert Millan]

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


This fixes boot for memtest86+ (and actually, Linux zImages since they were
all affected).

The problem was that the first code in grub_linux_boot_zimage copied the
payload from  0x100000 to 0x10000.  Since GRUB starts at 0x8200 and is
typicaly more than 8 kiB in size, it'll most likely overwrite part of it.

A bit of reestructuring was necessary to allow grub_dl_unload_all() to happen
unconditionally as first step, since the information on whether this is a
"big linux" was not promptly available.

Comments?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

[-- Attachment #2: memtest.diff --]
[-- Type: text/x-diff, Size: 4152 bytes --]

2008-01-02  Robert Millan  <rmh@aybabtu.com>

	* kern/i386/loader.S (grub_linux_big): New variable.
	(grub_linux_boot_zimage): Rename to ...
	(grub_linux_boot): ... this.
	(grub_linux_boot_bzimage): Merge with `grub_linux_boot_zimage'.
	(grub_linux_boot_zimage): Conditionalize zImage copy.

	* include/grub/i386/loader.h (grub_linux_big): Add prototype.
	(grub_linux_boot_bzimage): Remove prototype.
	(grub_linux_boot_zimage): Rename to ...
	(grub_linux_boot): ... this.

	* loader/i386/pc/linux.c (big_linux): Replace with `grub_linux_big'.
	(grub_linux_boot): Remove function.

diff -urp grub2/include/grub/i386/loader.h grub2.mt/include/grub/i386/loader.h
--- grub2/include/grub/i386/loader.h	2007-10-17 11:38:55.000000000 +0200
+++ grub2.mt/include/grub/i386/loader.h	2008-01-02 17:51:45.000000000 +0100
@@ -26,11 +26,11 @@
 extern grub_uint32_t EXPORT_VAR(grub_linux_prot_size);
 extern char *EXPORT_VAR(grub_linux_tmp_addr);
 extern char *EXPORT_VAR(grub_linux_real_addr);
+extern grub_int32_t EXPORT_VAR(grub_linux_big);
 extern grub_addr_t EXPORT_VAR(grub_os_area_addr);
 extern grub_size_t EXPORT_VAR(grub_os_area_size);
 
-void EXPORT_FUNC(grub_linux_boot_zimage) (void) __attribute__ ((noreturn));
-void EXPORT_FUNC(grub_linux_boot_bzimage) (void) __attribute__ ((noreturn));
+void EXPORT_FUNC(grub_linux_boot) (void) __attribute__ ((noreturn));
 
 /* The asm part of the multiboot loader.  */
 void EXPORT_FUNC(grub_multiboot_real_boot) (grub_addr_t entry, 
diff -urp grub2/kern/i386/loader.S grub2.mt/kern/i386/loader.S
--- grub2/kern/i386/loader.S	2007-10-17 22:04:23.000000000 +0200
+++ grub2.mt/kern/i386/loader.S	2008-01-02 17:52:42.000000000 +0100
@@ -56,8 +56,17 @@ VARIABLE(grub_linux_tmp_addr)
 	.long	0
 VARIABLE(grub_linux_real_addr)
 	.long	0
+VARIABLE(grub_linux_big)
+	.long	0
 	
-FUNCTION(grub_linux_boot_zimage)
+FUNCTION(grub_linux_boot)
+	/* Must be done before zImage copy.  */
+	call	EXT_C(grub_dl_unload_all)
+
+	movl	EXT_C(grub_linux_big), %ebx
+	test	%ebx, %ebx
+	jne bzimage
+
 	/* copy the kernel */
 	movl	EXT_C(grub_linux_prot_size), %ecx
 	addl	$3, %ecx
@@ -68,9 +77,7 @@ FUNCTION(grub_linux_boot_zimage)
 	rep
 	movsl
 
-FUNCTION(grub_linux_boot_bzimage)
-	call	EXT_C(grub_dl_unload_all)
-	
+bzimage:
 	movl	EXT_C(grub_linux_real_addr), %ebx
 
 	/* copy the real mode code */
diff -urp grub2/loader/i386/pc/linux.c grub2.mt/loader/i386/pc/linux.c
--- grub2/loader/i386/pc/linux.c	2008-01-02 16:28:50.000000000 +0100
+++ grub2.mt/loader/i386/pc/linux.c	2008-01-02 17:51:35.000000000 +0100
@@ -33,23 +33,10 @@
 
 static grub_dl_t my_mod;
 
-static int big_linux;
 static grub_size_t linux_mem_size;
 static int loaded;
 
 static grub_err_t
-grub_linux_boot (void)
-{
-  if (big_linux)
-    grub_linux_boot_bzimage ();
-  else
-    grub_linux_boot_zimage ();
-
-  /* Never reach here.  */
-  return GRUB_ERR_NONE;
-}
-
-static grub_err_t
 grub_linux_unload (void)
 {
   grub_dl_unref (my_mod);
@@ -106,14 +93,14 @@ grub_rescue_cmd_linux (int argc, char *a
       goto fail;
     }
 
-  big_linux = 0;
+  grub_linux_big = 0;
   setup_sects = lh.setup_sects;
   linux_mem_size = 0;
   
   if (lh.header == grub_cpu_to_le32 (GRUB_LINUX_MAGIC_SIGNATURE)
       && grub_le_to_cpu16 (lh.version) >= 0x0200)
     {
-      big_linux = (lh.loadflags & GRUB_LINUX_FLAG_BIG_KERNEL);
+      grub_linux_big = (lh.loadflags & GRUB_LINUX_FLAG_BIG_KERNEL);
       lh.type_of_loader = GRUB_LINUX_BOOT_LOADER_TYPE;
       
       /* Put the real mode part at as a high location as possible.  */
@@ -158,7 +145,7 @@ grub_rescue_cmd_linux (int argc, char *a
   
   grub_linux_tmp_addr = (char *) GRUB_LINUX_BZIMAGE_ADDR + prot_size;
 
-  if (! big_linux
+  if (! grub_linux_big
       && prot_size > (grub_size_t) (grub_linux_real_addr
 				    - (char *) GRUB_LINUX_ZIMAGE_ADDR))
     {
@@ -177,7 +164,7 @@ grub_rescue_cmd_linux (int argc, char *a
     }
 
   grub_printf ("   [Linux-%s, setup=0x%x, size=0x%x]\n",
-	       big_linux ? "bzImage" : "zImage", real_size, prot_size);
+	       grub_linux_big ? "bzImage" : "zImage", real_size, prot_size);
 
   for (i = 1; i < argc; i++)
     if (grub_memcmp (argv[i], "vga=", 4) == 0)

Re: [PATCH] memtest86+ fix

[Yoshinori K. Okuji]

On Wednesday 02 January 2008 18:05, Robert Millan wrote:
> This fixes boot for memtest86+ (and actually, Linux zImages since they were
> all affected).
>
> The problem was that the first code in grub_linux_boot_zimage copied the
> payload from  0x100000 to 0x10000.  Since GRUB starts at 0x8200 and is
> typicaly more than 8 kiB in size, it'll most likely overwrite part of it.
>
> A bit of reestructuring was necessary to allow grub_dl_unload_all() to
> happen unconditionally as first step, since the information on whether this
> is a "big linux" was not promptly available.
>
> Comments?

It is okay, except for "grub_linux_big"... I cannot figure out what it wants 
to say. As you want to make it global, please choose a more expressive name, 
such as "grub_big_linux_loaded".

Okuji

Re: [PATCH] memtest86+ fix

[Robert Millan]

On Thu, Jan 03, 2008 at 12:46:15AM +0100, Yoshinori K. Okuji wrote:
> On Wednesday 02 January 2008 18:05, Robert Millan wrote:
> > This fixes boot for memtest86+ (and actually, Linux zImages since they were
> > all affected).
> >
> > The problem was that the first code in grub_linux_boot_zimage copied the
> > payload from  0x100000 to 0x10000.  Since GRUB starts at 0x8200 and is
> > typicaly more than 8 kiB in size, it'll most likely overwrite part of it.
> >
> > A bit of reestructuring was necessary to allow grub_dl_unload_all() to
> > happen unconditionally as first step, since the information on whether this
> > is a "big linux" was not promptly available.
> >
> > Comments?
> 
> It is okay, except for "grub_linux_big"... I cannot figure out what it wants 
> to say. As you want to make it global, please choose a more expressive name, 
> such as "grub_big_linux_loaded".

How about "grub_linux_big_loaded", for consistency with the other vars in
<machine/kernel.h> that also start with grub_linux_ ?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

Re: [PATCH] memtest86+ fix

[Yoshinori K. Okuji]

On Thursday 03 January 2008 02:06, Robert Millan wrote:
> On Thu, Jan 03, 2008 at 12:46:15AM +0100, Yoshinori K. Okuji wrote:
> > On Wednesday 02 January 2008 18:05, Robert Millan wrote:
> > > This fixes boot for memtest86+ (and actually, Linux zImages since they
> > > were all affected).
> > >
> > > The problem was that the first code in grub_linux_boot_zimage copied
> > > the payload from  0x100000 to 0x10000.  Since GRUB starts at 0x8200 and
> > > is typicaly more than 8 kiB in size, it'll most likely overwrite part
> > > of it.
> > >
> > > A bit of reestructuring was necessary to allow grub_dl_unload_all() to
> > > happen unconditionally as first step, since the information on whether
> > > this is a "big linux" was not promptly available.
> > >
> > > Comments?
> >
> > It is okay, except for "grub_linux_big"... I cannot figure out what it
> > wants to say. As you want to make it global, please choose a more
> > expressive name, such as "grub_big_linux_loaded".
>
> How about "grub_linux_big_loaded", for consistency with the other vars in
> <machine/kernel.h> that also start with grub_linux_ ?

"big_loaded" sounds horrible to me. How about grub_linux_big_image_loaded?

Okuji

Re: [PATCH] memtest86+ fix

[Pavel Roskin]

On Sat, 2008-01-05 at 02:09 +0100, Yoshinori K. Okuji wrote:

> "big_loaded" sounds horrible to me. How about grub_linux_big_image_loaded?

"big image" is normally abbreviated as bzimage, as opposed to zimage, so
I would suggest something of this kind:

grub_is_bzimage
grub_linux_is_bzimage
grub_bzimage_loaded
grub_linux_bzimage_loaded

-- 
Regards,
Pavel Roskin

Re: [PATCH] memtest86+ fix

[Robert Millan]

On Fri, Jan 04, 2008 at 08:18:19PM -0500, Pavel Roskin wrote:
> On Sat, 2008-01-05 at 02:09 +0100, Yoshinori K. Okuji wrote:
> 
> > "big_loaded" sounds horrible to me. How about grub_linux_big_image_loaded?
> 
> "big image" is normally abbreviated as bzimage, as opposed to zimage, so
> I would suggest something of this kind:
> 
> grub_is_bzimage
> grub_linux_is_bzimage
> grub_bzimage_loaded
> grub_linux_bzimage_loaded

Any name that matches "grub_linux_.*" is fine with me.  Can we just agree on
something ?  There are people waiting to use memtest86+ !  :-)

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

Re: [PATCH] memtest86+ fix

[Yoshinori K. Okuji]

On Saturday 05 January 2008 12:50, Robert Millan wrote:
> On Fri, Jan 04, 2008 at 08:18:19PM -0500, Pavel Roskin wrote:
> > On Sat, 2008-01-05 at 02:09 +0100, Yoshinori K. Okuji wrote:
> > > "big_loaded" sounds horrible to me. How about
> > > grub_linux_big_image_loaded?
> >
> > "big image" is normally abbreviated as bzimage, as opposed to zimage, so
> > I would suggest something of this kind:
> >
> > grub_is_bzimage
> > grub_linux_is_bzimage
> > grub_bzimage_loaded
> > grub_linux_bzimage_loaded
>
> Any name that matches "grub_linux_.*" is fine with me.  Can we just agree
> on something ?  There are people waiting to use memtest86+ !  :-)

grub_linux_is_bzimage sounds good to me.

Okuji

Re: [PATCH] memtest86+ fix

[Robert Millan]

On Sat, Jan 05, 2008 at 01:04:24PM +0100, Yoshinori K. Okuji wrote:
> On Saturday 05 January 2008 12:50, Robert Millan wrote:
> > On Fri, Jan 04, 2008 at 08:18:19PM -0500, Pavel Roskin wrote:
> > > On Sat, 2008-01-05 at 02:09 +0100, Yoshinori K. Okuji wrote:
> > > > "big_loaded" sounds horrible to me. How about
> > > > grub_linux_big_image_loaded?
> > >
> > > "big image" is normally abbreviated as bzimage, as opposed to zimage, so
> > > I would suggest something of this kind:
> > >
> > > grub_is_bzimage
> > > grub_linux_is_bzimage
> > > grub_bzimage_loaded
> > > grub_linux_bzimage_loaded
> >
> > Any name that matches "grub_linux_.*" is fine with me.  Can we just agree
> > on something ?  There are people waiting to use memtest86+ !  :-)
> 
> grub_linux_is_bzimage sounds good to me.

Ok, committed.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)