patch for kern/efi/mm.c (big memmap)

patch for kern/efi/mm.c (big memmap)

[tgingold]

[-- Attachment #1: Type: text/plain, Size: 105 bytes --]



Hi,

some systems have a really big memmap.
This patch remove the memmap size limit.

Tristan.

[-- Attachment #2: mm.diffs --]
[-- Type: application/octet-stream, Size: 1057 bytes --]

33,36d32
< /* The size of a memory map obtained from the firmware. This must be
<    a multiplier of 4KB.  */
< #define MEMORY_MAP_SIZE	0x1000
< 
348a345,346
>   grub_efi_uintn_t memory_map_size;
>   int res;
359,360c357,363
<   memory_map = grub_efi_allocate_pages (0,
< 					2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
---
>   memory_map_size = 0;
>   res = grub_efi_get_memory_map (&memory_map_size, NULL, 0, &desc_size, 0);
>   if (res != 0)
>     grub_fatal ("cannot get memory map size");
> 
>   memory_map = grub_efi_allocate_pages
>     (0, 2 * BYTES_TO_PAGES (memory_map_size + 0x1000));
364c367
<   filtered_memory_map = NEXT_MEMORY_DESCRIPTOR (memory_map, MEMORY_MAP_SIZE);
---
>   filtered_memory_map = NEXT_MEMORY_DESCRIPTOR (memory_map, memory_map_size);
367c370
<   map_size = MEMORY_MAP_SIZE;
---
>   map_size = memory_map_size;
376c379
<   
---
> 
396c399
<   map_size = MEMORY_MAP_SIZE;
---
>   map_size = memory_map_size;
409c412
< 		       2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
---
> 		       2 * BYTES_TO_PAGES (memory_map_size + 0x1000));

Re: patch for kern/efi/mm.c (big memmap)

[Marco Gerards]

tgingold@free.fr writes:

Hi,

First of all, thanks a lot for this patch.

> some systems have a really big memmap.

What kind of system are you testing GRUB 2 (EFI) on?  It would be nice
to know which systems are already supported.

> This patch remove the memmap size limit.

Cool.  Can you please send in this patch in unified diff format (diff
-up) and provide a changelog entry?  And while you on it, most people
here prefer patches being sent inline so it is easier to read and to
give feedback on.

Thanks,
Marco

Re: patch for kern/efi/mm.c (big memmap)

[tgingold]

Quoting Marco Gerards <mgerards@xs4all.nl>:

> tgingold@free.fr writes:
>
> Hi,
>
> First of all, thanks a lot for this patch.
>
> > some systems have a really big memmap.
>
> What kind of system are you testing GRUB 2 (EFI) on?  It would be nice
> to know which systems are already supported.
This is on an ia64 system (Tiger).

> > This patch remove the memmap size limit.
>
> Cool.  Can you please send in this patch in unified diff format (diff
> -up) and provide a changelog entry?  And while you on it, most people
> here prefer patches being sent inline so it is easier to read and to
> give feedback on.
Sorry, this is my first patch!

Tristan.

2006-09-28  Tristan Gingold  <tristan.gingold@bull.net>

	* kern/efi/mm.c (grub_efi_mm_init): MEMORY_MAP_SIZE removed,
	dynamically get the size of the memory map.


--- grub2.cvs/kern/efi/mm.c	2006-05-29 01:01:43.000000000 +0200
+++ grub2/kern/efi/mm.c	2006-09-28 11:25:47.000000000 +0200
@@ -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
 {
@@ -346,6 +342,8 @@
   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
@@ -356,15 +354,20 @@
   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));
+  memory_map_size = 0;
+  res = grub_efi_get_memory_map (&memory_map_size, NULL, 0, &desc_size, 0);
+  if (res != 0)
+    grub_fatal ("cannot get memory map size");
+
+  memory_map = grub_efi_allocate_pages
+    (0, 2 * BYTES_TO_PAGES (memory_map_size + 0x1000));
   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, 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");
@@ -373,7 +376,7 @@

   filtered_memory_map_end = filter_memory_map (memory_map, filtered_memory_map,
 					       desc_size, memory_map_end);
-
+
   /* By default, request a quarter of the available memory.  */
   total_pages = get_total_pages (filtered_memory_map, desc_size,
 				 filtered_memory_map_end);
@@ -393,7 +396,7 @@

 #if 0
   /* For debug.  */
-  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 +409,7 @@

   /* Release the memory maps.  */
   grub_efi_free_pages ((grub_addr_t) memory_map,
-		       2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
+		       2 * BYTES_TO_PAGES (memory_map_size + 0x1000));
 }

 void

Re: patch for kern/efi/mm.c (big memmap)

[Marco Gerards]

tgingold@free.fr writes:

> This is on an ia64 system (Tiger).

I just noticed that because of your other patches, neat! :-)

>> Cool.  Can you please send in this patch in unified diff format (diff
>> -up) and provide a changelog entry?  And while you on it, most people
>> here prefer patches being sent inline so it is easier to read and to
>> give feedback on.
> Sorry, this is my first patch!

Hopefully you didn't see my feedback as evil comments ;-).  My reason
to tell you all this is to get patches in the form we would like to
see them for easy review and quick integration.

> Tristan.
>
> 2006-09-28  Tristan Gingold  <tristan.gingold@bull.net>
>
> 	* kern/efi/mm.c (grub_efi_mm_init): MEMORY_MAP_SIZE removed,
> 	dynamically get the size of the memory map.

Great.  There are a few changes I will make before committing this
patch.  Please have a look at the commit email to see what was changed
and why.  I will most likely commit this patch Sunday evening,
certainly not before that, sorry.

--
Marco

Re: patch for kern/efi/mm.c (big memmap)

[Johan Rydberg]

[-- Attachment #1.1: Type: text/plain, Size: 674 bytes --]

tgingold@free.fr writes:

> some systems have a really big memmap.  This patch remove the memmap
> size limit.

Overall the patch looks good, I have one comment through; (the patch
is for kern/efi/mm.c if someone wonders.)

+  memory_map = grub_efi_allocate_pages
+    (0, 2 * BYTES_TO_PAGES (memory_map_size + 0x1000));

I suppose you add 0x1000 to round it up.  Maybe we should change the
BYTES_TO_PAGES macro to do roundup.

I'll attach a unified diff, since those is easier to review.

Even through this patch is quite trivial and small, I believe we need
you to sign over the copyright for it to FSF.  Okuji, Marco, what are
your opinions?  

~j


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: grub.efi-mm.1.patch --]
[-- Type: text/x-patch, Size: 2666 bytes --]

Index: kern/efi/mm.c
===================================================================
RCS file: /sources/grub/grub2/kern/efi/mm.c,v
retrieving revision 1.3
diff -u -r1.3 mm.c
--- kern/efi/mm.c	28 May 2006 23:01:43 -0000	1.3
+++ kern/efi/mm.c	28 Sep 2006 13:29:16 -0000
@@ -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
 {
@@ -346,6 +342,8 @@
   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
@@ -356,15 +354,20 @@
   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));
+  memory_map_size = 0;
+  res = grub_efi_get_memory_map (&memory_map_size, NULL, 0, &desc_size, 0);
+  if (res != 0)
+    grub_fatal ("cannot get memory map size");
+
+  memory_map = grub_efi_allocate_pages
+    (0, 2 * BYTES_TO_PAGES (memory_map_size + 0x1000));
   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, 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");
@@ -373,7 +376,7 @@
   
   filtered_memory_map_end = filter_memory_map (memory_map, filtered_memory_map,
 					       desc_size, memory_map_end);
-  
+
   /* By default, request a quarter of the available memory.  */
   total_pages = get_total_pages (filtered_memory_map, desc_size,
 				 filtered_memory_map_end);
@@ -393,7 +396,7 @@
 
 #if 0
   /* For debug.  */
-  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 +409,7 @@
   
   /* Release the memory maps.  */
   grub_efi_free_pages ((grub_addr_t) memory_map,
-		       2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
+		       2 * BYTES_TO_PAGES (memory_map_size + 0x1000));
 }
 
 void

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

Re: patch for kern/efi/mm.c (big memmap)

[Marco Gerards]

Johan Rydberg <jrydberg@night.trouble.net> writes:

> +  memory_map = grub_efi_allocate_pages
> +    (0, 2 * BYTES_TO_PAGES (memory_map_size + 0x1000));
>
> I suppose you add 0x1000 to round it up.  Maybe we should change the
> BYTES_TO_PAGES macro to do roundup.

Agreed.  Can you make this change as well?

> I'll attach a unified diff, since those is easier to review.

Wow, I have two unified diffs now, you both make my day. ;-)

> Even through this patch is quite trivial and small, I believe we need
> you to sign over the copyright for it to FSF.  Okuji, Marco, what are
> your opinions?  

This patch is trivial enough to apply without the paperwork, I think.

--
Marco

Re: patch for kern/efi/mm.c (big memmap)

[Johan Rydberg]

[-- Attachment #1.1: Type: text/plain, Size: 646 bytes --]

Marco Gerards <mgerards@xs4all.nl> writes:

>> +  memory_map = grub_efi_allocate_pages
>> +    (0, 2 * BYTES_TO_PAGES (memory_map_size + 0x1000));
>>
>> I suppose you add 0x1000 to round it up.  Maybe we should change
>> the BYTES_TO_PAGES macro to do roundup.
>
> Agreed.  Can you make this change as well?

Done.  See attached patch.

>> Even through this patch is quite trivial and small, I believe we
>> need you to sign over the copyright for it to FSF.  Okuji, Marco,
>> what are your opinions?
>
> This patch is trivial enough to apply without the paperwork, I
> think.

OK.

I'll commit it after I get a few minutes over to test it.

~j


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: grub.efi-mm.2.patch --]
[-- Type: text/x-patch, Size: 2753 bytes --]

Index: kern/efi/mm.c
===================================================================
RCS file: /sources/grub/grub2/kern/efi/mm.c,v
retrieving revision 1.3
diff -u -p -r1.3 mm.c
--- kern/efi/mm.c	28 May 2006 23:01:43 -0000	1.3
+++ kern/efi/mm.c	28 Sep 2006 13:57:30 -0000
@@ -27,13 +27,9 @@
 #define NEXT_MEMORY_DESCRIPTOR(desc, size)	\
   ((grub_efi_memory_descriptor_t *) ((char *) (desc) + (size)))
 
-#define BYTES_TO_PAGES(bytes)	((bytes) >> 12)
+#define BYTES_TO_PAGES(bytes)	(((bytes) + 4095) >> 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
 {
@@ -346,6 +342,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 +353,23 @@ 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 = 0;
+  res = grub_efi_get_memory_map (&memory_map_size, NULL, 0, &desc_size, 0);
+  if (res != 0)
+    grub_fatal ("cannot get memory map size");
+
+  memory_map = grub_efi_allocate_pages
+    (0, 2 * BYTES_TO_PAGES (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, 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");
@@ -393,7 +398,7 @@ grub_efi_mm_init (void)
 
 #if 0
   /* For debug.  */
-  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 +411,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 * BYTES_TO_PAGES (memory_map_size));
 }
 
 void

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

Re: patch for kern/efi/mm.c (big memmap)

[Marco Gerards]

Johan Rydberg <jrydberg@night.trouble.net> writes:

> Marco Gerards <mgerards@xs4all.nl> writes:
>
>>> +  memory_map = grub_efi_allocate_pages
>>> +    (0, 2 * BYTES_TO_PAGES (memory_map_size + 0x1000));
>>>
>>> I suppose you add 0x1000 to round it up.  Maybe we should change
>>> the BYTES_TO_PAGES macro to do roundup.
>>
>> Agreed.  Can you make this change as well?
>
> Done.  See attached patch.

Great, I assume you can fixup the changelog entry as well?

> I'll commit it after I get a few minutes over to test it.

Cool.

--
Marco