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

[Christian Franke <Christian.Franke@t-online.de>]

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