[RFC] Multiboot ammendment: non-VBE video

[RFC] Multiboot ammendment: non-VBE video

[Vladimir 'phcoder' Serbinenko]

Hello. I'm implementing video part of multiboot specification.
Currently the only defined interface is for providing VBE info. I
propose following way to set fields if video is non VBE:
vbe_control_info=0xffffffff
When vbe_control_info is set to 0xffffffff all VBE-specific fields are invalid
vbe_mode set to 0xffff
vbe_interface_seg=0xffff
vbe_interface_off=0xffff
vbe_interface_len=0xff
vbe_mode_info points to structure similar to vbe_mode_info but with
all vbe-specific fields set to zero. Remaining (valid) fields are
(full structur is in include/grub/i386/pc/vbe.h)

  grub_uint16_t bytes_per_scan_line;
  grub_uint16_t x_resolution;
  grub_uint16_t y_resolution;
  grub_uint8_t bits_per_pixel;
  grub_uint8_t memory_model;

  grub_uint8_t red_mask_size;
  grub_uint8_t red_field_position;
  grub_uint8_t green_mask_size;
  grub_uint8_t green_field_position;
  grub_uint8_t blue_mask_size;
  grub_uint8_t blue_field_position;
  grub_uint8_t rsvd_mask_size;
  grub_uint8_t rsvd_field_position;
  grub_uint8_t direct_color_mode_info;

  grub_vbe_physptr_t phys_base_addr;

  grub_uint16_t lin_bytes_per_scan_line;
  grub_uint8_t lin_red_mask_size;
  grub_uint8_t lin_red_field_position;
  grub_uint8_t lin_green_mask_size;
  grub_uint8_t lin_green_field_position;
  grub_uint8_t lin_blue_mask_size;
  grub_uint8_t lin_blue_field_position;
  grub_uint8_t lin_rsvd_mask_size;
  grub_uint8_t lin_rsvd_field_position;


-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [RFC] Multiboot ammendment: non-VBE video

[Robert Millan]

On Tue, Sep 01, 2009 at 05:37:11PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> Hello. I'm implementing video part of multiboot specification.
> Currently the only defined interface is for providing VBE info. I
> propose following way to set fields if video is non VBE:
> vbe_control_info=0xffffffff
> When vbe_control_info is set to 0xffffffff all VBE-specific fields are invalid
> vbe_mode set to 0xffff
> vbe_interface_seg=0xffff
> vbe_interface_off=0xffff
> vbe_interface_len=0xff
> vbe_mode_info points to structure similar to vbe_mode_info but with
> all vbe-specific fields set to zero. Remaining (valid) fields are
> (full structur is in include/grub/i386/pc/vbe.h)

This seems like one of these cases in which legacy gets in the way.  Why
don't we just replace the legacy structure with something that doesn't need
dummy fields?

-- 
Robert Millan

  "Be the change you want to see in the world" -- Gandhi

Re: [RFC] Multiboot ammendment: non-VBE video

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

Robert Millan wrote:
> On Tue, Sep 01, 2009 at 05:37:11PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>   
>> Hello. I'm implementing video part of multiboot specification.
>> Currently the only defined interface is for providing VBE info. I
>> propose following way to set fields if video is non VBE:
>> vbe_control_info=0xffffffff
>> When vbe_control_info is set to 0xffffffff all VBE-specific fields are invalid
>> vbe_mode set to 0xffff
>> vbe_interface_seg=0xffff
>> vbe_interface_off=0xffff
>> vbe_interface_len=0xff
>> vbe_mode_info points to structure similar to vbe_mode_info but with
>> all vbe-specific fields set to zero. Remaining (valid) fields are
>> (full structur is in include/grub/i386/pc/vbe.h)
>>     
>
> This seems like one of these cases in which legacy gets in the way.  Why
> don't we just replace the legacy structure with something that doesn't need
> dummy fields?
>   
Actually it seems like we already define possible dummy fields if
vbe_interface isn't available. vbe_mode_info although contains some vbe
callback info and framebuffer information info. We need only second. My
idea was that payloads that only use info for framebuffer may be readily
working on non-VBE. Alternatively we may define a structure which
contains only framebuffer info. This would avoid the requirement to set
vbe display line to 0 before calling the payload. The draft I have
locally is the following:
=== modified file 'docs/multiboot.texi'
--- docs/multiboot.texi    2008-09-03 20:15:15 +0000
+++ docs/multiboot.texi    2009-12-17 20:04:47 +0000
@@ -479,7 +479,8 @@
 preferred graphics mode. Note that that is only a @emph{recommended}
 mode by the OS image. If the mode exists, the boot loader should set
 it, when the user doesn't specify a mode explicitly. Otherwise, the
-boot loader should fall back to a similar mode, if available.
+boot loader should fall back to a similar mode, if available. Boot loader
+may also choose another mode if it sees fit.
 
 The meaning of each is as follows:
 
@@ -488,7 +489,9 @@
 Contains @samp{0} for linear graphics mode or @samp{1} for
 EGA-standard text mode. Everything else is reserved for future
 expansion. Note that the boot loader may set a text mode, even if this
-field contains @samp{0}.
+field contains @samp{0}. If video adapter doesn't support EGA text mode or
+bootloader doesn't know how to initialise this mode it may set video
+mode even if field contains @samp{1}
 
 @item width
 Contains the number of the columns. This is specified in pixels in a
@@ -917,16 +920,62 @@
 Management (APM) BIOS Interface Specification}, for more information.
 
 If bit 11 in the @samp{flags} is set, the graphics table is available.
-This must only be done if the kernel has indicated in the
-@samp{Multiboot Header} that it accepts a graphics mode.
 
 The fields @samp{vbe_control_info} and @samp{vbe_mode_info} contain
 the physical addresses of @sc{vbe} control information returned by the
 @sc{vbe} Function 00h and @sc{vbe} mode information returned by the
-@sc{vbe} Function 01h, respectively.
+@sc{vbe} Function 01h, respectively. In case of non-@sc{vbe} video driver
+@samp{vbe_control_info} contains zero and @samp{vbe_mode_info} points
to the
+following 256-byte structure:
+
+@example
+@group
+        +----------------------+
+0       | flags                |
+2       | zeros                |
+16      | pitch                |
+18      | width                |
+20      | height               |
+22      | zeros                |
+25      | bits per pixel       |
+26      | zeros                |
+27      | memory model         |
+28      | zeros                |
+31      | red mask size        |
+32      | red field position   |
+33      | green mask size      |
+34      | green field position |
+35      | blue mask size       |
+36      | blue field position  |
+37      | alpha mask size      |
+38      | alpha field position |
+39      | zero                 |
+40      | framebuffer address  |
+44      | zero                 |
+50      | pitch                |
+52      | zero                 |
+54      | red mask size        |
+55      | red field position   |
+56      | green mask size      |
+57      | green field position |
+58      | blue mask size       |
+59      | blue field position  |
+60      | alpha mask size      |
+61      | alpha field position |
+62      | zeros                |
+        +----------------------+
+@end group
+@end example
+
+All fields have the same meaning as the corresponding VBE mode info
+structure except fields marked as zeros which are zero-filled.
+
+If video is a linear framebuffer boot loader has to ensure that
+displayed region starts at offset @samp{0} of linear framebuffer.
 
 The field @samp{vbe_mode} indicates current video mode in the format
-specified in @sc{vbe} 3.0.
+specified in @sc{vbe} 3.0. In case of non-@sc{vbe} video driver
+@samp{vbe_mode} contains 0xffff.
 
 The rest fields @samp{vbe_interface_seg}, @samp{vbe_interface_off}, and
 @samp{vbe_interface_len} contain the table of a protected mode interface




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



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

Re: [RFC] Multiboot ammendment: non-VBE video

[Robert Millan]

On Mon, Dec 28, 2009 at 01:07:10PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> Robert Millan wrote:
> > On Tue, Sep 01, 2009 at 05:37:11PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> >   
> >> Hello. I'm implementing video part of multiboot specification.
> >> Currently the only defined interface is for providing VBE info. I
> >> propose following way to set fields if video is non VBE:
> >> vbe_control_info=0xffffffff
> >> When vbe_control_info is set to 0xffffffff all VBE-specific fields are invalid
> >> vbe_mode set to 0xffff
> >> vbe_interface_seg=0xffff
> >> vbe_interface_off=0xffff
> >> vbe_interface_len=0xff
> >> vbe_mode_info points to structure similar to vbe_mode_info but with
> >> all vbe-specific fields set to zero. Remaining (valid) fields are
> >> (full structur is in include/grub/i386/pc/vbe.h)
> >>     
> >
> > This seems like one of these cases in which legacy gets in the way.  Why
> > don't we just replace the legacy structure with something that doesn't need
> > dummy fields?
> >   
> Actually it seems like we already define possible dummy fields if
> vbe_interface isn't available.

Yes.  That's the case for vbe_interface_* fields.  But extending this to
vbe_control_info and vbe_mode_info is backward-incompatible.

I don't object to backward-incompatible changes, but I would expect that
when we make one, we get the benefit of a clean, legacy-free interface.  In
this case we carry on with legacy baggage to archieve half-way backward
compatibility.

In purely practical terms, all of this only has a minimal effect.  GRUB Legacy
didn't implement these extensions, and GRUB 2 hasn't yet, so use of this
feature in loadees isn't widespread.  But it has a major impact when it comes
to reputation.  If we make incompatible changes in the text, we should be
honest about it.  I don't want people to think they can't rely on Multiboot
because its maintainers sometimes stretch the definitions in ways not
permitted by the text.

Since it's obvious we want to change something (rather than implement
VBE-specific extensions), I propose we start with:

-If bit 11 in the @samp{flags} is set, the graphics table is available.
+If bit 12 in the @samp{flags} is set, the graphics table is available.

at this point, we can change anything we like.

-- 
Robert Millan

  "Be the change you want to see in the world" -- Gandhi

Re: [RFC] Multiboot ammendment: non-VBE video

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

Robert Millan wrote:
> On Mon, Dec 28, 2009 at 01:07:10PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>   
>> Robert Millan wrote:
>>     
>>> On Tue, Sep 01, 2009 at 05:37:11PM +0200, Vladimir 'phcoder' Serbinenko wrote:
>>>   
>>>       
>>>> Hello. I'm implementing video part of multiboot specification.
>>>> Currently the only defined interface is for providing VBE info. I
>>>> propose following way to set fields if video is non VBE:
>>>> vbe_control_info=0xffffffff
>>>> When vbe_control_info is set to 0xffffffff all VBE-specific fields are invalid
>>>> vbe_mode set to 0xffff
>>>> vbe_interface_seg=0xffff
>>>> vbe_interface_off=0xffff
>>>> vbe_interface_len=0xff
>>>> vbe_mode_info points to structure similar to vbe_mode_info but with
>>>> all vbe-specific fields set to zero. Remaining (valid) fields are
>>>> (full structur is in include/grub/i386/pc/vbe.h)
>>>>     
>>>>         
>>> This seems like one of these cases in which legacy gets in the way.  Why
>>> don't we just replace the legacy structure with something that doesn't need
>>> dummy fields?
>>>   
>>>       
>> Actually it seems like we already define possible dummy fields if
>> vbe_interface isn't available.
>>     
>
> Yes.  That's the case for vbe_interface_* fields.  But extending this to
> vbe_control_info and vbe_mode_info is backward-incompatible.
>
> I don't object to backward-incompatible changes, but I would expect that
> when we make one, we get the benefit of a clean, legacy-free interface.  In
> this case we carry on with legacy baggage to archieve half-way backward
> compatibility.
>
> In purely practical terms, all of this only has a minimal effect.  GRUB Legacy
> didn't implement these extensions, and GRUB 2 hasn't yet, so use of this
> feature in loadees isn't widespread.  But it has a major impact when it comes
> to reputation.  If we make incompatible changes in the text, we should be
> honest about it.  I don't want people to think they can't rely on Multiboot
> because its maintainers sometimes stretch the definitions in ways not
> permitted by the text.
>
> Since it's obvious we want to change something (rather than implement
> VBE-specific extensions), I propose we start with:
>
> -If bit 11 in the @samp{flags} is set, the graphics table is available.
> +If bit 12 in the @samp{flags} is set, the graphics table is available.
>
> at this point, we can change anything we like.
>
>   
I followed your suggestions. I attach new draft together with drawing of
a blue diagonal line in example kernel (it would be way better to make
it use it for output but it's intended to just show how to use passed
info). I also attach an implementation of multiboot on EFI which uses
this draft. Note that this ammendment is nothing EFI-specific and can be
used with any linear framebuffer driver. The only reason it's in the
same patch is because it was the branch in which I was working on
multiboot improvements and haven't yet splitted individual changes yet

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


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

=== modified file 'doc/boot.S'
--- doc/boot.S	2009-12-24 13:34:49 +0000
+++ doc/boot.S	2010-01-02 17:13:52 +0000
@@ -30,9 +30,9 @@
 
 /* The flags for the Multiboot header.  */
 #ifdef __ELF__
-# define MULTIBOOT_HEADER_FLAGS		0x00000003
+# define MULTIBOOT_HEADER_FLAGS		0x00000007
 #else
-# define MULTIBOOT_HEADER_FLAGS		0x00010003
+# define MULTIBOOT_HEADER_FLAGS		0x00010007
 #endif
 	
 	.text
@@ -64,7 +64,17 @@
 	.long	_end
 	/* entry_addr */
 	.long	multiboot_entry
-#endif /* ! __ELF__ */
+#else /* ! __ELF__ */
+	.long	0
+	.long	0
+	.long	0
+	.long	0
+	.long	0	
+#endif /* __ELF__ */
+	.long 0
+	.long 1024
+	.long 768
+	.long 32
 
 multiboot_entry:
 	/* Initialize the stack pointer.  */

=== modified file 'doc/kernel.c'
--- doc/kernel.c	2009-12-24 14:25:44 +0000
+++ doc/kernel.c	2010-01-02 17:04:15 +0000
@@ -150,6 +150,87 @@
 		mmap->len & 0xffffffff,
 		(unsigned) mmap->type);
     }
+
+  /* Draw diagonal blue line.  */
+  if (CHECK_FLAG (mbi->flags, 12))
+    {
+      multiboot_uint32_t color;
+      unsigned i;
+      void *fb = (void *) (unsigned long) mbi->framebuffer_addr;
+
+      switch (mbi->framebuffer_type)
+	{
+	case MULTIBOOT_FRAMEBUFFER_TYPE_INDEXED:
+	  {
+	    unsigned best_distance, distance;
+	    struct multiboot_color *palette;
+	    
+	    palette = (struct multiboot_color *) mbi->framebuffer_palette_addr;
+
+	    color = 0;
+	    best_distance = 4*256*256;
+	    
+	    for (i = 0; i < mbi->framebuffer_palette_num_colors; i++)
+	      {
+		distance = (0xff - palette[i].blue) * (0xff - palette[i].blue)
+		  + palette[i].red * palette[i].red
+		  + palette[i].green * palette[i].green;
+		if (distance < best_distance)
+		  {
+		    color = i;
+		    best_distance = distance;
+		  }
+	      }
+	  }
+	  break;
+
+	case MULTIBOOT_FRAMEBUFFER_TYPE_RGB:
+	  color = ((1 << mbi->framebuffer_blue_mask_size) - 1) 
+	    << mbi->framebuffer_blue_field_position;
+	  break;
+
+	default:
+	  color = 0xffffffff;
+	  break;
+	}
+      for (i = 0; i < mbi->framebuffer_width
+	     && i < mbi->framebuffer_height; i++)
+	{
+	  switch (mbi->framebuffer_bpp)
+	    {
+	    case 8:
+	      {
+		multiboot_uint8_t *pixel = fb + mbi->framebuffer_pitch * i + i;
+		*pixel = color;
+	      }
+	      break;
+	    case 15:
+	    case 16:
+	      {
+		multiboot_uint16_t *pixel
+		  = fb + mbi->framebuffer_pitch * i + 2 * i;
+		*pixel = color;
+	      }
+	      break;
+	    case 24:
+	      {
+		multiboot_uint32_t *pixel
+		  = fb + mbi->framebuffer_pitch * i + 3 * i;
+		*pixel = (color & 0xffffff) | (*pixel & 0xff000000);
+	      }
+	      break;
+
+	    case 32:
+	      {
+		multiboot_uint32_t *pixel
+		  = fb + mbi->framebuffer_pitch * i + 4 * i;
+		*pixel = color;
+	      }
+	      break;
+	    }
+	}
+    }
+
 }    
 
 /* Clear the screen and initialize VIDEO, XPOS and YPOS.  */

=== modified file 'doc/multiboot.h'
--- doc/multiboot.h	2009-12-24 14:25:44 +0000
+++ doc/multiboot.h	2010-01-02 12:58:41 +0000
@@ -32,7 +32,7 @@
 #define MULTIBOOT_BOOTLOADER_MAGIC		0x2BADB002
 
 /* The bits in the required part of flags field we don't support.  */
-#define MULTIBOOT_UNSUPPORTED			0x0000fffc
+#define MULTIBOOT_UNSUPPORTED                   0x0000fff8
 
 /* Alignment of multiboot modules.  */
 #define MULTIBOOT_MOD_ALIGN			0x00001000
@@ -88,10 +88,12 @@
 #define MULTIBOOT_INFO_APM_TABLE		0x00000400
 
 /* Is there video information?  */
-#define MULTIBOOT_INFO_VIDEO_INFO		0x00000800
+#define MULTIBOOT_INFO_VBE_INFO		        0x00000800
+#define MULTIBOOT_INFO_FRAMEBUFFER_INFO	        0x00001000
 
 #ifndef ASM_FILE
 
+typedef unsigned char		multiboot_uint8_t;
 typedef unsigned short		multiboot_uint16_t;
 typedef unsigned int		multiboot_uint32_t;
 typedef unsigned long long	multiboot_uint64_t;
@@ -190,9 +192,43 @@
   multiboot_uint16_t vbe_interface_seg;
   multiboot_uint16_t vbe_interface_off;
   multiboot_uint16_t vbe_interface_len;
+
+  multiboot_uint64_t framebuffer_addr;
+  multiboot_uint32_t framebuffer_pitch;
+  multiboot_uint32_t framebuffer_width;
+  multiboot_uint32_t framebuffer_height;
+  multiboot_uint8_t framebuffer_bpp;
+  multiboot_uint8_t framebuffer_type;
+  union
+  {
+    struct
+    {
+      multiboot_uint32_t framebuffer_palette_addr;
+      multiboot_uint16_t framebuffer_palette_num_colors;
+    };
+    struct
+    {
+      multiboot_uint8_t framebuffer_red_field_position;
+      multiboot_uint8_t framebuffer_red_mask_size;
+      multiboot_uint8_t framebuffer_green_field_position;
+      multiboot_uint8_t framebuffer_green_mask_size;
+      multiboot_uint8_t framebuffer_blue_field_position;
+      multiboot_uint8_t framebuffer_blue_mask_size;
+    };
+  };
+};
+
+struct multiboot_color
+{
+  multiboot_uint8_t red;
+  multiboot_uint8_t green;
+  multiboot_uint8_t blue;
 };
 typedef struct multiboot_info multiboot_info_t;
 
+#define MULTIBOOT_FRAMEBUFFER_TYPE_INDEXED 0
+#define MULTIBOOT_FRAMEBUFFER_TYPE_RGB     1
+
 struct multiboot_mmap_entry
 {
   multiboot_uint32_t size;

=== modified file 'doc/multiboot.texi'
--- doc/multiboot.texi	2010-01-01 20:02:24 +0000
+++ doc/multiboot.texi	2010-01-02 16:58:33 +0000
@@ -479,7 +479,8 @@
 preferred graphics mode. Note that that is only a @emph{recommended}
 mode by the OS image. If the mode exists, the boot loader should set
 it, when the user doesn't specify a mode explicitly. Otherwise, the
-boot loader should fall back to a similar mode, if available.
+boot loader should fall back to a similar mode, if available. Boot loader
+may also choose another mode if it sees fit.
 
 The meaning of each is as follows:
 
@@ -488,7 +489,9 @@
 Contains @samp{0} for linear graphics mode or @samp{1} for
 EGA-standard text mode. Everything else is reserved for future
 expansion. Note that the boot loader may set a text mode, even if this
-field contains @samp{0}.
+field contains @samp{0}. If video adapter doesn't support EGA text mode or
+bootloader doesn't know how to initialise this mode it may set video
+mode even if field contains @samp{1}
 
 @item width
 Contains the number of the columns. This is specified in pixels in a
@@ -635,6 +638,15 @@
 84      | vbe_interface_off |
 86      | vbe_interface_len |
         +-------------------+
+88      | framebuffer_addr  |    (present if flags[12] is set)
+96      | framebuffer_pitch |
+100     | framebuffer_width |
+104     | framebuffer_height|
+108     | framebuffer_bpp   |
+109     | framebuffer_type  |
+110-115 | color_info        |
+        +-------------------+
+
 @end group
 @end example
 
@@ -911,9 +923,7 @@
 @uref{http://www.microsoft.com/hwdev/busbios/amp_12.htm, Advanced Power
 Management (APM) BIOS Interface Specification}, for more information.
 
-If bit 11 in the @samp{flags} is set, the graphics table is available.
-This must only be done if the kernel has indicated in the
-@samp{Multiboot Header} that it accepts a graphics mode.
+If bit 11 in the @samp{flags} is set, the @sc{vbe} table is available.
 
 The fields @samp{vbe_control_info} and @samp{vbe_mode_info} contain
 the physical addresses of @sc{vbe} control information returned by the
@@ -935,6 +945,42 @@
 Multiboot boot loaders may simulate @sc{vbe} on non-@sc{vbe} modes, as
 if they were @sc{vbe} modes.
 
+If bit 12 in the @samp{flags} is set, the @sc{Framebuffer} table is available.
+
+The field @samp{framebuffer_addr} contains framebuffer physical address. This field is 64-bit wide but bootloader @dfn{should} set it under 4GiB if possible for compatibility with payloads which aren't aware of PAE or amd64. The field @samp{framebuffer_pitch} contains pitch in bytes. The fields @samp{framebuffer_width}, @samp{framebuffer_height} contain framebuffer dimensions in pixels. The field @samp{framebuffer_bpp} contains number of bits per pixel. If @samp{framebuffer_type} is set to 0 it means indexed color. In this case color_info is defined as following:
+@example
+@group
+        +----------------------------------+
+110     | framebuffer_palette_addr         |
+114     | framebuffer_palette_num_colors   |
+        +----------------------------------+
+@end group
+@end example
+@samp{framebuffer_palette_addr} contains address of array of @samp{framebuffer_palette_num_colors} following structures:
+@example
+@group
+        +-------------+
+0       | red_value   |
+1       | green_value |
+2       | blue_value  |
+        +-------------+
+@end group
+@end example
+If @samp{framebuffer_type} is set to 1 it direct RGB color. Then color_type is defined as following:
+
+@example
+@group
+        +----------------------------------+
+110     | framebuffer_red_field_position   |
+111     | framebuffer_red_mask_size        |
+112     | framebuffer_green_field_position |
+113     | framebuffer_green_mask_size      |
+114     | framebuffer_blue_field_position  |
+115     | framebuffer_blue_mask_size       |
+        +----------------------------------+
+@end group
+@end example
+All further values of @samp{framebuffer_type} are reserved for future expansion
 
 @node Examples
 @chapter Examples


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

=== added file 'ChangeLog.efiboot'
--- ChangeLog.efiboot	1970-01-01 00:00:00 +0000
+++ ChangeLog.efiboot	2010-01-02 01:48:33 +0000
@@ -0,0 +1,19 @@
+2009-11-28  Vladimir Serbinenko  <phcoder@gmail.com>
+
+	Multiboot support on i386-efi.
+
+	* conf/i386-efi.rmk (pkglib_MODULES): Add multiboot.mod.
+	(multiboot_mod_SOURCES): New variable.
+	(multiboot_mod_CFLAGS): Likewise.
+	(multiboot_mod_LDFLAGS): Likewise.
+	(multiboot_mod_ASFLAGS): Likewise.
+	* include/grub/i386/multiboot.h (grub_multiboot_real_boot): Explicitly
+	declare as regparm (3).
+	(grub_multiboot2_real_boot): Likewise.
+	(grub_multiboot_boot): Finish boot services.
+	(grub_fill_multiboot_mmap): Explicitly parse memory map types.
+	(grub_multiboot): Declare as noreturn to avoid finishing console.
+	* loader/multiboot_loader.c (grub_cmd_multiboot_loader): Enable
+	multiboot1 on i386-efi and disable multiboot2 on this platform.
+	(grub_cmd_module_loader): Likewise.
+

=== modified file 'conf/i386-coreboot.rmk'
--- conf/i386-coreboot.rmk	2009-12-25 22:06:52 +0000
+++ conf/i386-coreboot.rmk	2010-01-02 01:48:37 +0000
@@ -142,6 +142,7 @@
 
 # For multiboot.mod.
 multiboot_mod_SOURCES = loader/i386/multiboot.c \
+			loader/i386/multiboot_mbi.c \
 			loader/i386/multiboot_helper.S \
                          loader/i386/pc/multiboot2.c \
                          loader/multiboot2.c \

=== modified file 'conf/i386-efi.rmk'
--- conf/i386-efi.rmk	2009-12-25 22:06:52 +0000
+++ conf/i386-efi.rmk	2010-01-02 01:48:37 +0000
@@ -33,7 +33,15 @@
 pkglib_MODULES = kernel.img chain.mod appleldr.mod \
 	linux.mod halt.mod reboot.mod pci.mod lspci.mod \
 	datetime.mod date.mod datehook.mod loadbios.mod \
-	fixvideo.mod mmap.mod acpi.mod
+	fixvideo.mod mmap.mod acpi.mod multiboot.mod
+
+# For multiboot.mod.
+multiboot_mod_SOURCES = loader/i386/multiboot.c \
+			loader/i386/multiboot_mbi.c \
+                        loader/multiboot_loader.c
+multiboot_mod_CFLAGS = $(COMMON_CFLAGS)
+multiboot_mod_LDFLAGS = $(COMMON_LDFLAGS)
+multiboot_mod_ASFLAGS = $(COMMON_ASFLAGS)
 
 # For kernel.img.
 kernel_img_EXPORTS = no

=== modified file 'conf/i386-pc.rmk'
--- conf/i386-pc.rmk	2009-12-25 22:06:52 +0000
+++ conf/i386-pc.rmk	2010-01-02 01:48:37 +0000
@@ -206,6 +206,7 @@
 
 # For multiboot.mod.
 multiboot_mod_SOURCES = loader/i386/multiboot.c \
+			loader/i386/multiboot_mbi.c \
 			loader/i386/multiboot_helper.S \
                         loader/i386/pc/multiboot2.c \
                         loader/multiboot2.c \

=== modified file 'conf/x86_64-efi.rmk'
--- conf/x86_64-efi.rmk	2009-12-25 22:06:52 +0000
+++ conf/x86_64-efi.rmk	2010-01-02 01:48:37 +0000
@@ -172,4 +172,13 @@
 relocator_mod_ASFLAGS = $(COMMON_ASFLAGS)
 relocator_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
+# For multiboot.mod.
+pkglib_MODULES += multiboot.mod
+multiboot_mod_SOURCES = loader/i386/multiboot.c \
+                        loader/multiboot_loader.c \
+			loader/i386/multiboot_mbi.c
+multiboot_mod_CFLAGS = $(COMMON_CFLAGS)
+multiboot_mod_LDFLAGS = $(COMMON_LDFLAGS)
+multiboot_mod_ASFLAGS = $(COMMON_ASFLAGS)
+
 include $(srcdir)/conf/common.mk

=== modified file 'include/grub/i386/multiboot.h'
--- include/grub/i386/multiboot.h	2009-12-13 18:37:44 +0000
+++ include/grub/i386/multiboot.h	2010-01-02 01:48:33 +0000
@@ -20,16 +20,23 @@
 #define GRUB_MULTIBOOT_CPU_HEADER	1
 
 /* The asm part of the multiboot loader.  */
-void grub_multiboot_real_boot (grub_addr_t entry,
-			       struct multiboot_info *mbi)
-     __attribute__ ((noreturn));
 void grub_multiboot2_real_boot (grub_addr_t entry,
 				struct multiboot_info *mbi)
-     __attribute__ ((noreturn));
+     __attribute__ ((noreturn,regparm (3)));
 
 extern grub_uint32_t grub_multiboot_payload_eip;
 extern char *grub_multiboot_payload_orig;
 extern grub_addr_t grub_multiboot_payload_dest;
-extern grub_size_t grub_multiboot_payload_size;
+extern grub_size_t grub_multiboot_pure_size;
+
+grub_size_t grub_multiboot_get_mbi_size (void);
+grub_err_t grub_multiboot_make_mbi (void *orig, grub_uint32_t dest,
+				    grub_off_t buf_off, grub_size_t bufsize);
+void grub_multiboot_free_mbi (void);
+grub_err_t grub_multiboot_init_mbi (int argc, char *argv[]);
+grub_err_t grub_multiboot_add_module (grub_off_t start, grub_size_t size,
+				      int argc, char *argv[]);
+void grub_multiboot_set_bootdev (void);
+void grub_multiboot_set_accepts_video (int val);
 
 #endif /* ! GRUB_MULTIBOOT_CPU_HEADER */

=== modified file 'include/grub/video.h'
--- include/grub/video.h	2009-08-14 12:41:58 +0000
+++ include/grub/video.h	2010-01-02 01:48:33 +0000
@@ -234,6 +234,8 @@
 };
 typedef struct grub_video_adapter *grub_video_adapter_t;
 
+const char *grub_video_get_active_adapter_name (void);
+
 void grub_video_register (grub_video_adapter_t adapter);
 void grub_video_unregister (grub_video_adapter_t adapter);
 void grub_video_iterate (int (*hook) (grub_video_adapter_t adapter));

=== added file 'include/grub/x86_64/multiboot.h'
--- include/grub/x86_64/multiboot.h	1970-01-01 00:00:00 +0000
+++ include/grub/x86_64/multiboot.h	2010-01-02 01:48:33 +0000
@@ -0,0 +1,1 @@
+#include <grub/i386/multiboot.h>

=== modified file 'include/multiboot.h'
--- include/multiboot.h	2009-12-24 14:19:22 +0000
+++ include/multiboot.h	2010-01-02 12:38:41 +0000
@@ -32,7 +32,7 @@
 #define MULTIBOOT_BOOTLOADER_MAGIC		0x2BADB002
 
 /* The bits in the required part of flags field we don't support.  */
-#define MULTIBOOT_UNSUPPORTED			0x0000fffc
+#define MULTIBOOT_UNSUPPORTED                   0x0000fff8
 
 /* Alignment of multiboot modules.  */
 #define MULTIBOOT_MOD_ALIGN			0x00001000
@@ -88,10 +88,12 @@
 #define MULTIBOOT_INFO_APM_TABLE		0x00000400
 
 /* Is there video information?  */
-#define MULTIBOOT_INFO_VIDEO_INFO		0x00000800
+#define MULTIBOOT_INFO_VBE_INFO		        0x00000800
+#define MULTIBOOT_INFO_FRAMEBUFFER_INFO	        0x00001000
 
 #ifndef ASM_FILE
 
+typedef unsigned char		multiboot_uint8_t;
 typedef unsigned short		multiboot_uint16_t;
 typedef unsigned int		multiboot_uint32_t;
 typedef unsigned long long	multiboot_uint64_t;
@@ -190,9 +192,43 @@
   multiboot_uint16_t vbe_interface_seg;
   multiboot_uint16_t vbe_interface_off;
   multiboot_uint16_t vbe_interface_len;
+
+  multiboot_uint64_t framebuffer_addr;
+  multiboot_uint32_t framebuffer_pitch;
+  multiboot_uint32_t framebuffer_width;
+  multiboot_uint32_t framebuffer_height;
+  multiboot_uint8_t framebuffer_bpp;
+  multiboot_uint8_t framebuffer_type;
+  union
+  {
+    struct
+    {
+      multiboot_uint32_t framebuffer_palette_addr;
+      multiboot_uint16_t framebuffer_palette_num_colors;
+    };
+    struct
+    {
+      multiboot_uint8_t framebuffer_red_field_position;
+      multiboot_uint8_t framebuffer_red_mask_size;
+      multiboot_uint8_t framebuffer_green_field_position;
+      multiboot_uint8_t framebuffer_green_mask_size;
+      multiboot_uint8_t framebuffer_blue_field_position;
+      multiboot_uint8_t framebuffer_blue_mask_size;
+    };
+  };
+};
+
+struct multiboot_color
+{
+  multiboot_uint8_t red;
+  multiboot_uint8_t green;
+  multiboot_uint8_t blue;
 };
 typedef struct multiboot_info multiboot_info_t;
 
+#define MULTIBOOT_FRAMEBUFFER_TYPE_INDEXED 0
+#define MULTIBOOT_FRAMEBUFFER_TYPE_RGB     1
+
 struct multiboot_mmap_entry
 {
   multiboot_uint32_t size;

=== modified file 'loader/i386/multiboot.c'
--- loader/i386/multiboot.c	2009-12-25 11:43:20 +0000
+++ loader/i386/multiboot.c	2010-01-02 17:12:40 +0000
@@ -20,7 +20,6 @@
 /*
  *  FIXME: The following features from the Multiboot specification still
  *         need to be implemented:
- *  - VBE support
  *  - symbol table
  *  - drives table
  *  - ROM configuration table
@@ -30,8 +29,6 @@
 #include <grub/loader.h>
 #include <grub/machine/loader.h>
 #include <grub/multiboot.h>
-#include <grub/machine/init.h>
-#include <grub/machine/memory.h>
 #include <grub/cpu/multiboot.h>
 #include <grub/elf.h>
 #include <grub/aout.h>
@@ -42,31 +39,30 @@
 #include <grub/misc.h>
 #include <grub/gzio.h>
 #include <grub/env.h>
-#ifdef GRUB_MACHINE_PCBIOS
-#include <grub/machine/biosnum.h>
-#include <grub/disk.h>
-#include <grub/device.h>
-#include <grub/partition.h>
-#endif
 #include <grub/i386/relocator.h>
 
+#include <grub/cpu/multiboot.h>
+
+#ifdef GRUB_MACHINE_EFI
+#include <grub/efi/efi.h>
+#endif
+
 extern grub_dl_t my_mod;
-static struct multiboot_info *mbi, *mbi_dest;
-
-static grub_size_t code_size;
+static grub_size_t code_size, alloc_mbi;
 
 char *grub_multiboot_payload_orig;
 grub_addr_t grub_multiboot_payload_dest;
-grub_size_t grub_multiboot_payload_size;
+grub_size_t grub_multiboot_pure_size;
 grub_uint32_t grub_multiboot_payload_eip;
 
 static grub_err_t
 grub_multiboot_boot (void)
 {
+  grub_size_t mbi_size;
+  grub_err_t err;
   struct grub_relocator32_state state =
     {
       .eax = MULTIBOOT_BOOTLOADER_MAGIC,
-      .ebx = PTR_TO_UINT32 (mbi_dest),
       .ecx = 0,
       .edx = 0,
       .eip = grub_multiboot_payload_eip,
@@ -75,6 +71,29 @@
       .esp = 0x7ff00
     };
 
+  mbi_size = grub_multiboot_get_mbi_size ();
+  if (alloc_mbi < mbi_size)
+    {
+      grub_multiboot_payload_orig
+	= grub_relocator32_realloc (grub_multiboot_payload_orig,
+				    grub_multiboot_pure_size + mbi_size);
+      if (!grub_multiboot_payload_orig)
+	return grub_errno;
+      alloc_mbi = mbi_size;
+    }
+
+  state.ebx = grub_multiboot_payload_dest + grub_multiboot_pure_size;
+  err = grub_multiboot_make_mbi (grub_multiboot_payload_orig,
+				 grub_multiboot_payload_dest,
+				 grub_multiboot_pure_size, mbi_size);
+  if (err)
+    return err;
+
+#ifdef GRUB_MACHINE_EFI
+  if (! grub_efi_finish_boot_services ())
+     grub_fatal ("cannot exit boot services");
+#endif
+
   grub_relocator32_boot (grub_multiboot_payload_orig,
 			 grub_multiboot_payload_dest,
 			 state);
@@ -86,69 +105,18 @@
 static grub_err_t
 grub_multiboot_unload (void)
 {
-  if (mbi)
-    {
-      unsigned int i;
-      for (i = 0; i < mbi->mods_count; i++)
-	{
-	  grub_free ((void *)
-		     ((struct multiboot_mod_list *) mbi->mods_addr)[i].mod_start);
-	  grub_free ((void *)
-		     ((struct multiboot_mod_list *) mbi->mods_addr)[i].cmdline);
-	}
-      grub_free ((void *) mbi->mods_addr);
-    }
+  grub_multiboot_free_mbi ();
+
   grub_relocator32_free (grub_multiboot_payload_orig);
 
-  mbi = NULL;
+  alloc_mbi = 0;
+
   grub_multiboot_payload_orig = NULL;
   grub_dl_unref (my_mod);
 
   return GRUB_ERR_NONE;
 }
 
-/* Return the length of the Multiboot mmap that will be needed to allocate
-   our platform's map.  */
-static grub_uint32_t
-grub_get_multiboot_mmap_len (void)
-{
-  grub_size_t count = 0;
-
-  auto int NESTED_FUNC_ATTR hook (grub_uint64_t, grub_uint64_t, grub_uint32_t);
-  int NESTED_FUNC_ATTR hook (grub_uint64_t addr __attribute__ ((unused)),
-			     grub_uint64_t size __attribute__ ((unused)),
-			     grub_uint32_t type __attribute__ ((unused)))
-    {
-      count++;
-      return 0;
-    }
-
-  grub_mmap_iterate (hook);
-
-  return count * sizeof (struct multiboot_mmap_entry);
-}
-
-/* Fill previously allocated Multiboot mmap.  */
-static void
-grub_fill_multiboot_mmap (struct multiboot_mmap_entry *first_entry)
-{
-  struct multiboot_mmap_entry *mmap_entry = (struct multiboot_mmap_entry *) first_entry;
-
-  auto int NESTED_FUNC_ATTR hook (grub_uint64_t, grub_uint64_t, grub_uint32_t);
-  int NESTED_FUNC_ATTR hook (grub_uint64_t addr, grub_uint64_t size, grub_uint32_t type)
-    {
-      mmap_entry->addr = addr;
-      mmap_entry->len = size;
-      mmap_entry->type = type;
-      mmap_entry->size = sizeof (struct multiboot_mmap_entry) - sizeof (mmap_entry->size);
-      mmap_entry++;
-
-      return 0;
-    }
-
-  grub_mmap_iterate (hook);
-}
-
 #define MULTIBOOT_LOAD_ELF64
 #include "multiboot_elfxx.c"
 #undef MULTIBOOT_LOAD_ELF64
@@ -169,58 +137,13 @@
   return grub_error (GRUB_ERR_UNKNOWN_OS, "unknown ELF class");
 }
 
-static int
-grub_multiboot_get_bootdev (grub_uint32_t *bootdev)
-{
-#ifdef GRUB_MACHINE_PCBIOS
-  char *p;
-  grub_uint32_t biosdev, slice = ~0, part = ~0;
-  grub_device_t dev;
-
-  biosdev = grub_get_root_biosnumber ();
-
-  dev = grub_device_open (0);
-  if (dev && dev->disk && dev->disk->partition)
-    {
-
-      p = dev->disk->partition->partmap->get_name (dev->disk->partition);
-      if (p)
-	{
-	  if ((p[0] >= '0') && (p[0] <= '9'))
-	    {
-	      slice = grub_strtoul (p, &p, 0) - 1;
-
-	      if ((p) && (p[0] == ','))
-		p++;
-	    }
-
-	  if ((p[0] >= 'a') && (p[0] <= 'z'))
-	    part = p[0] - 'a';
-	}
-    }
-  if (dev)
-    grub_device_close (dev);
-
-  *bootdev = ((biosdev & 0xff) << 24) | ((slice & 0xff) << 16)
-    | ((part & 0xff) << 8) | 0xff;
-  return (biosdev != ~0UL);
-#else
-  *bootdev = 0xffffffff;
-  return 0;
-#endif
-}
-
 void
 grub_multiboot (int argc, char *argv[])
 {
   grub_file_t file = 0;
-  char buffer[MULTIBOOT_SEARCH], *cmdline = 0, *p;
+  char buffer[MULTIBOOT_SEARCH];
   struct multiboot_header *header;
-  grub_ssize_t len, cmdline_length, boot_loader_name_length;
-  grub_uint32_t mmap_length;
-  int i;
-  int cmdline_argc;
-  char **cmdline_argv;
+  grub_ssize_t len;
 
   grub_loader_unset ();
 
@@ -271,31 +194,8 @@
   grub_relocator32_free (grub_multiboot_payload_orig);
   grub_multiboot_payload_orig = NULL;
 
-  mmap_length = grub_get_multiboot_mmap_len ();
-
-  /* Figure out cmdline length.  */
   /* Skip filename.  */
-  cmdline_argc = argc - 1;
-  cmdline_argv = argv + 1;
-
-  for (i = 0, cmdline_length = 0; i < cmdline_argc; i++)
-    cmdline_length += grub_strlen (cmdline_argv[i]) + 1;
-
-  if (cmdline_length == 0)
-    cmdline_length = 1;
-
-  boot_loader_name_length = sizeof(PACKAGE_STRING);
-
-#define cmdline_addr(x)		((void *) ((x) + code_size))
-#define boot_loader_name_addr(x) \
-				((void *) ((x) + code_size + cmdline_length))
-#define mbi_addr(x)		((void *) ((x) + code_size + cmdline_length + boot_loader_name_length))
-#define mmap_addr(x)		((void *) ((x) + code_size + cmdline_length + boot_loader_name_length + sizeof (struct multiboot_info)))
-
-  grub_multiboot_payload_size = cmdline_length
-    /* boot_loader_name_length might need to grow for mbi,etc to be aligned (see below) */
-    + boot_loader_name_length + 3
-    + sizeof (struct multiboot_info) + mmap_length;
+  grub_multiboot_init_mbi (argc - 1, argv + 1);
 
   if (header->flags & MULTIBOOT_AOUT_KLUDGE)
     {
@@ -310,10 +210,11 @@
 	code_size = load_size;
       grub_multiboot_payload_dest = header->load_addr;
 
-      grub_multiboot_payload_size += code_size;
+      grub_multiboot_pure_size += code_size;
 
+      alloc_mbi = grub_multiboot_get_mbi_size ();
       grub_multiboot_payload_orig
-	= grub_relocator32_alloc (grub_multiboot_payload_size);
+	= grub_relocator32_alloc (grub_multiboot_pure_size + alloc_mbi);
 
       if (! grub_multiboot_payload_orig)
 	goto fail;
@@ -335,53 +236,37 @@
   else if (grub_multiboot_load_elf (file, buffer) != GRUB_ERR_NONE)
     goto fail;
 
-  /* This provides alignment for the MBI, the memory map and the backward relocator.  */
-  boot_loader_name_length += (0x04 - ((unsigned long) mbi_addr (grub_multiboot_payload_dest) & 0x03));
-
-  mbi = mbi_addr (grub_multiboot_payload_orig);
-  mbi_dest = mbi_addr (grub_multiboot_payload_dest);
-  grub_memset (mbi, 0, sizeof (struct multiboot_info));
-  mbi->mmap_length = mmap_length;
-
-  grub_fill_multiboot_mmap (mmap_addr (grub_multiboot_payload_orig));
-
-  /* FIXME: grub_uint32_t will break for addresses above 4 GiB, but is mandated
-     by the spec.  Is there something we can do about it?  */
-  mbi->mmap_addr = (grub_uint32_t) mmap_addr (grub_multiboot_payload_dest);
-  mbi->flags |= MULTIBOOT_INFO_MEM_MAP;
-
-  /* Convert from bytes to kilobytes.  */
-  mbi->mem_lower = grub_mmap_get_lower () / 1024;
-  mbi->mem_upper = grub_mmap_get_upper () / 1024;
-  mbi->flags |= MULTIBOOT_INFO_MEMORY;
-
-  cmdline = p = cmdline_addr (grub_multiboot_payload_orig);
-  if (! cmdline)
-    goto fail;
-
-  for (i = 0; i < cmdline_argc; i++)
+  if (header->flags & MULTIBOOT_VIDEO_MODE)
     {
-      p = grub_stpcpy (p, cmdline_argv[i]);
-      *(p++) = ' ';
+      switch (header->mode_type)
+	{
+	case 1:
+	  grub_env_set ("gfxpayload", "text");
+	  break;
+
+	case 0:
+	  {
+	    char buf[sizeof ("XXXXXXXXXXxXXXXXXXXXXxXXXXXXXXXX,XXXXXXXXXXxXXXXXXXXXX,auto")];
+	    if (header->depth && header->width && header->height)
+	      grub_sprintf (buf, "%dx%dx%d,%dx%d,auto", header->width,
+			    header->height, header->depth, header->width,
+			    header->height);
+	    else if (header->width && header->height)
+	      grub_sprintf (buf, "%dx%d,auto", header->width, header->height);
+	    else
+	      grub_sprintf (buf, "auto");
+
+	    grub_env_set ("gfxpayload", buf);
+	    break;
+	  }
+	}
     }
 
-  /* Remove the space after the last word.  */
-  if (p != cmdline)
-    p--;
-  *p = 0;
-
-  mbi->flags |= MULTIBOOT_INFO_CMDLINE;
-  mbi->cmdline = (grub_uint32_t) cmdline_addr (grub_multiboot_payload_dest);
-
-
-  grub_strcpy (boot_loader_name_addr (grub_multiboot_payload_orig), PACKAGE_STRING);
-  mbi->flags |= MULTIBOOT_INFO_BOOT_LOADER_NAME;
-  mbi->boot_loader_name = (grub_uint32_t) boot_loader_name_addr (grub_multiboot_payload_dest);
-
-  if (grub_multiboot_get_bootdev (&mbi->boot_device))
-    mbi->flags |= MULTIBOOT_INFO_BOOTDEV;
-
-  grub_loader_set (grub_multiboot_boot, grub_multiboot_unload, 1);
+  grub_multiboot_set_accepts_video (!!(header->flags & MULTIBOOT_VIDEO_MODE));
+
+  grub_multiboot_set_bootdev ();
+
+  grub_loader_set (grub_multiboot_boot, grub_multiboot_unload, 0);
 
  fail:
   if (file)
@@ -389,22 +274,19 @@
 
   if (grub_errno != GRUB_ERR_NONE)
     {
-      grub_free (cmdline);
-      grub_free (mbi);
+      grub_relocator32_free (grub_multiboot_payload_orig);
       grub_dl_unref (my_mod);
     }
 }
 
-
 void
 grub_module  (int argc, char *argv[])
 {
   grub_file_t file = 0;
-  grub_ssize_t size, len = 0;
-  char *module = 0, *cmdline = 0, *p;
-  int i;
-  int cmdline_argc;
-  char **cmdline_argv;
+  grub_ssize_t size;
+  char *module = 0;
+  grub_size_t mbi_size;
+  grub_err_t err;
 
   if (argc == 0)
     {
@@ -412,7 +294,7 @@
       goto fail;
     }
 
-  if (!mbi)
+  if (!grub_multiboot_payload_orig)
     {
       grub_error (GRUB_ERR_BAD_ARGUMENT,
 		  "you need to load the multiboot kernel first");
@@ -424,9 +306,24 @@
     goto fail;
 
   size = grub_file_size (file);
-  module = grub_memalign (MULTIBOOT_MOD_ALIGN, size);
-  if (! module)
-    goto fail;
+  mbi_size = grub_multiboot_get_mbi_size ();
+
+  /* Skip module name.  */
+  err = grub_multiboot_add_module (grub_multiboot_pure_size, size,
+				   argc - 1, argv + 1);
+  if (err)
+    goto fail;
+
+  /* Allocate space on relocator for both module and its descriptor.  */
+  grub_multiboot_payload_orig
+    = grub_relocator32_realloc (grub_multiboot_payload_orig,
+				grub_multiboot_pure_size + size);
+  if (!grub_multiboot_payload_orig)
+    goto fail;
+  alloc_mbi = mbi_size;
+
+  module = grub_multiboot_payload_orig + grub_multiboot_pure_size;
+  grub_multiboot_pure_size += size;
 
   if (grub_file_read (file, module, size) != size)
     {
@@ -434,67 +331,8 @@
       goto fail;
     }
 
-  /* Skip module name.  */
-  cmdline_argc = argc - 1;
-  cmdline_argv = argv + 1;
-
-  for (i = 0; i < cmdline_argc; i++)
-    len += grub_strlen (cmdline_argv[i]) + 1;
-
-  if (len == 0)
-    len = 1;
-
-  cmdline = p = grub_malloc (len);
-  if (! cmdline)
-    goto fail;
-
-  for (i = 0; i < cmdline_argc; i++)
-    {
-      p = grub_stpcpy (p, cmdline_argv[i]);
-      *(p++) = ' ';
-    }
-
-  /* Remove the space after the last word.  */
-  if (p != cmdline)
-    p--;
-  *p = '\0';
-
-  if (mbi->flags & MULTIBOOT_INFO_MODS)
-    {
-      struct multiboot_mod_list *modlist = (struct multiboot_mod_list *) mbi->mods_addr;
-
-      modlist = grub_realloc (modlist, (mbi->mods_count + 1)
-			               * sizeof (struct multiboot_mod_list));
-      if (! modlist)
-	goto fail;
-      mbi->mods_addr = (grub_uint32_t) modlist;
-      modlist += mbi->mods_count;
-      modlist->mod_start = (grub_uint32_t) module;
-      modlist->mod_end = (grub_uint32_t) module + size;
-      modlist->cmdline = (grub_uint32_t) cmdline;
-      modlist->pad = 0;
-      mbi->mods_count++;
-    }
-  else
-    {
-      struct multiboot_mod_list *modlist = grub_zalloc (sizeof (struct multiboot_mod_list));
-      if (! modlist)
-	goto fail;
-      modlist->mod_start = (grub_uint32_t) module;
-      modlist->mod_end = (grub_uint32_t) module + size;
-      modlist->cmdline = (grub_uint32_t) cmdline;
-      mbi->mods_count = 1;
-      mbi->mods_addr = (grub_uint32_t) modlist;
-      mbi->flags |= MULTIBOOT_INFO_MODS;
-    }
-
  fail:
   if (file)
     grub_file_close (file);
-
-  if (grub_errno != GRUB_ERR_NONE)
-    {
-      grub_free (module);
-      grub_free (cmdline);
-    }
 }
+

=== modified file 'loader/i386/multiboot_elfxx.c'
--- loader/i386/multiboot_elfxx.c	2009-12-13 18:29:15 +0000
+++ loader/i386/multiboot_elfxx.c	2010-01-02 01:48:33 +0000
@@ -100,10 +100,11 @@
   code_size = (phdr(highest_segment)->p_paddr + phdr(highest_segment)->p_memsz) - phdr(lowest_segment)->p_paddr;
   grub_multiboot_payload_dest = phdr(lowest_segment)->p_paddr;
 
-  grub_multiboot_payload_size += code_size;
+  grub_multiboot_pure_size += code_size;
 
+  alloc_mbi = grub_multiboot_get_mbi_size ();
   grub_multiboot_payload_orig
-    = grub_relocator32_alloc (grub_multiboot_payload_size);
+    = grub_relocator32_alloc (grub_multiboot_pure_size + alloc_mbi);
 
   if (!grub_multiboot_payload_orig)
     return grub_errno;

=== added file 'loader/i386/multiboot_mbi.c'
--- loader/i386/multiboot_mbi.c	1970-01-01 00:00:00 +0000
+++ loader/i386/multiboot_mbi.c	2010-01-02 12:12:35 +0000
@@ -0,0 +1,518 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 1999,2000,2001,2002,2003,2004,2005,2007,2008,2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/machine/memory.h>
+#include <grub/memory.h>
+#ifdef GRUB_MACHINE_PCBIOS
+#include <grub/machine/biosnum.h>
+#endif
+#include <grub/multiboot.h>
+#include <grub/cpu/multiboot.h>
+#include <grub/disk.h>
+#include <grub/device.h>
+#include <grub/partition.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+#include <grub/i386/pc/vbe.h>
+#include <grub/env.h>
+
+#if defined (GRUB_MACHINE_PCBIOS) || defined (GRUB_MACHINE_COREBOOT) || defined (GRUB_MACHINE_QEMU)
+#define DEFAULT_VIDEO_MODE "text"
+#define HAS_VGA_TEXT 1
+#define HAS_VBE 1
+#else
+#define DEFAULT_VIDEO_MODE "auto"
+#define HAS_VGA_TEXT 0
+#define HAS_VBE 0
+#endif
+
+struct module
+{
+  struct module *next;
+  grub_off_t start;
+  grub_size_t size;
+  char *cmdline;
+  int cmdline_size;
+};
+
+struct module *modules, *modules_last;
+static grub_size_t cmdline_size;
+static grub_size_t total_modcmd;
+static unsigned modcnt;
+static char *cmdline = NULL;
+static grub_uint32_t bootdev;
+static int bootdev_set;
+static int accepts_video;
+
+void
+grub_multiboot_set_accepts_video (int val)
+{
+  accepts_video = val;
+}
+
+/* Return the length of the Multiboot mmap that will be needed to allocate
+   our platform's map.  */
+static grub_uint32_t
+grub_get_multiboot_mmap_len (void)
+{
+  grub_size_t count = 0;
+
+  auto int NESTED_FUNC_ATTR hook (grub_uint64_t, grub_uint64_t, grub_uint32_t);
+  int NESTED_FUNC_ATTR hook (grub_uint64_t addr __attribute__ ((unused)),
+			     grub_uint64_t size __attribute__ ((unused)),
+			     grub_uint32_t type __attribute__ ((unused)))
+    {
+      count++;
+      return 0;
+    }
+
+  grub_mmap_iterate (hook);
+
+  return count * sizeof (struct multiboot_mmap_entry);
+}
+
+grub_size_t
+grub_multiboot_get_mbi_size (void)
+{
+  return sizeof (struct multiboot_info) + ALIGN_UP (cmdline_size, 4)
+    + modcnt * sizeof (struct multiboot_mod_list) + total_modcmd
+    + ALIGN_UP (sizeof(PACKAGE_STRING), 4) + grub_get_multiboot_mmap_len ()
+    + sizeof (struct grub_vbe_info_block)
+    + sizeof (struct grub_vbe_mode_info_block)
+    + 256 * sizeof (struct multiboot_color);
+}
+
+/* Fill previously allocated Multiboot mmap.  */
+static void
+grub_fill_multiboot_mmap (struct multiboot_mmap_entry *first_entry)
+{
+  struct multiboot_mmap_entry *mmap_entry = (struct multiboot_mmap_entry *) first_entry;
+
+  auto int NESTED_FUNC_ATTR hook (grub_uint64_t, grub_uint64_t, grub_uint32_t);
+  int NESTED_FUNC_ATTR hook (grub_uint64_t addr, grub_uint64_t size, grub_uint32_t type)
+    {
+      mmap_entry->addr = addr;
+      mmap_entry->len = size;
+      switch (type)
+	{
+	case GRUB_MACHINE_MEMORY_AVAILABLE:
+ 	  mmap_entry->type = MULTIBOOT_MEMORY_AVAILABLE;
+ 	  break;
+	  
+ 	default:
+ 	  mmap_entry->type = MULTIBOOT_MEMORY_RESERVED;
+ 	  break;
+ 	}
+      mmap_entry->size = sizeof (struct multiboot_mmap_entry) - sizeof (mmap_entry->size);
+      mmap_entry++;
+
+      return 0;
+    }
+
+  grub_mmap_iterate (hook);
+}
+
+static grub_err_t
+set_video_mode (void)
+{
+  grub_err_t err;
+  const char *modevar;
+
+  if (accepts_video || !HAS_VGA_TEXT)
+    {
+      modevar = grub_env_get ("gfxpayload");
+      if (! modevar || *modevar == 0)
+	err = grub_video_set_mode (DEFAULT_VIDEO_MODE, 0);
+      else
+	{
+	  char *tmp;
+	  tmp = grub_malloc (grub_strlen (modevar)
+			     + sizeof (DEFAULT_VIDEO_MODE) + 1);
+	  if (! tmp)
+	    return grub_errno;
+	  grub_sprintf (tmp, "%s;" DEFAULT_VIDEO_MODE, modevar);
+	  err = grub_video_set_mode (tmp, 0);
+	  grub_free (tmp);
+	}
+    }
+  else
+    err = grub_video_set_mode ("text", 0);
+
+  return err;
+}
+
+#if HAS_VBE
+static grub_err_t
+fill_vbe_info (struct multiboot_info *mbi, grub_uint8_t *ptrorig,
+	       grub_uint32_t ptrdest)
+{
+  grub_vbe_status_t status;
+  grub_uint32_t vbe_mode;
+  void *scratch = (void *) GRUB_MEMORY_MACHINE_SCRATCH_ADDR;
+    
+  status = grub_vbe_bios_get_controller_info (scratch);
+  if (status != GRUB_VBE_STATUS_OK)
+    return grub_error (GRUB_ERR_IO, "Can't get controller info.");
+  
+  mbi->vbe_control_info = ptrdest;
+  grub_memcpy (ptrorig, scratch, sizeof (struct grub_vbe_info_block));
+  ptrorig += sizeof (struct grub_vbe_info_block);
+  ptrdest += sizeof (struct grub_vbe_info_block);
+  
+  status = grub_vbe_bios_get_mode (scratch);
+  vbe_mode = *(grub_uint32_t *) scratch;
+  if (status != GRUB_VBE_STATUS_OK)
+    return grub_error (GRUB_ERR_IO, "Can't get VBE mode.");
+  mbi->vbe_mode = vbe_mode;
+
+  status = grub_vbe_bios_get_mode_info (vbe_mode, scratch);
+  if (status != GRUB_VBE_STATUS_OK)
+    return grub_error (GRUB_ERR_IO, "Can't get mode info.");
+  mbi->vbe_mode_info = ptrdest;
+  grub_memcpy (ptrorig, scratch, sizeof (struct grub_vbe_mode_info_block));
+  ptrorig += sizeof (struct grub_vbe_mode_info_block);
+  ptrdest += sizeof (struct grub_vbe_mode_info_block);
+      
+  /* FIXME: retrieve those.  */
+  mbi->vbe_interface_seg = 0;
+  mbi->vbe_interface_off = 0;
+  mbi->vbe_interface_len = 0;
+  
+  mbi->flags |= MULTIBOOT_INFO_VBE_INFO;
+
+  return GRUB_ERR_NONE;
+}
+#endif
+
+static grub_err_t
+retrieve_video_parameters (struct multiboot_info *mbi,
+			   grub_uint8_t *ptrorig, grub_uint32_t ptrdest)
+{
+  grub_err_t err;
+  struct grub_video_mode_info mode_info;
+  void *framebuffer;
+#if HAS_VBE
+  int vbe_active;
+#endif
+  struct grub_video_palette_data palette[256];
+
+  err = set_video_mode ();
+  if (err)
+    return err;
+
+  grub_video_get_palette (0, ARRAY_SIZE (palette), palette);
+
+#if HAS_VBE
+  {
+    const char *adapter_name;
+    adapter_name = grub_video_get_active_adapter_name ();
+    
+    vbe_active = ((adapter_name
+		   && grub_strcmp (adapter_name,
+				   "VESA BIOS Extension Video Driver") == 0)
+		  || (HAS_VGA_TEXT && !adapter_name));
+  }
+#endif
+
+  err = grub_video_get_info_and_fini (&mode_info, &framebuffer);
+  if (err)
+    return err;
+
+  mbi->framebuffer_addr = (grub_addr_t) framebuffer;
+  mbi->framebuffer_pitch = mode_info.pitch;
+
+  mbi->framebuffer_width = mode_info.width;
+  mbi->framebuffer_height = mode_info.height;
+
+  mbi->framebuffer_bpp = mode_info.bpp;
+      
+  if (mode_info.mode_type & GRUB_VIDEO_MODE_TYPE_INDEX_COLOR)
+    {
+      struct multiboot_color *mb_palette;
+      unsigned i;
+      mbi->framebuffer_type = MULTIBOOT_FRAMEBUFFER_TYPE_INDEXED;
+      mbi->framebuffer_palette_addr = ptrdest;
+      mb_palette = (struct multiboot_color *) ptrorig;
+      for (i = 0; i < ARRAY_SIZE (palette); i++)
+	{
+	  mb_palette[i].red = palette[i].r;
+	  mb_palette[i].green = palette[i].g;
+	  mb_palette[i].blue = palette[i].b;
+	}
+      ptrorig += ARRAY_SIZE (palette) * sizeof (struct multiboot_color);
+      ptrdest += ARRAY_SIZE (palette) * sizeof (struct multiboot_color);
+    }
+  else
+    {
+      mbi->framebuffer_type = MULTIBOOT_FRAMEBUFFER_TYPE_RGB;
+      mbi->framebuffer_red_field_position = mode_info.green_field_pos;
+      mbi->framebuffer_red_mask_size = mode_info.green_mask_size;
+      mbi->framebuffer_green_field_position = mode_info.green_field_pos;
+      mbi->framebuffer_green_mask_size = mode_info.green_mask_size;
+      mbi->framebuffer_blue_field_position = mode_info.blue_field_pos;
+      mbi->framebuffer_blue_mask_size = mode_info.blue_mask_size;
+  
+      mbi->flags |= MULTIBOOT_INFO_FRAMEBUFFER_INFO;
+    }
+
+#if HAS_VBE
+  if (vbe_active)
+    fill_vbe_info (mbi, ptrorig, ptrdest);
+#endif
+
+  return GRUB_ERR_NONE;
+}
+
+grub_err_t
+grub_multiboot_make_mbi (void *orig, grub_uint32_t dest, grub_off_t buf_off,
+			 grub_size_t bufsize)
+{
+  grub_uint8_t *ptrorig = (grub_uint8_t *) orig + buf_off;
+  grub_uint32_t ptrdest = dest + buf_off;
+  struct multiboot_info *mbi;
+  struct multiboot_mod_list *modlist;
+  unsigned i;
+  struct module *cur;
+  grub_size_t mmap_size;
+  grub_err_t err;
+
+  if (bufsize < grub_multiboot_get_mbi_size ())
+    return grub_error (GRUB_ERR_OUT_OF_MEMORY, "mbi buffer is too small");
+
+  mbi = (struct multiboot_info *) ptrorig;
+  ptrorig += sizeof (*mbi);
+  ptrdest += sizeof (*mbi);
+  grub_memset (mbi, 0, sizeof (*mbi));
+
+  grub_memcpy (ptrorig, cmdline, cmdline_size);
+  mbi->flags |= MULTIBOOT_INFO_CMDLINE;
+  mbi->cmdline = ptrdest;
+  ptrorig += ALIGN_UP (cmdline_size, 4);
+  ptrdest += ALIGN_UP (cmdline_size, 4);
+
+  grub_memcpy (ptrorig, PACKAGE_STRING, sizeof(PACKAGE_STRING));
+  mbi->flags |= MULTIBOOT_INFO_BOOT_LOADER_NAME;
+  mbi->boot_loader_name = ptrdest;
+  ptrorig += ALIGN_UP (sizeof(PACKAGE_STRING), 4);
+  ptrdest += ALIGN_UP (sizeof(PACKAGE_STRING), 4);
+
+  if (modcnt)
+    {
+      mbi->flags |= MULTIBOOT_INFO_MODS;
+      mbi->mods_addr = ptrdest;
+      mbi->mods_count = modcnt;
+      modlist = (struct multiboot_mod_list *) ptrorig;
+      ptrorig += modcnt * sizeof (struct multiboot_mod_list);
+      ptrdest += modcnt * sizeof (struct multiboot_mod_list);
+
+      for (i = 0, cur = modules; i < modcnt; i++, cur = cur->next)
+	{
+	  modlist[i].mod_start = ptrdest + cur->start;
+	  modlist[i].mod_end = modlist[i].mod_start + cur->size;
+	  modlist[i].cmdline = ptrdest;
+	  grub_memcpy (ptrorig, cur->cmdline, cur->cmdline_size);
+	  ptrorig += ALIGN_UP (cur->cmdline_size, 4);
+	  ptrdest += ALIGN_UP (cur->cmdline_size, 4);
+	}
+    }
+  else
+    {
+      mbi->mods_addr = 0;
+      mbi->mods_count = 0;
+    }
+
+  mmap_size = grub_get_multiboot_mmap_len (); 
+  grub_fill_multiboot_mmap ((struct multiboot_mmap_entry *) ptrorig);
+  mbi->mmap_length = mmap_size;
+  mbi->mmap_addr = ptrdest;
+  mbi->flags |= MULTIBOOT_INFO_MEM_MAP;
+  ptrorig += mmap_size;
+  ptrdest += mmap_size;
+
+  /* Convert from bytes to kilobytes.  */
+  mbi->mem_lower = grub_mmap_get_lower () / 1024;
+  mbi->mem_upper = grub_mmap_get_upper () / 1024;
+  mbi->flags |= MULTIBOOT_INFO_MEMORY;
+
+  if (bootdev_set)
+    {
+      mbi->boot_device = bootdev;
+      mbi->flags |= MULTIBOOT_INFO_BOOTDEV;
+    }
+
+  err = retrieve_video_parameters (mbi, ptrorig, ptrdest);
+  if (err)
+    {
+      grub_print_error ();
+      grub_errno = GRUB_ERR_NONE;
+    }
+  ptrorig += sizeof (struct grub_vbe_info_block);
+  ptrdest += sizeof (struct grub_vbe_info_block);
+  ptrorig += sizeof (struct grub_vbe_mode_info_block);
+  ptrdest += sizeof (struct grub_vbe_mode_info_block);
+
+  return GRUB_ERR_NONE;
+}
+
+void
+grub_multiboot_free_mbi (void)
+{
+  struct module *cur, *next;
+
+  cmdline_size = 0;
+  total_modcmd = 0;
+  modcnt = 0;
+  grub_free (cmdline);
+  cmdline = NULL;
+  bootdev_set = 0;
+
+  for (cur = modules; cur; cur = next)
+    {
+      next = cur->next;
+      grub_free (cur->cmdline);
+      grub_free (cur);
+    }
+  modules = NULL;
+  modules_last = NULL;
+}
+
+grub_err_t
+grub_multiboot_init_mbi (int argc, char *argv[])
+{
+  grub_ssize_t len = 0;
+  char *p;
+  int i;
+
+  grub_multiboot_free_mbi ();
+
+  for (i = 0; i < argc; i++)
+    len += grub_strlen (argv[i]) + 1;
+  if (len == 0)
+    len = 1;
+
+  cmdline = p = grub_malloc (len);
+  if (! cmdline)
+    return grub_errno;
+  cmdline_size = len;
+
+  for (i = 0; i < argc; i++)
+    {
+      p = grub_stpcpy (p, argv[i]);
+      *(p++) = ' ';
+    }
+
+  /* Remove the space after the last word.  */
+  if (p != cmdline)
+    p--;
+  *p = '\0';
+
+  return GRUB_ERR_NONE;
+}
+
+grub_err_t
+grub_multiboot_add_module (grub_off_t start, grub_size_t size,
+			   int argc, char *argv[])
+{
+  struct module *newmod;
+  char *p;
+  grub_ssize_t len = 0;
+  int i;
+
+  newmod = grub_malloc (sizeof (*newmod));
+  if (!newmod)
+    return grub_errno;
+  newmod->start = start;
+  newmod->size = size;
+
+  for (i = 0; i < argc; i++)
+    len += grub_strlen (argv[i]) + 1;
+
+  if (len == 0)
+    len = 1;
+
+  newmod->cmdline = p = grub_malloc (len);
+  if (! newmod->cmdline)
+    {
+      grub_free (newmod);
+      return grub_errno;
+    }
+  newmod->cmdline_size = len;
+  total_modcmd += ALIGN_UP (len, 4);
+
+  for (i = 0; i < argc; i++)
+    {
+      p = grub_stpcpy (p, argv[i]);
+      *(p++) = ' ';
+    }
+
+  /* Remove the space after the last word.  */
+  if (p != newmod->cmdline)
+    p--;
+  *p = '\0';
+
+  if (modules_last)
+    modules_last->next = newmod;
+  else
+    {
+      modules = newmod;
+      modules_last->next = NULL;
+    }
+  modules_last = newmod;
+
+  return GRUB_ERR_NONE;
+}
+
+void
+grub_multiboot_set_bootdev (void)
+{
+  char *p;
+  grub_uint32_t biosdev, slice = ~0, part = ~0;
+  grub_device_t dev;
+
+#ifdef GRUB_MACHINE_PCBIOS
+  biosdev = grub_get_root_biosnumber ();
+#else
+  biosdev = 0xffffffff;
+#endif
+
+  dev = grub_device_open (0);
+  if (dev && dev->disk && dev->disk->partition)
+    {
+
+      p = dev->disk->partition->partmap->get_name (dev->disk->partition);
+      if (p)
+	{
+	  if ((p[0] >= '0') && (p[0] <= '9'))
+	    {
+	      slice = grub_strtoul (p, &p, 0) - 1;
+
+	      if ((p) && (p[0] == ','))
+		p++;
+	    }
+
+	  if ((p[0] >= 'a') && (p[0] <= 'z'))
+	    part = p[0] - 'a';
+	}
+    }
+  if (dev)
+    grub_device_close (dev);
+
+  bootdev = ((biosdev & 0xff) << 24) | ((slice & 0xff) << 16) 
+    | ((part & 0xff) << 8) | 0xff;
+  bootdev_set = 1;
+}

=== modified file 'loader/i386/pc/xnu.c'
--- loader/i386/pc/xnu.c	2009-10-15 12:40:13 +0000
+++ loader/i386/pc/xnu.c	2010-01-02 01:48:33 +0000
@@ -43,7 +43,8 @@
 {
   struct grub_video_mode_info mode_info;
   int ret;
-  char *tmp, *modevar;
+  char *tmp;
+  const char *modevar;
   void *framebuffer;
   grub_err_t err;
 
@@ -55,8 +56,7 @@
       tmp = grub_malloc (grub_strlen (modevar)
 			 + sizeof (DEFAULT_VIDEO_MODE) + 1);
       if (! tmp)
-	return grub_error (GRUB_ERR_OUT_OF_MEMORY,
-			   "couldn't allocate temporary storag");
+	return grub_errno;
       grub_sprintf (tmp, "%s;" DEFAULT_VIDEO_MODE, modevar);
       err = grub_video_set_mode (tmp, video_hook);
       grub_free (tmp);

=== modified file 'loader/multiboot_loader.c'
--- loader/multiboot_loader.c	2009-12-26 10:01:33 +0000
+++ loader/multiboot_loader.c	2010-01-02 01:48:37 +0000
@@ -139,7 +139,7 @@
   /* XXX Find a better way to identify this.
      This is for i386-pc */
 #if defined(GRUB_MACHINE_PCBIOS) || defined(GRUB_MACHINE_COREBOOT) || \
-    defined(GRUB_MACHINE_QEMU)
+  defined(GRUB_MACHINE_QEMU) || defined(GRUB_MACHINE_EFI)
   if (header_multi_ver_found == 1)
     {
       grub_dprintf ("multiboot_loader",
@@ -148,6 +148,7 @@
       module_version_status = 1;
     }
 #endif
+#if !defined(GRUB_MACHINE_EFI)
   if (header_multi_ver_found == 0 || header_multi_ver_found == 2)
     {
       grub_dprintf ("multiboot_loader",
@@ -155,6 +156,7 @@
       grub_multiboot2 (argc, argv);
       module_version_status = 2;
     }
+#endif
 
   return grub_errno;
 
@@ -173,7 +175,7 @@
 {
 
 #if defined(GRUB_MACHINE_PCBIOS) || defined(GRUB_MACHINE_COREBOOT) || \
-    defined(GRUB_MACHINE_QEMU)
+    defined(GRUB_MACHINE_QEMU) || defined(GRUB_MACHINE_EFI)
   if (module_version_status == 1)
     {
       grub_dprintf("multiboot_loader",
@@ -181,13 +183,14 @@
       grub_module (argc, argv);
     }
 #endif
+#if !defined(GRUB_MACHINE_EFI)
   if (module_version_status == 2)
     {
       grub_dprintf("multiboot_loader",
           "Launching multiboot 2 grub_module2() function\n");
       grub_module2 (argc, argv);
     }
-
+#endif
   return grub_errno;
 }
 

=== modified file 'video/video.c'
--- video/video.c	2009-12-24 22:53:05 +0000
+++ video/video.c	2010-01-02 01:48:37 +0000
@@ -399,6 +399,15 @@
   return grub_video_adapter_active->get_active_render_target (target);
 }
 
+const char *
+grub_video_get_active_adapter_name (void)
+{
+  if (grub_video_adapter_active)
+    return grub_video_adapter_active->name;
+  return NULL;
+}
+
+
 grub_err_t
 grub_video_set_mode (const char *modestring,
 		     int NESTED_FUNC_ATTR (*hook) (grub_video_adapter_t p,


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

Re: [RFC] Multiboot ammendment: non-VBE video

[Robert Millan]

On Sat, Jan 02, 2010 at 07:26:08PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> I followed your suggestions. I attach new draft together with drawing of
> a blue diagonal line in example kernel (it would be way better to make
> it use it for output but it's intended to just show how to use passed
> info). I also attach an implementation of multiboot on EFI which uses
> this draft. Note that this ammendment is nothing EFI-specific and can be
> used with any linear framebuffer driver. The only reason it's in the
> same patch is because it was the branch in which I was working on
> multiboot improvements and haven't yet splitted individual changes yet

Excellent.  I'll review the Multiboot part first.

>  /* The flags for the Multiboot header.  */
>  #ifdef __ELF__
> -# define MULTIBOOT_HEADER_FLAGS		0x00000003
> +# define MULTIBOOT_HEADER_FLAGS		0x00000007
>  #else
> -# define MULTIBOOT_HEADER_FLAGS		0x00010003
> +# define MULTIBOOT_HEADER_FLAGS		0x00010007
>  #endif

This was a bit messy.  I just replaced it with an ORed macro list.  I
should have done this before, but I didn't have time to.  Sorry to break
your patch, but it's a trivial fix ;-)

> +	.long 0
> +	.long 1024
> +	.long 768
> +	.long 32

Maybe better to use 640x480 instead?  Not everyone has a large display.

>  /* The bits in the required part of flags field we don't support.  */
> -#define MULTIBOOT_UNSUPPORTED			0x0000fffc
> +#define MULTIBOOT_UNSUPPORTED                   0x0000fff8

Shouldn't this be 0xeffc ?  (0xfffc & ~0x1000)

> === modified file 'doc/multiboot.texi'
> --- doc/multiboot.texi	2010-01-01 20:02:24 +0000
> +++ doc/multiboot.texi	2010-01-02 16:58:33 +0000
> @@ -479,7 +479,8 @@
>  preferred graphics mode. Note that that is only a @emph{recommended}
>  mode by the OS image. If the mode exists, the boot loader should set
>  it, when the user doesn't specify a mode explicitly. Otherwise, the
> -boot loader should fall back to a similar mode, if available.
> +boot loader should fall back to a similar mode, if available. Boot loader
> +may also choose another mode if it sees fit.

I agree with changing this, but the phrases seem to contradict each other.  If
the mode exists, what should bootloader do?

I suggest we remove from "If the mode exists..." untill
"...if available", leaving your "Boot loader may also..." only.

> @@ -488,7 +489,9 @@
>  Contains @samp{0} for linear graphics mode or @samp{1} for
>  EGA-standard text mode. Everything else is reserved for future
>  expansion. Note that the boot loader may set a text mode, even if this
> -field contains @samp{0}.
> +field contains @samp{0}. If video adapter doesn't support EGA text mode or
> +bootloader doesn't know how to initialise this mode it may set video
> +mode even if field contains @samp{1}

If the EGA option is obsolete/useless, I'd just make it:

  Reserved for backward compatibility.  Always contains @samp{0}.

and schedule it for removal in next major version.

>  84      | vbe_interface_off |
>  86      | vbe_interface_len |
>          +-------------------+
> +88      | framebuffer_addr  |    (present if flags[12] is set)
> +96      | framebuffer_pitch |
> +100     | framebuffer_width |
> +104     | framebuffer_height|
> +108     | framebuffer_bpp   |
> +109     | framebuffer_type  |
> +110-115 | color_info        |

Perhaps it would be simpler for us to arrange it so that flags 11 and 12
can't be used at the same time.  We could just say that flag 11 is reserved
and unused, and then put these declarations in the offset that used to be for
VBE.  IIRC there were no possible sections after VBE, so the Framebuffer
section size wouldn't be constrained by that.

But if you prefer to keep flag 11 operational, I don't object.  I would
probably object to GRUB implementing it though.

> In this case color_info is defined as following:

I'd appreciate if a native English speaker confirms this, but I believe
this should say "as follows" (same for the other instances of this
construct).

> +@samp{framebuffer_palette_addr} contains address of array of @samp{framebuffer_palette_num_colors} following structures:

There seems to be some word missing here.

-- 
Robert Millan

  "Be the change you want to see in the world" -- Gandhi

Re: [RFC] Multiboot ammendment: non-VBE video

[Isaac Dupree]

Robert Millan wrote:
>> In this case color_info is defined as following:
> 
> I'd appreciate if a native English speaker confirms this, but I believe
> this should say "as follows" (same for the other instances of this
> construct).

"as follows" reads naturally to me, as a native speaker (also checked: 
online dictionaries mention the phrase "as follows" without saying it's 
specific to my area -- northeast U.S. -- so I think it's right to use)

>> +@samp{framebuffer_palette_addr} contains address of array of @samp{framebuffer_palette_num_colors} following structures:
> 
> There seems to be some word missing here.

"the following structures"?
"as follows" would also work well if "structures" isn't an important 
word to keep?
wait, it's like saying "array of 3 following structures", each of the 
same type. hmm. "@samp{framebuffer_palette_num_colors} structures as 
follows"?

-Isaac

Re: [RFC] Multiboot ammendment: non-VBE video

[richardvoigt]

On Sun, Jan 3, 2010 at 10:35 AM, Isaac Dupree
<ml@isaac.cedarswampstudios.org> wrote:
> Robert Millan wrote:
>>>
>>> In this case color_info is defined as following:
>>
>> I'd appreciate if a native English speaker confirms this, but I believe
>> this should say "as follows" (same for the other instances of this
>> construct).
>
> "as follows" reads naturally to me, as a native speaker (also checked:
> online dictionaries mention the phrase "as follows" without saying it's
> specific to my area -- northeast U.S. -- so I think it's right to use)

+1 for as follows

>
>>> +@samp{framebuffer_palette_addr} contains address of array of
>>> @samp{framebuffer_palette_num_colors} following structures:
>>
>> There seems to be some word missing here.
>
> "the following structures"?
> "as follows" would also work well if "structures" isn't an important word to
> keep?
> wait, it's like saying "array of 3 following structures", each of the same
> type. hmm. "@samp{framebuffer_palette_num_colors} structures as follows"?

How about this?

XYZZY contains _the_ address of _an_ array of N structures which follow

>
> -Isaac
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>

Re: [RFC] Multiboot ammendment: non-VBE video

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

Robert Millan wrote:
>   
>> +	.long 0
>> +	.long 1024
>> +	.long 768
>> +	.long 32
>>     
>
> Maybe better to use 640x480 instead?  Not everyone has a large display.
>
>   
It's only a recommended resolution. Bootloader will use a mode it
chooses. Perhaps we should remove recommended mode fields from the spec
altogether or make them somehow optional
>>  /* The bits in the required part of flags field we don't support.  */
>> -#define MULTIBOOT_UNSUPPORTED			0x0000fffc
>> +#define MULTIBOOT_UNSUPPORTED                   0x0000fff8
>>     
>
> Shouldn't this be 0xeffc ?  (0xfffc & ~0x1000)
>
>   
AOUT_KLUDGE field is 0x10000 and not 0x1000
>> === modified file 'doc/multiboot.texi'
>> --- doc/multiboot.texi	2010-01-01 20:02:24 +0000
>> +++ doc/multiboot.texi	2010-01-02 16:58:33 +0000
>> @@ -479,7 +479,8 @@
>>  preferred graphics mode. Note that that is only a @emph{recommended}
>>  mode by the OS image. If the mode exists, the boot loader should set
>>  it, when the user doesn't specify a mode explicitly. Otherwise, the
>> -boot loader should fall back to a similar mode, if available.
>> +boot loader should fall back to a similar mode, if available. Boot loader
>> +may also choose another mode if it sees fit.
>>     
>
> I agree with changing this, but the phrases seem to contradict each other.  If
> the mode exists, what should bootloader do?
>
> I suggest we remove from "If the mode exists..." untill
> "...if available", leaving your "Boot loader may also..." only.
>
>   
Actually they were contradicting because "should" means that loader may
choose not to follow the rule
>> @@ -488,7 +489,9 @@
>>  Contains @samp{0} for linear graphics mode or @samp{1} for
>>  EGA-standard text mode. Everything else is reserved for future
>>  expansion. Note that the boot loader may set a text mode, even if this
>> -field contains @samp{0}.
>> +field contains @samp{0}. If video adapter doesn't support EGA text mode or
>> +bootloader doesn't know how to initialise this mode it may set video
>> +mode even if field contains @samp{1}
>>     
>
> If the EGA option is obsolete/useless, I'd just make it:
>
>   Reserved for backward compatibility.  Always contains @samp{0}.
>
> and schedule it for removal in next major version.
>
>   
It is useful. It allows payload to say: "I'm ok with video mode but
prefer text one"
>>  84      | vbe_interface_off |
>>  86      | vbe_interface_len |
>>          +-------------------+
>> +88      | framebuffer_addr  |    (present if flags[12] is set)
>> +96      | framebuffer_pitch |
>> +100     | framebuffer_width |
>> +104     | framebuffer_height|
>> +108     | framebuffer_bpp   |
>> +109     | framebuffer_type  |
>> +110-115 | color_info        |
>>     
>
> Perhaps it would be simpler for us to arrange it so that flags 11 and 12
> can't be used at the same time.  We could just say that flag 11 is reserved
> and unused, and then put these declarations in the offset that used to be for
> VBE.  IIRC there were no possible sections after VBE, so the Framebuffer
> section size wouldn't be constrained by that.
>
> But if you prefer to keep flag 11 operational, I don't object.  I would
> probably object to GRUB implementing it though.
>
>   
If payload wants to use VBE it will. I think that providing this
informtion saves payload a trip to real mode. So this way we encourage
saner real-mode-free OS design.
Do you think it would be useful to make flag 12 structure describe EGA
text too? I attach new diff for multiboot.texi. If it's approved it
should be simple to implement in grub2 and example kernel

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



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

Re: [RFC] Multiboot ammendment: non-VBE video

[Robert Millan]

On Mon, Jan 04, 2010 at 08:35:38PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> Robert Millan wrote:
> >   
> >> +	.long 0
> >> +	.long 1024
> >> +	.long 768
> >> +	.long 32
> >>     
> >
> > Maybe better to use 640x480 instead?  Not everyone has a large display.
> >
> >   
> It's only a recommended resolution.

Ok then, no big deal.

> Bootloader will use a mode it
> chooses. Perhaps we should remove recommended mode fields from the spec
> altogether or make them somehow optional

Is that important?  I'm hesitant to do that untill we have better understanding
on what lead to this decision.

> >>  /* The bits in the required part of flags field we don't support.  */
> >> -#define MULTIBOOT_UNSUPPORTED			0x0000fffc
> >> +#define MULTIBOOT_UNSUPPORTED                   0x0000fff8
> >>     
> >
> > Shouldn't this be 0xeffc ?  (0xfffc & ~0x1000)
> >
> >   
> AOUT_KLUDGE field is 0x10000 and not 0x1000

But that's a completely different bit?  I'm a bit confused.

Btw, as you pointed out on IRC, MULTIBOOT_UNSUPPORTED is a GRUB-specific
mask and shouldn't be part of <multiboot.h> anyway.

> >>  preferred graphics mode. Note that that is only a @emph{recommended}
> >>  mode by the OS image. If the mode exists, the boot loader should set
> >>  it, when the user doesn't specify a mode explicitly. Otherwise, the
> >> -boot loader should fall back to a similar mode, if available.
> >> +boot loader should fall back to a similar mode, if available. Boot loader
> >> +may also choose another mode if it sees fit.
> >>     
> >
> > I agree with changing this, but the phrases seem to contradict each other.  If
> > the mode exists, what should bootloader do?
> >
> > I suggest we remove from "If the mode exists..." untill
> > "...if available", leaving your "Boot loader may also..." only.
> >
> >   
> Actually they were contradicting because "should" means that loader may
> choose not to follow the rule

Sorry, my oversight.  Well it is redundant, not contradictory.  I didn't
express it properly :-)  (some part of my brain doesn't play well with the
rest).

> >>  Contains @samp{0} for linear graphics mode or @samp{1} for
> >>  EGA-standard text mode. Everything else is reserved for future
> >>  expansion. Note that the boot loader may set a text mode, even if this
> >> -field contains @samp{0}.
> >> +field contains @samp{0}. If video adapter doesn't support EGA text mode or
> >> +bootloader doesn't know how to initialise this mode it may set video
> >> +mode even if field contains @samp{1}
> >>     
> >
> > If the EGA option is obsolete/useless, I'd just make it:
> >
> >   Reserved for backward compatibility.  Always contains @samp{0}.
> >
> > and schedule it for removal in next major version.
> >
> >   
> It is useful. It allows payload to say: "I'm ok with video mode but
> prefer text one"

Oh, I think I missunderstood what "EGA-standard text mode" means.  Is this
the same text mode that survived to this day, and is known in GRUB codebase
as "vga_text"?

> > Perhaps it would be simpler for us to arrange it so that flags 11 and 12
> > can't be used at the same time.  We could just say that flag 11 is reserved
> > and unused, and then put these declarations in the offset that used to be for
> > VBE.  IIRC there were no possible sections after VBE, so the Framebuffer
> > section size wouldn't be constrained by that.
> >
> > But if you prefer to keep flag 11 operational, I don't object.  I would
> > probably object to GRUB implementing it though.
> >
> >   
> If payload wants to use VBE it will. I think that providing this
> informtion saves payload a trip to real mode. So this way we encourage
> saner real-mode-free OS design.

I'd rather encourage them to use modern flag 12 instead.  In any case, it
doesn't to keep flag 11 as part of the standard as long as it's optional.
I'm still not convinced GRUB should implement it, but that's a separate
discussion.

> Do you think it would be useful to make flag 12 structure describe EGA
> text too? I attach new diff for multiboot.texi. If it's approved it
> should be simple to implement in grub2 and example kernel

You forgot to attach it.

-- 
Robert Millan

  "Be the change you want to see in the world" -- Gandhi

Re: [RFC] Multiboot ammendment: non-VBE video

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

Robert Millan wrote:
>
>> Bootloader will use a mode it
>> chooses. Perhaps we should remove recommended mode fields from the spec
>> altogether or make them somehow optional
>>     
>
> Is that important?  I'm hesitant to do that untill we have better understanding
> on what lead to this decision.
>   
>>>   
>>>       
>> It is useful. It allows payload to say: "I'm ok with video mode but
>> prefer text one"
>>     
>
> Oh, I think I missunderstood what "EGA-standard text mode" means.  Is this
> the same text mode that survived to this day, and is known in GRUB codebase
> as "vga_text"?
>
>   
Yes
>>> Perhaps it would be simpler for us to arrange it so that flags 11 and 12
>>> can't be used at the same time.  We could just say that flag 11 is reserved
>>> and unused, and then put these declarations in the offset that used to be for
>>> VBE.  IIRC there were no possible sections after VBE, so the Framebuffer
>>> section size wouldn't be constrained by that.
>>>
>>> But if you prefer to keep flag 11 operational, I don't object.  I would
>>> probably object to GRUB implementing it though.
>>>
>>>   
>>>       
>> If payload wants to use VBE it will. I think that providing this
>> informtion saves payload a trip to real mode. So this way we encourage
>> saner real-mode-free OS design.
>>     
>
> I'd rather encourage them to use modern flag 12 instead.  In any case, it
> doesn't to keep flag 11 as part of the standard as long as it's optional.
> I'm still not convinced GRUB should implement it, but that's a separate
> discussion.
>
>   
Flag 12 doesn't allow to e.g. change the resolution or do the double
buffering.
>> Do you think it would be useful to make flag 12 structure describe EGA
>> text too? I attach new diff for multiboot.texi. If it's approved it
>> should be simple to implement in grub2 and example kernel
>>     
>
> You forgot to attach it.
>
>   
Fixed


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


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

=== modified file 'doc/multiboot.texi'
--- doc/multiboot.texi	2010-01-01 20:02:24 +0000
+++ doc/multiboot.texi	2010-01-04 19:32:13 +0000
@@ -477,9 +477,7 @@
 
 All of the graphics fields are enabled by flag bit 2. They specify the
 preferred graphics mode. Note that that is only a @emph{recommended}
-mode by the OS image. If the mode exists, the boot loader should set
-it, when the user doesn't specify a mode explicitly. Otherwise, the
-boot loader should fall back to a similar mode, if available.
+mode by the OS image. Boot loader may also choose another mode if it sees fit.
 
 The meaning of each is as follows:
 
@@ -488,7 +486,9 @@
 Contains @samp{0} for linear graphics mode or @samp{1} for
 EGA-standard text mode. Everything else is reserved for future
 expansion. Note that the boot loader may set a text mode, even if this
-field contains @samp{0}.
+field contains @samp{0}. If video adapter doesn't support EGA text mode or
+bootloader doesn't know how to initialise this mode it may set video
+mode even if field contains @samp{1}
 
 @item width
 Contains the number of the columns. This is specified in pixels in a
@@ -635,6 +635,15 @@
 84      | vbe_interface_off |
 86      | vbe_interface_len |
         +-------------------+
+88      | framebuffer_addr  |    (present if flags[12] is set)
+96      | framebuffer_pitch |
+100     | framebuffer_width |
+104     | framebuffer_height|
+108     | framebuffer_bpp   |
+109     | framebuffer_type  |
+110-115 | color_info        |
+        +-------------------+
+
 @end group
 @end example
 
@@ -911,9 +920,7 @@
 @uref{http://www.microsoft.com/hwdev/busbios/amp_12.htm, Advanced Power
 Management (APM) BIOS Interface Specification}, for more information.
 
-If bit 11 in the @samp{flags} is set, the graphics table is available.
-This must only be done if the kernel has indicated in the
-@samp{Multiboot Header} that it accepts a graphics mode.
+If bit 11 in the @samp{flags} is set, the @sc{vbe} table is available.
 
 The fields @samp{vbe_control_info} and @samp{vbe_mode_info} contain
 the physical addresses of @sc{vbe} control information returned by the
@@ -935,6 +942,44 @@
 Multiboot boot loaders may simulate @sc{vbe} on non-@sc{vbe} modes, as
 if they were @sc{vbe} modes.
 
+If bit 12 in the @samp{flags} is set, the @sc{Framebuffer} table is available.
+
+The field @samp{framebuffer_addr} contains framebuffer physical address. This field is 64-bit wide but bootloader @dfn{should} set it under 4GiB if possible for compatibility with payloads which aren't aware of PAE or amd64. The field @samp{framebuffer_pitch} contains pitch in bytes. The fields @samp{framebuffer_width}, @samp{framebuffer_height} contain framebuffer dimensions in pixels. The field @samp{framebuffer_bpp} contains number of bits per pixel. If @samp{framebuffer_type} is set to 0 it means indexed color. In this case color_info is defined as follows:
+@example
+@group
+        +----------------------------------+
+110     | framebuffer_palette_addr         |
+114     | framebuffer_palette_num_colors   |
+        +----------------------------------+
+@end group
+@end example
+@samp{framebuffer_palette_addr} contains address of palette. Palette is an array of colour descriptors. Each colour descriptor has following structure:
+@example
+@group
+        +-------------+
+0       | red_value   |
+1       | green_value |
+2       | blue_value  |
+        +-------------+
+@end group
+@end example
+If @samp{framebuffer_type} is set to 1 it means direct RGB color. Then color_type is defined as follows:
+
+@example
+@group
+        +----------------------------------+
+110     | framebuffer_red_field_position   |
+111     | framebuffer_red_mask_size        |
+112     | framebuffer_green_field_position |
+113     | framebuffer_green_mask_size      |
+114     | framebuffer_blue_field_position  |
+115     | framebuffer_blue_mask_size       |
+        +----------------------------------+
+@end group
+@end example
+
+If @samp{framebuffer_type} is set to 2 it means EGA text. In this case @samp{framebuffer_width} and @samp{framebuffer_height} are expressed in characters and not in pixels. @samp{framebuffer_bpp} is equal 16 (16 bits per character) and @samp{framebuffer_pitch} is expressed in bytes per text line.
+All further values of @samp{framebuffer_type} are reserved for future expansion
 
 @node Examples
 @chapter Examples


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

Re: [RFC] Multiboot ammendment: non-VBE video

[Robert Millan]

Hi,

I committed your last patch with some minor stylistic adjustments, plus
your changes to the example kernel.

-- 
Robert Millan

  "Be the change you want to see in the world" -- Gandhi

Multiboot video in GRUB (Re: [RFC] Multiboot ammendment: non-VBE video)

[Robert Millan]

On Sat, Jan 02, 2010 at 07:26:08PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> === added file 'ChangeLog.efiboot'
> --- ChangeLog.efiboot	1970-01-01 00:00:00 +0000
> +++ ChangeLog.efiboot	2010-01-02 01:48:33 +0000
> @@ -0,0 +1,19 @@
> +2009-11-28  Vladimir Serbinenko  <phcoder@gmail.com>
> +
> +	Multiboot support on i386-efi.
> +
> +	* conf/i386-efi.rmk (pkglib_MODULES): Add multiboot.mod.
> +	(multiboot_mod_SOURCES): New variable.
> +	(multiboot_mod_CFLAGS): Likewise.
> +	(multiboot_mod_LDFLAGS): Likewise.
> +	(multiboot_mod_ASFLAGS): Likewise.
> +	* include/grub/i386/multiboot.h (grub_multiboot_real_boot): Explicitly
> +	declare as regparm (3).
> +	(grub_multiboot2_real_boot): Likewise.
> +	(grub_multiboot_boot): Finish boot services.
> +	(grub_fill_multiboot_mmap): Explicitly parse memory map types.
> +	(grub_multiboot): Declare as noreturn to avoid finishing console.
> +	* loader/multiboot_loader.c (grub_cmd_multiboot_loader): Enable
> +	multiboot1 on i386-efi and disable multiboot2 on this platform.
> +	(grub_cmd_module_loader): Likewise.

Could the EFI parts be split into a separate patch?  I definitely want both
things, but they're separate changes, would be good to have the additional
granularity if possible.

-- 
Robert Millan

  "Be the change you want to see in the world" -- Gandhi