[bug #36532] boot in EFI mode (x86_64) fails on some systems

[bug #36532] boot in EFI mode (x86_64) fails on some systems

[Stuart_Hayes]

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

Hello...

I just submitted a grub2 bug, along with a patch, at <http://savannah.gnu.org/bugs/?36532>.

Is that the right place for a patch, or should I sent it to this mailing list?

Thanks!
Stuart


[-- Attachment #2: Type: text/html, Size: 4970 bytes --]

Re: [bug #36532] boot in EFI mode (x86_64) fails on some systems

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 24.05.2012 19:27, Stuart_Hayes@Dell.com wrote:

> Hello…
> 
>  
> 
> I just submitted a grub2 bug, along with a patch, at
> <http://savannah.gnu.org/bugs/?36532>.
> 
>  
> 
> Is that the right place for a patch, or should I sent it to this mailing
> list?

This patch is for 1.99. Could you update it to 2.00~beta5 and post here
inline? Also problematic are:
- Huge copy-paste for determining load address. It should be something
like having a function for allocation of initrd-like chunk.
- No need to probe for mmap size if we don't use the size. It's
extremely unlikely that this call fails for any reason other than small
buffer.
- You allocate the exact size of mmap. This is wrong as allocating may
increase the size of mmap slightly. You need to allocate at least 4K more.

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [bug #36532] boot in EFI mode (x86_64) fails on some systems

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 24.05.2012 19:27, Stuart_Hayes@Dell.com wrote:

> Hello…
> 
>  
> 
> I just submitted a grub2 bug, along with a patch, at
> <http://savannah.gnu.org/bugs/?36532>.

Could you test the attached patch on top of HEAD?
-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: efi.diff --]
[-- Type: text/x-diff; name="efi.diff", Size: 12328 bytes --]

=== modified file 'grub-core/loader/efi/chainloader.c'
--- grub-core/loader/efi/chainloader.c	2012-03-10 19:41:28 +0000
+++ grub-core/loader/efi/chainloader.c	2012-05-27 14:29:35 +0000
@@ -111,14 +111,16 @@
 
   fp->header.type = GRUB_EFI_MEDIA_DEVICE_PATH_TYPE;
   fp->header.subtype = GRUB_EFI_FILE_PATH_DEVICE_PATH_SUBTYPE;
-  size = len * sizeof (grub_efi_char16_t) + sizeof (*fp);
+
+  size = grub_utf8_to_utf16 (fp->path_name, len * GRUB_MAX_UTF16_PER_UTF8,
+			     (const grub_uint8_t *) str, len, 0);
+  for (p = fp->path_name; p < fp->path_name + size; p++)
+    if (*p == '/')
+      *p = '\\';
+
+  size = size * sizeof (grub_efi_char16_t) + sizeof (*fp);
   fp->header.length[0] = (grub_efi_uint8_t) (size & 0xff);
   fp->header.length[1] = (grub_efi_uint8_t) (size >> 8);
-  for (p = fp->path_name; len > 0; len--, p++, str++)
-    {
-      /* FIXME: this assumes that the path is in ASCII.  */
-      *p = (grub_efi_char16_t) (*str == '/' ? '\\' : *str);
-    }
 }
 
 static grub_efi_device_path_t *
@@ -154,6 +156,7 @@
 
   file_path = grub_malloc (size
 			   + ((grub_strlen (dir_start) + 1)
+			      * GRUB_MAX_UTF16_PER_UTF8
 			      * sizeof (grub_efi_char16_t))
 			   + sizeof (grub_efi_file_path_device_path_t) * 2);
   if (! file_path)

=== modified file 'grub-core/loader/i386/linux.c'
--- grub-core/loader/i386/linux.c	2012-04-17 11:00:06 +0000
+++ grub-core/loader/i386/linux.c	2012-05-27 14:44:23 +0000
@@ -59,14 +59,10 @@
 #define ACCEPTS_PURE_TEXT 1
 #endif
 
-#define GRUB_LINUX_CL_OFFSET		0x1000
-
 static grub_dl_t my_mod;
 
 static grub_size_t linux_mem_size;
 static int loaded;
-static void *real_mode_mem;
-static grub_addr_t real_mode_target;
 static void *prot_mode_mem;
 static grub_addr_t prot_mode_target;
 static void *initrd_mem;
@@ -78,6 +74,8 @@
 static struct grub_relocator *relocator = NULL;
 static void *efi_mmap_buf;
 static grub_size_t maximal_cmdline_size;
+static struct linux_kernel_params linux_params;
+static char *linux_cmdline;
 #ifdef GRUB_MACHINE_EFI
 static grub_efi_uintn_t efi_mmap_size;
 #else
@@ -183,8 +181,8 @@
 {
   grub_relocator_unload (relocator);
   relocator = NULL;
-  real_mode_mem = prot_mode_mem = initrd_mem = 0;
-  real_mode_target = prot_mode_target = initrd_mem_target = 0;
+  prot_mode_mem = initrd_mem = 0;
+  prot_mode_target = initrd_mem_target = 0;
 }
 
 /* Allocate pages for the real mode code and the protected mode code
@@ -194,31 +192,13 @@
 		grub_size_t min_align, int relocatable,
 		grub_uint64_t prefered_address)
 {
-  grub_size_t real_size, mmap_size;
   grub_err_t err;
-
-  /* Make sure that each size is aligned to a page boundary.  */
-  real_size = GRUB_LINUX_CL_OFFSET + maximal_cmdline_size;
-  prot_size = page_align (prot_size);
-  mmap_size = find_mmap_size ();
-
-#ifdef GRUB_MACHINE_EFI
-  efi_mmap_size = find_efi_mmap_size ();
-  if (efi_mmap_size == 0)
-    return grub_errno;
-#endif
-
-  grub_dprintf ("linux", "real_size = %x, prot_size = %x, mmap_size = %x\n",
-		(unsigned) real_size, (unsigned) prot_size, (unsigned) 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 + efi_mmap_size) >> 12);
   prot_mode_pages = (prot_size >> 12);
 
   /* Initialize the memory pointers with NULL for convenience.  */
   free_pages ();
 
+  prot_size = page_align (prot_size);
   relocator = grub_relocator_new ();
   if (!relocator)
     {
@@ -229,59 +209,6 @@
   /* FIXME: Should request low memory from the heap when this feature is
      implemented.  */
 
-  auto int NESTED_FUNC_ATTR hook (grub_uint64_t, grub_uint64_t,
-				  grub_memory_type_t);
-  int NESTED_FUNC_ATTR hook (grub_uint64_t addr, grub_uint64_t size,
-			     grub_memory_type_t type)
-    {
-      /* We must put real mode code in the traditional space.  */
-
-      if (type == GRUB_MEMORY_AVAILABLE
-	  && addr <= 0x90000)
-	{
-	  if (addr < 0x10000)
-	    {
-	      size += addr - 0x10000;
-	      addr = 0x10000;
-	    }
-
-	  if (addr + size > 0x90000)
-	    size = 0x90000 - addr;
-
-	  if (real_size + mmap_size + efi_mmap_size > size)
-	    return 0;
-
-	  real_mode_target = ((addr + size) - (real_size + mmap_size + efi_mmap_size));
-	  return 1;
-	}
-
-      return 0;
-    }
-#ifdef GRUB_MACHINE_EFI
-  grub_efi_mmap_iterate (hook, 1);
-  if (! real_mode_target)
-    grub_efi_mmap_iterate (hook, 0);
-#else
-  grub_mmap_iterate (hook);
-#endif
-  if (! real_mode_target)
-    {
-      err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate real mode pages");
-      goto fail;
-    }
-
-  {
-    grub_relocator_chunk_t ch;
-    err = grub_relocator_alloc_chunk_addr (relocator, &ch,
-					   real_mode_target,
-					   (real_size + mmap_size 
-					    + efi_mmap_size));
-    if (err)
-      goto fail;
-    real_mode_mem = get_virtual_current_address (ch);
-  }
-  efi_mmap_buf = (grub_uint8_t *) real_mode_mem + real_size + mmap_size;
-
   {
     grub_relocator_chunk_t ch;
     if (relocatable)
@@ -315,9 +242,6 @@
     prot_mode_target = get_physical_target_address (ch);
   }
 
-  grub_dprintf ("linux", "real_mode_mem = %lx, real_mode_target = %lx, real_mode_pages = %x\n",
-                (unsigned long) real_mode_mem, (unsigned long) real_mode_target,
-		(unsigned) real_mode_pages);
   grub_dprintf ("linux", "prot_mode_mem = %lx, prot_mode_target = %lx, prot_mode_pages = %x\n",
                 (unsigned long) prot_mode_mem, (unsigned long) prot_mode_target,
 		(unsigned) prot_mode_pages);
@@ -335,12 +259,6 @@
 {
   int n = *e820_num;
 
-  if (n >= GRUB_E820_MAX_ENTRY)
-    {
-      return grub_error (GRUB_ERR_OUT_OF_RANGE,
-			 "Too many e820 memory map entries");
-    }
-
   if ((n > 0) && (e820_map[n - 1].addr + e820_map[n - 1].size == start) &&
       (e820_map[n - 1].type == type))
       e820_map[n - 1].size += size;
@@ -462,9 +380,89 @@
   const char *modevar;
   char *tmp;
   struct grub_relocator32_state state;
+  void *real_mode_mem;
+  grub_addr_t real_mode_target;
+  grub_size_t real_size, mmap_size;
+  grub_size_t cl_offset;
+
+  mmap_size = find_mmap_size ();
+  /* Make sure that each size is aligned to a page boundary.  */
+  cl_offset = ALIGN_UP (mmap_size + sizeof (*params), 4096);
+  real_size = cl_offset + maximal_cmdline_size;
+
+#ifdef GRUB_MACHINE_EFI
+  efi_mmap_size = find_efi_mmap_size ();
+  if (efi_mmap_size == 0)
+    return grub_errno;
+#endif
+
+  grub_dprintf ("linux", "real_size = %x, mmap_size = %x\n",
+		(unsigned) real_size, (unsigned) mmap_size);
+
+  /* Calculate the number of pages; Combine the real mode code with
+     the memory map buffer for simplicity.  */
+  real_mode_pages = ((real_size + efi_mmap_size) >> 12);
+
+  auto int NESTED_FUNC_ATTR hook (grub_uint64_t, grub_uint64_t,
+				  grub_memory_type_t);
+  int NESTED_FUNC_ATTR hook (grub_uint64_t addr, grub_uint64_t size,
+			     grub_memory_type_t type)
+    {
+      /* We must put real mode code in the traditional space.  */
+
+      if (type == GRUB_MEMORY_AVAILABLE
+	  && addr <= 0x90000)
+	{
+	  if (addr < 0x10000)
+	    {
+	      size += addr - 0x10000;
+	      addr = 0x10000;
+	    }
+
+	  if (addr + size > 0x90000)
+	    size = 0x90000 - addr;
+
+	  if (real_size + efi_mmap_size > size)
+	    return 0;
+
+	  real_mode_target = ((addr + size) - (real_size + efi_mmap_size));
+	  return 1;
+	}
+
+      return 0;
+    }
+#ifdef GRUB_MACHINE_EFI
+  grub_efi_mmap_iterate (hook, 1);
+  if (! real_mode_target)
+    grub_efi_mmap_iterate (hook, 0);
+#else
+  grub_mmap_iterate (hook);
+#endif
+  if (! real_mode_target)
+    return grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate real mode pages");
+
+  {
+    grub_relocator_chunk_t ch;
+    err = grub_relocator_alloc_chunk_addr (relocator, &ch,
+					   real_mode_target,
+					   (real_size + efi_mmap_size));
+    if (err)
+     return err;
+    real_mode_mem = get_virtual_current_address (ch);
+  }
+  efi_mmap_buf = (grub_uint8_t *) real_mode_mem + real_size;
+
+  grub_dprintf ("linux", "real_mode_mem = %lx, real_mode_target = %lx, real_mode_pages = %x\n",
+                (unsigned long) real_mode_mem, (unsigned long) real_mode_target,
+		(unsigned) real_mode_pages);
 
   params = real_mode_mem;
 
+  *params = linux_params;
+  params->cmd_line_ptr = real_mode_target + cl_offset;
+  grub_memcpy ((char *) params + cl_offset, linux_cmdline,
+	       maximal_cmdline_size);
+
 #ifdef GRUB_MACHINE_IEEE1275
   {
     const char *bootpath;
@@ -482,10 +480,10 @@
   grub_dprintf ("linux", "code32_start = %x\n",
 		(unsigned) params->code32_start);
 
-  auto int NESTED_FUNC_ATTR hook (grub_uint64_t, grub_uint64_t,
+  auto int NESTED_FUNC_ATTR hook_fill (grub_uint64_t, grub_uint64_t,
 				  grub_memory_type_t);
-  int NESTED_FUNC_ATTR hook (grub_uint64_t addr, grub_uint64_t size, 
-			     grub_memory_type_t type)
+  int NESTED_FUNC_ATTR hook_fill (grub_uint64_t addr, grub_uint64_t size, 
+				  grub_memory_type_t type)
     {
       grub_uint32_t e820_type;
       switch (type)
@@ -517,7 +515,7 @@
     }
 
   e820_num = 0;
-  if (grub_mmap_iterate (hook))
+  if (grub_mmap_iterate (hook_fill))
     return grub_errno;
   params->mmap_size = e820_num;
 
@@ -652,6 +650,8 @@
 {
   grub_dl_unref (my_mod);
   loaded = 0;
+  grub_free (linux_cmdline);
+  linux_cmdline = 0;
   return GRUB_ERR_NONE;
 }
 
@@ -781,16 +781,16 @@
 		      preffered_address))
     goto fail;
 
-  params = (struct linux_kernel_params *) real_mode_mem;
-  grub_memset (params, 0, GRUB_LINUX_CL_OFFSET + maximal_cmdline_size);
+  params = (struct linux_kernel_params *) &linux_params;
+  grub_memset (params, 0, sizeof (*params));
   grub_memcpy (&params->setup_sects, &lh.setup_sects, sizeof (lh) - 0x1F1);
 
   params->code32_start = prot_mode_target + lh.code32_start - GRUB_LINUX_BZIMAGE_ADDR;
   params->kernel_alignment = (1 << align);
   params->ps_mouse = params->padding10 =  0;
 
-  len = 0x400 - sizeof (lh);
-  if (grub_file_read (file, (char *) real_mode_mem + sizeof (lh), len) != len)
+  len = sizeof (*params) - sizeof (lh);
+  if (grub_file_read (file, (char *) params + sizeof (lh), len) != len)
     {
       if (!grub_errno)
 	grub_error (GRUB_ERR_BAD_OS, N_("premature end of file %s"),
@@ -805,7 +805,6 @@
   params->cl_magic = GRUB_LINUX_CL_MAGIC;
   params->cl_offset = 0x1000;
 
-  params->cmd_line_ptr = real_mode_target + 0x1000;
   params->ramdisk_image = 0;
   params->ramdisk_size = 0;
 
@@ -978,10 +977,12 @@
       }
 
   /* Create kernel command line.  */
-  grub_memcpy ((char *)real_mode_mem + GRUB_LINUX_CL_OFFSET, LINUX_IMAGE,
-	      sizeof (LINUX_IMAGE));
+  linux_cmdline = grub_malloc (maximal_cmdline_size);
+  if (!linux_cmdline)
+    goto fail;
+  grub_memcpy (linux_cmdline, LINUX_IMAGE, sizeof (LINUX_IMAGE));
   grub_create_loader_cmdline (argc, argv,
-			      (char *)real_mode_mem + GRUB_LINUX_CL_OFFSET
+			      linux_cmdline
 			      + sizeof (LINUX_IMAGE) - 1,
 			      maximal_cmdline_size
 			      - (sizeof (LINUX_IMAGE) - 1));
@@ -1054,7 +1055,7 @@
 
   initrd_pages = (page_align (size) >> 12);
 
-  lh = (struct linux_kernel_header *) real_mode_mem;
+  lh = (struct linux_kernel_header *) &linux_params;
 
   /* Get the highest address available for the initrd.  */
   if (grub_le_to_cpu16 (lh->version) >= 0x0203)

=== modified file 'include/grub/i386/linux.h'
--- include/grub/i386/linux.h	2012-03-04 11:13:05 +0000
+++ include/grub/i386/linux.h	2012-05-27 14:34:29 +0000
@@ -69,8 +69,6 @@
 #define GRUB_E820_NVS        4
 #define GRUB_E820_BADRAM     5
 
-#define GRUB_E820_MAX_ENTRY  128
-
 struct grub_e820_mmap
 {
   grub_uint64_t addr;
@@ -298,7 +296,7 @@
   grub_uint32_t payload_length;
   grub_uint64_t setup_data;
   grub_uint8_t pad2[120];		/* 258 */
-  struct grub_e820_mmap e820_map[GRUB_E820_MAX_ENTRY];	/* 2d0 */
+  struct grub_e820_mmap e820_map[(0x400 - 0x2d0) / 20];	/* 2d0 */
 
 } __attribute__ ((packed));
 #endif /* ! ASM_FILE */


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

RE: [bug #36532] boot in EFI mode (x86_64) fails on some systems

[Stuart_Hayes]

> 
> Well, this works, too (on top of HEAD from 7 june).
> 
> 
> --- ../../grub/grub-core/loader/i386/linux.c	2012-05-31
> 12:59:19.000000000 -0400
> +++ grub-core/loader/i386/linux.c	2012-06-12 20:47:51.455718055 -0400
> @@ -139,7 +139,7 @@ find_efi_mmap_size (void)
> 
>    /* Increase the size a bit for safety, because GRUB allocates more
> on
>       later, and EFI itself may allocate more.  */
> -  mmap_size += (1 << 12);
> +  mmap_size += (3 << 12);
> 
>    mmap_size = page_align (mmap_size);
>    return mmap_size;

Vladimir,

The 2.00rc1 version of grub2 still doesn't fix the efi memory map buffer size I've been working on (though I can see you are now allocating the efi memory map buffer very shortly before you are calling grub_efi_finish_boot_services()).

Increasing the mmap_size in find_efi_mmap_size()--as in the patch immediately above this text--does fix the problem.  Even adding (2 << 12) (instead of (1 << 12)) to the mmap_size will work on the system I'm testing with.

As is, though, 2.00rc1 fails to boot, with the error message "memory map buffer is too small".

Thanks,
Stuart

Re: [bug #36532] boot in EFI mode (x86_64) fails on some systems

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 27.06.2012 21:59, Stuart_Hayes@Dell.com wrote:

>>
>> Well, this works, too (on top of HEAD from 7 june).
>>
>>
>> --- ../../grub/grub-core/loader/i386/linux.c	2012-05-31
>> 12:59:19.000000000 -0400
>> +++ grub-core/loader/i386/linux.c	2012-06-12 20:47:51.455718055 -0400
>> @@ -139,7 +139,7 @@ find_efi_mmap_size (void)
>>
>>    /* Increase the size a bit for safety, because GRUB allocates more
>> on
>>       later, and EFI itself may allocate more.  */
>> -  mmap_size += (1 << 12);
>> +  mmap_size += (3 << 12);
>>
>>    mmap_size = page_align (mmap_size);
>>    return mmap_size;
> 
> Vladimir,
> 
> The 2.00rc1 version of grub2 still doesn't fix the efi memory map buffer size I've been working on (though I can see you are now allocating the efi memory map buffer very shortly before you are calling grub_efi_finish_boot_services()).
> 
> Increasing the mmap_size in find_efi_mmap_size()--as in the patch immediately above this text--does fix the problem.  Even adding (2 << 12) (instead of (1 << 12)) to the mmap_size will work on the system I'm testing with.
> 

I've changed it to 3. Thanks. It's annoying that even such simple
operations as we have between find_efi_mmap and finish drastically
increase memory map size.

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

RE: [bug #36532] boot in EFI mode (x86_64) fails on some systems

[Stuart_Hayes]

> -----Original Message-----
> From: grub-devel-bounces+stuart_hayes=dell.com@gnu.org [mailto:grub-
> devel-bounces+stuart_hayes=dell.com@gnu.org] On Behalf Of Vladimir 'f-
> coder/phcoder' Serbinenko
> Sent: Wednesday, June 27, 2012 3:51 PM
> To: Hayes, Stuart
> Cc: bug-grub@gnu.org; grub-devel@gnu.org; Dominguez, Jared
> Subject: Re: [bug #36532] boot in EFI mode (x86_64) fails on some
> systems
> 
> On 27.06.2012 21:59, Stuart_Hayes@Dell.com wrote:
> 
> >>
> >> Well, this works, too (on top of HEAD from 7 june).
> >>
> >>
> >> --- ../../grub/grub-core/loader/i386/linux.c	2012-05-31
> >> 12:59:19.000000000 -0400
> >> +++ grub-core/loader/i386/linux.c	2012-06-12 20:47:51.455718055
> -0400
> >> @@ -139,7 +139,7 @@ find_efi_mmap_size (void)
> >>
> >>    /* Increase the size a bit for safety, because GRUB allocates
> more
> >> on
> >>       later, and EFI itself may allocate more.  */
> >> -  mmap_size += (1 << 12);
> >> +  mmap_size += (3 << 12);
> >>
> >>    mmap_size = page_align (mmap_size);
> >>    return mmap_size;
> >
> > Vladimir,
> >
> > The 2.00rc1 version of grub2 still doesn't fix the efi memory map
> buffer size I've been working on (though I can see you are now
> allocating the efi memory map buffer very shortly before you are
> calling grub_efi_finish_boot_services()).
> >
> > Increasing the mmap_size in find_efi_mmap_size()--as in the patch
> immediately above this text--does fix the problem.  Even adding (2 <<
> 12) (instead of (1 << 12)) to the mmap_size will work on the system I'm
> testing with.
> >
> 
> I've changed it to 3. Thanks. It's annoying that even such simple
> operations as we have between find_efi_mmap and finish drastically
> increase memory map size.
> 
> --
> Regards
> Vladimir 'φ-coder/phcoder' Serbinenko

Thanks!  I completely agree that it is annoying.

RE: [bug #36532] boot in EFI mode (x86_64) fails on some systems

[Stuart_Hayes]

> > >
> > > Vladimir,
> > >
> > > The 2.00rc1 version of grub2 still doesn't fix the efi memory map
> > buffer size I've been working on (though I can see you are now
> > allocating the efi memory map buffer very shortly before you are
> > calling grub_efi_finish_boot_services()).
> > >
> > > Increasing the mmap_size in find_efi_mmap_size()--as in the patch
> > immediately above this text--does fix the problem.  Even adding (2 <<
> > 12) (instead of (1 << 12)) to the mmap_size will work on the system
> I'm
> > testing with.
> > >
> >
> > I've changed it to 3. Thanks. It's annoying that even such simple
> > operations as we have between find_efi_mmap and finish drastically
> > increase memory map size.
> >
> > --
> > Regards
> > Vladimir 'φ-coder/phcoder' Serbinenko
> 
> Thanks!  I completely agree that it is annoying.
> 

FYI, while grub-2.00 works on my system now, someone else here at Dell has tested it and found that it still doesn't work.  (I verified that they were indeed using version 2.00, and saw the error myself).

I have no idea how or why the memory map size is growing that much.  As I get time, I'll try to figure that out...

RE: [bug #36532] boot in EFI mode (x86_64) fails on some systems

[Stuart_Hayes]

> > > > Vladimir,
> > > >
> > > > The 2.00rc1 version of grub2 still doesn't fix the efi memory map
> > > buffer size I've been working on (though I can see you are now
> > > allocating the efi memory map buffer very shortly before you are
> > > calling grub_efi_finish_boot_services()).
> > > >
> > > > Increasing the mmap_size in find_efi_mmap_size()--as in the patch
> > > immediately above this text--does fix the problem.  Even adding (2
> <<
> > > 12) (instead of (1 << 12)) to the mmap_size will work on the system
> > I'm
> > > testing with.
> > > >
> > >
> > > I've changed it to 3. Thanks. It's annoying that even such simple
> > > operations as we have between find_efi_mmap and finish drastically
> > > increase memory map size.
> > >
> > > --
> > > Regards
> > > Vladimir 'φ-coder/phcoder' Serbinenko
> >
> > Thanks!  I completely agree that it is annoying.
> >
> 
> FYI, while grub-2.00 works on my system now, someone else here at Dell
> has tested it and found that it still doesn't work.  (I verified that
> they were indeed using version 2.00, and saw the error myself).
> 
> I have no idea how or why the memory map size is growing that much.  As
> I get time, I'll try to figure that out...

FYI again:

I've found the problem.  The memory map is not growing wildly... it is more of a firmware "quirk."  The firmware is thinking it needs a larger buffer than it actually needs.  So calls to GetMemoryMap with a buffer that is smaller than (say) 52368 bytes will return EFI_BUFFER_TOO_SMALL and say that the buffer needs to be 52368 bytes, but then when you call it with a buffer that's 52368 bytes, it will put the memory map into the buffer and tell you that the memory map is in fact only (say) 24576 bytes.

Unfortunately, right now, find_efi_mmap_size() will then return 24576 bytes (plus 3 pages, aligned to a page size), which isn't enough, but not because the memory map size is growing...

I'll send in a patch for grub that will make it immune to this quirk... it should be pretty low risk (I'm thinking modify find_efi_mmap_size() to use the value of mmap_size that was passed to grub_efi_get_memory_map(), rather than the value of mmap_size that was returned from that function.)  With that, I don't believe it would even need to add 3 pages to the memory map size (I watched the memory map size, and it is not growing significantly).

Stuart

Re: [bug #36532] boot in EFI mode (x86_64) fails on some systems

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 29.06.2012 22:49, Stuart_Hayes@Dell.com wrote:

>>>>> Vladimir,
>>>>>
>>>>> The 2.00rc1 version of grub2 still doesn't fix the efi memory map
>>>> buffer size I've been working on (though I can see you are now
>>>> allocating the efi memory map buffer very shortly before you are
>>>> calling grub_efi_finish_boot_services()).
>>>>>
>>>>> Increasing the mmap_size in find_efi_mmap_size()--as in the patch
>>>> immediately above this text--does fix the problem.  Even adding (2
>> <<
>>>> 12) (instead of (1 << 12)) to the mmap_size will work on the system
>>> I'm
>>>> testing with.
>>>>>
>>>>
>>>> I've changed it to 3. Thanks. It's annoying that even such simple
>>>> operations as we have between find_efi_mmap and finish drastically
>>>> increase memory map size.
>>>>
>>>> --
>>>> Regards
>>>> Vladimir 'φ-coder/phcoder' Serbinenko
>>>
>>> Thanks!  I completely agree that it is annoying.
>>>
>>
>> FYI, while grub-2.00 works on my system now, someone else here at Dell
>> has tested it and found that it still doesn't work.  (I verified that
>> they were indeed using version 2.00, and saw the error myself).
>>
>> I have no idea how or why the memory map size is growing that much.  As
>> I get time, I'll try to figure that out...
> 
> FYI again:
> 
> I've found the problem.  The memory map is not growing wildly... it is more of a firmware "quirk."  The firmware is thinking it needs a larger buffer than it actually needs.  So calls to GetMemoryMap with a buffer that is smaller than (say) 52368 bytes will return EFI_BUFFER_TOO_SMALL and say that the buffer needs to be 52368 bytes, but then when you call it with a buffer that's 52368 bytes, it will put the memory map into the buffer and tell you that the memory map is in fact only (say) 24576 bytes.
> 
> Unfortunately, right now, find_efi_mmap_size() will then return 24576 bytes (plus 3 pages, aligned to a page size), which isn't enough, but not because the memory map size is growing...
> 
> I'll send in a patch for grub that will make it immune to this quirk... it should be pretty low risk (I'm thinking modify find_efi_mmap_size() to use the value of mmap_size that was passed to grub_efi_get_memory_map(), rather than the value of mmap_size that was returned from that function.)  With that, I don't believe it would even need to add 3 pages to the memory map size (I watched the memory map size, and it is not growing significantly).
> 

What about this:
=== modified file 'grub-core/loader/i386/linux.c'
--- grub-core/loader/i386/linux.c	2012-06-27 20:55:09 +0000
+++ grub-core/loader/i386/linux.c	2012-06-29 21:12:53 +0000
@@ -118,12 +118,13 @@ find_efi_mmap_size (void)
       int ret;
       grub_efi_memory_descriptor_t *mmap;
       grub_efi_uintn_t desc_size;
+      grub_efi_uintn_t cur_mmap_size = mmap_size;
 
-      mmap = grub_malloc (mmap_size);
+      mmap = grub_malloc (cur_mmap_size);
       if (! mmap)
 	return 0;
 
-      ret = grub_efi_get_memory_map (&mmap_size, mmap, 0, &desc_size, 0);
+      ret = grub_efi_get_memory_map (&cur_mmap_size, mmap, 0, &desc_size, 0);
       grub_free (mmap);
 
       if (ret < 0)
@@ -134,6 +135,8 @@ find_efi_mmap_size (void)
       else if (ret > 0)
 	break;
 
+      if (mmap_size < cur_mmap_size)
+	mmap_size = cur_mmap_size;
       mmap_size += (1 << 12);
     }
 

It will take the largest size returned which should be safe.

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

RE: [bug #36532] boot in EFI mode (x86_64) fails on some systems

[Stuart_Hayes]

> 
> On 29.06.2012 22:49, Stuart_Hayes@Dell.com wrote:
> 
> >>>>> Vladimir,
> >>>>>
> >>>>> The 2.00rc1 version of grub2 still doesn't fix the efi memory map
> >>>> buffer size I've been working on (though I can see you are now
> >>>> allocating the efi memory map buffer very shortly before you are
> >>>> calling grub_efi_finish_boot_services()).
> >>>>>
> >>>>> Increasing the mmap_size in find_efi_mmap_size()--as in the patch
> >>>> immediately above this text--does fix the problem.  Even adding (2
> >> <<
> >>>> 12) (instead of (1 << 12)) to the mmap_size will work on the
> system
> >>> I'm
> >>>> testing with.
> >>>>>
> >>>>
> >>>> I've changed it to 3. Thanks. It's annoying that even such simple
> >>>> operations as we have between find_efi_mmap and finish drastically
> >>>> increase memory map size.
> >>>>
> >>>> --
> >>>> Regards
> >>>> Vladimir 'φ-coder/phcoder' Serbinenko
> >>>
> >>> Thanks!  I completely agree that it is annoying.
> >>>
> >>
> >> FYI, while grub-2.00 works on my system now, someone else here at
> >> Dell has tested it and found that it still doesn't work.  (I
> verified
> >> that they were indeed using version 2.00, and saw the error myself).
> >>
> >> I have no idea how or why the memory map size is growing that much.
> >> As I get time, I'll try to figure that out...
> >
> > FYI again:
> >
> > I've found the problem.  The memory map is not growing wildly... it
> is more of a firmware "quirk."  The firmware is thinking it needs a
> larger buffer than it actually needs.  So calls to GetMemoryMap with a
> buffer that is smaller than (say) 52368 bytes will return
> EFI_BUFFER_TOO_SMALL and say that the buffer needs to be 52368 bytes,
> but then when you call it with a buffer that's 52368 bytes, it will put
> the memory map into the buffer and tell you that the memory map is in
> fact only (say) 24576 bytes.
> >
> > Unfortunately, right now, find_efi_mmap_size() will then return 24576
> bytes (plus 3 pages, aligned to a page size), which isn't enough, but
> not because the memory map size is growing...
> >
> > I'll send in a patch for grub that will make it immune to this
> quirk... it should be pretty low risk (I'm thinking modify
> find_efi_mmap_size() to use the value of mmap_size that was passed to
> grub_efi_get_memory_map(), rather than the value of mmap_size that was
> returned from that function.)  With that, I don't believe it would even
> need to add 3 pages to the memory map size (I watched the memory map
> size, and it is not growing significantly).
> >
> 
> What about this:
> === modified file 'grub-core/loader/i386/linux.c'
> --- grub-core/loader/i386/linux.c	2012-06-27 20:55:09 +0000
> +++ grub-core/loader/i386/linux.c	2012-06-29 21:12:53 +0000
> @@ -118,12 +118,13 @@ find_efi_mmap_size (void)
>        int ret;
>        grub_efi_memory_descriptor_t *mmap;
>        grub_efi_uintn_t desc_size;
> +      grub_efi_uintn_t cur_mmap_size = mmap_size;
> 
> -      mmap = grub_malloc (mmap_size);
> +      mmap = grub_malloc (cur_mmap_size);
>        if (! mmap)
>  	return 0;
> 
> -      ret = grub_efi_get_memory_map (&mmap_size, mmap, 0, &desc_size,
> 0);
> +      ret = grub_efi_get_memory_map (&cur_mmap_size, mmap, 0,
> + &desc_size, 0);
>        grub_free (mmap);
> 
>        if (ret < 0)
> @@ -134,6 +135,8 @@ find_efi_mmap_size (void)
>        else if (ret > 0)
>  	break;
> 
> +      if (mmap_size < cur_mmap_size)
> +	mmap_size = cur_mmap_size;
>        mmap_size += (1 << 12);
>      }
> 
> 
> It will take the largest size returned which should be safe.
> 
> --
> Regards
> Vladimir 'φ-coder/phcoder' Serbinenko

That works great.  Tested on a couple systems, including the one on which grub-2.00 failed.

Thanks!
Stuart