Re: grub2 efi patches

[Marco Gerards <mgerards@xs4all.nl>]

Alexandre Boeglin <alex@boeglin.org> writes:

Hi,

> Here are a few patches for grub2, against cvs head:

Great!  Thanks for working on this!  Can you please send in ChangeLog
entries for these patches?

> grub2_arg_doubledash.patch fixes a bug in the handling of the '--'
> argument,
>
> grub2_efi_chainloader_options.patch adds support for efi chainload options,
> it is not really beautiful (for instance, the ascii to utf16 conversion),
> but it works for the MacOSX loader,
>
> grub2_efi_grub_prefix.patch uses the grub_prefix variable to set the prefix
> environment variable, instead of the directory in which grub.efi is. This
> allows to have grub.efi and the modules in different folders, and the old
> behaviour should be preserved, if the grub_prefix is an empty string,

How about passing it as an argument to GRUB 2?  I assume EFI can do
this?  This is what we do on open firmware, IIRC.

> grub2_efi_halt_reboot.patch adds support for the reboot and halt commands,
> provided by efi runtime services.
>
> Regards,
> Alex
>
> P.S.: I sent the patches as attachments, I don't know if they will be
> discarded by the list robot or not ...

No, this is even preferred.  Could you use "diff -up" for future
patches?

> Alexandre Boeglin
> email: alex (at) boeglin (dot) org
> jabber: alex (at) im (dot) apinc (dot) org
> website: http://boeglin.org/
>
> Index: normal/arg.c
> ===================================================================
> RCS file: /sources/grub/grub2/normal/arg.c,v
> retrieving revision 1.7
> diff -u -r1.7 arg.c
> --- normal/arg.c	21 Jul 2007 23:32:28 -0000	1.7
> +++ normal/arg.c	3 Nov 2007 22:44:41 -0000
> @@ -313,7 +313,7 @@
>  	  if (grub_strlen (arg) == 2)
>  	    {
>  	      for (curarg++; curarg < argc; curarg++)
> -		if (add_arg (arg) != 0)
> +		if (add_arg (argv[curarg]) != 0)
>  		  goto fail;
>  	      break;
>  	    }

Looks fine to me.

> Index: include/grub/efi/chainloader.h
> ===================================================================
> RCS file: /sources/grub/grub2/include/grub/efi/chainloader.h,v
> retrieving revision 1.2
> diff -u -r1.2 chainloader.h
> --- include/grub/efi/chainloader.h	21 Jul 2007 23:32:22 -0000	1.2
> +++ include/grub/efi/chainloader.h	3 Nov 2007 22:42:01 -0000
> @@ -19,6 +19,6 @@
>  #ifndef GRUB_EFI_CHAINLOADER_HEADER
>  #define GRUB_EFI_CHAINLOADER_HEADER	1
>  
> -void grub_chainloader_cmd (const char *filename);
> +void grub_chainloader_cmd (int argc, char *argv[]);
>  
>  #endif /* ! GRUB_EFI_CHAINLOADER_HEADER */
> Index: loader/efi/chainloader.c
> ===================================================================
> RCS file: /sources/grub/grub2/loader/efi/chainloader.c,v
> retrieving revision 1.2
> diff -u -r1.2 chainloader.c
> --- loader/efi/chainloader.c	21 Jul 2007 23:32:28 -0000	1.2
> +++ loader/efi/chainloader.c	3 Nov 2007 22:42:47 -0000
> @@ -17,8 +17,6 @@
>   *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -/* TODO: support load options.  */
> -
>  #include <grub/loader.h>
>  #include <grub/file.h>
>  #include <grub/err.h>
> @@ -175,7 +173,7 @@
>  }
>  
>  void
> -grub_chainloader_cmd (const char *filename)
> +grub_chainloader_cmd (int argc, char *argv[])
>  {
>    grub_file_t file = 0;
>    grub_ssize_t size;
> @@ -185,6 +183,11 @@
>    grub_device_t dev = 0;
>    grub_efi_device_path_t *dp = 0;
>    grub_efi_loaded_image_t *loaded_image;
> +  char *filename = argv[0];
> +  grub_efi_char16_t **options = NULL, *p;
> +  int i, j, options_len = 0;
> +
> +  b = grub_efi_system_table->boot_services;
>    
>    grub_dl_ref (my_mod);
>  
> @@ -192,6 +195,30 @@
>    address = 0;
>    image_handle = 0;
>    file_path = 0;
> +
> +  if (argc == 0)
> +  {
> +    grub_error (GRUB_ERR_BAD_ARGUMENT, "no file specified");
> +    goto fail;
> +  }

This indentation doesn't look right.  Same for the other "if"s.

> +  /* copy options */
> +  if (argc > 1)
> +  {
> +    for (i = 1; i < argc; i++)
> +      options_len += (grub_strlen (argv[i]) + 1) * sizeof (**options);
> +
> +    status = b->allocate_pool (GRUB_EFI_LOADER_DATA, options_len + sizeof (**options), options);

Do you deallocate when an error occurs?

> +    if (status != GRUB_EFI_SUCCESS)
> +      goto fail;
> +    p = *options;
> +
> +    for (i = 1; i < argc; i++){
> +      *p++ = ' ';
> +      for (j = 0; j < grub_strlen (argv[i]) + 1; j++)
> +        p[j] = argv[i][j];
> +    }

This indentation is not right.

> +  }
>    
>    b = grub_efi_system_table->boot_services;
>  
> @@ -267,6 +294,10 @@
>        goto fail;
>      }
>    loaded_image->device_handle = dev_handle;
> +
> +  if (*options)
> +    loaded_image->load_options = *options;
> +    loaded_image->load_options_size = options_len + 1;

This indentation is weird.  Did you forget { and }?  That's what this
indentation suggests.
    
>    grub_file_close (file);
>    grub_loader_set (grub_chainloader_boot, grub_chainloader_unload, 0);
> @@ -292,10 +323,7 @@
>  static void
>  grub_rescue_cmd_chainloader (int argc, char *argv[])
>  {
> -  if (argc == 0)
> -    grub_error (GRUB_ERR_BAD_ARGUMENT, "no file specified");
> -  else
> -    grub_chainloader_cmd (argv[0]);
> +  grub_chainloader_cmd (argc, argv);
>  }
>  
>  static const char loader_name[] = "chainloader";
> Index: loader/efi/chainloader_normal.c
> ===================================================================
> RCS file: /sources/grub/grub2/loader/efi/chainloader_normal.c,v
> retrieving revision 1.2
> diff -u -r1.2 chainloader_normal.c
> --- loader/efi/chainloader_normal.c	21 Jul 2007 23:32:28 -0000	1.2
> +++ loader/efi/chainloader_normal.c	3 Nov 2007 22:42:47 -0000
> @@ -26,10 +26,7 @@
>  chainloader_command (struct grub_arg_list *state __attribute__ ((unused)),
>  		     int argc, char **args)
>  {
> -  if (argc == 0)
> -    grub_error (GRUB_ERR_BAD_ARGUMENT, "no file specified");
> -  else
> -    grub_chainloader_cmd (args[0]);
> +  grub_chainloader_cmd (argc, args);
>    return grub_errno;
>  }

Did you have a look at GRUB_COMMAND_FLAG_NO_ARG_PARSE?  I think it
might be useful in your case.  It disables the argument parser so you
do not have to use "--".


> Index: kern/efi/init.c
> ===================================================================
> RCS file: /sources/grub/grub2/kern/efi/init.c,v
> retrieving revision 1.5
> diff -u -r1.5 init.c
> --- kern/efi/init.c	21 Jul 2007 23:32:26 -0000	1.5
> +++ kern/efi/init.c	3 Nov 2007 19:05:00 -0000
> @@ -53,7 +53,19 @@
>        device = grub_efidisk_get_device_name (image->device_handle);
>        file = grub_efi_get_filename (image->file_path);
>        
> -      if (device && file)
> +      if (grub_prefix[0] != '\0' && device)
> +	{
> +	  char *prefix;
> +	  prefix = grub_malloc (1 + grub_strlen (device) + 1
> +				+ grub_strlen (grub_prefix) + 1);
> +	  if (prefix)
> +	    {
> +	      grub_sprintf (prefix, "(%s)%s", device, grub_prefix);
> +	      grub_env_set ("prefix", prefix);
> +	      grub_free (prefix);
> +	    }
> +	}
> +      else if (device && file)
>  	{
>  	  char *p;
>  	  char *prefix;
>
> diff -ruNa -x CVS -x '*.mk' -x grub.efi -x '*.d' grub2_orig/commands/i386/efi/halt.c grub2/commands/i386/efi/halt.c
> --- grub2_orig/commands/i386/efi/halt.c	1970-01-01 01:00:00.000000000 +0100
> +++ grub2/commands/i386/efi/halt.c	2007-11-03 19:14:05.905493750 +0100
> @@ -0,0 +1,47 @@
> +/* halt.c - command to halt the computer.  */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2005,2007  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/normal.h>
> +#include <grub/dl.h>
> +#include <grub/arg.h>
> +#include <grub/efi/efi.h>
> +
> +static grub_err_t
> +grub_cmd_halt (struct grub_arg_list *state __attribute__ ((unused)),
> +		 int argc __attribute__ ((unused)),
> +		 char **args __attribute__ ((unused)))
> +
> +{
> +  grub_halt ();
> +  return 0;
> +}
> +
> +
> +
> +GRUB_MOD_INIT(halt)
> +{
> +  (void)mod;			/* To stop warning. */
> +  grub_register_command ("halt", grub_cmd_halt, GRUB_COMMAND_FLAG_BOTH,
> +			 "halt", "Halt the computer", 0);
> +}
> +
> +GRUB_MOD_FINI(halt)
> +{
> +  grub_unregister_command ("halt");
> +}
> diff -ruNa -x CVS -x '*.mk' -x grub.efi -x '*.d' grub2_orig/commands/i386/efi/reboot.c grub2/commands/i386/efi/reboot.c
> --- grub2_orig/commands/i386/efi/reboot.c	1970-01-01 01:00:00.000000000 +0100
> +++ grub2/commands/i386/efi/reboot.c	2007-11-03 19:14:22.850552750 +0100
> @@ -0,0 +1,47 @@
> +/* reboot.c - command to reboot the computer.  */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2005,2007  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/normal.h>
> +#include <grub/dl.h>
> +#include <grub/arg.h>
> +#include <grub/efi/efi.h>
> +
> +static grub_err_t
> +grub_cmd_reboot (struct grub_arg_list *state __attribute__ ((unused)),
> +		 int argc __attribute__ ((unused)),
> +		 char **args __attribute__ ((unused)))
> +
> +{
> +  grub_reboot ();
> +  return 0;
> +}
> +
> +
> +
> +GRUB_MOD_INIT(reboot)
> +{
> +  (void)mod;			/* To stop warning. */
> +  grub_register_command ("reboot", grub_cmd_reboot, GRUB_COMMAND_FLAG_BOTH,
> +			 "reboot", "Reboot the computer", 0);
> +}
> +
> +GRUB_MOD_FINI(reboot)
> +{
> +  grub_unregister_command ("reboot");
> +}
> diff -ruNa -x CVS -x '*.mk' -x grub.efi -x '*.d' grub2_orig/conf/i386-efi.rmk grub2/conf/i386-efi.rmk
> --- grub2_orig/conf/i386-efi.rmk	2007-10-22 21:59:32.000000000 +0200
> +++ grub2/conf/i386-efi.rmk	2007-11-03 19:15:48.259890500 +0100
> @@ -76,7 +76,7 @@
>  
>  # Modules.
>  pkgdata_MODULES = kernel.mod normal.mod _chain.mod chain.mod \
> -	_linux.mod linux.mod cpuid.mod
> +	_linux.mod linux.mod cpuid.mod halt.mod reboot.mod
>  
>  # For kernel.mod.
>  kernel_mod_EXPORTS = no
> @@ -140,4 +140,14 @@
>  cpuid_mod_CFLAGS = $(COMMON_CFLAGS)
>  cpuid_mod_LDFLAGS = $(COMMON_LDFLAGS)
>  
> +# For halt.mod.
> +halt_mod_SOURCES = commands/i386/efi/halt.c
> +halt_mod_CFLAGS = $(COMMON_CFLAGS)
> +halt_mod_LDFLAGS = $(COMMON_LDFLAGS)
> +
> +# For reboot.mod.
> +reboot_mod_SOURCES = commands/i386/efi/reboot.c
> +reboot_mod_CFLAGS = $(COMMON_CFLAGS)
> +reboot_mod_LDFLAGS = $(COMMON_LDFLAGS)
> +
>  include $(srcdir)/conf/common.mk
> diff -ruNa -x CVS -x '*.mk' -x grub.efi -x '*.d' grub2_orig/include/grub/efi/efi.h grub2/include/grub/efi/efi.h
> --- grub2_orig/include/grub/efi/efi.h	2007-07-22 01:32:23.000000000 +0200
> +++ grub2/include/grub/efi/efi.h	2007-11-03 19:12:12.762422750 +0100
> @@ -54,6 +54,8 @@
>  grub_efi_device_path_t *
>  EXPORT_FUNC(grub_efi_get_device_path) (grub_efi_handle_t handle);
>  int EXPORT_FUNC(grub_efi_exit_boot_services) (grub_efi_uintn_t map_key);
> +void EXPORT_FUNC (grub_reboot) (void);
> +void EXPORT_FUNC (grub_halt) (void);
>  
>  void grub_efi_mm_init (void);
>  void grub_efi_mm_fini (void);
> diff -ruNa -x CVS -x '*.mk' -x grub.efi -x '*.d' grub2_orig/kern/efi/efi.c grub2/kern/efi/efi.c
> --- grub2_orig/kern/efi/efi.c	2007-07-22 01:32:26.000000000 +0200
> +++ grub2/kern/efi/efi.c	2007-11-03 19:26:16.879176750 +0100
> @@ -162,6 +162,22 @@
>  					      0, 0);
>  }
>  
> +void
> +grub_reboot (void)
> +{
> +  grub_efi_fini ();
> +  grub_efi_system_table->runtime_services->reset_system (
> +      GRUB_EFI_RESET_COLD, GRUB_EFI_SUCCESS, 0, NULL);

Please move the "(" to the next line.

> +}
> +
> +void
> +grub_halt (void)
> +{
> +  grub_efi_fini ();
> +  grub_efi_system_table->runtime_services->reset_system (
> +      GRUB_EFI_RESET_SHUTDOWN, GRUB_EFI_SUCCESS, 0, NULL);
> +}

Same here.

--
Marco