Re: [PATCH] Video mode fixes in linux loader

["Yoshinori K. Okuji" <okuji@enbug.org>]

On Tuesday 07 April 2009 00:34:03 Pavel Roskin wrote:
> Hello!
>
> This is an attempt to fix all issues with the video mode handling in the
> new Linux loader.
>
> First of all, free_page() doesn't belong to grub_linux_unload().  The
> later function is called after the new kernel has been loaded, just
> before the boot.  Thus it obliterates the data set up by the last
> "linux" command if the previous Linux boot failed.
>
> Instead, free_page() should be called from allocate_pages() to reset not
> only real_mode_mem and prot_mode_mem, but also initrd_mem.
>
> Next problem is that grub_linux_boot() can access linux_vesafb_modes
> with a wrong index.  I believe we should threat modes that don't fit the
> array as text modes.
>
> I introduced GRUB_LINUX_VID_MODE_VESA_START for the first VESA mode we
> support.  Also, I introduced a macro ARRAY_SIZE to calculate the array
> size.
>
> The default VGA mode is now GRUB_LINUX_VID_MODE_NORMAL, not the mode of
> the kernel we tried to load before.  Finally, "vga=ask" is now
> recognized.
>
> ChangeLog:
>
>         * include/grub/misc.h (ARRAY_SIZE): New macro.
>         * include/grub/i386/linux.h (GRUB_LINUX_VID_MODE_VESA_START):
>         New macro.
>         * loader/i386/linux.c (allocate_pages): Use free_pages().
>         (grub_linux_unload): Don't use free_pages().
>         (grub_linux_boot): Prevent accessing linux_vesafb_modes with a
>         wrong index.  Treat all other modes as text modes.
>         (grub_cmd_linux): Initialize vid_mode unconditionally to
>         GRUB_LINUX_VID_MODE_NORMAL.  Recognize and support "vga=ask".
>
> Index: include/grub/misc.h
> ===================================================================
> --- include/grub/misc.h	(revision 2068)
> +++ include/grub/misc.h	(working copy)
> @@ -26,6 +26,7 @@
>  #include <grub/err.h>
>
>  #define ALIGN_UP(addr, align) (((grub_uint64_t)addr + align - 1) & ~(align
> - 1)) +#define ARRAY_SIZE(array) (sizeof (array) / sizeof (array[0]))
>
>  #define grub_dprintf(condition, fmt, args...) grub_real_dprintf(__FILE__,
> __LINE__, condition, fmt, ## args); /* XXX: If grub_memmove is too slow, we
> must implement grub_memcpy.  */ Index: include/grub/i386/linux.h
> ===================================================================
> --- include/grub/i386/linux.h	(revision 2068)
> +++ include/grub/i386/linux.h	(working copy)
> @@ -38,6 +38,7 @@
>  #define GRUB_LINUX_VID_MODE_NORMAL	0xFFFF
>  #define GRUB_LINUX_VID_MODE_EXTENDED	0xFFFE
>  #define GRUB_LINUX_VID_MODE_ASK		0xFFFD
> +#define GRUB_LINUX_VID_MODE_VESA_START	0x0301
>
>  #define GRUB_LINUX_SETUP_MOVE_SIZE	0x9100
>  #define GRUB_LINUX_CL_MAGIC		0xA33F
> Index: loader/i386/linux.c
> ===================================================================
> --- loader/i386/linux.c	(revision 2068)
> +++ loader/i386/linux.c	(working copy)
> @@ -206,8 +206,7 @@ allocate_pages (grub_size_t prot_size)
>    prot_mode_pages = (prot_size >> 12);
>
>    /* Initialize the memory pointers with NULL for convenience.  */
> -  real_mode_mem = 0;
> -  prot_mode_mem = 0;
> +  free_pages();

Please insert a space before the parenthesis.

>
>    /* FIXME: Should request low memory from the heap when this feature is
>       implemented.  */
> @@ -332,7 +331,9 @@ grub_linux_boot (void)
>
>    params = real_mode_mem;
>
> -  if (vid_mode == GRUB_LINUX_VID_MODE_NORMAL || vid_mode ==
> GRUB_LINUX_VID_MODE_EXTENDED) +  if (vid_mode <
> GRUB_LINUX_VID_MODE_VESA_START ||
> +      vid_mode >= GRUB_LINUX_VID_MODE_VESA_START +
> +		  ARRAY_SIZE (linux_vesafb_modes))
>      grub_video_restore ();
>    else if (vid_mode)
>      {
> @@ -340,7 +341,7 @@ grub_linux_boot (void)
>        int depth, flags;
>
>        flags = 0;
> -      linux_mode = &linux_vesafb_modes[vid_mode - 0x301];
> +      linux_mode = &linux_vesafb_modes[vid_mode -
> GRUB_LINUX_VID_MODE_VESA_START]; depth = linux_mode->depth;
>
>        /* If we have 8 or less bits, then assume that it is indexed color
> mode.  */ @@ -443,7 +444,6 @@ grub_linux_boot (void)
>  static grub_err_t
>  grub_linux_unload (void)
>  {
> -  free_pages ();
>    grub_dl_unref (my_mod);
>    loaded = 0;
>    return GRUB_ERR_NONE;
> @@ -570,6 +570,7 @@ grub_cmd_linux (grub_command_t cmd __att
>  	       (unsigned) real_size, (unsigned) prot_size);
>
>    /* Detect explicitly specified memory size, if any.  */

This is not due to you, but the comment above is not synchronized with the 
code apparently. Can you fix it?

> +  vid_mode = GRUB_LINUX_VID_MODE_NORMAL;
>    linux_mem_size = 0;
>    for (i = 1; i < argc; i++)
>      if (grub_memcmp (argv[i], "vga=", 4) == 0)
> @@ -581,6 +582,8 @@ grub_cmd_linux (grub_command_t cmd __att
>  	  vid_mode = GRUB_LINUX_VID_MODE_NORMAL;
>  	else if (grub_strcmp (val, "ext") == 0)
>  	  vid_mode = GRUB_LINUX_VID_MODE_EXTENDED;
> +	else if (grub_strcmp (val, "ask") == 0)
> +	  vid_mode = GRUB_LINUX_VID_MODE_ASK;
>  	else
>  	  vid_mode = (grub_uint16_t) grub_strtoul (val, 0, 0);

Regards,
Okuji