Fix eisa_mmap evaluation, add memory existence check

PATCH In review Christian Franke 3 participants 22 messages Oct 23, 2007 – Jan 4, 2008

Share & tools

Christian Franke

author

2007-10-23 19:06 UTC #0 lore raw

PATCH 1/1 Fix eisa_mmap evaluation, add memory existence check

This patch fixes the broken evaluation of the E801 EISA memory map. The shift was too much, the high word is already shifted :-) The bug was hidden until the E820 memory map evaluation was broken due to the struct packing issue fixed in my last patch.

The extra handling of "0x3C00" case is IMO not necessary. Regions are merged a few lines later.

During testing, I added a primitive memory to detect such problems early. It was difficult to find why grub crashes during module load.

Christian

2007-10-23 Christian Franke <[email protected]>

* kern/i386/pc/init.c (addr_is_valid): New function. (add_mem_region): Add memory existence check. (grub_machine_init): Fix evaluation of eisa_mmap.

kern/i386/pc/init.c +18 -7

@@ -83,6 +83,19 @@ make_install_device (void)
   return grub_prefix;
 }
 
+/* Check memory address */
+static int
+addr_is_valid (grub_addr_t addr)
+{
+  volatile unsigned char * p = (volatile unsigned char *)addr;
+  unsigned char x, y;
+  x = *p;
+  *p = x ^ 0xcf;
+  y = *p;
+  *p = x;
+  return y == (x ^ 0xcf);
+}
+
 /* Add a memory region.  */
 static void
 add_mem_region (grub_addr_t addr, grub_size_t size)

@@ -91,6 +104,9 @@ add_mem_region (grub_addr_t addr, grub_s
     /* Ignore.  */
     return;
 
+  if (!(addr + size > addr && addr_is_valid (addr) && addr_is_valid (addr+size-1)))
+    grub_fatal ("invalid memory region %p - %p", (char*)addr, (char*)addr+size-1);
+
   mem_regions[num_regions].addr = addr;
   mem_regions[num_regions].size = size;
   num_regions++;

@@ -199,13 +215,8 @@ grub_machine_init (void)
 
       if (eisa_mmap)
 	{
-	  if ((eisa_mmap & 0xFFFF) == 0x3C00)
-	    add_mem_region (0x100000, (eisa_mmap << 16) + 0x100000 * 15);
-	  else
-	    {
-	      add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
-	      add_mem_region (0x1000000, eisa_mmap << 16);
-	    }
+	  add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
+	  add_mem_region (0x1000000, eisa_mmap & ~0xFFFF);
 	}
       else
 	add_mem_region (0x100000, grub_get_memsize (1) << 10);

Robert Millan

2007-10-23 20:18 UTC #1 lore raw

Quoted 10 lines · Christian Franke

This patch fixes the broken evaluation of the E801 EISA memory map. The shift was too much, the high word is already shifted :-) The bug was hidden until the E820 memory map evaluation was broken due to the struct packing issue fixed in my last patch.
The extra handling of "0x3C00" case is IMO not necessary. Regions are merged a few lines later.
During testing, I added a primitive memory to detect such problems early. It was difficult to find why grub crashes during module load.

Is this related to Cygwin?

Signature · 5 lines

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 /.)

Christian Franke

author

2007-10-23 20:37 UTC #2 lore raw

Quoted 12 lines · Robert Millan

Quoted 6 lines · Christian Franke
This patch fixes the broken evaluation of the E801 EISA memory map. The shift was too much, the high word is already shifted :-) The bug was hidden until the E820 memory map evaluation was broken due to the struct packing issue fixed in my last patch.
The extra handling of "0x3C00" case is IMO not necessary. Regions are merged a few lines later.
During testing, I added a primitive memory to detect such problems early. It was difficult to find why grub crashes during module load.
Is this related to Cygwin?

No. The EISA memory map bug would crash GRUB on all systems w/o E820 memory map support.

The memory existence test is not necessary of course, but helped to check that memory map evaluation was finally working properly.

Christian

Marco Gerards

2007-11-09 14:16 UTC #3 lore raw

In reply to Christian Franke

Quoted 10 lines · Christian Franke

This patch fixes the broken evaluation of the E801 EISA memory map. The shift was too much, the high word is already shifted :-) The bug was hidden until the E820 memory map evaluation was broken due to the struct packing issue fixed in my last patch.
The extra handling of "0x3C00" case is IMO not necessary. Regions are merged a few lines later.
During testing, I added a primitive memory to detect such problems early. It was difficult to find why grub crashes during module load.

Too be honest, I do not know this code that well. Still, I will try to comment on it. Although most comments will be on style, and on the actual code itself.

Quoted 13 lines

Christian
2007-10-23 Christian Franke <[email protected]>
* kern/i386/pc/init.c (addr_is_valid): New function. (add_mem_region): Add memory existence check. (grub_machine_init): Fix evaluation of eisa_mmap.

kern/i386/pc/init.c make_install_device (void) · line 86

Quoted 4 lines

kern/i386/pc/init.c +1 -0

@@ -83,6 +83,19 @@ make_install_device (void)
   return grub_prefix;
 }
 
+/* Check memory address */

Please have a look at the GNU Coding Standards.

For comments, please use proper interpunction, so end with a .'. After the .', please add two spaces before ending the comment or before starting a new sentence.

kern/i386/pc/init.c make_install_device (void) · line 90

Quoted 4 lines

kern/i386/pc/init.c +4 -0

+static int
+addr_is_valid (grub_addr_t addr)
+{
+  volatile unsigned char * p = (volatile unsigned char *)addr;

Why volatile? I have the feeling it is not needed.

kern/i386/pc/init.c make_install_device (void) · line 97

Quoted 7 lines

kern/i386/pc/init.c +7 -0

+  unsigned char x, y;
+  x = *p;
+  *p = x ^ 0xcf;
+  y = *p;
+  *p = x;
+  return y == (x ^ 0xcf);
+}

Can you add some comments to this function? It is not obvious when and why an address is/isn't valid.

kern/i386/pc/init.c add_mem_region (grub_addr_t addr, grub_s · line 108

Quoted 9 lines

kern/i386/pc/init.c +2 -0

 /* Add a memory region.  */
 static void
 add_mem_region (grub_addr_t addr, grub_size_t size)

@@ -91,6 +104,9 @@ add_mem_region (grub_addr_t addr, grub_s
     /* Ignore.  */
     return;
 
+  if (!(addr + size > addr && addr_is_valid (addr) && addr_is_valid (addr+size-1)))
+    grub_fatal ("invalid memory region %p - %p", (char*)addr, (char*)addr+size-1);

Please use more spaces:

(char *) addr + size - 1

So a space around binary operators.

Quoted 23 lines

kern/i386/pc/init.c +2 -7

   mem_regions[num_regions].addr = addr;
   mem_regions[num_regions].size = size;
   num_regions++;

@@ -199,13 +215,8 @@ grub_machine_init (void)
 
       if (eisa_mmap)
 	{
-	  if ((eisa_mmap & 0xFFFF) == 0x3C00)
-	    add_mem_region (0x100000, (eisa_mmap << 16) + 0x100000 * 15);
-	  else
-	    {
-	      add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
-	      add_mem_region (0x1000000, eisa_mmap << 16);
-	    }
+	  add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
+	  add_mem_region (0x1000000, eisa_mmap & ~0xFFFF);
 	}
       else
 	add_mem_region (0x100000, grub_get_memsize (1) << 10);

_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Christian Franke

author

2007-11-09 23:12 UTC #4 lore raw

Quoted 20 lines · Marco Gerards

...
kern/i386/pc/init.c make_install_device (void) · line 90
Quoted 5 lines
kern/i386/pc/init.c +4 -0
+static int
+addr_is_valid (grub_addr_t addr)
+{
+  volatile unsigned char * p = (volatile unsigned char *)addr;
Why volatile? I have the feeling it is not needed.
Quoted 8 lines
kern/i386/pc/init.c +7 -0
+  unsigned char x, y;
+  x = *p;
+  *p = x ^ 0xcf;
+  y = *p;
+  *p = x;
+  return y == (x ^ 0xcf);
+}

volatile is necessary here to tell the complier that the memory address might not behave like regular memory. Otherwise, the optimizer might legitimately remove memory accesses and then constant propagation detects an unchanged value.

gcc actually does a very good job here. Result with volatile removed:

$ gcc -S -O -fomit-frame-pointer init.c && cat init.s
...
addr_is_valid:
   movl    $1, %eax
   ret
...

aka:

static int
addr_is_valid (grub_addr_t addr)
{
return 1;
}

This is at least a proof that the original function returns the correct result when real memory is present :-)

Christian

Marco Gerards

2007-11-10 15:58 UTC #5 lore raw

Quoted 22 lines · Christian Franke

Quoted 20 lines · Marco Gerards
...
kern/i386/pc/init.c make_install_device (void) · line 90
Quoted 5 lines
kern/i386/pc/init.c +4 -0
+static int
+addr_is_valid (grub_addr_t addr)
+{
+  volatile unsigned char * p = (volatile unsigned char *)addr;
Why volatile? I have the feeling it is not needed.
Quoted 8 lines
kern/i386/pc/init.c +7 -0
+  unsigned char x, y;
+  x = *p;
+  *p = x ^ 0xcf;
+  y = *p;
+  *p = x;
+  return y == (x ^ 0xcf);
+}

Quoted 6 lines

volatile is necessary here to tell the complier that the memory address might not behave like regular memory. Otherwise, the optimizer might legitimately remove memory accesses and then constant propagation detects an unchanged value.
gcc actually does a very good job here. Result with volatile removed:

I think this is just normal memory, not memory mapped hardware. Or perhaps I am misunderstanding something here.

Quoted 18 lines

$ gcc -S -O -fomit-frame-pointer init.c && cat init.s
...
addr_is_valid:
   movl    $1, %eax
   ret
...
aka:
static int
addr_is_valid (grub_addr_t addr)
{
return 1;
}

Quoted 2 lines

This is at least a proof that the original function returns the correct result when real memory is present :-)

:-)

--
Marco

Christian Franke

author

2007-11-10 19:54 UTC #6 lore raw

Quoted 8 lines · Marco Gerards

Quoted 6 lines · Christian Franke
...
volatile is necessary here to tell the complier that the memory address might not behave like regular memory. Otherwise, the optimizer might legitimately remove memory accesses and then constant propagation detects an unchanged value.

Quoted 7 lines

Quoted 2 lines
gcc actually does a very good job here. Result with volatile removed:
I think this is just normal memory, not memory mapped hardware. Or perhaps I am misunderstanding something here.

It is normal memory - but only if present :-) Like memory mapped hardware, missing memory violates the optimizers assumptions about memory accesses. Therefore, volatile is mandatory here.

Quoted 17 lines

Quoted 17 lines
$ gcc -S -O -fomit-frame-pointer init.c && cat init.s
...
addr_is_valid:
   movl    $1, %eax
   ret
...
aka:
static int
addr_is_valid (grub_addr_t addr)
{
return 1;
}

Quoted 7 lines

Quoted 3 lines
This is at least a proof that the original function returns the correct result when real memory is present :-)
:-)

BTW: this also proves that the routine leaves memory unchanged.

Christian

Robert Millan

2007-11-09 20:51 UTC #7 lore raw

In reply to Christian Franke

kern/i386/pc/init.c make_install_device (void) · line 98

Quoted 13 lines · Christian Franke

kern/i386/pc/init.c +13 -0

+/* Check memory address */
+static int
+addr_is_valid (grub_addr_t addr)
+{
+  volatile unsigned char * p = (volatile unsigned char *)addr;
+  unsigned char x, y;
+  x = *p;
+  *p = x ^ 0xcf;
+  y = *p;
+  *p = x;
+  return y == (x ^ 0xcf);
+}
+

I have a feeling this might be dangerous. Any comments on the warnings listed here:

http://www.osdev.org/wiki/How_Do_I_Determine_The_Amount_Of_RAM#Counting_RAM_by_direct_probing

Specially the bit about memory-mapped PCI.

Besides, if we really want it, perhaps it should be in a separate file so that other ports can use it if it's needed ?

Ah, and why 0xcf instead of 0xff ?

Signature · 5 lines

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 /.)

Christian Franke

author

2007-11-09 21:53 UTC #8 lore raw

Quoted 20 lines · Robert Millan

Quoted 4 lines · Christian Franke
kern/i386/pc/init.c +3 -0
+/* Check memory address */
+static int
+addr_is_valid (grub_addr_t addr)
kern/i386/pc/init.c make_install_device (void) · line 98
Quoted 10 lines
kern/i386/pc/init.c +10 -0
+{
+  volatile unsigned char * p = (volatile unsigned char *)addr;
+  unsigned char x, y;
+  x = *p;
+  *p = x ^ 0xcf;
+  y = *p;
+  *p = x;
+  return y == (x ^ 0xcf);
+}
+
I have a feeling this might be dangerous. Any comments on the warnings listed here:

Quoted 6 lines

http://www.osdev.org/wiki/How_Do_I_Determine_The_Amount_Of_RAM#Counting_RAM_by_direct_probing
?
Specially the bit about memory-mapped PCI.

This code does not check any memory-mapped PCI. It does only check the boundary returned by the BIOS memory map evaluation code (which didn't work in the E801 case).

Definitely not mandatory, but a IMO recommended assert-type check for experimental code.

Quoted 3 lines

Besides, if we really want it, perhaps it should be in a separate file so that other ports can use it if it's needed ?

Good point.

Quoted 3 lines

Ah, and why 0xcf instead of 0xff ?

... or 0xaa or 0x55.

c.f. :-)

Robert Millan

2007-11-09 22:51 UTC #9 lore raw

Quoted 5 lines · Christian Franke

Quoted 3 lines
Ah, and why 0xcf instead of 0xff ?
... or 0xaa or 0x55.

0xaa and 0x55 are typicaly used directly in memory because every bit is negated, which is precisely what `^ 0xff' would do.

Signature · 5 lines

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 /.)

Marco Gerards

2007-11-18 11:08 UTC #10 lore raw

Quoted 9 lines · Robert Millan

Quoted 5 lines · Christian Franke
Quoted 3 lines
Ah, and why 0xcf instead of 0xff ?
... or 0xaa or 0x55.
0xaa and 0x55 are typicaly used directly in memory because every bit is negated, which is precisely what `^ 0xff' would do.

Robert, can you take care of this patch? You have more expertise with this than I do :-)

--
Marco

Robert Millan

2007-11-18 11:21 UTC #11 lore raw

Quoted 7 lines · Marco Gerards

Quoted 5 lines · Robert Millan
Quoted 4 lines · Christian Franke
Quoted 3 lines
Ah, and why 0xcf instead of 0xff ?

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
... or 0xaa or 0x55.
0xaa and 0x55 are typicaly used directly in memory because every bit is negated, which is precisely what `^ 0xff' would do.
Robert, can you take care of this patch? You have more expertise with this than I do :-)

Sure.

Signature · 5 lines

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 /.)

Robert Millan

2007-11-18 11:27 UTC #12 lore raw

In reply to Christian Franke

kern/i386/pc/init.c make_install_device (void) · line 97

Quoted 12 lines · Christian Franke

kern/i386/pc/init.c +12 -0

+/* Check memory address */
+static int
+addr_is_valid (grub_addr_t addr)
+{
+  volatile unsigned char * p = (volatile unsigned char *)addr;
+  unsigned char x, y;
+  x = *p;
+  *p = x ^ 0xcf;
+  y = *p;
+  *p = x;
+  return y == (x ^ 0xcf);
+}

0xff would be better IMO.

kern/i386/pc/init.c add_mem_region (grub_addr_t addr, grub_s · line 108

Quoted 2 lines

kern/i386/pc/init.c +2 -0

+  if (!(addr + size > addr && addr_is_valid (addr) && addr_is_valid (addr+size-1)))
+    grub_fatal ("invalid memory region %p - %p", (char*)addr, (char*)addr+size-1);

Should addr + size > addr' be optimized out as size > 0' ? (or if we need it this way to check for overflows, should we prevent gcc from optimizing it?)

Signature · 5 lines

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 /.)

Robert Millan

2007-11-18 12:26 UTC #13 lore raw

Quoted 15 lines · Robert Millan

kern/i386/pc/init.c make_install_device (void) · line 97
Quoted 12 lines · Christian Franke
kern/i386/pc/init.c +12 -0
+/* Check memory address */
+static int
+addr_is_valid (grub_addr_t addr)
+{
+  volatile unsigned char * p = (volatile unsigned char *)addr;
+  unsigned char x, y;
+  x = *p;
+  *p = x ^ 0xcf;
+  y = *p;
+  *p = x;
+  return y == (x ^ 0xcf);
+}
0xff would be better IMO.

Uhm actually, I just remembered that we have the ~ operator precisely for that :-)

Signature · 5 lines

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 /.)

Christian Franke

author

2007-11-19 21:40 UTC #14 lore raw

In reply to Robert Millan

Quoted 19 lines · Robert Millan

Review

Quoted 13 lines · Christian Franke

(quoted diff) +12 -0

+/* Check memory address */
+static int
+addr_is_valid (grub_addr_t addr)
+{
+  volatile unsigned char * p = (volatile unsigned char *)addr;
+  unsigned char x, y;
+  x = *p;
+  *p = x ^ 0xcf;
+  y = *p;
+  *p = x;
+  return y == (x ^ 0xcf);
+}

0xff would be better IMO.

Done.

Quoted 8 lines

Review
Quoted 3 lines
(quoted diff) +2 -0
+  if (!(addr + size > addr && addr_is_valid (addr) && addr_is_valid (addr+size-1)))
+    grub_fatal ("invalid memory region %p - %p", (char*)addr, (char*)addr+size-1);
Should addr + size > addr' be optimized out as size > 0' ? (or if we need it this way to check for overflows, should we prevent gcc from optimizing it?)

Good point.

It worked (I usually test all corner cases before patch release). And a review of .S file shows that gcc (3.4.4) does not optimize here. I'm not sure whether that would be allowed.

But you are right - such check should not depend on specific overflow behaviour. I've changed this.

New patch attached, old changelog still valid.

Christian

kern/i386/pc/init.c +19 -7

@@ -83,6 +83,19 @@ make_install_device (void)
   return grub_prefix;
 }
 
+/* Check memory address.  */
+static int
+addr_is_valid (grub_addr_t addr)
+{
+  volatile unsigned char * p = (volatile unsigned char *)addr;
+  unsigned char x = *p;
+  unsigned char y = ~x;
+  *p = y;
+  unsigned char t = *p;
+  *p = x;
+  return y == t;
+}
+
 /* Add a memory region.  */
 static void
 add_mem_region (grub_addr_t addr, grub_size_t size)

@@ -91,6 +104,10 @@ add_mem_region (grub_addr_t addr, grub_s
     /* Ignore.  */
     return;
 
+  if (!(0 < size && size - 1 <= ~(grub_addr_t)0 - addr &&
+        addr_is_valid (addr) && addr_is_valid (addr + size - 1)))
+    grub_fatal ("invalid memory region %p - %p", (char*)addr, (char*)addr + size - 1);
+
   mem_regions[num_regions].addr = addr;
   mem_regions[num_regions].size = size;
   num_regions++;

@@ -199,13 +216,8 @@ grub_machine_init (void)
 
       if (eisa_mmap)
 	{
-	  if ((eisa_mmap & 0xFFFF) == 0x3C00)
-	    add_mem_region (0x100000, (eisa_mmap << 16) + 0x100000 * 15);
-	  else
-	    {
-	      add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
-	      add_mem_region (0x1000000, eisa_mmap << 16);
-	    }
+	  add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
+	  add_mem_region (0x1000000, eisa_mmap & ~0xFFFF);
 	}
       else
 	add_mem_region (0x100000, grub_get_memsize (1) << 10);

Christian Franke

author

2007-12-31 15:40 UTC #15 lore raw

This version of the patch contains only the fix for the E801 EISA memory map. The memory existence check was helpful for testing but is not really necessary.

But this bug should be fixed, otherwise GRUB2 would crash if BIOS does not provide the E820 memory map.

Christian

2007-12-31 Christian Franke <[email protected]>

* kern/i386/pc/init.c (grub_machine_init): Fix evaluation of eisa_mmap.

kern/i386/pc/init.c +2 -7

@@ -199,13 +199,8 @@ grub_machine_init (void)
 
       if (eisa_mmap)
 	{
-	  if ((eisa_mmap & 0xFFFF) == 0x3C00)
-	    add_mem_region (0x100000, (eisa_mmap << 16) + 0x100000 * 15);
-	  else
-	    {
-	      add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
-	      add_mem_region (0x1000000, eisa_mmap << 16);
-	    }
+	  add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
+	  add_mem_region (0x1000000, eisa_mmap & ~0xFFFF);
 	}
       else
 	add_mem_region (0x100000, grub_get_memsize (1) << 10);

Robert Millan

2008-01-01 11:47 UTC #16 lore raw

Quoted 3 lines · Christian Franke

This version of the patch contains only the fix for the E801 EISA memory map. The memory existence check was helpful for testing but is not really necessary.

I think the memory existence check might be good to have (I'd make sure it's not cpu-specific and move it to kern/whatever.c, though). As for the rest..

Quoted 10 lines

But this bug should be fixed, otherwise GRUB2 would crash if BIOS does not provide the E820 memory map.
Christian
2007-12-31 Christian Franke <[email protected]>
* kern/i386/pc/init.c (grub_machine_init): Fix evaluation of eisa_mmap.

I don't think I'm the most indicate to review this part of the patch, but since nobody else does...

Well you might find this funny, but try a google search for "e801 eisa" and see what the first hit is. :-)

Quoted 8 lines

kern/i386/pc/init.c +0 -2

@@ -199,13 +199,8 @@ grub_machine_init (void)
 
       if (eisa_mmap)
 	{
-	  if ((eisa_mmap & 0xFFFF) == 0x3C00)
-	    add_mem_region (0x100000, (eisa_mmap << 16) + 0x100000 * 15);

kern/i386/pc/init.c grub_machine_init (void) · line 206

Quoted 10 lines

kern/i386/pc/init.c +2 -5

-	  else
-	    {
-	      add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
-	      add_mem_region (0x1000000, eisa_mmap << 16);
-	    }
+	  add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
+	  add_mem_region (0x1000000, eisa_mmap & ~0xFFFF);
 	}
       else
 	add_mem_region (0x100000, grub_get_memsize (1) << 10);

Ok, as it seems, this comes from:

* grub_get_eisa_mmap() : return packed EISA memory map, lower 16 bits is
* memory between 1M and 16M in 1K parts, upper 16 bits is
* memory above 16M in 64K parts. If error, return zero.

So the replacement of "eisa_mmap << 16" seems obviously correct, but the "0x3C00" part you removed is completely misterious to me. Can you explain what was it supposed to be doing or why you removed it?

Signature · 5 lines

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 /.)

Christian Franke

author

2008-01-01 17:27 UTC #17 lore raw

Quoted 9 lines · Robert Millan

Quoted 2 lines · Christian Franke
This version of the patch contains only the fix for the E801 EISA memory map. The memory existence check was helpful for testing but is not really necessary.
I think the memory existence check might be good to have (I'd make sure it's not cpu-specific and move it to kern/whatever.c, though). As for the rest..

Yes, good point.

Quoted 5 lines

Quoted 5 lines
But this bug should be fixed, otherwise GRUB2 would crash if BIOS does not provide the E820 memory map.
Christian

Quoted 10 lines

Quoted 5 lines
2007-12-31 Christian Franke <[email protected]>
* kern/i386/pc/init.c (grub_machine_init): Fix evaluation of eisa_mmap.
I don't think I'm the most indicate to review this part of the patch, but since nobody else does...

Thanks!

Quoted 3 lines

Well you might find this funny, but try a google search for "e801 eisa" and see what the first hit is. :-)

:-)

Quoted 23 lines

Quoted 10 lines

kern/i386/pc/init.c +0 -2

@@ -199,13 +199,8 @@ grub_machine_init (void)
 
       if (eisa_mmap)
 	{
-	  if ((eisa_mmap & 0xFFFF) == 0x3C00)
-	    add_mem_region (0x100000, (eisa_mmap << 16) + 0x100000 * 15);

kern/i386/pc/init.c grub_machine_init (void) · line 206

Quoted 10 lines

kern/i386/pc/init.c +2 -5

-	  else
-	    {
-	      add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
-	      add_mem_region (0x1000000, eisa_mmap << 16);
-	    }
+	  add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
+	  add_mem_region (0x1000000, eisa_mmap & ~0xFFFF);
 	}
       else
 	add_mem_region (0x100000, grub_get_memsize (1) << 10);

Ok, as it seems, this comes from:

Quoted 9 lines

* grub_get_eisa_mmap() : return packed EISA memory map, lower 16 bits is
* memory between 1M and 16M in 1K parts, upper 16 bits is
* memory above 16M in 64K parts. If error, return zero.
So the replacement of "eisa_mmap << 16" seems obviously correct, but the "0x3C00" part you removed is completely misterious to me. Can you explain what was it supposed to be doing or why you removed it?

This part is intended to handle the (normal) case of one continuous region with not gap between 1M and 16M:
(0x3C00 << 10) = 0x100000 * 15 = 15M
But this part does not work due to the same bug.

It is IMO not necessary to make this distinction. The function compact_mem_regions() called a few lines later joins the two regions anyway.

Christian

Robert Millan

2008-01-01 17:45 UTC #18 lore raw

Quoted 32 lines · Christian Franke

Quoted 22 lines
Quoted 9 lines
kern/i386/pc/init.c +0 -2
@@ -199,13 +199,8 @@ grub_machine_init (void)
 
      if (eisa_mmap)
 	{
-	  if ((eisa_mmap & 0xFFFF) == 0x3C00)
-	    add_mem_region (0x100000, (eisa_mmap << 16) + 0x100000 * 15);
kern/i386/pc/init.c grub_machine_init (void) · line 206
Quoted 10 lines
kern/i386/pc/init.c +2 -5
-	  else
-	    {
-	      add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
-	      add_mem_region (0x1000000, eisa_mmap << 16);
-	    }
+	  add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
+	  add_mem_region (0x1000000, eisa_mmap & ~0xFFFF);
 	}
      else
 	add_mem_region (0x100000, grub_get_memsize (1) << 10);
Ok, as it seems, this comes from:
Quoted 9 lines
* grub_get_eisa_mmap() : return packed EISA memory map, lower 16 bits is
* memory between 1M and 16M in 1K parts, upper 16 bits is
* memory above 16M in 64K parts. If error, return zero.
So the replacement of "eisa_mmap << 16" seems obviously correct, but the "0x3C00" part you removed is completely misterious to me. Can you explain what was it supposed to be doing or why you removed it?

Quoted 7 lines

This part is intended to handle the (normal) case of one continuous region with not gap between 1M and 16M:
(0x3C00 << 10) = 0x100000 * 15 = 15M
But this part does not work due to the same bug.
It is IMO not necessary to make this distinction. The function compact_mem_regions() called a few lines later joins the two regions anyway.

Ah, ok. Have you verified that this is so? (setting debug=mem variable during init might help on that).

Sorry for being so inquisitive, but I need to understand things well before touching this part of GRUB, which as I said I'm not very familiar with.

Signature · 5 lines

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 /.)

Christian Franke

author

2008-01-01 18:03 UTC #19 lore raw

Quoted 6 lines · Robert Millan

Quoted 6 lines
Quoted 2 lines
...
This part is intended to handle the (normal) case of one continuous region with not gap between 1M and 16M:
(0x3C00 << 10) = 0x100000 * 15 = 15M
But this part does not work due to the same bug.

Quoted 7 lines

Quoted 2 lines
It is IMO not necessary to make this distinction. The function compact_mem_regions() called a few lines later joins the two regions anyway.
Ah, ok. Have you verified that this is so? (setting debug=mem variable during init might help on that).

Yes. During testing, I temporarily added some diagnostic output. The function compact_mem_regions() works as expected.

Quoted 4 lines

Sorry for being so inquisitive, but I need to understand things well before touching this part of GRUB, which as I said I'm not very familiar with.

A wrong memory map would break grub, therefore these parts should be changed very carefully :-)

Christian

Robert Millan

2008-01-01 18:33 UTC #20 lore raw

Quoted 6 lines · Christian Franke

Quoted 3 lines
Ah, ok. Have you verified that this is so? (setting debug=mem variable during init might help on that).
Yes. During testing, I temporarily added some diagnostic output. The function compact_mem_regions() works as expected.

Ok, if nobody has an objection, I'll check in the last patch sent by Christian.

The memory check would probably be nice as well, feel free to send in a separate patch for that.

Signature · 5 lines

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 /.)

Robert Millan

2008-01-04 11:54 UTC #21 lore raw

In reply to Christian Franke

Committed.

Quoted 40 lines · Christian Franke

This version of the patch contains only the fix for the E801 EISA memory map. The memory existence check was helpful for testing but is not really necessary.
But this bug should be fixed, otherwise GRUB2 would crash if BIOS does not provide the E820 memory map.
Christian
2007-12-31 Christian Franke <[email protected]>
* kern/i386/pc/init.c (grub_machine_init): Fix evaluation of eisa_mmap.
kern/i386/pc/init.c +2 -7
@@ -199,13 +199,8 @@ grub_machine_init (void)
 
       if (eisa_mmap)
 	{
-	  if ((eisa_mmap & 0xFFFF) == 0x3C00)
-	    add_mem_region (0x100000, (eisa_mmap << 16) + 0x100000 * 15);
-	  else
-	    {
-	      add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
-	      add_mem_region (0x1000000, eisa_mmap << 16);
-	    }
+	  add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
+	  add_mem_region (0x1000000, eisa_mmap & ~0xFFFF);
 	}
       else
 	add_mem_region (0x100000, grub_get_memsize (1) << 10);
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Signature · 5 lines

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 /.)