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

[Stuart_Hayes]

> >
> > It looks like the problem is that we set video mode. Other than that
> we
> > do only mmap iterating and allocating the memory in question. How
> about
> > this patch (not tested):
> > === modified file 'grub-core/loader/i386/linux.c'
> > --- grub-core/loader/i386/linux.c   2012-05-31 12:06:11 +0000
> > +++ grub-core/loader/i386/linux.c   2012-06-11 19:27:48 +0000
> > @@ -383,6 +383,103 @@
> >    grub_size_t real_size, mmap_size;
> >    grub_size_t cl_offset;
> >
> > +  modevar = grub_env_get ("gfxpayload");
> > +
> > +  /* Now all graphical modes are acceptable.
> > +     May change in future if we have modes without framebuffer.  */
> > +  if (modevar && *modevar != 0)
> > +    {
> > +      tmp = grub_xasprintf ("%s;" DEFAULT_VIDEO_MODE, modevar);
> > +      if (! tmp)
> > +   return grub_errno;
> > +#if ACCEPTS_PURE_TEXT
> > +      err = grub_video_set_mode (tmp, 0, 0); #else
> > +      err = grub_video_set_mode (tmp, GRUB_VIDEO_MODE_TYPE_PURE_TEXT,
> > +0); #endif
> > +      grub_free (tmp);
> > +    }
> > +  else
> > +    {
> > +#if ACCEPTS_PURE_TEXT
> > +      err = grub_video_set_mode (DEFAULT_VIDEO_MODE, 0, 0); #else
> > +      err = grub_video_set_mode (DEFAULT_VIDEO_MODE,
> > +                            GRUB_VIDEO_MODE_TYPE_PURE_TEXT, 0); #endif
> > +    }
> > +  if (err)
> > +    {
> > +      grub_print_error ();
> > +      grub_puts_ (N_("Booting in blind mode"));
> > +      grub_errno = GRUB_ERR_NONE;
> > +    }
> > +
> > +#ifdef GRUB_MACHINE_IEEE1275
> > +  {
> > +    const char *bootpath;
> > +    grub_ssize_t len;
> > +
> > +    bootpath = grub_env_get ("root");
> > +    if (bootpath)
> > +      grub_ieee1275_set_property (grub_ieee1275_chosen,
> > +                             "bootpath", bootpath,
> > +                             grub_strlen (bootpath) + 1,
> > +                             &len);
> > +  }
> > +#endif
> > +
> > +  if (grub_linux_setup_video (&linux_params))
> > +    {
> > +#if defined (GRUB_MACHINE_PCBIOS) || defined (GRUB_MACHINE_COREBOOT)
> > || defined (GRUB_MACHINE_QEMU)
> > +      linux_params.have_vga = GRUB_VIDEO_LINUX_TYPE_TEXT;
> > +      linux_params.video_mode = 0x3;
> > +#else
> > +      linux_params.have_vga = 0;
> > +      linux_params.video_mode = 0;
> > +      linux_params.video_width = 0;
> > +      linux_params.video_height = 0;
> > +#endif
> > +    }
> > +
> > +  /* Initialize these last, because terminal position could be
> > affected
> > +by printfs above.  */ #ifndef GRUB_MACHINE_IEEE1275
> > +  if (linux_params.have_vga == GRUB_VIDEO_LINUX_TYPE_TEXT) #endif
> > +    {
> > +      grub_term_output_t term;
> > +      int found = 0;
> > +      FOR_ACTIVE_TERM_OUTPUTS(term)
> > +   if (grub_strcmp (term->name, "vga_text") == 0
> > +       || grub_strcmp (term->name, "console") == 0
> > +       || grub_strcmp (term->name, "ofconsole") == 0)
> > +     {
> > +       grub_uint16_t pos = grub_term_getxy (term);
> > +       linux_params.video_cursor_x = pos >> 8;
> > +       linux_params.video_cursor_y = pos & 0xff;
> > +       linux_params.video_width = grub_term_width (term);
> > +       linux_params.video_height = grub_term_height (term);
> > +       found = 1;
> > +       break;
> > +     }
> > +      if (!found)
> > +   {
> > +     linux_params.video_cursor_x = 0;
> > +     linux_params.video_cursor_y = 0;
> > +     linux_params.video_width = 80;
> > +     linux_params.video_height = 25;
> > +   }
> > +    }
> > +
> > +#ifdef GRUB_MACHINE_IEEE1275
> > +  {
> > +    linux_params.ofw_signature = GRUB_LINUX_OFW_SIGNATURE;
> > +    linux_params.ofw_num_items = 1;
> > +    linux_params.ofw_cif_handler = (grub_uint32_t)
> > grub_ieee1275_entry_fn;
> > +    linux_params.ofw_idt = 0;
> > +  }
> > +#endif
> > +
> >    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); @@ -
> > 466,20 +563,6 @@
> >    grub_memcpy ((char *) params + cl_offset, linux_cmdline,
> >            maximal_cmdline_size);
> >
> > -#ifdef GRUB_MACHINE_IEEE1275
> > -  {
> > -    const char *bootpath;
> > -    grub_ssize_t len;
> > -
> > -    bootpath = grub_env_get ("root");
> > -    if (bootpath)
> > -      grub_ieee1275_set_property (grub_ieee1275_chosen,
> > -                             "bootpath", bootpath,
> > -                             grub_strlen (bootpath) + 1,
> > -                             &len);
> > -  }
> > -#endif
> > -
> >    grub_dprintf ("linux", "code32_start = %x\n",
> >             (unsigned) params->code32_start);
> >
> > @@ -522,89 +605,6 @@
> >      return grub_errno;
> >    params->mmap_size = e820_num;
> >
> > -  modevar = grub_env_get ("gfxpayload");
> > -
> > -  /* Now all graphical modes are acceptable.
> > -     May change in future if we have modes without framebuffer.  */
> > -  if (modevar && *modevar != 0)
> > -    {
> > -      tmp = grub_xasprintf ("%s;" DEFAULT_VIDEO_MODE, modevar);
> > -      if (! tmp)
> > -   return grub_errno;
> > -#if ACCEPTS_PURE_TEXT
> > -      err = grub_video_set_mode (tmp, 0, 0);
> > -#else
> > -      err = grub_video_set_mode (tmp, GRUB_VIDEO_MODE_TYPE_PURE_TEXT,
> > 0);
> > -#endif
> > -      grub_free (tmp);
> > -    }
> > -  else
> > -    {
> > -#if ACCEPTS_PURE_TEXT
> > -      err = grub_video_set_mode (DEFAULT_VIDEO_MODE, 0, 0);
> > -#else
> > -      err = grub_video_set_mode (DEFAULT_VIDEO_MODE,
> > -                            GRUB_VIDEO_MODE_TYPE_PURE_TEXT, 0);
> > -#endif
> > -    }
> > -  if (err)
> > -    {
> > -      grub_print_error ();
> > -      grub_puts_ (N_("Booting in blind mode"));
> > -      grub_errno = GRUB_ERR_NONE;
> > -    }
> > -
> > -  if (grub_linux_setup_video (params))
> > -    {
> > -#if defined (GRUB_MACHINE_PCBIOS) || defined (GRUB_MACHINE_COREBOOT)
> > || defined (GRUB_MACHINE_QEMU)
> > -      params->have_vga = GRUB_VIDEO_LINUX_TYPE_TEXT;
> > -      params->video_mode = 0x3;
> > -#else
> > -      params->have_vga = 0;
> > -      params->video_mode = 0;
> > -      params->video_width = 0;
> > -      params->video_height = 0;
> > -#endif
> > -    }
> > -
> > -  /* Initialize these last, because terminal position could be
> > affected by printfs above.  */ -#ifndef GRUB_MACHINE_IEEE1275
> > -  if (params->have_vga == GRUB_VIDEO_LINUX_TYPE_TEXT) -#endif
> > -    {
> > -      grub_term_output_t term;
> > -      int found = 0;
> > -      FOR_ACTIVE_TERM_OUTPUTS(term)
> > -   if (grub_strcmp (term->name, "vga_text") == 0
> > -       || grub_strcmp (term->name, "console") == 0
> > -       || grub_strcmp (term->name, "ofconsole") == 0)
> > -     {
> > -       grub_uint16_t pos = grub_term_getxy (term);
> > -       params->video_cursor_x = pos >> 8;
> > -       params->video_cursor_y = pos & 0xff;
> > -       params->video_width = grub_term_width (term);
> > -       params->video_height = grub_term_height (term);
> > -       found = 1;
> > -       break;
> > -     }
> > -      if (!found)
> > -   {
> > -     params->video_cursor_x = 0;
> > -     params->video_cursor_y = 0;
> > -     params->video_width = 80;
> > -     params->video_height = 25;
> > -   }
> > -    }
> > -
> > -#ifdef GRUB_MACHINE_IEEE1275
> > -  {
> > -    params->ofw_signature = GRUB_LINUX_OFW_SIGNATURE;
> > -    params->ofw_num_items = 1;
> > -    params->ofw_cif_handler = (grub_uint32_t) grub_ieee1275_entry_fn;
> > -    params->ofw_idt = 0;
> > -  }
> > -#endif
> > -
> >  #ifdef GRUB_MACHINE_EFI
> >    {
> >      grub_efi_uintn_t efi_desc_size;
> >
> >
> 
> Grub crashed with a GPF (13) when I tried to boot with that patch.
> 

I think that patch failed because the video setup is using params before it 
gets set... params doesn't get set until we get real_mode_mem, which includes 
the efi memory map.  (Which makes me wonder about that first "if" statement in 
grub_linux_boot(), which also dereferences params before it is set, but that's 
not relevant to this issue.)

However, the patch below works... at least, on this system.  It just makes the 
EFI memory map buffer 3 pages (12288 bytes) larger than it needs to be, instead 
of 1 page (4096 bytes) larger.  (It also makes the function neater... I wasn't 
sure why efi_find_mmap_size() was iterating with a real buffer, instead of just 
passing a NULL pointer to simply get the size of the required buffer...)

I'm attaching the patch as a file, too, in case this gets line wrapped.  :(


--- ../../grub/grub-core/loader/i386/linux.c    2012-05-31 12:59:19.000000000 
-0400
+++ grub-core/loader/i386/linux.c       2012-06-12 19:13:16.053987681 -0400
@@ -108,38 +108,15 @@ static grub_efi_uintn_t
 find_efi_mmap_size (void)
 {
   static grub_efi_uintn_t mmap_size = 0;
+  void *mmap_buf = 0;
+  grub_efi_uintn_t desc_size;
 
-  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_error (GRUB_ERR_IO, "cannot get memory map");
-         return 0;
-       }
-      else if (ret > 0)
-       break;
-
-      mmap_size += (1 << 12);
-    }
+  if (grub_efi_get_memory_map (&mmap_size, mmap_buf, 0, &desc_size, 0) < 0)
+    return grub_error (GRUB_ERR_IO, "couldn't retrieve memory map");
 
   /* 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;

find_efi_mmap_size_tweak_12jun.patch
Description: find_efi_mmap_size_tweak_12jun.patch