Christian Franke <[EMAIL PROTECTED]> 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 <[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. > > > > --- 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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel