From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1IqUf9-00080o-No for mharc-grub-devel@gnu.org; Fri, 09 Nov 2007 09:16:47 -0500 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1IqUf7-000805-Bj for grub-devel@gnu.org; Fri, 09 Nov 2007 09:16:45 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1IqUf6-0007zW-Nl for grub-devel@gnu.org; Fri, 09 Nov 2007 09:16:45 -0500 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1IqUf6-0007zT-Jk for grub-devel@gnu.org; Fri, 09 Nov 2007 09:16:44 -0500 Received: from smtp-vbr7.xs4all.nl ([194.109.24.27]) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1IqUf6-000522-3O for grub-devel@gnu.org; Fri, 09 Nov 2007 09:16:44 -0500 Received: from localhost.localdomain (249-174.surfsnel.dsl.internl.net [145.99.174.249]) by smtp-vbr7.xs4all.nl (8.13.8/8.13.8) with ESMTP id lA9EGgo5099782 for ; Fri, 9 Nov 2007 15:16:43 +0100 (CET) (envelope-from mgerards@xs4all.nl) From: Marco Gerards To: The development of GRUB 2 References: <471E4628.9030706@t-online.de> Mail-Copies-To: mgerards@xs4all.nl Date: Fri, 09 Nov 2007 15:17:13 +0100 In-Reply-To: <471E4628.9030706@t-online.de> (Christian Franke's message of "Tue, 23 Oct 2007 21:06:16 +0200") Message-ID: <87d4ujmrkm.fsf@xs4all.nl> User-Agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Virus-Scanned: by XS4ALL Virus Scanner X-detected-kernel: by monty-python.gnu.org: FreeBSD 4.6-4.9 Subject: Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Nov 2007 14:16:46 -0000 Christian Franke 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 > > * 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