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  <address@hidden>
> >
> >         * 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