From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1GcLpz-00060N-W3 for mharc-grub-devel@gnu.org; Tue, 24 Oct 2006 08:57:00 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1GcLpy-0005zm-2l for grub-devel@gnu.org; Tue, 24 Oct 2006 08:56:58 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1GcLpt-0005yo-Nj for grub-devel@gnu.org; Tue, 24 Oct 2006 08:56:57 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1GcLpt-0005yl-B8 for grub-devel@gnu.org; Tue, 24 Oct 2006 08:56:53 -0400 Received: from [195.54.107.76] (helo=mxfep03.bredband.com) by monty-python.gnu.org with esmtp (Exim 4.52) id 1GcLpt-0006Iz-6M for grub-devel@gnu.org; Tue, 24 Oct 2006 08:56:53 -0400 Received: from ironport2.bredband.com ([195.54.107.84] [195.54.107.84]) by mxfep03.bredband.com with ESMTP id <20061024125652.VFTT18511.mxfep03.bredband.com@ironport2.bredband.com> for ; Tue, 24 Oct 2006 14:56:52 +0200 Received: from c-e8df71d5.029-19-73746f13.cust.bredbandsbolaget.se (HELO localhost.localdomain) ([213.113.223.232]) by ironport2.bredband.com with ESMTP; 24 Oct 2006 14:56:51 +0200 From: Johan Rydberg To: The development of GRUB 2 References: <453DB9A3.8070809@intel.com> Date: Tue, 24 Oct 2006 15:26:01 +0200 In-Reply-To: <453DB9A3.8070809@intel.com> (mao bibo's message of "Tue, 24 Oct 2006 14:58:43 +0800") Message-ID: <87bqo13kwm.fsf@night.trouble.net> User-Agent: Gnus/5.110004 (No Gnus v0.4) Emacs/21.4 (gnu/linux) MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" Subject: Re: [PATCH 1/3] grub efi memory map patch 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: Tue, 24 Oct 2006 12:56:58 -0000 --=-=-= Content-Transfer-Encoding: quoted-printable "bibo,mao" writes: > This patch moves find_mmap_size from i386/efi/linux.c to > kern/efi/mm.c, and renamed as grub_efi_find_mmap_size, so that > other arch on EFI platform can use this function. Good call. > Also this function solves memory map too small problem, > on some EFI platform MEMORY_MAP_SIZE(0x1000) is a little smaller > than EFI memory map table. > > any suggestion is welcome. Just a few comments here. I will leave a full review to Okuji. > +#define GRUB_EFI_PAGE_SHIFT 12 > +#define GRUB_EFI_PAGE_SIZE (0x1UL << GRUB_EFI_PAGE_SHIFT) > +#define GRUB_EFI_PAGES(addr) (addr >> GRUB_EFI_PAGE_SHIFT) > +#define GRUB_EFI_PAGES_UP(addr) ((addr + GRUB_EFI_PAGE_SIZE - 1) >> GRU= B_EFI_PAGE_SHIFT) > +#define PAGE_DOWN(addr) ((addr) & (~(GRUB_EFI_PAGE_SIZE - 1))) > +#define PAGE_UP(addr) PAGE_DOWN(addr + GRUB_EFI_PAGE_SIZE - 1) The PAGE_DOWN and PAGE_UP macros should be named something else, with a GRUB_EFI prefix. What about GRUB_EFI_PAGE_TRUNC and GRUB_EFI_PAGE_ROUND? Not sure GRUB_EFI_PAGES_UP is needed, since GRUB_EFI_PAGES (GRUB_EFI_PAGE_ROUND (value)) can be used instead. If you find it too long, add a local macro to the file where you use it. Also, please slap a few parenthesis round marco arguments. > +/* Find the optimal number of pages for the memory map. */ > +grub_efi_uintn_t > +grub_efi_find_mmap_size (void) > +{ + static grub_efi_uintn_t mmap_size =3D 0; > + + if (mmap_size !=3D 0) > + return mmap_size; > + + mmap_size =3D GRUB_EFI_PAGE_SIZE; > + while (1) > + { > + int ret; > + grub_efi_memory_descriptor_t *mmap; > + grub_efi_uintn_t desc_size; > + + mmap =3D grub_efi_allocate_pages ( 0, > GRUB_EFI_PAGES_UP(mmap_size)); > + if (! mmap) > + return 0; > + + ret =3D grub_efi_get_memory_map (&mmap_size, mmap, 0, > &desc_size, 0); > + grub_efi_free_pages ((grub_addr_t) mmap, GRUB_EFI_PAGES_UP(mmap_si= ze)); > + + if (ret < 0) > + grub_fatal ("cannot get memory map"); > + else if (ret > 0) > + break; > + + mmap_size +=3D GRUB_EFI_PAGE_SIZE; > + } > + + /* Increase the size a bit for safety, because GRUB allocates > more on > + later, and EFI itself may allocate more. */ > + mmap_size +=3D GRUB_EFI_PAGE_SIZE; > + mmap_size =3D PAGE_UP(mmap_size); > + + return mmap_size; > +} Is this some patching gone wrong? What are all the "+"'s doing there? The specification states that GetMemoryMap will return EFI_BUFFER_TOO_SMALL and the memory map size in *MemoryMapSize if the memory map could not fit in the buffer (which may be NULL if provided buffer size is too small). Meaning that you can get the size of the memory map by simply doing; grub_efi_uintn_t grub_efi_find_mmap_size (void) { mmap_size =3D 0; err =3D grub_efi_get_memory_map (&mmap_size, NULL, 0, NULL, 0); if (err !=3D GRUB_EFI_BUFFER_TOO_SMALL) return err; return mmap_size; } (code not tested, nor compiled.) I think Tristan did this in his patch that addresses the same problem of that the memory map does not fit in a single page. > + > void > grub_efi_mm_init (void) Whitespace changes are normally not welcome. But they could be removed by the maintainer who commits it. No worries. B> { > @@ -346,6 +381,8 @@ grub_efi_mm_init (void) > grub_efi_uintn_t desc_size; > grub_efi_uint64_t total_pages; > grub_efi_uint64_t required_pages; > + grub_efi_uintn_t memory_map_size; > + int res; > > /* First of all, allocate pages to maintain allocations. */ > allocated_pages > @@ -355,16 +392,20 @@ grub_efi_mm_init (void) > > grub_memset (allocated_pages, 0, ALLOCATED_PAGES_SIZE); > - /* Prepare a memory region to store two memory maps. */ > - memory_map =3D grub_efi_allocate_pages (0, > - 2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE)); > + /* Prepare a memory region to store two memory maps. Obtain size of > + memory map by passing a NULL buffer and a buffer size of > + zero. */ > + memory_map_size =3D grub_efi_find_mmap_size( ); > + memory_map =3D grub_efi_allocate_pages(0, 2 * PAGE_UP(memory_map_size)= ); > + One space between function name and parenthesis, and no spaces within a empty argument list. > if (! memory_map) > grub_fatal ("cannot allocate memory"); > > - filtered_memory_map =3D NEXT_MEMORY_DESCRIPTOR (memory_map, MEMORY_MAP= _SIZE); > + filtered_memory_map =3D NEXT_MEMORY_DESCRIPTOR (memory_map, > + > PAGE_UP(memory_map_size)); I suppose this is a line-wrapped patch?=20 --=-=-= Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2.2 (GNU/Linux) iD8DBQBFPhRs3CqIy3K3X2ERAtb7AKDXIuK5YhJlza8y6meh/UlorGfmugCffNnZ dY9yqtm3CHovjLmk3+YK5eY= =i/XK -----END PGP SIGNATURE----- --=-=-=--