[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/3] grub efi memory map patch
|
From: |
bibo,mao |
|
Subject: |
Re: [PATCH 1/3] grub efi memory map patch |
|
Date: |
Wed, 25 Oct 2006 13:38:07 +0800 |
|
User-agent: |
Thunderbird 1.5.0.7 (Windows/20060909) |
hi johan,
Thanks for review my patch, I reply in lines.
Johan Rydberg wrote:
"bibo,mao" <address@hidden> 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");