From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1GcbT2-0005R1-Hz for mharc-grub-devel@gnu.org; Wed, 25 Oct 2006 01:38:20 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1GcbT0-0005QP-DI for grub-devel@gnu.org; Wed, 25 Oct 2006 01:38:18 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1GcbSw-0005PO-S9 for grub-devel@gnu.org; Wed, 25 Oct 2006 01:38:17 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1GcbSw-0005PJ-LR for grub-devel@gnu.org; Wed, 25 Oct 2006 01:38:14 -0400 Received: from [192.55.52.88] (helo=mga01.intel.com) by monty-python.gnu.org with esmtp (Exim 4.52) id 1GcbSw-00024W-Bt for grub-devel@gnu.org; Wed, 25 Oct 2006 01:38:14 -0400 Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by mga01.intel.com with ESMTP; 24 Oct 2006 22:38:13 -0700 Received: from bmao-mobl.ccr.corp.intel.com (HELO [10.239.22.151]) ([10.239.22.151]) by fmsmga001.fm.intel.com with ESMTP; 24 Oct 2006 22:38:10 -0700 X-ExtLoop1: 1 X-IronPort-AV: i="4.09,354,1157353200"; d="scan'208"; a="151636308:sNHT69755189" Message-ID: <453EF83F.5050601@intel.com> Date: Wed, 25 Oct 2006 13:38:07 +0800 From: "bibo,mao" User-Agent: Thunderbird 1.5.0.7 (Windows/20060909) MIME-Version: 1.0 To: The development of GRUB 2 References: <453DB9A3.8070809@intel.com> <87bqo13kwm.fsf@night.trouble.net> In-Reply-To: <87bqo13kwm.fsf@night.trouble.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Wed, 25 Oct 2006 05:38:18 -0000 hi johan, Thanks for review my patch, I reply in lines. Johan Rydberg wrote: > "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) >> > GRUB_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. It sounds better for me, I rename the macro name as GRUB_EFI_PAGE_TRUNC and GRUB_EFI_PAGE_ROUND, and delete GRUB_EFI_PAGES_UP macro. > 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 = 0; > > + + if (mmap_size != 0) > > + return mmap_size; > > + + mmap_size = GRUB_EFI_PAGE_SIZE; > > + while (1) > > + { > > + int ret; > > + grub_efi_memory_descriptor_t *mmap; > > + grub_efi_uintn_t desc_size; > > + + mmap = grub_efi_allocate_pages ( 0, > > GRUB_EFI_PAGES_UP(mmap_size)); > > + if (! mmap) > > + return 0; > > + + ret = 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_size)); > > + + if (ret < 0) > > + grub_fatal ("cannot get memory map"); > > + else if (ret > 0) > > + break; > > + + mmap_size += 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 += GRUB_EFI_PAGE_SIZE; > > + mmap_size = 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 = 0; > err = grub_efi_get_memory_map (&mmap_size, NULL, 0, NULL, 0); > if (err != 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. Yes, it is better. But there is one point reserved to notice, grub_efi_find_mmap_size will return current memory map size, if there is memory alloc function, map size will be larger. so allocated (mmap_size + one extra page) buffer for memory map buffer is more reasonable. > > + > > 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 = 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 = grub_efi_find_mmap_size( ); > > + memory_map = 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. I am not familiar with GNU coding style:( The second patch corrects the point this time. > > > if (! memory_map) > > grub_fatal ("cannot allocate memory"); > > > > - filtered_memory_map = NEXT_MEMORY_DESCRIPTOR (memory_map, MEMORY_MAP_SIZE); > > + filtered_memory_map = NEXT_MEMORY_DESCRIPTOR (memory_map, > > + > > PAGE_UP(memory_map_size)); > > I suppose this is a line-wrapped patch? > > diff -Nrup grub2.org/include/grub/efi/api.h grub2/include/grub/efi/api.h --- grub2.org/include/grub/efi/api.h 2006-10-25 12:16:38.000000000 +0800 +++ grub2/include/grub/efi/api.h 2006-10-25 11:01:32.000000000 +0800 @@ -206,6 +206,12 @@ typedef grub_efi_intn_t grub_efi_status_ #define GRUB_EFI_WARN_WRITE_FAILURE GRUB_EFI_WARNING_CODE (3) #define GRUB_EFI_WARN_BUFFER_TOO_SMALL GRUB_EFI_WARNING_CODE (4) +#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_PAGE_TRUNC(addr) ((addr) & (~(GRUB_EFI_PAGE_SIZE - 1))) +#define GRUB_EFI_PAGE_ROUND(addr) GRUB_EFI_PAGE_TRUNC(addr + GRUB_EFI_PAGE_SIZE - 1) + typedef void *grub_efi_handle_t; typedef void *grub_efi_event_t; typedef grub_efi_uint64_t grub_efi_lba_t; diff -Nrup grub2.org/include/grub/efi/efi.h grub2/include/grub/efi/efi.h --- grub2.org/include/grub/efi/efi.h 2006-10-25 12:16:38.000000000 +0800 +++ grub2/include/grub/efi/efi.h 2006-10-25 10:53:04.000000000 +0800 @@ -55,6 +55,7 @@ char *EXPORT_FUNC(grub_efi_get_filename) grub_efi_device_path_t * EXPORT_FUNC(grub_efi_get_device_path) (grub_efi_handle_t handle); int EXPORT_FUNC(grub_efi_exit_boot_services) (grub_efi_uintn_t map_key); +grub_efi_uintn_t EXPORT_FUNC(grub_efi_find_mmap_size) (void); void grub_efi_mm_init (void); void grub_efi_mm_fini (void); diff -Nrup grub2.org/kern/efi/mm.c grub2/kern/efi/mm.c --- grub2.org/kern/efi/mm.c 2006-10-25 12:16:38.000000000 +0800 +++ grub2/kern/efi/mm.c 2006-10-25 11:21:10.000000000 +0800 @@ -30,10 +30,6 @@ #define BYTES_TO_PAGES(bytes) ((bytes) >> 12) #define PAGES_TO_BYTES(pages) ((pages) << 12) -/* The size of a memory map obtained from the firmware. This must be - a multiplier of 4KB. */ -#define MEMORY_MAP_SIZE 0x1000 - /* Maintain the list of allocated pages. */ struct allocated_page { @@ -335,6 +331,21 @@ print_memory_map (grub_efi_memory_descri } #endif +/* Find the optimal number of pages for the memory map. */ +grub_efi_uintn_t +grub_efi_find_mmap_size(void) +{ + grub_efi_uintn_t mmap_size = 0; + grub_efi_uintn_t desc_size; + int ret; + + ret = grub_efi_get_memory_map (&mmap_size, NULL, 0, &desc_size, 0); + if (ret < 0) + grub_fatal ("cannot get memory map"); + + return mmap_size; +} + void grub_efi_mm_init (void) { @@ -346,6 +357,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 +368,25 @@ grub_efi_mm_init (void) grub_memset (allocated_pages, 0, ALLOCATED_PAGES_SIZE); - /* Prepare a memory region to store two memory maps. */ - memory_map = 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 = grub_efi_find_mmap_size(); + + /* there will be memory alloc after getting memory_map_size + * so here GRUB_EFI_PAGE_SIZE is added. */ + memory_map_size += GRUB_EFI_PAGE_SIZE; + memory_map = grub_efi_allocate_pages (0, + 2 * GRUB_EFI_PAGE_ROUND (memory_map_size)); + if (! memory_map) grub_fatal ("cannot allocate memory"); - filtered_memory_map = NEXT_MEMORY_DESCRIPTOR (memory_map, MEMORY_MAP_SIZE); + filtered_memory_map = NEXT_MEMORY_DESCRIPTOR (memory_map, + GRUB_EFI_PAGE_ROUND (memory_map_size)); /* Obtain descriptors for available memory. */ - map_size = MEMORY_MAP_SIZE; + map_size = memory_map_size; if (grub_efi_get_memory_map (&map_size, memory_map, 0, &desc_size, 0) < 0) grub_fatal ("cannot get memory map"); @@ -406,7 +428,7 @@ grub_efi_mm_init (void) /* Release the memory maps. */ grub_efi_free_pages ((grub_addr_t) memory_map, - 2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE)); + 2 * GRUB_EFI_PAGE_ROUND (memory_map_size)); } void diff -Nrup grub2.org/loader/i386/efi/linux.c grub2/loader/i386/efi/linux.c --- grub2.org/loader/i386/efi/linux.c 2006-10-25 12:16:45.000000000 +0800 +++ grub2/loader/i386/efi/linux.c 2006-10-25 11:40:24.000000000 +0800 @@ -93,45 +93,6 @@ page_align (grub_size_t size) return (size + (1 << 12) - 1) & (~((1 << 12) - 1)); } -/* Find the optimal number of pages for the memory map. Is it better to - move this code to efi/mm.c? */ -static grub_efi_uintn_t -find_mmap_size (void) -{ - static grub_efi_uintn_t mmap_size = 0; - - if (mmap_size != 0) - return mmap_size; - - mmap_size = (1 << 12); - while (1) - { - int ret; - grub_efi_memory_descriptor_t *mmap; - grub_efi_uintn_t desc_size; - - mmap = grub_malloc (mmap_size); - if (! mmap) - return 0; - - ret = grub_efi_get_memory_map (&mmap_size, mmap, 0, &desc_size, 0); - grub_free (mmap); - - if (ret < 0) - grub_fatal ("cannot get memory map"); - else if (ret > 0) - break; - - mmap_size += (1 << 12); - } - - /* Increase the size a bit for safety, because GRUB allocates more on - later, and EFI itself may allocate more. */ - mmap_size += (1 << 12); - - return page_align (mmap_size); -} - static void free_pages (void) { @@ -167,14 +128,14 @@ allocate_pages (grub_size_t real_size, g /* Make sure that each size is aligned to a page boundary. */ real_size = page_align (real_size + GRUB_DISK_SECTOR_SIZE); prot_size = page_align (prot_size); - mmap_size = find_mmap_size (); + mmap_size = grub_efi_find_mmap_size(); grub_dprintf ("linux", "real_size = %x, prot_size = %x, mmap_size = %x\n", real_size, prot_size, mmap_size); /* Calculate the number of pages; Combine the real mode code with the memory map buffer for simplicity. */ - real_mode_pages = ((real_size + mmap_size) >> 12); + real_mode_pages = ((real_size + mmap_size + GRUB_EFI_PAGE_SIZE) >> 12); prot_mode_pages = (prot_size >> 12); /* Initialize the memory pointers with NULL for convenience. */ @@ -214,7 +175,7 @@ allocate_pages (grub_size_t real_size, g grub_dprintf ("linux", "physical_start = %x, physical_end = %x\n", (unsigned) desc->physical_start, (unsigned) physical_end); - addr = physical_end - real_size - mmap_size; + addr = physical_end - (real_mode_pages << GRUB_EFI_PAGE_SHIFT); if (addr < 0x10000) continue; @@ -275,7 +236,7 @@ grub_linux_boot (void) grub_dprintf ("linux", "idt = %x:%x, gdt = %x:%x\n", (unsigned) idt_desc.limit, (unsigned) idt_desc.base, (unsigned) gdt_desc.limit, (unsigned) gdt_desc.base); - mmap_size = find_mmap_size (); + mmap_size = grub_efi_find_mmap_size(); if (grub_efi_get_memory_map (&mmap_size, mmap_buf, &map_key, &desc_size, &desc_version) <= 0) grub_fatal ("cannot get memory map"); @@ -616,7 +577,7 @@ grub_rescue_cmd_initrd (int argc, char * addr_min = (grub_addr_t) prot_mode_mem + ((prot_mode_pages * 3) << 12); /* Find the highest address to put the initrd. */ - mmap_size = find_mmap_size (); + mmap_size = grub_efi_find_mmap_size(); if (grub_efi_get_memory_map (&mmap_size, mmap_buf, 0, &desc_size, 0) <= 0) grub_fatal ("cannot get memory map");