All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries
@ 2022-07-19 20:39 Robbie Harwood
  2022-08-03 15:26 ` Daniel Kiper
  2022-08-08 21:43 ` Vladimir 'phcoder' Serbinenko
  0 siblings, 2 replies; 11+ messages in thread
From: Robbie Harwood @ 2022-07-19 20:39 UTC (permalink / raw)
  To: grub-devel
  Cc: Raymund Will, John Jolly, Javier Martinez Canillas,
	Robbie Harwood

From: Raymund Will <rw@suse.com>

The GRUB emulator is used as a debugging utility but it could also be used
as a user-space bootloader if there is support to boot an operating system.

The Linux kernel is already able to (re)boot another kernel via the kexec
boot mechanism. So the grub-emu tool could rely on this feature and have
linux and initrd commands that are used to pass a kernel, initramfs image
and command line parameters to kexec for booting a selected menu entry.

By default the systemctl kexec option is used so systemd can shutdown all
of the running services before doing a reboot using kexec. But if this is
not present, it fallbacks to executing the kexec user-space tool directly.

Signed-off-by: Raymund Will <rw@suse.com>
Signed-off-by: John Jolly <jjolly@suse.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 grub-core/Makefile.am        |   1 +
 grub-core/Makefile.core.def  |   2 +-
 grub-core/kern/emu/main.c    |   4 +
 grub-core/kern/emu/misc.c    |  18 +++-
 grub-core/loader/emu/linux.c | 182 +++++++++++++++++++++++++++++++++++
 include/grub/emu/exec.h      |   4 +-
 include/grub/emu/hostfile.h  |   3 +-
 include/grub/emu/misc.h      |   3 +
 8 files changed, 213 insertions(+), 4 deletions(-)
 create mode 100644 grub-core/loader/emu/linux.c

diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index ee88e44e97..80e7a83edf 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -307,6 +307,7 @@ KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/net.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostdisk.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostfile.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/extcmd.h
+KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/exec.h
 if COND_GRUB_EMU_SDL
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/sdl.h
 endif
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 7159948721..5350408601 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1816,9 +1816,9 @@ module = {
   arm64 = loader/arm64/linux.c;
   riscv32 = loader/riscv/linux.c;
   riscv64 = loader/riscv/linux.c;
+  emu = loader/emu/linux.c;
   common = loader/linux.c;
   common = lib/cmdline.c;
-  enable = noemu;
 };
 
 module = {
diff --git a/grub-core/kern/emu/main.c b/grub-core/kern/emu/main.c
index 44e087e988..b4da9d21cf 100644
--- a/grub-core/kern/emu/main.c
+++ b/grub-core/kern/emu/main.c
@@ -107,6 +107,7 @@ static struct argp_option options[] = {
    N_("use GRUB files in the directory DIR [default=%s]"), 0},
   {"verbose",     'v', 0,      0, N_("print verbose messages."), 0},
   {"hold",     'H', N_("SECS"),      OPTION_ARG_OPTIONAL, N_("wait until a debugger will attach"), 0},
+  {"kexec",       'X', 0,      0, N_("use kexec to boot Linux kernels."), 0},
   { 0, 0, 0, 0, 0, 0 }
 };
 
@@ -164,6 +165,9 @@ argp_parser (int key, char *arg, struct argp_state *state)
     case 'v':
       verbosity++;
       break;
+    case 'X':
+      grub_util_set_kexecute ();
+      break;
 
     case ARGP_KEY_ARG:
       {
diff --git a/grub-core/kern/emu/misc.c b/grub-core/kern/emu/misc.c
index d0e7a107e7..521220b49d 100644
--- a/grub-core/kern/emu/misc.c
+++ b/grub-core/kern/emu/misc.c
@@ -39,6 +39,7 @@
 #include <grub/emu/misc.h>
 
 int verbosity;
+int kexecute;
 
 void
 grub_util_warn (const char *fmt, ...)
@@ -82,7 +83,7 @@ grub_util_error (const char *fmt, ...)
   vfprintf (stderr, fmt, ap);
   va_end (ap);
   fprintf (stderr, ".\n");
-  exit (1);
+  grub_exit ();
 }
 
 void *
@@ -153,6 +154,9 @@ xasprintf (const char *fmt, ...)
 void
 grub_exit (void)
 {
+#if defined (GRUB_KERNEL)
+  grub_reboot ();
+#endif
   exit (1);
 }
 #endif
@@ -214,3 +218,15 @@ grub_util_load_image (const char *path, char *buf)
 
   fclose (fp);
 }
+
+void
+grub_util_set_kexecute (void)
+{
+  kexecute++;
+}
+
+int
+grub_util_get_kexecute (void)
+{
+  return kexecute;
+}
diff --git a/grub-core/loader/emu/linux.c b/grub-core/loader/emu/linux.c
new file mode 100644
index 0000000000..020cf56cd1
--- /dev/null
+++ b/grub-core/loader/emu/linux.c
@@ -0,0 +1,182 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2006,2007,2008,2009,2010  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/loader.h>
+#include <grub/dl.h>
+#include <grub/command.h>
+#include <grub/time.h>
+
+#include <grub/emu/exec.h>
+#include <grub/emu/hostfile.h>
+#include <grub/emu/misc.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+static grub_dl_t my_mod;
+
+static char *kernel_path;
+static char *initrd_path;
+static char *boot_cmdline;
+
+static grub_err_t
+grub_linux_boot (void)
+{
+  grub_err_t rc = GRUB_ERR_NONE;
+  char *initrd_param;
+  const char *kexec[] = { "kexec", "-l", kernel_path, boot_cmdline, NULL, NULL };
+  const char *systemctl[] = { "systemctl", "kexec", NULL };
+  int kexecute = grub_util_get_kexecute ();
+
+  if (initrd_path)
+    {
+      initrd_param = grub_xasprintf ("--initrd=%s", initrd_path);
+      kexec[3] = initrd_param;
+      kexec[4] = boot_cmdline;
+    }
+  else
+    {
+      initrd_param = grub_xasprintf ("%s", "");
+    }
+
+  grub_dprintf ("linux", "%serforming 'kexec -l %s %s %s'\n",
+                (kexecute) ? "P" : "Not p",
+                kernel_path, initrd_param, boot_cmdline);
+
+  if (kexecute)
+    rc = grub_util_exec (kexec);
+
+  grub_free(initrd_param);
+
+  if (rc != GRUB_ERR_NONE)
+    {
+      grub_error (rc, N_("Error trying to perform kexec load operation."));
+      grub_sleep (3);
+      return rc;
+    }
+
+  if (kexecute < 1)
+    grub_fatal (N_("Use '"PACKAGE"-emu --kexec' to force a system restart."));
+
+  grub_dprintf ("linux", "Performing 'systemctl kexec' (%s) ",
+		(kexecute==1) ? "do-or-die" : "just-in-case");
+  rc = grub_util_exec (systemctl);
+
+  if (rc == GRUB_ERR_NONE)
+    return rc;
+
+  grub_error (rc, N_("Error trying to perform 'systemctl kexec'"));
+
+  /* need to check read-only root before resetting hard!? */
+  grub_dprintf ("linux", "Performing 'kexec -e -x'");
+  kexec[1] = "-e";
+  kexec[2] = "-x";
+  kexec[3] = NULL;
+  rc = grub_util_exec (kexec);
+  if ( rc != GRUB_ERR_NONE )
+    grub_fatal (N_("Error trying to directly perform 'kexec -e'."));
+
+  return rc;
+}
+
+static grub_err_t
+grub_linux_unload (void)
+{
+  grub_dl_unref (my_mod);
+  if ( boot_cmdline != NULL )
+    grub_free (boot_cmdline);
+  boot_cmdline = NULL;
+  return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)), int argc, char *argv[])
+{
+  int i;
+  char *tempstr;
+
+  grub_dl_ref (my_mod);
+
+  if (argc == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+
+  if ( !grub_util_is_regular (argv[0]) )
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find kernel file %s"), argv[0]);
+
+  if ( kernel_path != NULL )
+    grub_free (kernel_path);
+
+  kernel_path = grub_xasprintf ("%s", argv[0]);
+
+  if ( boot_cmdline != NULL )
+    {
+      grub_free(boot_cmdline);
+      boot_cmdline = NULL;
+    }
+
+  if ( argc > 1 )
+    {
+      boot_cmdline = grub_xasprintf("--command-line=%s", argv[1]);
+      for ( i = 2; i < argc; i++ )
+        {
+          tempstr = grub_xasprintf("%s %s", boot_cmdline, argv[i]);
+          grub_free(boot_cmdline);
+          boot_cmdline = tempstr;
+        }
+    }
+
+  grub_loader_set (grub_linux_boot, grub_linux_unload, 0);
+
+  return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), int argc, char *argv[])
+{
+  if (argc == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
+
+  if ( !grub_util_is_regular (argv[0]) )
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find initrd file %s"), argv[0]);
+
+  if ( initrd_path != NULL )
+    grub_free (initrd_path);
+
+  initrd_path = grub_xasprintf("%s", argv[0]);
+
+  grub_dl_unref (my_mod);
+
+  return GRUB_ERR_NONE;
+}
+
+static grub_command_t cmd_linux, cmd_initrd;
+
+GRUB_MOD_INIT (linux)
+{
+  cmd_linux = grub_register_command ("linux", grub_cmd_linux, 0, N_("Load Linux."));
+  cmd_initrd = grub_register_command ("initrd", grub_cmd_initrd, 0, N_("Load initrd."));
+  my_mod = mod;
+  kernel_path = NULL;
+  initrd_path = NULL;
+  boot_cmdline = NULL;
+}
+
+GRUB_MOD_FINI (linux)
+{
+  grub_unregister_command (cmd_linux);
+  grub_unregister_command (cmd_initrd);
+}
diff --git a/include/grub/emu/exec.h b/include/grub/emu/exec.h
index d1073ef86a..1b61b4a2e5 100644
--- a/include/grub/emu/exec.h
+++ b/include/grub/emu/exec.h
@@ -23,6 +23,8 @@
 #include <stdarg.h>
 
 #include <sys/types.h>
+#include <grub/symbol.h>
+
 pid_t
 grub_util_exec_pipe (const char *const *argv, int *fd);
 pid_t
@@ -32,7 +34,7 @@ int
 grub_util_exec_redirect_all (const char *const *argv, const char *stdin_file,
 			     const char *stdout_file, const char *stderr_file);
 int
-grub_util_exec (const char *const *argv);
+EXPORT_FUNC(grub_util_exec) (const char *const *argv);
 int
 grub_util_exec_redirect (const char *const *argv, const char *stdin_file,
 			 const char *stdout_file);
diff --git a/include/grub/emu/hostfile.h b/include/grub/emu/hostfile.h
index cfb1e2b566..a61568e36e 100644
--- a/include/grub/emu/hostfile.h
+++ b/include/grub/emu/hostfile.h
@@ -22,6 +22,7 @@
 #include <grub/disk.h>
 #include <grub/partition.h>
 #include <sys/types.h>
+#include <grub/symbol.h>
 #include <grub/osdep/hostfile.h>
 
 int
@@ -29,7 +30,7 @@ grub_util_is_directory (const char *path);
 int
 grub_util_is_special_file (const char *path);
 int
-grub_util_is_regular (const char *path);
+EXPORT_FUNC(grub_util_is_regular) (const char *path);
 
 char *
 grub_util_path_concat (size_t n, ...);
diff --git a/include/grub/emu/misc.h b/include/grub/emu/misc.h
index ff9c48a649..01056954b9 100644
--- a/include/grub/emu/misc.h
+++ b/include/grub/emu/misc.h
@@ -57,6 +57,9 @@ void EXPORT_FUNC(grub_util_warn) (const char *fmt, ...) __attribute__ ((format (
 void EXPORT_FUNC(grub_util_info) (const char *fmt, ...) __attribute__ ((format (GNU_PRINTF, 1, 2)));
 void EXPORT_FUNC(grub_util_error) (const char *fmt, ...) __attribute__ ((format (GNU_PRINTF, 1, 2), noreturn));
 
+void EXPORT_FUNC(grub_util_set_kexecute) (void);
+int EXPORT_FUNC(grub_util_get_kexecute) (void) WARN_UNUSED_RESULT;
+
 grub_uint64_t EXPORT_FUNC (grub_util_get_cpu_time_ms) (void);
 
 #ifdef HAVE_DEVICE_MAPPER
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries
  2022-07-19 20:39 [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries Robbie Harwood
@ 2022-08-03 15:26 ` Daniel Kiper
  2022-08-08 18:58   ` Robbie Harwood
  2022-08-08 21:43 ` Vladimir 'phcoder' Serbinenko
  1 sibling, 1 reply; 11+ messages in thread
From: Daniel Kiper @ 2022-08-03 15:26 UTC (permalink / raw)
  To: Robbie Harwood
  Cc: grub-devel, Raymund Will, John Jolly, Javier Martinez Canillas

On Tue, Jul 19, 2022 at 04:39:34PM -0400, Robbie Harwood wrote:
> From: Raymund Will <rw@suse.com>
>
> The GRUB emulator is used as a debugging utility but it could also be used
> as a user-space bootloader if there is support to boot an operating system.
>
> The Linux kernel is already able to (re)boot another kernel via the kexec
> boot mechanism. So the grub-emu tool could rely on this feature and have
> linux and initrd commands that are used to pass a kernel, initramfs image
> and command line parameters to kexec for booting a selected menu entry.
>
> By default the systemctl kexec option is used so systemd can shutdown all
> of the running services before doing a reboot using kexec. But if this is
> not present, it fallbacks to executing the kexec user-space tool directly.
>
> Signed-off-by: Raymund Will <rw@suse.com>
> Signed-off-by: John Jolly <jjolly@suse.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> ---
>  grub-core/Makefile.am        |   1 +
>  grub-core/Makefile.core.def  |   2 +-
>  grub-core/kern/emu/main.c    |   4 +
>  grub-core/kern/emu/misc.c    |  18 +++-
>  grub-core/loader/emu/linux.c | 182 +++++++++++++++++++++++++++++++++++
>  include/grub/emu/exec.h      |   4 +-
>  include/grub/emu/hostfile.h  |   3 +-
>  include/grub/emu/misc.h      |   3 +
>  8 files changed, 213 insertions(+), 4 deletions(-)
>  create mode 100644 grub-core/loader/emu/linux.c
>
> diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
> index ee88e44e97..80e7a83edf 100644
> --- a/grub-core/Makefile.am
> +++ b/grub-core/Makefile.am
> @@ -307,6 +307,7 @@ KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/net.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostdisk.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostfile.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/extcmd.h
> +KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/exec.h
>  if COND_GRUB_EMU_SDL
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/sdl.h
>  endif
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 7159948721..5350408601 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1816,9 +1816,9 @@ module = {
>    arm64 = loader/arm64/linux.c;
>    riscv32 = loader/riscv/linux.c;
>    riscv64 = loader/riscv/linux.c;
> +  emu = loader/emu/linux.c;
>    common = loader/linux.c;
>    common = lib/cmdline.c;
> -  enable = noemu;
>  };
>
>  module = {
> diff --git a/grub-core/kern/emu/main.c b/grub-core/kern/emu/main.c
> index 44e087e988..b4da9d21cf 100644
> --- a/grub-core/kern/emu/main.c
> +++ b/grub-core/kern/emu/main.c
> @@ -107,6 +107,7 @@ static struct argp_option options[] = {
>     N_("use GRUB files in the directory DIR [default=%s]"), 0},
>    {"verbose",     'v', 0,      0, N_("print verbose messages."), 0},
>    {"hold",     'H', N_("SECS"),      OPTION_ARG_OPTIONAL, N_("wait until a debugger will attach"), 0},
> +  {"kexec",       'X', 0,      0, N_("use kexec to boot Linux kernels."), 0},
>    { 0, 0, 0, 0, 0, 0 }
>  };
>
> @@ -164,6 +165,9 @@ argp_parser (int key, char *arg, struct argp_state *state)
>      case 'v':
>        verbosity++;
>        break;
> +    case 'X':
> +      grub_util_set_kexecute ();
> +      break;
>
>      case ARGP_KEY_ARG:
>        {
> diff --git a/grub-core/kern/emu/misc.c b/grub-core/kern/emu/misc.c
> index d0e7a107e7..521220b49d 100644
> --- a/grub-core/kern/emu/misc.c
> +++ b/grub-core/kern/emu/misc.c
> @@ -39,6 +39,7 @@
>  #include <grub/emu/misc.h>
>
>  int verbosity;
> +int kexecute;
>
>  void
>  grub_util_warn (const char *fmt, ...)
> @@ -82,7 +83,7 @@ grub_util_error (const char *fmt, ...)
>    vfprintf (stderr, fmt, ap);
>    va_end (ap);
>    fprintf (stderr, ".\n");
> -  exit (1);
> +  grub_exit ();
>  }
>
>  void *
> @@ -153,6 +154,9 @@ xasprintf (const char *fmt, ...)
>  void
>  grub_exit (void)
>  {
> +#if defined (GRUB_KERNEL)
> +  grub_reboot ();
> +#endif
>    exit (1);
>  }
>  #endif
> @@ -214,3 +218,15 @@ grub_util_load_image (const char *path, char *buf)
>
>    fclose (fp);
>  }
> +
> +void
> +grub_util_set_kexecute (void)
> +{
> +  kexecute++;
> +}
> +
> +int
> +grub_util_get_kexecute (void)
> +{
> +  return kexecute;
> +}
> diff --git a/grub-core/loader/emu/linux.c b/grub-core/loader/emu/linux.c
> new file mode 100644
> index 0000000000..020cf56cd1
> --- /dev/null
> +++ b/grub-core/loader/emu/linux.c
> @@ -0,0 +1,182 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2006,2007,2008,2009,2010  Free Software Foundation, Inc.

s/2006,2007,2008,2009,2010/2022/

> + *  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/>.

Please add a blurb, after an empty line, saying what this code does.
You can find good example in grub-core/kern/efi/sb.c.

> + */
> +
> +#include <grub/loader.h>
> +#include <grub/dl.h>
> +#include <grub/command.h>
> +#include <grub/time.h>
> +
> +#include <grub/emu/exec.h>
> +#include <grub/emu/hostfile.h>
> +#include <grub/emu/misc.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static grub_dl_t my_mod;
> +
> +static char *kernel_path;
> +static char *initrd_path;
> +static char *boot_cmdline;
> +
> +static grub_err_t
> +grub_linux_boot (void)
> +{
> +  grub_err_t rc = GRUB_ERR_NONE;
> +  char *initrd_param;
> +  const char *kexec[] = { "kexec", "-l", kernel_path, boot_cmdline, NULL, NULL };
> +  const char *systemctl[] = { "systemctl", "kexec", NULL };

I would prefer if we do not hardcode these commands. E.g. kexec
command has many options which can be useful for debugging. If we
hardcode the command here we cannot use these options.

> +  int kexecute = grub_util_get_kexecute ();
> +
> +  if (initrd_path)
> +    {
> +      initrd_param = grub_xasprintf ("--initrd=%s", initrd_path);
> +      kexec[3] = initrd_param;
> +      kexec[4] = boot_cmdline;
> +    }
> +  else
> +    {
> +      initrd_param = grub_xasprintf ("%s", "");
> +    }

Please drop these curly brackets.

> +  grub_dprintf ("linux", "%serforming 'kexec -l %s %s %s'\n",
> +                (kexecute) ? "P" : "Not p",
> +                kernel_path, initrd_param, boot_cmdline);
> +
> +  if (kexecute)
> +    rc = grub_util_exec (kexec);
> +
> +  grub_free(initrd_param);

Missing space before "(".

> +  if (rc != GRUB_ERR_NONE)
> +    {
> +      grub_error (rc, N_("Error trying to perform kexec load operation."));
> +      grub_sleep (3);
> +      return rc;
> +    }
> +
> +  if (kexecute < 1)

s/kexecute < 1/kexecute/

> +    grub_fatal (N_("Use '"PACKAGE"-emu --kexec' to force a system restart."));
> +
> +  grub_dprintf ("linux", "Performing 'systemctl kexec' (%s) ",
> +		(kexecute==1) ? "do-or-die" : "just-in-case");

s/kexecute==1/kexecute/

Please be more consistent how you check kexecute.

> +  rc = grub_util_exec (systemctl);
> +
> +  if (rc == GRUB_ERR_NONE)
> +    return rc;
> +
> +  grub_error (rc, N_("Error trying to perform 'systemctl kexec'"));
> +
> +  /* need to check read-only root before resetting hard!? */
> +  grub_dprintf ("linux", "Performing 'kexec -e -x'");

I would really do not fall back to 'kexec -e' by default. It is too
dangerous. And again I would not hardcode this command too.

> +  kexec[1] = "-e";
> +  kexec[2] = "-x";
> +  kexec[3] = NULL;
> +  rc = grub_util_exec (kexec);
> +  if ( rc != GRUB_ERR_NONE )
> +    grub_fatal (N_("Error trying to directly perform 'kexec -e'."));
> +
> +  return rc;
> +}
> +
> +static grub_err_t
> +grub_linux_unload (void)
> +{
> +  grub_dl_unref (my_mod);
> +  if ( boot_cmdline != NULL )

You can drop this if.

> +    grub_free (boot_cmdline);

What about kernel_path and initrd_path?

> +  boot_cmdline = NULL;

Module will be destroyed. So, I do not think this is needed.

> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)), int argc, char *argv[])
> +{
> +  int i;
> +  char *tempstr;
> +
> +  grub_dl_ref (my_mod);
> +
> +  if (argc == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +
> +  if ( !grub_util_is_regular (argv[0]) )

Too many spaces...

> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find kernel file %s"), argv[0]);

s/Cannot/cannot/

> +  if ( kernel_path != NULL )

You can drop this if. Of course everything should be initialized to NULL
above.

> +    grub_free (kernel_path);
> +
> +  kernel_path = grub_xasprintf ("%s", argv[0]);
> +
> +  if ( boot_cmdline != NULL )

Again, too many spaces... But this if seems redundant too.

> +    {
> +      grub_free(boot_cmdline);

Missing space before "("... :-)

> +      boot_cmdline = NULL;
> +    }
> +
> +  if ( argc > 1 )

Ditto and below please...

> +    {
> +      boot_cmdline = grub_xasprintf("--command-line=%s", argv[1]);
> +      for ( i = 2; i < argc; i++ )
> +        {
> +          tempstr = grub_xasprintf("%s %s", boot_cmdline, argv[i]);
> +          grub_free(boot_cmdline);
> +          boot_cmdline = tempstr;
> +        }
> +    }
> +
> +  grub_loader_set (grub_linux_boot, grub_linux_unload, 0);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), int argc, char *argv[])
> +{
> +  if (argc == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +
> +  if ( !grub_util_is_regular (argv[0]) )
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find initrd file %s"), argv[0]);

s/Cannot/cannot/ All error messages should start with lowercase.

> +  if ( initrd_path != NULL )
> +    grub_free (initrd_path);

Again, redundant if...

> +
> +  initrd_path = grub_xasprintf("%s", argv[0]);
> +
> +  grub_dl_unref (my_mod);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_command_t cmd_linux, cmd_initrd;
> +
> +GRUB_MOD_INIT (linux)
> +{
> +  cmd_linux = grub_register_command ("linux", grub_cmd_linux, 0, N_("Load Linux."));
> +  cmd_initrd = grub_register_command ("initrd", grub_cmd_initrd, 0, N_("Load initrd."));
> +  my_mod = mod;
> +  kernel_path = NULL;
> +  initrd_path = NULL;
> +  boot_cmdline = NULL;

This can be done in variable definitions above.

Daniel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries
  2022-08-03 15:26 ` Daniel Kiper
@ 2022-08-08 18:58   ` Robbie Harwood
  2022-08-11 18:08     ` Daniel Kiper
  0 siblings, 1 reply; 11+ messages in thread
From: Robbie Harwood @ 2022-08-08 18:58 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: grub-devel, Raymund Will, John Jolly, Javier Martinez Canillas

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

Daniel Kiper <dkiper@net-space.pl> writes:

> On Tue, Jul 19, 2022 at 04:39:34PM -0400, Robbie Harwood wrote:
>
>> +static grub_err_t
>> +grub_linux_boot (void)
>> +{
>> +  grub_err_t rc = GRUB_ERR_NONE;
>> +  char *initrd_param;
>> +  const char *kexec[] = { "kexec", "-l", kernel_path, boot_cmdline, NULL, NULL };
>> +  const char *systemctl[] = { "systemctl", "kexec", NULL };
>
> I would prefer if we do not hardcode these commands. E.g. kexec
> command has many options which can be useful for debugging. If we
> hardcode the command here we cannot use these options.

Can you clarify what you would like to see instead?  I'm not sure what
the alternative would be.

>> +  rc = grub_util_exec (systemctl);
>> +
>> +  if (rc == GRUB_ERR_NONE)
>> +    return rc;
>> +
>> +  grub_error (rc, N_("Error trying to perform 'systemctl kexec'"));
>> +
>> +  /* need to check read-only root before resetting hard!? */
>> +  grub_dprintf ("linux", "Performing 'kexec -e -x'");
>
> I would really do not fall back to 'kexec -e' by default. It is too
> dangerous. And again I would not hardcode this command too.

Same question as above regarding the alternative... also, can you
elaborate on the danger you see here?

>> +    grub_fatal (N_("Use '"PACKAGE"-emu --kexec' to force a system restart."));
>> +
>> +  grub_dprintf ("linux", "Performing 'systemctl kexec' (%s) ",
>> +		(kexecute==1) ? "do-or-die" : "just-in-case");
>
> s/kexecute==1/kexecute/
>
> Please be more consistent how you check kexecute.

None of this is my code yet - I just rebased the existing patch - but
I will make these and other requested changes :)  Thanks for the review;
I'll cut another version once we resolve the conversations above.

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries
  2022-07-19 20:39 [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries Robbie Harwood
  2022-08-03 15:26 ` Daniel Kiper
@ 2022-08-08 21:43 ` Vladimir 'phcoder' Serbinenko
  2022-08-11 18:02   ` Robbie Harwood
  2022-08-11 18:13   ` Daniel Kiper
  1 sibling, 2 replies; 11+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2022-08-08 21:43 UTC (permalink / raw)
  To: The development of GNU GRUB

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

I like it in general however I had a comment: in future GRUB could be able
to do the same through second approach: load a constructed kexec blob with
all the parts. This would allow to e.g. kexec FreeBSD. This didn't have to
be implemented now. Meanwhile can we use "kexec" as command name? It's fine
to have aliases "linux" and co, just let's have a command line that won't
be broken if second approach becomes a reality

Le mar. 19 juil. 2022, 22:40, Robbie Harwood <rharwood@redhat.com> a écrit :

> From: Raymund Will <rw@suse.com>
>
> The GRUB emulator is used as a debugging utility but it could also be used
> as a user-space bootloader if there is support to boot an operating system.
>
> The Linux kernel is already able to (re)boot another kernel via the kexec
> boot mechanism. So the grub-emu tool could rely on this feature and have
> linux and initrd commands that are used to pass a kernel, initramfs image
> and command line parameters to kexec for booting a selected menu entry.
>
> By default the systemctl kexec option is used so systemd can shutdown all
> of the running services before doing a reboot using kexec. But if this is
> not present, it fallbacks to executing the kexec user-space tool directly.
>
> Signed-off-by: Raymund Will <rw@suse.com>
> Signed-off-by: John Jolly <jjolly@suse.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> ---
>  grub-core/Makefile.am        |   1 +
>  grub-core/Makefile.core.def  |   2 +-
>  grub-core/kern/emu/main.c    |   4 +
>  grub-core/kern/emu/misc.c    |  18 +++-
>  grub-core/loader/emu/linux.c | 182 +++++++++++++++++++++++++++++++++++
>  include/grub/emu/exec.h      |   4 +-
>  include/grub/emu/hostfile.h  |   3 +-
>  include/grub/emu/misc.h      |   3 +
>  8 files changed, 213 insertions(+), 4 deletions(-)
>  create mode 100644 grub-core/loader/emu/linux.c
>
> diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
> index ee88e44e97..80e7a83edf 100644
> --- a/grub-core/Makefile.am
> +++ b/grub-core/Makefile.am
> @@ -307,6 +307,7 @@ KERNEL_HEADER_FILES +=
> $(top_srcdir)/include/grub/emu/net.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostdisk.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/hostfile.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/extcmd.h
> +KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/exec.h
>  if COND_GRUB_EMU_SDL
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/sdl.h
>  endif
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 7159948721..5350408601 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1816,9 +1816,9 @@ module = {
>    arm64 = loader/arm64/linux.c;
>    riscv32 = loader/riscv/linux.c;
>    riscv64 = loader/riscv/linux.c;
> +  emu = loader/emu/linux.c;
>    common = loader/linux.c;
>    common = lib/cmdline.c;
> -  enable = noemu;
>  };
>
>  module = {
> diff --git a/grub-core/kern/emu/main.c b/grub-core/kern/emu/main.c
> index 44e087e988..b4da9d21cf 100644
> --- a/grub-core/kern/emu/main.c
> +++ b/grub-core/kern/emu/main.c
> @@ -107,6 +107,7 @@ static struct argp_option options[] = {
>     N_("use GRUB files in the directory DIR [default=%s]"), 0},
>    {"verbose",     'v', 0,      0, N_("print verbose messages."), 0},
>    {"hold",     'H', N_("SECS"),      OPTION_ARG_OPTIONAL, N_("wait until
> a debugger will attach"), 0},
> +  {"kexec",       'X', 0,      0, N_("use kexec to boot Linux kernels."),
> 0},
>    { 0, 0, 0, 0, 0, 0 }
>  };
>
> @@ -164,6 +165,9 @@ argp_parser (int key, char *arg, struct argp_state
> *state)
>      case 'v':
>        verbosity++;
>        break;
> +    case 'X':
> +      grub_util_set_kexecute ();
> +      break;
>
>      case ARGP_KEY_ARG:
>        {
> diff --git a/grub-core/kern/emu/misc.c b/grub-core/kern/emu/misc.c
> index d0e7a107e7..521220b49d 100644
> --- a/grub-core/kern/emu/misc.c
> +++ b/grub-core/kern/emu/misc.c
> @@ -39,6 +39,7 @@
>  #include <grub/emu/misc.h>
>
>  int verbosity;
> +int kexecute;
>
>  void
>  grub_util_warn (const char *fmt, ...)
> @@ -82,7 +83,7 @@ grub_util_error (const char *fmt, ...)
>    vfprintf (stderr, fmt, ap);
>    va_end (ap);
>    fprintf (stderr, ".\n");
> -  exit (1);
> +  grub_exit ();
>  }
>
>  void *
> @@ -153,6 +154,9 @@ xasprintf (const char *fmt, ...)
>  void
>  grub_exit (void)
>  {
> +#if defined (GRUB_KERNEL)
> +  grub_reboot ();
> +#endif
>    exit (1);
>  }
>  #endif
> @@ -214,3 +218,15 @@ grub_util_load_image (const char *path, char *buf)
>
>    fclose (fp);
>  }
> +
> +void
> +grub_util_set_kexecute (void)
> +{
> +  kexecute++;
> +}
> +
> +int
> +grub_util_get_kexecute (void)
> +{
> +  return kexecute;
> +}
> diff --git a/grub-core/loader/emu/linux.c b/grub-core/loader/emu/linux.c
> new file mode 100644
> index 0000000000..020cf56cd1
> --- /dev/null
> +++ b/grub-core/loader/emu/linux.c
> @@ -0,0 +1,182 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2006,2007,2008,2009,2010  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/loader.h>
> +#include <grub/dl.h>
> +#include <grub/command.h>
> +#include <grub/time.h>
> +
> +#include <grub/emu/exec.h>
> +#include <grub/emu/hostfile.h>
> +#include <grub/emu/misc.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static grub_dl_t my_mod;
> +
> +static char *kernel_path;
> +static char *initrd_path;
> +static char *boot_cmdline;
> +
> +static grub_err_t
> +grub_linux_boot (void)
> +{
> +  grub_err_t rc = GRUB_ERR_NONE;
> +  char *initrd_param;
> +  const char *kexec[] = { "kexec", "-l", kernel_path, boot_cmdline, NULL,
> NULL };
> +  const char *systemctl[] = { "systemctl", "kexec", NULL };
> +  int kexecute = grub_util_get_kexecute ();
> +
> +  if (initrd_path)
> +    {
> +      initrd_param = grub_xasprintf ("--initrd=%s", initrd_path);
> +      kexec[3] = initrd_param;
> +      kexec[4] = boot_cmdline;
> +    }
> +  else
> +    {
> +      initrd_param = grub_xasprintf ("%s", "");
> +    }
> +
> +  grub_dprintf ("linux", "%serforming 'kexec -l %s %s %s'\n",
> +                (kexecute) ? "P" : "Not p",
> +                kernel_path, initrd_param, boot_cmdline);
> +
> +  if (kexecute)
> +    rc = grub_util_exec (kexec);
> +
> +  grub_free(initrd_param);
> +
> +  if (rc != GRUB_ERR_NONE)
> +    {
> +      grub_error (rc, N_("Error trying to perform kexec load
> operation."));
> +      grub_sleep (3);
> +      return rc;
> +    }
> +
> +  if (kexecute < 1)
> +    grub_fatal (N_("Use '"PACKAGE"-emu --kexec' to force a system
> restart."));
> +
> +  grub_dprintf ("linux", "Performing 'systemctl kexec' (%s) ",
> +               (kexecute==1) ? "do-or-die" : "just-in-case");
> +  rc = grub_util_exec (systemctl);
> +
> +  if (rc == GRUB_ERR_NONE)
> +    return rc;
> +
> +  grub_error (rc, N_("Error trying to perform 'systemctl kexec'"));
> +
> +  /* need to check read-only root before resetting hard!? */
> +  grub_dprintf ("linux", "Performing 'kexec -e -x'");
> +  kexec[1] = "-e";
> +  kexec[2] = "-x";
> +  kexec[3] = NULL;
> +  rc = grub_util_exec (kexec);
> +  if ( rc != GRUB_ERR_NONE )
> +    grub_fatal (N_("Error trying to directly perform 'kexec -e'."));
> +
> +  return rc;
> +}
> +
> +static grub_err_t
> +grub_linux_unload (void)
> +{
> +  grub_dl_unref (my_mod);
> +  if ( boot_cmdline != NULL )
> +    grub_free (boot_cmdline);
> +  boot_cmdline = NULL;
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)), int argc,
> char *argv[])
> +{
> +  int i;
> +  char *tempstr;
> +
> +  grub_dl_ref (my_mod);
> +
> +  if (argc == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +
> +  if ( !grub_util_is_regular (argv[0]) )
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find kernel
> file %s"), argv[0]);
> +
> +  if ( kernel_path != NULL )
> +    grub_free (kernel_path);
> +
> +  kernel_path = grub_xasprintf ("%s", argv[0]);
> +
> +  if ( boot_cmdline != NULL )
> +    {
> +      grub_free(boot_cmdline);
> +      boot_cmdline = NULL;
> +    }
> +
> +  if ( argc > 1 )
> +    {
> +      boot_cmdline = grub_xasprintf("--command-line=%s", argv[1]);
> +      for ( i = 2; i < argc; i++ )
> +        {
> +          tempstr = grub_xasprintf("%s %s", boot_cmdline, argv[i]);
> +          grub_free(boot_cmdline);
> +          boot_cmdline = tempstr;
> +        }
> +    }
> +
> +  grub_loader_set (grub_linux_boot, grub_linux_unload, 0);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), int argc,
> char *argv[])
> +{
> +  if (argc == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> +
> +  if ( !grub_util_is_regular (argv[0]) )
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("Cannot find initrd
> file %s"), argv[0]);
> +
> +  if ( initrd_path != NULL )
> +    grub_free (initrd_path);
> +
> +  initrd_path = grub_xasprintf("%s", argv[0]);
> +
> +  grub_dl_unref (my_mod);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_command_t cmd_linux, cmd_initrd;
> +
> +GRUB_MOD_INIT (linux)
> +{
> +  cmd_linux = grub_register_command ("linux", grub_cmd_linux, 0, N_("Load
> Linux."));
> +  cmd_initrd = grub_register_command ("initrd", grub_cmd_initrd, 0,
> N_("Load initrd."));
> +  my_mod = mod;
> +  kernel_path = NULL;
> +  initrd_path = NULL;
> +  boot_cmdline = NULL;
> +}
> +
> +GRUB_MOD_FINI (linux)
> +{
> +  grub_unregister_command (cmd_linux);
> +  grub_unregister_command (cmd_initrd);
> +}
> diff --git a/include/grub/emu/exec.h b/include/grub/emu/exec.h
> index d1073ef86a..1b61b4a2e5 100644
> --- a/include/grub/emu/exec.h
> +++ b/include/grub/emu/exec.h
> @@ -23,6 +23,8 @@
>  #include <stdarg.h>
>
>  #include <sys/types.h>
> +#include <grub/symbol.h>
> +
>  pid_t
>  grub_util_exec_pipe (const char *const *argv, int *fd);
>  pid_t
> @@ -32,7 +34,7 @@ int
>  grub_util_exec_redirect_all (const char *const *argv, const char
> *stdin_file,
>                              const char *stdout_file, const char
> *stderr_file);
>  int
> -grub_util_exec (const char *const *argv);
> +EXPORT_FUNC(grub_util_exec) (const char *const *argv);
>  int
>  grub_util_exec_redirect (const char *const *argv, const char *stdin_file,
>                          const char *stdout_file);
> diff --git a/include/grub/emu/hostfile.h b/include/grub/emu/hostfile.h
> index cfb1e2b566..a61568e36e 100644
> --- a/include/grub/emu/hostfile.h
> +++ b/include/grub/emu/hostfile.h
> @@ -22,6 +22,7 @@
>  #include <grub/disk.h>
>  #include <grub/partition.h>
>  #include <sys/types.h>
> +#include <grub/symbol.h>
>  #include <grub/osdep/hostfile.h>
>
>  int
> @@ -29,7 +30,7 @@ grub_util_is_directory (const char *path);
>  int
>  grub_util_is_special_file (const char *path);
>  int
> -grub_util_is_regular (const char *path);
> +EXPORT_FUNC(grub_util_is_regular) (const char *path);
>
>  char *
>  grub_util_path_concat (size_t n, ...);
> diff --git a/include/grub/emu/misc.h b/include/grub/emu/misc.h
> index ff9c48a649..01056954b9 100644
> --- a/include/grub/emu/misc.h
> +++ b/include/grub/emu/misc.h
> @@ -57,6 +57,9 @@ void EXPORT_FUNC(grub_util_warn) (const char *fmt, ...)
> __attribute__ ((format (
>  void EXPORT_FUNC(grub_util_info) (const char *fmt, ...) __attribute__
> ((format (GNU_PRINTF, 1, 2)));
>  void EXPORT_FUNC(grub_util_error) (const char *fmt, ...) __attribute__
> ((format (GNU_PRINTF, 1, 2), noreturn));
>
> +void EXPORT_FUNC(grub_util_set_kexecute) (void);
> +int EXPORT_FUNC(grub_util_get_kexecute) (void) WARN_UNUSED_RESULT;
> +
>  grub_uint64_t EXPORT_FUNC (grub_util_get_cpu_time_ms) (void);
>
>  #ifdef HAVE_DEVICE_MAPPER
> --
> 2.35.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries
  2022-08-08 21:43 ` Vladimir 'phcoder' Serbinenko
@ 2022-08-11 18:02   ` Robbie Harwood
  2022-08-11 18:13   ` Daniel Kiper
  1 sibling, 0 replies; 11+ messages in thread
From: Robbie Harwood @ 2022-08-11 18:02 UTC (permalink / raw)
  To: Vladimir 'phcoder' Serbinenko,
	The development of GNU GRUB

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

"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> writes:

> I like it in general however I had a comment: in future GRUB could be able
> to do the same through second approach: load a constructed kexec blob with
> all the parts. This would allow to e.g. kexec FreeBSD. This didn't have to
> be implemented now. Meanwhile can we use "kexec" as command name? It's fine
> to have aliases "linux" and co, just let's have a command line that won't
> be broken if second approach becomes a reality

I don't think I follow.  In this hypothetical future, couldn't we just
rename this code to something else and avoid the problem entirely?  But
maybe I'm misunderstanding.

In any case, I don't see support for aliases anywhere, so I'm not sure
what you're referring to.

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries
  2022-08-08 18:58   ` Robbie Harwood
@ 2022-08-11 18:08     ` Daniel Kiper
  2022-08-15 13:16       ` Raymund Will
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Kiper @ 2022-08-11 18:08 UTC (permalink / raw)
  To: Robbie Harwood
  Cc: grub-devel, Raymund Will, John Jolly, Javier Martinez Canillas

On Mon, Aug 08, 2022 at 02:58:06PM -0400, Robbie Harwood wrote:
> Daniel Kiper <dkiper@net-space.pl> writes:
>
> > On Tue, Jul 19, 2022 at 04:39:34PM -0400, Robbie Harwood wrote:
> >
> >> +static grub_err_t
> >> +grub_linux_boot (void)
> >> +{
> >> +  grub_err_t rc = GRUB_ERR_NONE;
> >> +  char *initrd_param;
> >> +  const char *kexec[] = { "kexec", "-l", kernel_path, boot_cmdline, NULL, NULL };
> >> +  const char *systemctl[] = { "systemctl", "kexec", NULL };
> >
> > I would prefer if we do not hardcode these commands. E.g. kexec
> > command has many options which can be useful for debugging. If we
> > hardcode the command here we cannot use these options.
>
> Can you clarify what you would like to see instead?  I'm not sure what
> the alternative would be.

E.g. grub.cfg could contain:

set kexec_load_cmd=/sbin/kexec
set kexec_load_opts='-l'

Then GRUB should replace "linux" command with combined kexec_load_cmd,
kexec_load_opts, boot_cmdline, etc. Of course we can assume some reasonable
defaults if kexec_load_cmd and/or kexec_load_opts variables are not present.

> >> +  rc = grub_util_exec (systemctl);
> >> +
> >> +  if (rc == GRUB_ERR_NONE)
> >> +    return rc;
> >> +
> >> +  grub_error (rc, N_("Error trying to perform 'systemctl kexec'"));
> >> +
> >> +  /* need to check read-only root before resetting hard!? */
> >> +  grub_dprintf ("linux", "Performing 'kexec -e -x'");
> >
> > I would really do not fall back to 'kexec -e' by default. It is too
> > dangerous. And again I would not hardcode this command too.
>
> Same question as above regarding the alternative... also, can you

set kexec_exec_cmd=/bin/systemctl
set kexec_exec_opts=kexec

set kexec_exec_force_cmd=/sbin/kexec
set kexec_exec_force_opts='-e -x'

Etc...

Maybe names are not perfect but I hope you know what I mean...

> elaborate on the danger you see here?

The "kexec -e" executes the new kernel immediately without unmounting/remounting
filesystems, etc. So, ... I think this feature should be disabled by default.
If somebody knows what is doing then they should be able to enable it, e.g.,
by setting a GRUB env variable.

> >> +    grub_fatal (N_("Use '"PACKAGE"-emu --kexec' to force a system restart."));
> >> +
> >> +  grub_dprintf ("linux", "Performing 'systemctl kexec' (%s) ",
> >> +		(kexecute==1) ? "do-or-die" : "just-in-case");
> >
> > s/kexecute==1/kexecute/
> >
> > Please be more consistent how you check kexecute.
>
> None of this is my code yet - I just rebased the existing patch - but
> I will make these and other requested changes :)  Thanks for the review;
> I'll cut another version once we resolve the conversations above.

Cool! Thank you!

Please do not forget about docs... :-)

Daniel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries
  2022-08-08 21:43 ` Vladimir 'phcoder' Serbinenko
  2022-08-11 18:02   ` Robbie Harwood
@ 2022-08-11 18:13   ` Daniel Kiper
  1 sibling, 0 replies; 11+ messages in thread
From: Daniel Kiper @ 2022-08-11 18:13 UTC (permalink / raw)
  To: Vladimir 'phcoder' Serbinenko; +Cc: The development of GNU GRUB

On Mon, Aug 08, 2022 at 11:43:15PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> I like it in general however I had a comment: in future GRUB could be able to
> do the same through second approach: load a constructed kexec blob with all the
> parts. This would allow to e.g. kexec FreeBSD. This didn't have to be

I do not fully understand what you mean by "load a constructed kexec blob
with all the parts". Could you elaborate on this?

Daniel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries
  2022-08-11 18:08     ` Daniel Kiper
@ 2022-08-15 13:16       ` Raymund Will
  2022-08-16 16:07         ` Robbie Harwood
  0 siblings, 1 reply; 11+ messages in thread
From: Raymund Will @ 2022-08-15 13:16 UTC (permalink / raw)
  To: Daniel Kiper
  Cc: Robbie Harwood, grub-devel, John Jolly, Javier Martinez Canillas

Hi,

please let me try to add a bit of context and explain three IMHO
crucial points of the proposed patch.

First, it is meant to work without any changes to config-files
on 'linux'-systems, simply by calling `grub-emu --kexec`.
And, called this way, it actually uses `systemctl kexec` exclusively
to instruct `systemd` to perform the "kexec"-reboot in a sane and
safe manner.

Second, it only supports a very limited set of commands in `grub.cfg`,
as `grub-emu` can not implement the full functionality of a firmware-
loaded `grub` binary (like raw-device access) and it hinges massively
on proper `kexec`-support from the machine/firmware, so unfortunately
it won't be universally useful,

Third, for use in a "pre-boot environment" (i.e. initrd/chroot), which
has full control over it's state, but no (fully) working `systemd`,
a call to `grub-emu --kexec --kexec` changes the behavior to allow a
fall-forward to `kexec -e`.  As a safe-guard for the adventurously
minded `systemctl kexec` is still tried first!

This third point describes the use-case the original patch-set was
developed for and the second doesn't hurt (much) on the systems it
is deployed/used in the field.  But the first was found to be really
useful, especially on machines, which can reliably `kexec`, but are
dead slow through cold-boot (think "huge memory", "tons of devices")
and/or have "exotic" console setups ("3215" anybody?).  Note that,
as the boot configuration (namely `grub.cfg`) wasn't changed, regular
boot can't be affected by this short-cut (particularly, when `kexec`
might have failed).

Granted, the duplication of `--kexec` to signify "force it", might
as well be spelled out as `--force-kexec` (or something similar).
(But that change will provoke inconsistencies during an indefinite
migration phase, where pre-boot images don't match binaries in the
root filesystem, notably, when rollback snapshots come into play.)

Config-overrides in `grub.cfg` in turn would be a nice addition, but
are relatively expensive to implement, as they'd probably need to be
parsed and split into an array for `grub_util_exec()`, right?

But, please, still leave sane defaults in the binary, for out-of-
the-box, no-config-changes-necessary usage, pretty please!

Would it be possible to re-evaluate the proposed patch with this
in mind?

PS: in light of my statements above, the "description" of this
patch definitely needs re-wording...

Thanks,
-- 
Raymund WILL                                                  rw@SUSE.de
SUSE Software Solutions Germany GmbH, Frankenstr. 146, D-90461 Nuernberg
Geschaeftsfuehrer: Ivo Totev, et al            (HRB 36809, AG Nuernberg)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries
  2022-08-15 13:16       ` Raymund Will
@ 2022-08-16 16:07         ` Robbie Harwood
  2022-08-20 11:23           ` Daniel Kiper
  0 siblings, 1 reply; 11+ messages in thread
From: Robbie Harwood @ 2022-08-16 16:07 UTC (permalink / raw)
  To: Raymund Will, Daniel Kiper
  Cc: grub-devel, John Jolly, Javier Martinez Canillas

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

Raymund Will <rw@suse.de> writes:

> Hi,
>
> please let me try to add a bit of context and explain three IMHO
> crucial points of the proposed patch.
>
> First, it is meant to work without any changes to config-files
> on 'linux'-systems, simply by calling `grub-emu --kexec`.
> And, called this way, it actually uses `systemctl kexec` exclusively
> to instruct `systemd` to perform the "kexec"-reboot in a sane and
> safe manner.
>
> Second, it only supports a very limited set of commands in `grub.cfg`,
> as `grub-emu` can not implement the full functionality of a firmware-
> loaded `grub` binary (like raw-device access) and it hinges massively
> on proper `kexec`-support from the machine/firmware, so unfortunately
> it won't be universally useful,
>
> Third, for use in a "pre-boot environment" (i.e. initrd/chroot), which
> has full control over it's state, but no (fully) working `systemd`,
> a call to `grub-emu --kexec --kexec` changes the behavior to allow a
> fall-forward to `kexec -e`.  As a safe-guard for the adventurously
> minded `systemctl kexec` is still tried first!
>
> This third point describes the use-case the original patch-set was
> developed for and the second doesn't hurt (much) on the systems it
> is deployed/used in the field.  But the first was found to be really
> useful, especially on machines, which can reliably `kexec`, but are
> dead slow through cold-boot (think "huge memory", "tons of devices")
> and/or have "exotic" console setups ("3215" anybody?).  Note that,
> as the boot configuration (namely `grub.cfg`) wasn't changed, regular
> boot can't be affected by this short-cut (particularly, when `kexec`
> might have failed).
>
> Granted, the duplication of `--kexec` to signify "force it", might
> as well be spelled out as `--force-kexec` (or something similar).
> (But that change will provoke inconsistencies during an indefinite
> migration phase, where pre-boot images don't match binaries in the
> root filesystem, notably, when rollback snapshots come into play.)

Passing --kexec twice (or --force-kexec) doesn't appear to change
anything in the versions of this patch I can easily find.  We could add
the behavior you're describing though - Daniel, would that help with
your concerns about it?

> Config-overrides in `grub.cfg` in turn would be a nice addition, but
> are relatively expensive to implement, as they'd probably need to be
> parsed and split into an array for `grub_util_exec()`, right?

Yes.  It's inevitably best-effort, especially if we can't depend on a
working shell.

> But, please, still leave sane defaults in the binary, for out-of-
> the-box, no-config-changes-necessary usage, pretty please!
>
> Would it be possible to re-evaluate the proposed patch with this
> in mind?
>
> PS: in light of my statements above, the "description" of this
> patch definitely needs re-wording...

Any interest in doing that, or should I?

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries
  2022-08-16 16:07         ` Robbie Harwood
@ 2022-08-20 11:23           ` Daniel Kiper
  2022-08-23 21:03             ` Robbie Harwood
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Kiper @ 2022-08-20 11:23 UTC (permalink / raw)
  To: Robbie Harwood; +Cc: Raymund Will, grub-devel, Javier Martinez Canillas

On Tue, Aug 16, 2022 at 12:07:06PM -0400, Robbie Harwood wrote:
> Raymund Will <rw@suse.de> writes:
>
> > Hi,
> >
> > please let me try to add a bit of context and explain three IMHO
> > crucial points of the proposed patch.
> >
> > First, it is meant to work without any changes to config-files
> > on 'linux'-systems, simply by calling `grub-emu --kexec`.
> > And, called this way, it actually uses `systemctl kexec` exclusively
> > to instruct `systemd` to perform the "kexec"-reboot in a sane and
> > safe manner.
> >
> > Second, it only supports a very limited set of commands in `grub.cfg`,
> > as `grub-emu` can not implement the full functionality of a firmware-
> > loaded `grub` binary (like raw-device access) and it hinges massively
> > on proper `kexec`-support from the machine/firmware, so unfortunately
> > it won't be universally useful,
> >
> > Third, for use in a "pre-boot environment" (i.e. initrd/chroot), which
> > has full control over it's state, but no (fully) working `systemd`,
> > a call to `grub-emu --kexec --kexec` changes the behavior to allow a
> > fall-forward to `kexec -e`.  As a safe-guard for the adventurously
> > minded `systemctl kexec` is still tried first!
> >
> > This third point describes the use-case the original patch-set was
> > developed for and the second doesn't hurt (much) on the systems it
> > is deployed/used in the field.  But the first was found to be really
> > useful, especially on machines, which can reliably `kexec`, but are
> > dead slow through cold-boot (think "huge memory", "tons of devices")
> > and/or have "exotic" console setups ("3215" anybody?).  Note that,
> > as the boot configuration (namely `grub.cfg`) wasn't changed, regular
> > boot can't be affected by this short-cut (particularly, when `kexec`
> > might have failed).
> >
> > Granted, the duplication of `--kexec` to signify "force it", might
> > as well be spelled out as `--force-kexec` (or something similar).
> > (But that change will provoke inconsistencies during an indefinite
> > migration phase, where pre-boot images don't match binaries in the
> > root filesystem, notably, when rollback snapshots come into play.)
>
> Passing --kexec twice (or --force-kexec) doesn't appear to change
> anything in the versions of this patch I can easily find.  We could add

Yeah, I think Raymund is talking about a bit different version of the
patch. Raymund, could you provide us the one which has that features,
and potentially others, implemented?

> the behavior you're describing though - Daniel, would that help with
> your concerns about it?

I would prefer --force-kexec but if double --kexec is used in existing
environments I am OK with the latter. However, please document this
behavior in the GRUB's docs.

> > Config-overrides in `grub.cfg` in turn would be a nice addition, but
> > are relatively expensive to implement, as they'd probably need to be
> > parsed and split into an array for `grub_util_exec()`, right?
>
> Yes.  It's inevitably best-effort, especially if we can't depend on a
> working shell.

I would prefer to have "config-overrides" but if it requires tons of
work I am OK with existing implementation, +/- minor tweaks/fixes,
assuming its assumptions and limitations are properly documented.

> > But, please, still leave sane defaults in the binary, for out-of-
> > the-box, no-config-changes-necessary usage, pretty please!

As I said earlier, I am (mostly) OK with current defaults.

> > Would it be possible to re-evaluate the proposed patch with this
> > in mind?

Yep!

Daniel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries
  2022-08-20 11:23           ` Daniel Kiper
@ 2022-08-23 21:03             ` Robbie Harwood
  0 siblings, 0 replies; 11+ messages in thread
From: Robbie Harwood @ 2022-08-23 21:03 UTC (permalink / raw)
  To: Daniel Kiper; +Cc: Raymund Will, grub-devel, Javier Martinez Canillas

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

Daniel Kiper <dkiper@net-space.pl> writes:

> On Tue, Aug 16, 2022 at 12:07:06PM -0400, Robbie Harwood wrote:
>> Raymund Will <rw@suse.de> writes:
>>
>>> Granted, the duplication of `--kexec` to signify "force it", might
>>> as well be spelled out as `--force-kexec` (or something similar).
>>> (But that change will provoke inconsistencies during an indefinite
>>> migration phase, where pre-boot images don't match binaries in the
>>> root filesystem, notably, when rollback snapshots come into play.)
>>
>> Passing --kexec twice (or --force-kexec) doesn't appear to change
>> anything in the versions of this patch I can easily find.  We could
>> add
>
> Yeah, I think Raymund is talking about a bit different version of the
> patch. Raymund, could you provide us the one which has that features,
> and potentially others, implemented?

openSUSE's version of this patch has support for that which I'll
incorporate in the next version.

>> the behavior you're describing though - Daniel, would that help with
>> your concerns about it?
>
> I would prefer --force-kexec but if double --kexec is used in existing
> environments I am OK with the latter. However, please document this
> behavior in the GRUB's docs.

Appears to be in use in openSUSE; I imagine they'll want that
preserved.  Could probably add --force-kexec as well if that's desired.

>>> Config-overrides in `grub.cfg` in turn would be a nice addition, but
>>> are relatively expensive to implement, as they'd probably need to be
>>> parsed and split into an array for `grub_util_exec()`, right?
>>
>> Yes.  It's inevitably best-effort, especially if we can't depend on a
>> working shell.
>
> I would prefer to have "config-overrides" but if it requires tons of
> work I am OK with existing implementation, +/- minor tweaks/fixes,
> assuming its assumptions and limitations are properly documented.

I think the reason Raymund and I are hesitant is due to the lexing of
arguments.  We need to split them to pass to grub_util_exec(), which
means we need to know how to split them.  And we can't just split on the
space character because of things like --append - i.e., it can
reasonably have spaces in it.  Quoting also is a problem for similar
reasons (and the suggested syntax uses single quotes).

So I don't see a simple way out, but maybe I've missed something.  Are
there particular arguments you have in mind for adding?  Maybe they
should be made default, or have specific options.

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-08-23 21:04 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-19 20:39 [PATCH v2 1/1] Add support for grub-emu to kexec Linux menu entries Robbie Harwood
2022-08-03 15:26 ` Daniel Kiper
2022-08-08 18:58   ` Robbie Harwood
2022-08-11 18:08     ` Daniel Kiper
2022-08-15 13:16       ` Raymund Will
2022-08-16 16:07         ` Robbie Harwood
2022-08-20 11:23           ` Daniel Kiper
2022-08-23 21:03             ` Robbie Harwood
2022-08-08 21:43 ` Vladimir 'phcoder' Serbinenko
2022-08-11 18:02   ` Robbie Harwood
2022-08-11 18:13   ` Daniel Kiper

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.