[PATCH] Fix eisa_mmap evaluation, add memory existence check

[PATCH] Fix eisa_mmap evaluation, add memory existence check

[Christian Franke]

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

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  <franke@computer.org>

	* 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.




[-- Attachment #2: grub2-init-eisa_mmap.patch --]
[-- Type: text/x-patch, Size: 1427 bytes --]

--- grub2.orig/kern/i386/pc/init.c	2007-10-22 22:22:51.359375000 +0200
+++ grub2/kern/i386/pc/init.c	2007-10-22 22:25:44.546875000 +0200
@@ -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);

Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check

[Robert Millan]

On Tue, Oct 23, 2007 at 09:06:16PM +0200, Christian Franke wrote:
> 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?

-- 
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] Fix eisa_mmap evaluation, add memory existence check

[Christian Franke]

Robert Millan wrote:
> On Tue, Oct 23, 2007 at 09:06:16PM +0200, Christian Franke wrote:
>   
>> 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

Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check

[Marco Gerards]

Christian Franke <Christian.Franke@t-online.de> writes:

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

> Christian
>
> 2007-10-23  Christian Franke  <franke@computer.org>
>
> 	* 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.
>
>
>
> --- grub2.orig/kern/i386/pc/init.c	2007-10-22 22:22:51.359375000 +0200
> +++ grub2/kern/i386/pc/init.c	2007-10-22 22:25:44.546875000 +0200
> @@ -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.

> +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.

> +  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.

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

>    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
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check

[Christian Franke]

Marco Gerards wrote:
> ...
>> +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.
>
>   
>> +  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

Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check

[Marco Gerards]

Christian Franke <Christian.Franke@t-online.de> writes:

> Marco Gerards wrote:
>> ...
>>> +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.
>>
>>
>>> +  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:

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

>
> $ 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 :-)

:-)

--
Marco

Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check

[Christian Franke]

Marco Gerards wrote:
> Christian Franke <Christian.Franke@t-online.de> writes:
>   
>> ...
>> 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.
>
>   

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.


>> $ 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 :-)
>>     
>
> :-)
>
>   
BTW: this also proves that the routine leaves memory unchanged.

Christian

Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check

[Robert Millan]

On Tue, Oct 23, 2007 at 09:06:16PM +0200, Christian Franke wrote:
> +/* 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 ?

-- 
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] Fix eisa_mmap evaluation, add memory existence check

[Christian Franke]

Robert Millan wrote:
> On Tue, Oct 23, 2007 at 09:06:16PM +0200, Christian Franke wrote:
>   
>> +/* 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.
>   

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.



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


> Ah, and why 0xcf instead of 0xff ?
>
>   

... or 0xaa or 0x55.

c.f. :-)

Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check

[Robert Millan]

On Fri, Nov 09, 2007 at 10:53:23PM +0100, Christian Franke wrote:
> >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 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] Fix eisa_mmap evaluation, add memory existence check

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Fri, Nov 09, 2007 at 10:53:23PM +0100, Christian Franke wrote:
>> >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

Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check

[Robert Millan]

On Sun, Nov 18, 2007 at 12:09:25PM +0100, Marco Gerards wrote:
> Robert Millan <rmh@aybabtu.com> writes:
> 
> > On Fri, Nov 09, 2007 at 10:53:23PM +0100, Christian Franke wrote:
> >> >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 :-)

Sure.

-- 
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] Fix eisa_mmap evaluation, add memory existence check

[Robert Millan]

On Tue, Oct 23, 2007 at 09:06:16PM +0200, Christian Franke wrote:
> +/* 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.

> +  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?)

-- 
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] Fix eisa_mmap evaluation, add memory existence check

[Robert Millan]

On Sun, Nov 18, 2007 at 12:27:37PM +0100, Robert Millan wrote:
> On Tue, Oct 23, 2007 at 09:06:16PM +0200, Christian Franke wrote:
> > +/* 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 :-)

-- 
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] Fix eisa_mmap evaluation, add memory existence check

[Christian Franke]

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

Robert Millan wrote:
> On Tue, Oct 23, 2007 at 09:06:16PM +0200, Christian Franke wrote:
>   
>> +/* 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.

>> +  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


[-- Attachment #2: grub2-init-eisa_mmap-2.patch --]
[-- Type: text/x-patch, Size: 1489 bytes --]

--- grub2.orig/kern/i386/pc/init.c	2007-10-22 22:22:51.359375000 +0200
+++ grub2/kern/i386/pc/init.c	2007-11-19 22:11:19.796875000 +0100
@@ -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);

Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check

[Christian Franke]

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

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  <franke@computer.org>

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



[-- Attachment #2: grub2-init-eisa_mmap-3.patch --]
[-- Type: text/x-patch, Size: 645 bytes --]

--- grub2.orig/kern/i386/pc/init.c	2007-10-22 22:22:51.359375000 +0200
+++ grub2/kern/i386/pc/init.c	2007-12-31 16:05:59.953125000 +0100
@@ -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);

Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check

[Robert Millan]

On Mon, Dec 31, 2007 at 04:40:00PM +0100, Christian Franke wrote:
> 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..

> 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  <franke@computer.org>
> 
> 	* 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. :-)

> --- grub2.orig/kern/i386/pc/init.c	2007-10-22 22:22:51.359375000 +0200
> +++ grub2/kern/i386/pc/init.c	2007-12-31 16:05:59.953125000 +0100
> @@ -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);

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?

-- 
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] Fix eisa_mmap evaluation, add memory existence check

[Christian Franke]

Robert Millan wrote:
> On Mon, Dec 31, 2007 at 04:40:00PM +0100, Christian Franke wrote:
>   
>> 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.


>> 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  <franke@computer.org>
>>
>> 	* 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!


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

:-)


>   
>> --- grub2.orig/kern/i386/pc/init.c	2007-10-22 22:22:51.359375000 +0200
>> +++ grub2/kern/i386/pc/init.c	2007-12-31 16:05:59.953125000 +0100
>> @@ -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);
>>     
>
> 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?
>
>   

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

Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check

[Robert Millan]

On Tue, Jan 01, 2008 at 06:26:49PM +0100, Christian Franke wrote:
> >>--- grub2.orig/kern/i386/pc/init.c	2007-10-22 22:22:51.359375000 +0200
> >>+++ grub2/kern/i386/pc/init.c	2007-12-31 16:05:59.953125000 +0100
> >>@@ -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);
> >>    
> >
> >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?
> >
> >  
> 
> 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.

-- 
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] Fix eisa_mmap evaluation, add memory existence check

[Christian Franke]

Robert Millan wrote:
>>> ... 
>>>       
>> 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).
>
>   

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


> 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

Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check

[Robert Millan]

On Tue, Jan 01, 2008 at 07:03:36PM +0100, Christian Franke wrote:
> >
> >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.

-- 
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] Fix eisa_mmap evaluation, add memory existence check

[Robert Millan]

Committed.

On Mon, Dec 31, 2007 at 04:40:00PM +0100, Christian Franke wrote:
> 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  <franke@computer.org>
> 
> 	* kern/i386/pc/init.c (grub_machine_init): Fix
> 	evaluation of eisa_mmap.
> 
> 

> --- grub2.orig/kern/i386/pc/init.c	2007-10-22 22:22:51.359375000 +0200
> +++ grub2/kern/i386/pc/init.c	2007-12-31 16:05:59.953125000 +0100
> @@ -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
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


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