[PATCH 0/4] Turn-key PV-GRUB2 installation

[PATCH 0/4] Turn-key PV-GRUB2 installation

[Colin Watson]

This patch set should make it easier to maintain PV-GRUB2 installations.
The general idea is based on discussions I had with Xen developers
(mainly Ian Jackson) at the Ubuntu Developer Summit in May 2011; though
I never did manage to get the core port done and Vladimir beat me to
that, I think the configuration approach we discussed there is still
valid and useful.

The idea here is that people maintaining dom0s, especially in big cloud
installations, will probably not want to update their PV-GRUB2 image
(which will be read from the dom0 filesystem but will run in the domU)
very often; but on the other hand people maintaining distributions will
want to be able to move on and make use of latest-and-greatest features.
To make this work, we build a dom0-filesystem-suitable grub.xen image
which just tries to chainload another grub.xen from a couple of
reasonably standard locations in the domU's filesystem
(/boot/grub/grub.xen and /boot/grub2/grub.xen).

I've arranged that you can either use that very same grub.xen image in
the domU by way of a new mechanism that passes GRUB environment
variables via the Xen guest command line so that we can tell which
context we're in, or you can run "grub-install --target=x86_64-xen" in
the domU which will build and install a suitable one in more or less the
usual way.

Colin Watson (4):
  Add an option to exclude devices from search results.
  Accept environment variables on the command line for Xen.
  Build grub.xen.
  Improve installation on Xen.

 .gitignore                       |  1 +
 ChangeLog                        | 45 ++++++++++++++++++++++++++++++++++++++++
 Makefile.am                      | 14 +++++++++++++
 configure.ac                     |  1 +
 docs/grub.texi                   |  7 ++++++-
 grub-core/Makefile.core.def      |  7 +++++++
 grub-core/boot/xen/xen.cfg.in    | 31 +++++++++++++++++++++++++++
 grub-core/commands/search.c      | 15 ++++++++++++--
 grub-core/commands/search_wrap.c | 27 +++++++++++++++++++-----
 grub-core/kern/xen/init.c        | 44 +++++++++++++++++++++++++++++++++++++++
 include/grub/search.h            |  9 +++++---
 util/grub-install.c              | 11 ++++++++--
 12 files changed, 199 insertions(+), 13 deletions(-)
 create mode 100644 grub-core/boot/xen/xen.cfg.in

-- 
1.8.4.4

[PATCH 1/4] Add an option to exclude devices from search results.

[Colin Watson]

* grub-core/commands/search.c (struct search_ctx): Add excludes and
nexcludes.
(iterate_device): Ignore devices listed in ctx->excludes.
(FUNC_NAME): Accept excludes and nexcludes parameters.
(grub_cmd_do_search): Pass empty excludes.
* grub-core/commands/search_wrap.c (options): Add --exclude option.
(grub_cmd_search): Handle --exclude.
* include/grub/search.h (grub_search_fs_file): Update prototype.
(grub_search_fs_uuid): Likewise.
(grub_search_label): Likewise.
* docs/grub.texi (search): Document --exclude.
---
 ChangeLog                        | 16 ++++++++++++++++
 docs/grub.texi                   |  7 ++++++-
 grub-core/commands/search.c      | 15 +++++++++++++--
 grub-core/commands/search_wrap.c | 27 ++++++++++++++++++++++-----
 include/grub/search.h            |  9 ++++++---
 5 files changed, 63 insertions(+), 11 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 9cec63f..766fe4b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
+
+	Add an option to exclude devices from search results.
+
+	* grub-core/commands/search.c (struct search_ctx): Add excludes and
+	nexcludes.
+	(iterate_device): Ignore devices listed in ctx->excludes.
+	(FUNC_NAME): Accept excludes and nexcludes parameters.
+	(grub_cmd_do_search): Pass empty excludes.
+	* grub-core/commands/search_wrap.c (options): Add --exclude option.
+	(grub_cmd_search): Handle --exclude.
+	* include/grub/search.h (grub_search_fs_file): Update prototype.
+	(grub_search_fs_uuid): Likewise.
+	(grub_search_label): Likewise.
+	* docs/grub.texi (search): Document --exclude.
+
 2013-12-11  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	* grub-core/normal/charset.c: Fix premature line wrap and crash.
diff --git a/docs/grub.texi b/docs/grub.texi
index 91fa1de..4c53665 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4731,7 +4731,8 @@ unbootable. @xref{Using digital signatures}, for more information.
 
 @deffn Command search @
  [@option{--file}|@option{--label}|@option{--fs-uuid}] @
- [@option{--set} [var]] [@option{--no-floppy}] name
+ [@option{--set} [var]] [@option{--no-floppy}] @
+ [@option{--exclude} @var{name} ...] name
 Search devices by file (@option{-f}, @option{--file}), filesystem label
 (@option{-l}, @option{--label}), or filesystem UUID (@option{-u},
 @option{--fs-uuid}).
@@ -4743,6 +4744,10 @@ value of environment variable @var{var}.  The default variable is
 The @option{--no-floppy} option prevents searching floppy devices, which can
 be slow.
 
+The @option{--exclude} option excludes any matches of the given GRUB device
+name.  For example, this may be used to search for any file named
+@file{/boot/grub/grub.cfg} other than the one in a memdisk.
+
 The @samp{search.file}, @samp{search.fs_label}, and @samp{search.fs_uuid}
 commands are aliases for @samp{search --file}, @samp{search --label}, and
 @samp{search --fs-uuid} respectively.
diff --git a/grub-core/commands/search.c b/grub-core/commands/search.c
index 16143a3..03cfea1 100644
--- a/grub-core/commands/search.c
+++ b/grub-core/commands/search.c
@@ -50,6 +50,8 @@ struct search_ctx
   int no_floppy;
   char **hints;
   unsigned nhints;
+  char **excludes;
+  unsigned nexcludes;
   int count;
   int is_cache;
 };
@@ -59,8 +61,15 @@ static int
 iterate_device (const char *name, void *data)
 {
   struct search_ctx *ctx = data;
+  unsigned i;
   int found = 0;
 
+  for (i = 0; i < ctx->nexcludes; i++)
+    {
+      if (grub_strcmp (name, ctx->excludes[i]) == 0)
+	return 0;
+    }
+
   /* Skip floppy drives when requested.  */
   if (ctx->no_floppy &&
       name[0] == 'f' && name[1] == 'd' && name[2] >= '0' && name[2] <= '9')
@@ -262,7 +271,7 @@ try (struct search_ctx *ctx)
 
 void
 FUNC_NAME (const char *key, const char *var, int no_floppy,
-	   char **hints, unsigned nhints)
+	   char **hints, unsigned nhints, char **excludes, unsigned nexcludes)
 {
   struct search_ctx ctx = {
     .key = key,
@@ -270,6 +279,8 @@ FUNC_NAME (const char *key, const char *var, int no_floppy,
     .no_floppy = no_floppy,
     .hints = hints,
     .nhints = nhints,
+    .excludes = excludes,
+    .nexcludes = nexcludes,
     .count = 0,
     .is_cache = 0
   };
@@ -304,7 +315,7 @@ grub_cmd_do_search (grub_command_t cmd __attribute__ ((unused)), int argc,
     return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument expected"));
 
   FUNC_NAME (args[0], argc == 1 ? 0 : args[1], 0, (args + 2),
-	     argc > 2 ? argc - 2 : 0);
+	     argc > 2 ? argc - 2 : 0, NULL, 0);
 
   return grub_errno;
 }
diff --git a/grub-core/commands/search_wrap.c b/grub-core/commands/search_wrap.c
index 3f75fec..7493df4 100644
--- a/grub-core/commands/search_wrap.c
+++ b/grub-core/commands/search_wrap.c
@@ -63,6 +63,9 @@ static const struct grub_arg_option options[] =
      N_("First try the device HINT if currently running on ARC."
 	" If HINT ends in comma, also try subpartitions"),
      N_("HINT"), ARG_TYPE_STRING},
+    {"exclude",		0, GRUB_ARG_OPTION_REPEATABLE,
+     N_("Exclude the device whose GRUB device name is NAME."), N_("NAME"),
+     ARG_TYPE_STRING},
     {0, 0, 0, 0, 0, 0}
   };
 
@@ -79,6 +82,7 @@ enum options
     SEARCH_HINT_BAREMETAL,
     SEARCH_HINT_EFI,
     SEARCH_HINT_ARC,
+    SEARCH_EXCLUDE,
  };
 
 static grub_err_t
@@ -87,8 +91,8 @@ grub_cmd_search (grub_extcmd_context_t ctxt, int argc, char **args)
   struct grub_arg_list *state = ctxt->state;
   const char *var = 0;
   const char *id = 0;
-  int i = 0, j = 0, nhints = 0;
-  char **hints = NULL;
+  int i = 0, j = 0, nhints = 0, nexcludes = 0;
+  char **hints = NULL, **excludes = NULL;
 
   if (state[SEARCH_HINT].set)
     for (i = 0; state[SEARCH_HINT].args[i]; i++)
@@ -164,6 +168,19 @@ grub_cmd_search (grub_extcmd_context_t ctxt, int argc, char **args)
     if (grub_memcmp (args[j], "--hint-", sizeof ("--hint-") - 1) != 0)
       break;
 
+  if (state[SEARCH_EXCLUDE].set)
+    {
+      for (i = 0; state[SEARCH_EXCLUDE].args[i]; i++)
+	nexcludes++;
+
+      excludes = grub_malloc (sizeof (excludes[0]) * nexcludes);
+      if (!excludes)
+	return grub_errno;
+
+      for (i = 0; state[SEARCH_EXCLUDE].args[i]; i++)
+	excludes[i] = state[SEARCH_EXCLUDE].args[i];
+    }
+
   if (state[SEARCH_SET].set)
     var = state[SEARCH_SET].arg ? state[SEARCH_SET].arg : "root";
 
@@ -179,13 +196,13 @@ grub_cmd_search (grub_extcmd_context_t ctxt, int argc, char **args)
 
   if (state[SEARCH_LABEL].set)
     grub_search_label (id, var, state[SEARCH_NO_FLOPPY].set, 
-		       hints, nhints);
+		       hints, nhints, excludes, nexcludes);
   else if (state[SEARCH_FS_UUID].set)
     grub_search_fs_uuid (id, var, state[SEARCH_NO_FLOPPY].set,
-			 hints, nhints);
+			 hints, nhints, excludes, nexcludes);
   else if (state[SEARCH_FILE].set)
     grub_search_fs_file (id, var, state[SEARCH_NO_FLOPPY].set, 
-			 hints, nhints);
+			 hints, nhints, excludes, nexcludes);
   else
     return grub_error (GRUB_ERR_INVALID_COMMAND, "unspecified search type");
 
diff --git a/include/grub/search.h b/include/grub/search.h
index d80347d..e7bcd4b 100644
--- a/include/grub/search.h
+++ b/include/grub/search.h
@@ -20,10 +20,13 @@
 #define GRUB_SEARCH_HEADER 1
 
 void grub_search_fs_file (const char *key, const char *var, int no_floppy,
-			  char **hints, unsigned nhints);
+			  char **hints, unsigned nhints,
+			  char **excludes, unsigned nexcludes);
 void grub_search_fs_uuid (const char *key, const char *var, int no_floppy,
-			  char **hints, unsigned nhints);
+			  char **hints, unsigned nhints,
+			  char **excludes, unsigned nexcludes);
 void grub_search_label (const char *key, const char *var, int no_floppy,
-			char **hints, unsigned nhints);
+			char **hints, unsigned nhints,
+			char **excludes, unsigned nexcludes);
 
 #endif
-- 
1.8.4.4

Re: [Xen-devel] [PATCH 1/4] Add an option to exclude devices from search results.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 12.12.2013 16:37, Colin Watson wrote:
> * grub-core/commands/search.c (struct search_ctx): Add excludes and
> nexcludes.
> (iterate_device): Ignore devices listed in ctx->excludes.
> (FUNC_NAME): Accept excludes and nexcludes parameters.
> (grub_cmd_do_search): Pass empty excludes.
> * grub-core/commands/search_wrap.c (options): Add --exclude option.
> (grub_cmd_search): Handle --exclude.
> * include/grub/search.h (grub_search_fs_file): Update prototype.
> (grub_search_fs_uuid): Likewise.
> (grub_search_label): Likewise.
> * docs/grub.texi (search): Document --exclude.
> ---
>  ChangeLog                        | 16 ++++++++++++++++
>  docs/grub.texi                   |  7 ++++++-
>  grub-core/commands/search.c      | 15 +++++++++++++--
>  grub-core/commands/search_wrap.c | 27 ++++++++++++++++++++++-----
>  include/grub/search.h            |  9 ++++++---
>  5 files changed, 63 insertions(+), 11 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 9cec63f..766fe4b 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,19 @@
> +2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
> +
> +	Add an option to exclude devices from search results.
> +
> +	* grub-core/commands/search.c (struct search_ctx): Add excludes and
> +	nexcludes.
> +	(iterate_device): Ignore devices listed in ctx->excludes.
> +	(FUNC_NAME): Accept excludes and nexcludes parameters.
> +	(grub_cmd_do_search): Pass empty excludes.
> +	* grub-core/commands/search_wrap.c (options): Add --exclude option.
> +	(grub_cmd_search): Handle --exclude.
> +	* include/grub/search.h (grub_search_fs_file): Update prototype.
> +	(grub_search_fs_uuid): Likewise.
> +	(grub_search_label): Likewise.
> +	* docs/grub.texi (search): Document --exclude.
> +
>  2013-12-11  Vladimir Serbinenko  <phcoder@gmail.com>
>  
>  	* grub-core/normal/charset.c: Fix premature line wrap and crash.
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 91fa1de..4c53665 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4731,7 +4731,8 @@ unbootable. @xref{Using digital signatures}, for more information.
>  
>  @deffn Command search @
>   [@option{--file}|@option{--label}|@option{--fs-uuid}] @
> - [@option{--set} [var]] [@option{--no-floppy}] name
> + [@option{--set} [var]] [@option{--no-floppy}] @
> + [@option{--exclude} @var{name} ...] name
>  Search devices by file (@option{-f}, @option{--file}), filesystem label
>  (@option{-l}, @option{--label}), or filesystem UUID (@option{-u},
>  @option{--fs-uuid}).
> @@ -4743,6 +4744,10 @@ value of environment variable @var{var}.  The default variable is
>  The @option{--no-floppy} option prevents searching floppy devices, which can
>  be slow.
>  
> +The @option{--exclude} option excludes any matches of the given GRUB device
> +name.  For example, this may be used to search for any file named
> +@file{/boot/grub/grub.cfg} other than the one in a memdisk.
> +
>  The @samp{search.file}, @samp{search.fs_label}, and @samp{search.fs_uuid}
>  commands are aliases for @samp{search --file}, @samp{search --label}, and
>  @samp{search --fs-uuid} respectively.
> diff --git a/grub-core/commands/search.c b/grub-core/commands/search.c
> index 16143a3..03cfea1 100644
> --- a/grub-core/commands/search.c
> +++ b/grub-core/commands/search.c
> @@ -50,6 +50,8 @@ struct search_ctx
>    int no_floppy;
>    char **hints;
>    unsigned nhints;
> +  char **excludes;
> +  unsigned nexcludes;
>    int count;
>    int is_cache;
>  };
> @@ -59,8 +61,15 @@ static int
>  iterate_device (const char *name, void *data)
>  {
>    struct search_ctx *ctx = data;
> +  unsigned i;
>    int found = 0;
>  
> +  for (i = 0; i < ctx->nexcludes; i++)
> +    {
> +      if (grub_strcmp (name, ctx->excludes[i]) == 0)
> +	return 0;
> +    }
> +
This makes behaviour strongly dependent on the unstable names. In case
of memdisk it's not a problem because its name is stable but outside
this specific use it's easily misusable command. E.g.
search --exclude=hd0,1
would be heavily dependent on disk naming and won't work because this
check considers hd0,1 and hd0,msdos1 as distict. The only way to sanely
use this interface is to do sth like
search --exclude $boot
to find next configfile, a bit like include_next directive but it
creates a problem that the same disk may have several names. E.g. LVM
have names base on name and UUID.
This doesn't handle some configs like 2 autodetecting scripts including
each other.
This is complex problem and I'm unsure how to solve it.


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

Re: [Xen-devel] [PATCH 1/4] Add an option to exclude devices from search results.

[Colin Watson]

On Fri, Dec 13, 2013 at 01:27:37PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> This is complex problem and I'm unsure how to solve it.

Yes, you're right.  None of this would have been a problem at all except
that the memdisk config had to be /boot/grub/grub.cfg.  Maybe we could
just dodge the problem by making it easier to rename that?  We could
take either or both of two approaches:

 1) add an environment variable that lets you change the name of
    grub.cfg as well as its directory (as prefix does)
 2) make it possible for grub-mkstandalone to build images with the
    prefix set to something other than (memdisk)/boot/grub

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

[PATCH] add --boot-directory option to grub-mkstandalone

[Andrey Borzenkov]

> 2) make it possible for grub-mkstandalone to build images with the
>    prefix set to something other than (memdisk)/boot/grub

Like below? I'm not keen on using --boot-directory; may be using full
--grub-directory (without implied ../grub) will actually be more logical.

---

This allows changing of hardcoded /boot/grub path on memory disk so it does
not conflict with on-disk /boot directories.

Suggested by Colin Watson

---
 util/grub-mkstandalone.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/util/grub-mkstandalone.c b/util/grub-mkstandalone.c
index 6ee5e58..3cb5d7c 100644
--- a/util/grub-mkstandalone.c
+++ b/util/grub-mkstandalone.c
@@ -30,15 +30,20 @@ static char **files;
 static int nfiles;
 const struct grub_install_image_target_desc *format;
 static FILE *memdisk;
+static char *bootdir;
 
 enum
   {
     OPTION_OUTPUT = 'o',
-    OPTION_FORMAT = 'O'
+    OPTION_FORMAT = 'O',
+    OPTION_BOOT_DIRECTORY = 0x301,
+
   };
 
 static struct argp_option options[] = {
   GRUB_INSTALL_OPTIONS,
+  {"boot-directory", OPTION_BOOT_DIRECTORY, N_("DIR"),
+   0, N_("install GRUB images under the directory DIR/grub instead of the /boot/grub directory"), 2},
   {"output", 'o', N_("FILE"),
    0, N_("save output in FILE [required]"), 2},
   {"format", 'O', N_("FILE"), 0, 0, 2},
@@ -76,6 +81,11 @@ argp_parser (int key, char *arg, struct argp_state *state)
   switch (key)
     {
 
+    case OPTION_BOOT_DIRECTORY:
+      free (bootdir);
+      bootdir = xstrdup (arg);
+      return 0;
+
     case 'o':
       if (output_image)
 	free (output_image);
@@ -294,6 +304,9 @@ main (int argc, char *argv[])
 
   argp_parse (&argp, argc, argv, 0, 0, 0);
 
+  if (!bootdir)
+    bootdir = xstrdup ("/boot");
+
   pkglibdir = grub_util_get_pkglibdir ();
 
   if (!output_image)
@@ -308,7 +321,7 @@ main (int argc, char *argv[])
   enum grub_install_plat plat = grub_install_get_target (grub_install_source_directory);
 
   char *memdisk_dir = grub_util_make_temporary_dir ();
-  char *boot_grub = grub_util_path_concat (3, memdisk_dir, "boot", "grub");
+  char *boot_grub = grub_util_path_concat (3, memdisk_dir, bootdir, "grub");
   grub_install_copy_files (grub_install_source_directory,
 			   boot_grub, plat);
 
@@ -346,8 +359,8 @@ main (int argc, char *argv[])
   grub_install_push_module ("tar");
 
   grub_install_make_image_wrap (grub_install_source_directory,
-				"(memdisk)/boot/grub", output_image,
-				memdisk_img, NULL,
+				xasprintf ("(memdisk)%s/%s", bootdir, "grub"),
+				output_image, memdisk_img, NULL,
 				grub_util_get_target_name (format), 0);
 
   grub_util_unlink (memdisk_img);
-- 
tg: (0df77d7..) u/grub-directory (depends on: master)

Re: [PATCH] add --boot-directory option to grub-mkstandalone

[Colin Watson]

On Fri, Dec 13, 2013 at 07:40:54PM +0400, Andrey Borzenkov wrote:
> > 2) make it possible for grub-mkstandalone to build images with the
> >    prefix set to something other than (memdisk)/boot/grub
> 
> Like below?

Indeed, I like this general approach, and it's probably better than
fiddling around with "search".

> I'm not keen on using --boot-directory; may be using full
> --grub-directory (without implied ../grub) will actually be more
> logical.

I agree - I would prefer to explicitly specify the whole thing.  Could
you redo the patch that way?

>  enum
>    {
>      OPTION_OUTPUT = 'o',
> -    OPTION_FORMAT = 'O'
> +    OPTION_FORMAT = 'O',
> +    OPTION_BOOT_DIRECTORY = 0x301,
> +
>    };

Stray newline.

> @@ -346,8 +359,8 @@ main (int argc, char *argv[])
>    grub_install_push_module ("tar");
>  
>    grub_install_make_image_wrap (grub_install_source_directory,
> -				"(memdisk)/boot/grub", output_image,
> -				memdisk_img, NULL,
> +				xasprintf ("(memdisk)%s/%s", bootdir, "grub"),
> +				output_image, memdisk_img, NULL,
>  				grub_util_get_target_name (format), 0);
>  
>    grub_util_unlink (memdisk_img);

I'd probably assign the result of that xasprintf to a variable in order
to be able to free it, for valgrind-friendliness.

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] add --boot-directory option to grub-mkstandalone

[Andrey Borzenkov]

В Пт, 20/12/2013 в 12:16 +0000, Colin Watson пишет:
> On Fri, Dec 13, 2013 at 07:40:54PM +0400, Andrey Borzenkov wrote:
> > > 2) make it possible for grub-mkstandalone to build images with the
> > >    prefix set to something other than (memdisk)/boot/grub
> > 
> > Like below?
> 
> Indeed, I like this general approach, and it's probably better than
> fiddling around with "search".
> 
> > I'm not keen on using --boot-directory; may be using full
> > --grub-directory (without implied ../grub) will actually be more
> > logical.
> 
> I agree - I would prefer to explicitly specify the whole thing.  Could
> you redo the patch that way?
> 

See below

> >  enum
> >    {
> >      OPTION_OUTPUT = 'o',
> > -    OPTION_FORMAT = 'O'
> > +    OPTION_FORMAT = 'O',
> > +    OPTION_BOOT_DIRECTORY = 0x301,
> > +
> >    };
> 
> Stray newline.
> 
Fixed

> > @@ -346,8 +359,8 @@ main (int argc, char *argv[])
> >    grub_install_push_module ("tar");
> >  
> >    grub_install_make_image_wrap (grub_install_source_directory,
> > -				"(memdisk)/boot/grub", output_image,
> > -				memdisk_img, NULL,
> > +				xasprintf ("(memdisk)%s/%s", bootdir, "grub"),
> > +				output_image, memdisk_img, NULL,
> >  				grub_util_get_target_name (format), 0);
> >  
> >    grub_util_unlink (memdisk_img);
> 
> I'd probably assign the result of that xasprintf to a variable in order
> to be able to free it, for valgrind-friendliness.
> 

Well, I could do it but as part of separate clean up patch. There are
many non-freed allocations besides it.

From: Andrey Borzenkov <arvidjaar@gmail.com>
Subject: [PATCH] add --boot-directory option to grub-mkstandalone

This allows changing of hardcoded /boot/grub path on memory disk so it does
not conflict with on-disk /boot directories.

Suggested by Colin Watson

---
 util/grub-mkstandalone.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/util/grub-mkstandalone.c b/util/grub-mkstandalone.c
index 576ab40..77c1998 100644
--- a/util/grub-mkstandalone.c
+++ b/util/grub-mkstandalone.c
@@ -30,15 +30,19 @@ static char **files;
 static int nfiles;
 const struct grub_install_image_target_desc *format;
 static FILE *memdisk;
+static char *grubdir;
 
 enum
   {
     OPTION_OUTPUT = 'o',
-    OPTION_FORMAT = 'O'
+    OPTION_FORMAT = 'O',
+    OPTION_GRUB_DIRECTORY = 0x301,
   };
 
 static struct argp_option options[] = {
   GRUB_INSTALL_OPTIONS,
+  {"grub-directory", OPTION_GRUB_DIRECTORY, N_("DIR"),
+   0, N_("install GRUB images under the directory DIR instead of the /boot/grub directory"), 2},
   {"output", 'o', N_("FILE"),
    0, N_("save output in FILE [required]"), 2},
   {"format", 'O', N_("FILE"), 0, 0, 2},
@@ -76,6 +80,11 @@ argp_parser (int key, char *arg, struct argp_state *state)
   switch (key)
     {
 
+    case OPTION_GRUB_DIRECTORY:
+      free (grubdir);
+      grubdir = xstrdup (arg);
+      return 0;
+
     case 'o':
       if (output_image)
 	free (output_image);
@@ -294,6 +303,9 @@ main (int argc, char *argv[])
 
   argp_parse (&argp, argc, argv, 0, 0, 0);
 
+  if (!grubdir)
+    grubdir = xstrdup ("/boot/grub");
+
   pkglibdir = grub_util_get_pkglibdir ();
 
   if (!output_image)
@@ -308,7 +320,7 @@ main (int argc, char *argv[])
   enum grub_install_plat plat = grub_install_get_target (grub_install_source_directory);
 
   char *memdisk_dir = grub_util_make_temporary_dir ();
-  char *boot_grub = grub_util_path_concat (3, memdisk_dir, "boot", "grub");
+  char *boot_grub = grub_util_path_concat (2, memdisk_dir, grubdir);
   grub_install_copy_files (grub_install_source_directory,
 			   boot_grub, plat);
 
@@ -346,8 +358,8 @@ main (int argc, char *argv[])
   grub_install_push_module ("tar");
 
   grub_install_make_image_wrap (grub_install_source_directory,
-				"(memdisk)/boot/grub", output_image,
-				memdisk_img, NULL,
+				xasprintf ("(memdisk)%s", bootdir),
+				output_image, memdisk_img, NULL,
 				grub_util_get_target_name (format), 0);
 
   grub_util_unlink (memdisk_img);
-- 
tg: (065adc3..) u/grub-directory (depends on: master)

Re: [PATCH] add --boot-directory option to grub-mkstandalone

[Andrey Borzenkov]

В Сб, 21/12/2013 в 14:29 +0400, Andrey Borzenkov пишет:
> +				xasprintf ("(memdisk)%s", bootdir),

Fixed as well.

Re: [Xen-devel] [PATCH 1/4] Add an option to exclude devices from search results.

[Jordan Uggla]

On Dec 13, 2013 6:28 AM, "Vladimir 'φ-coder/phcoder' Serbinenko"
<phcoder@gmail.com> wrote:
>
> On 12.12.2013 16:37, Colin Watson wrote:
> > * grub-core/commands/search.c (struct search_ctx): Add excludes and
> > nexcludes.
> > (iterate_device): Ignore devices listed in ctx->excludes.
> > (FUNC_NAME): Accept excludes and nexcludes parameters.
> > (grub_cmd_do_search): Pass empty excludes.
> > * grub-core/commands/search_wrap.c (options): Add --exclude option.
> > (grub_cmd_search): Handle --exclude.
> > * include/grub/search.h (grub_search_fs_file): Update prototype.
> > (grub_search_fs_uuid): Likewise.
> > (grub_search_label): Likewise.
> > * docs/grub.texi (search): Document --exclude.
> > ---
> >  ChangeLog                        | 16 ++++++++++++++++
> >  docs/grub.texi                   |  7 ++++++-
> >  grub-core/commands/search.c      | 15 +++++++++++++--
> >  grub-core/commands/search_wrap.c | 27 ++++++++++++++++++++++-----
> >  include/grub/search.h            |  9 ++++++---
> >  5 files changed, insertions(+), 11 deletions(-)
> >
> > diff --git a/ChangeLog b/ChangeLog
> > index 9cec63f..766fe4b 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,3 +1,19 @@
> > +2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
> > +
> > +     Add an option to exclude devices from search results.
> > +
> > +     * grub-core/commands/search.c (struct search_ctx): Add excludes and
> > +     nexcludes.
> > +     (iterate_device): Ignore devices listed in ctx->excludes.
> > +     (FUNC_NAME): Accept excludes and nexcludes parameters.
> > +     (grub_cmd_do_search): Pass empty excludes.
> > +     * grub-core/commands/search_wrap.c (options): Add --exclude option.
> > +     (grub_cmd_search): Handle --exclude.
> > +     * include/grub/search.h (grub_search_fs_file): Update prototype.
> > +     (grub_search_fs_uuid): Likewise.
> > +     (grub_search_label): Likewise.
> > +     * docs/grub.texi (search): Document --exclude.
> > +
> >  2013-12-11  Vladimir Serbinenko  <phcoder@gmail.com>
> >
> >       * grub-core/normal/charset.c: Fix premature line wrap and crash.
> > diff --git a/docs/grub.texi b/docs/grub.texi
> > index 91fa1de..4c53665 100644
> > --- a/docs/grub.texi
> > +++ b/docs/grub.texi
> > @@ -4731,7 +4731,8 @@ unbootable. @xref{Using digital signatures}, for more information.
> >
> >  @deffn Command search @
> >   [@option{--file}|@option{--label}|@option{--fs-uuid}] @
> > - [@option{--set} [var]] [@option{--no-floppy}] name
> > + [@option{--set} [var]] [@option{--no-floppy}] @
> > + [@option{--exclude} @var{name} ...] name
> >  Search devices by file (@option{-f}, @option{--file}), filesystem label
> >  (@option{-l}, @option{--label}), or filesystem UUID (@option{-u},
> >  @option{--fs-uuid}).
> > @@ -4743,6 +4744,10 @@ value of environment variable @var{var}.  The default variable is
> >  The @option{--no-floppy} option prevents searching floppy devices, which can
> >  be slow.
> >
> > +The @option{--exclude} option excludes any matches of the given GRUB device
> > +name.  For example, this may be used to search for any file named
> > +@file{/boot/grub/grub.cfg} other than the one in a memdisk.
> > +
> >  The @samp{search.file}, @samp{search.fs_label}, and @samp{search.fs_uuid}
> >  commands are aliases for @samp{search --file}, @samp{search --label}, and
> >  @samp{search --fs-uuid} respectively.
> > diff --git a/grub-core/commands/search.c b/grub-core/commands/search.c
> > index 16143a3..03cfea1 100644
> > --- a/grub-core/commands/search.c
> > +++ b/grub-core/commands/search.c
> > @@ -50,6 +50,8 @@ struct search_ctx
> >    int no_floppy;
> >    char **hints;
> >    unsigned nhints;
> > +  char **excludes;
> > +  unsigned nexcludes;
> >    int count;
> >    int is_cache;
> >  };
> > @@ -59,8 +61,15 @@ static int
> >  iterate_device (const char *name, void *data)
> >  {
> >    struct search_ctx *ctx = data;
> > +  unsigned i;
> >    int found = 0;
> >
> > +  for (i = 0; i < ctx->nexcludes; i++)
> > +    {
> > +      if (grub_strcmp (name, ctx->excludes[i]) == 0)
> > +     return 0;
> > +    }
> > +
> This makes behaviour strongly dependent on the unstable names. In case
> of memdisk it's not a problem because its name is stable but outside
> this specific use it's easily misusable command. E.g.
> search --exclude=hd0,1
> would be heavily dependent on disk naming and won't work because this
> check considers hd0,1 and hd0,msdos1 as distict. The only way to sanely
> use this interface is to do sth like
> search --exclude $boot
> to find next configfile, a bit like include_next directive but it
> creates a problem that the same disk may have several names. E.g. LVM
> have names base on name and UUID.
> This doesn't handle some configs like 2 autodetecting scripts including
> each other.
> This is complex problem and I'm unsure how to solve it.

How about excluding by uuid (or uuid-file)?

For example, the following (untested) grub.cfg provides what I expect
would be a nice interface for Xen users, using existing grub features
only:

# Search all devices for grub.cfg files to use, presenting a menu to
chose among them if
# more than  one is found.

timeout=5

probe --set=this_uuid --fs-uuid "(${root})"

insmod regexp

found_a_cfg=false
found_multiple_cfgs=false

for cfg in (*)/boot/grub/grub.cfg (*)/grub/grub.cfg (*)/boot/grub2/grub.cfg \
           (*)/grub2/grub.cfg; do
  regexp --set=1:cfg_device '^\((.*)\)/' "$cfg"
  probe --set=cfg_device_uuid --fs-uuid "(${cfg_device})"

  if [ "$this_uuid" != "$cfg_device_uuid" ]; then
    if [ "$found_a_cfg" = true ]; then found_multiple_cfgs=true; fi

    found_a_cfg=true

    menuentry "$cfg" "$cfg_device" "$cfg" {
      cfg_device="$2"
      cfg="$3"

      root="$cfg_device"
      configfile "$cfg"
    }
  fi
done

if [ "$cfg_found" != true ]; then
  echo $"Error: Could not find any grub.cfg files."
else if [ "$multiple_cfg_found" != true ]; then # We found exactly one grub.cfg
  timeout=0 # Don't bother showing the menu, just use the grub.cfg we found.
fi

One obvious downside of the above is that all devices (including
possibly slow ones) are searched rather than stopping at the first
grub.cfg found. To fix that properly would require a way to exclude
devices with certain UUIDs (or UUID-files) from being searched by the
search command.

-- 
Jordan Uggla (Jordan_U on irc.freenode.net)

[PATCH 2/4] Accept environment variables on the command line for Xen.

[Colin Watson]

* grub-core/kern/xen/init.c (fetch_command_line_word): New function.
(parse_command_line): Likewise.
(grub_machine_init): Call parse_command_line.
---
 ChangeLog                 |  8 ++++++++
 grub-core/kern/xen/init.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 766fe4b..fc86601 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,13 @@
 2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
 
+	Accept environment variables on the command line for Xen.
+
+	* grub-core/kern/xen/init.c (fetch_command_line_word): New function.
+	(parse_command_line): Likewise.
+	(grub_machine_init): Call parse_command_line.
+
+2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
+
 	Add an option to exclude devices from search results.
 
 	* grub-core/commands/search.c (struct search_ctx): Add excludes and
diff --git a/grub-core/kern/xen/init.c b/grub-core/kern/xen/init.c
index 1d8eaec..eb9b8b3 100644
--- a/grub-core/kern/xen/init.c
+++ b/grub-core/kern/xen/init.c
@@ -525,6 +525,48 @@ map_all_pages (void)
   grub_mm_init_region ((void *) heap_start, heap_end - heap_start);
 }
 
+static char *
+fetch_command_line_word (char *pos, char **word)
+{
+  while (grub_isspace (*pos))
+    pos++;
+
+  if (!*pos)
+    return NULL;
+
+  *word = pos;
+  while (*pos && !grub_isspace (*pos))
+    pos++;
+  if (*pos)
+    *pos++ = '\0';
+  return pos;
+}
+
+static void
+parse_command_line (void)
+{
+  char *cmd_line;
+  char *pos, *word;
+
+  cmd_line = grub_malloc (MAX_GUEST_CMDLINE + 1);
+  grub_memcpy (cmd_line, grub_xen_start_page_addr->cmd_line,
+	       MAX_GUEST_CMDLINE);
+  cmd_line[MAX_GUEST_CMDLINE] = '\0';
+  pos = cmd_line;
+  while ((pos = fetch_command_line_word (pos, &word)) != NULL)
+    {
+      char *equals;
+
+      equals = grub_strchr (word, '=');
+      if (!equals)
+	continue;
+
+      *equals = '\0';
+      grub_env_set (word, equals + 1);
+    }
+  grub_free (cmd_line);
+}
+
 extern char _end[];
 
 void
@@ -547,6 +589,8 @@ grub_machine_init (void)
   grub_xendisk_init ();
 
   grub_boot_init ();
+
+  parse_command_line ();
 }
 
 void
-- 
1.8.4.4

[PATCH 2/4] Accept environment variables on the command line for Xen.

[Colin Watson]

* grub-core/kern/xen/init.c (fetch_command_line_word): New function.
(parse_command_line): Likewise.
(grub_machine_init): Call parse_command_line.
---
 ChangeLog                 |  8 ++++++++
 grub-core/kern/xen/init.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 766fe4b..fc86601 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,13 @@
 2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
 
+	Accept environment variables on the command line for Xen.
+
+	* grub-core/kern/xen/init.c (fetch_command_line_word): New function.
+	(parse_command_line): Likewise.
+	(grub_machine_init): Call parse_command_line.
+
+2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
+
 	Add an option to exclude devices from search results.
 
 	* grub-core/commands/search.c (struct search_ctx): Add excludes and
diff --git a/grub-core/kern/xen/init.c b/grub-core/kern/xen/init.c
index 1d8eaec..eb9b8b3 100644
--- a/grub-core/kern/xen/init.c
+++ b/grub-core/kern/xen/init.c
@@ -525,6 +525,48 @@ map_all_pages (void)
   grub_mm_init_region ((void *) heap_start, heap_end - heap_start);
 }
 
+static char *
+fetch_command_line_word (char *pos, char **word)
+{
+  while (grub_isspace (*pos))
+    pos++;
+
+  if (!*pos)
+    return NULL;
+
+  *word = pos;
+  while (*pos && !grub_isspace (*pos))
+    pos++;
+  if (*pos)
+    *pos++ = '\0';
+  return pos;
+}
+
+static void
+parse_command_line (void)
+{
+  char *cmd_line;
+  char *pos, *word;
+
+  cmd_line = grub_malloc (MAX_GUEST_CMDLINE + 1);
+  grub_memcpy (cmd_line, grub_xen_start_page_addr->cmd_line,
+	       MAX_GUEST_CMDLINE);
+  cmd_line[MAX_GUEST_CMDLINE] = '\0';
+  pos = cmd_line;
+  while ((pos = fetch_command_line_word (pos, &word)) != NULL)
+    {
+      char *equals;
+
+      equals = grub_strchr (word, '=');
+      if (!equals)
+	continue;
+
+      *equals = '\0';
+      grub_env_set (word, equals + 1);
+    }
+  grub_free (cmd_line);
+}
+
 extern char _end[];
 
 void
@@ -547,6 +589,8 @@ grub_machine_init (void)
   grub_xendisk_init ();
 
   grub_boot_init ();
+
+  parse_command_line ();
 }
 
 void
-- 
1.8.4.4

Re: [PATCH 2/4] Accept environment variables on the command line for Xen.

[Andrey Borzenkov]

В Thu, 12 Dec 2013 15:37:22 +0000
Colin Watson <cjwatson@ubuntu.com> пишет:

> * grub-core/kern/xen/init.c (fetch_command_line_word): New function.
> (parse_command_line): Likewise.
> (grub_machine_init): Call parse_command_line.
> ---
>  ChangeLog                 |  8 ++++++++
>  grub-core/kern/xen/init.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 766fe4b..fc86601 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,13 @@
>  2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
>  
> +	Accept environment variables on the command line for Xen.
> +

Thank you!

It may make sense to generalize it to other archs. At least EFI
definitely comes in mind. In this case platform specific part is only
to fetch parameter string(s). What about

- move fetch_command_line_word and parse_command_line to separate file
  that is included for specific platforms only
- make each platform that can accept fetch parameters in platform
  specific way and call common code (parse_command_line)

Does it make sense?

I will add EFI part then later.

> +	* grub-core/kern/xen/init.c (fetch_command_line_word): New function.
> +	(parse_command_line): Likewise.
> +	(grub_machine_init): Call parse_command_line.
> +
> +2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
> +
>  	Add an option to exclude devices from search results.
>  
>  	* grub-core/commands/search.c (struct search_ctx): Add excludes and
> diff --git a/grub-core/kern/xen/init.c b/grub-core/kern/xen/init.c
> index 1d8eaec..eb9b8b3 100644
> --- a/grub-core/kern/xen/init.c
> +++ b/grub-core/kern/xen/init.c
> @@ -525,6 +525,48 @@ map_all_pages (void)
>    grub_mm_init_region ((void *) heap_start, heap_end - heap_start);
>  }
>  
> +static char *
> + (char *pos, char **word)
> +{
> +  while (grub_isspace (*pos))
> +    pos++;
> +
> +  if (!*pos)
> +    return NULL;
> +
> +  *word = pos;
> +  while (*pos && !grub_isspace (*pos))
> +    pos++;
> +  if (*pos)
> +    *pos++ = '\0';
> +  return pos;
> +}
> +
> +static void
> +parse_command_line (void)
> +{
> +  char *cmd_line;
> +  char *pos, *word;
> +
> +  cmd_line = grub_malloc (MAX_GUEST_CMDLINE + 1);
> +  grub_memcpy (cmd_line, grub_xen_start_page_addr->cmd_line,
> +	       MAX_GUEST_CMDLINE);
> +  cmd_line[MAX_GUEST_CMDLINE] = '\0';
> +  pos = cmd_line;
> +  while ((pos = fetch_command_line_word (pos, &word)) != NULL)
> +    {
> +      char *equals;
> +
> +      equals = grub_strchr (word, '=');
> +      if (!equals)
> +	continue;
> +
> +      *equals = '\0';
> +      grub_env_set (word, equals + 1);
> +    }
> +  grub_free (cmd_line);
> +}
> +
>  extern char _end[];
>  
>  void
> @@ -547,6 +589,8 @@ grub_machine_init (void)
>    grub_xendisk_init ();
>  
>    grub_boot_init ();
> +
> +  parse_command_line ();
>  }
>  
>  void

Re: [PATCH 2/4] Accept environment variables on the command line for Xen.

[Colin Watson]

On Thu, Dec 12, 2013 at 07:48:05PM +0400, Andrey Borzenkov wrote:
> > +	Accept environment variables on the command line for Xen.
> 
> Thank you!
> 
> It may make sense to generalize it to other archs. At least EFI
> definitely comes in mind. In this case platform specific part is only
> to fetch parameter string(s). What about
> 
> - move fetch_command_line_word and parse_command_line to separate file
>   that is included for specific platforms only
> - make each platform that can accept fetch parameters in platform
>   specific way and call common code (parse_command_line)
> 
> Does it make sense?

That might well be sensible, yes (multiboot too IIRC), but I was
concerned about generalising this too early since some ports already
have specialised requirements for command-line parsing - e.g. I think
mipsel-loongson has some oddities.  Maybe it would be best to do that
kind of thing as a follow-up patch series?

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH 2/4] Accept environment variables on the command line for Xen.

[Vladimir 'phcoder' Serbinenko]

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

loongson currently parses only automatic arguments. This shouldn't be a
problem
On Dec 12, 2013 5:12 PM, "Colin Watson" <cjwatson@ubuntu.com> wrote:

> On Thu, Dec 12, 2013 at 07:48:05PM +0400, Andrey Borzenkov wrote:
> > > +   Accept environment variables on the command line for Xen.
> >
> > Thank you!
> >
> > It may make sense to generalize it to other archs. At least EFI
> > definitely comes in mind. In this case platform specific part is only
> > to fetch parameter string(s). What about
> >
> > - move fetch_command_line_word and parse_command_line to separate file
> >   that is included for specific platforms only
> > - make each platform that can accept fetch parameters in platform
> >   specific way and call common code (parse_command_line)
> >
> > Does it make sense?
>
> That might well be sensible, yes (multiboot too IIRC), but I was
> concerned about generalising this too early since some ports already
> have specialised requirements for command-line parsing - e.g. I think
> mipsel-loongson has some oddities.  Maybe it would be best to do that
> kind of thing as a follow-up patch series?
>
> --
> Colin Watson                                       [cjwatson@ubuntu.com]
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

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

Re: [PATCH 2/4] Accept environment variables on the command line for Xen.

[Vladimir 'phcoder' Serbinenko]

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

I was about to post similar remarks. I'm on phone so will tell just the key
points.
- Most of GRUB platforms have command lines. I.a xen, efi, loongson,
ieee1275, arc.
- Many platforms add their own command line arguments even if user didn't
ask for it. Those shouldn't interfere. In your patch root=sda1 from xen
will either be lost or will clobber GRUB root.
- IEEE1275 code already looks into command line. If it does sth sane it
could be used as base, otherwise it should be replaced with sth common.
- Perhaps we should use some prefix to avoid unintended clobbering.
- There should be a way to emulate pvgrub1 behaviour which took config name
from commandline. Perhaps pvgrub2 should even do it by default (by using
legacy_configfile)
On Dec 12, 2013 4:48 PM, "Andrey Borzenkov" <arvidjaar@gmail.com> wrote:

> В Thu, 12 Dec 2013 15:37:22 +0000
> Colin Watson <cjwatson@ubuntu.com> пишет:
>
> > * grub-core/kern/xen/init.c (fetch_command_line_word): New function.
> > (parse_command_line): Likewise.
> > (grub_machine_init): Call parse_command_line.
> > ---
> >  ChangeLog                 |  8 ++++++++
> >  grub-core/kern/xen/init.c | 44
> ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 52 insertions(+)
> >
> > diff --git a/ChangeLog b/ChangeLog
> > index 766fe4b..fc86601 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,5 +1,13 @@
> >  2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
> >
> > +     Accept environment variables on the command line for Xen.
> > +
>
> Thank you!
>
> It may make sense to generalize it to other archs. At least EFI
> definitely comes in mind. In this case platform specific part is only
> to fetch parameter string(s). What about
>
> - move fetch_command_line_word and parse_command_line to separate file
>   that is included for specific platforms only
> - make each platform that can accept fetch parameters in platform
>   specific way and call common code (parse_command_line)
>
> Does it make sense?
>
> I will add EFI part then later.
>
> > +     * grub-core/kern/xen/init.c (fetch_command_line_word): New
> function.
> > +     (parse_command_line): Likewise.
> > +     (grub_machine_init): Call parse_command_line.
> > +
> > +2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
> > +
> >       Add an option to exclude devices from search results.
> >
> >       * grub-core/commands/search.c (struct search_ctx): Add excludes and
> > diff --git a/grub-core/kern/xen/init.c b/grub-core/kern/xen/init.c
> > index 1d8eaec..eb9b8b3 100644
> > --- a/grub-core/kern/xen/init.c
> > +++ b/grub-core/kern/xen/init.c
> > @@ -525,6 +525,48 @@ map_all_pages (void)
> >    grub_mm_init_region ((void *) heap_start, heap_end - heap_start);
> >  }
> >
> > +static char *
> > + (char *pos, char **word)
> > +{
> > +  while (grub_isspace (*pos))
> > +    pos++;
> > +
> > +  if (!*pos)
> > +    return NULL;
> > +
> > +  *word = pos;
> > +  while (*pos && !grub_isspace (*pos))
> > +    pos++;
> > +  if (*pos)
> > +    *pos++ = '\0';
> > +  return pos;
> > +}
> > +
> > +static void
> > +parse_command_line (void)
> > +{
> > +  char *cmd_line;
> > +  char *pos, *word;
> > +
> > +  cmd_line = grub_malloc (MAX_GUEST_CMDLINE + 1);
> > +  grub_memcpy (cmd_line, grub_xen_start_page_addr->cmd_line,
> > +            MAX_GUEST_CMDLINE);
> > +  cmd_line[MAX_GUEST_CMDLINE] = '\0';
> > +  pos = cmd_line;
> > +  while ((pos = fetch_command_line_word (pos, &word)) != NULL)
> > +    {
> > +      char *equals;
> > +
> > +      equals = grub_strchr (word, '=');
> > +      if (!equals)
> > +     continue;
> > +
> > +      *equals = '\0';
> > +      grub_env_set (word, equals + 1);
> > +    }
> > +  grub_free (cmd_line);
> > +}
> > +
> >  extern char _end[];
> >
> >  void
> > @@ -547,6 +589,8 @@ grub_machine_init (void)
> >    grub_xendisk_init ();
> >
> >    grub_boot_init ();
> > +
> > +  parse_command_line ();
> >  }
> >
> >  void
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

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

Re: [PATCH 2/4] Accept environment variables on the command line for Xen.

[Andrey Borzenkov]

В Thu, 12 Dec 2013 17:13:13 +0100
"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:

> - Many platforms add their own command line arguments even if user didn't
> ask for it. Those shouldn't interfere. In your patch root=sda1 from xen
> will either be lost or will clobber GRUB root.

My be we could adopt current Linux convention and prefix variables for
GRUB with grub.foo=xxx (stripping "grub." prefix). I do not know whether
it could have compatibility implications for current code though.

The reasoning is, it would allow overwriting variables very early
without need for scripting that is available only after normal is
loaded.

Re: [PATCH 2/4] Accept environment variables on the command line for Xen.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 12.12.2013 18:12, Andrey Borzenkov wrote:
> В Thu, 12 Dec 2013 17:13:13 +0100
> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:
> 
>> - Many platforms add their own command line arguments even if user didn't
>> ask for it. Those shouldn't interfere. In your patch root=sda1 from xen
>> will either be lost or will clobber GRUB root.
> 
> My be we could adopt current Linux convention and prefix variables for
> GRUB with grub.foo=xxx (stripping "grub." prefix). I do not know whether
> it could have compatibility implications for current code though.
> 
> The reasoning is, it would allow overwriting variables very early
> without need for scripting that is available only after normal is
> loaded.
> 
My response was terse and it seems that you misunderstood it, sorry. I
meant more along the lines that define:
cmdline.# as number of arguments
cmdline.1,..., cmdline.N are the raw arguments
cmdline.var.<name> are parsed variants of <name>=<value>

E.g. If the commandline is "root=/dev/sda1 ro"
Then
cmdline.#=2
cmdline.1=root=/dev/sda1
cmdline.2=ro
cmdline.var.root=/dev/sda1

Would that be satisfactory for your usecases? What are the usecases?
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



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

Re: [PATCH 2/4] Accept environment variables on the command line for Xen.

[Colin Watson]

On Thu, Dec 12, 2013 at 06:58:23PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> My response was terse and it seems that you misunderstood it, sorry. I
> meant more along the lines that define:
> cmdline.# as number of arguments
> cmdline.1,..., cmdline.N are the raw arguments
> cmdline.var.<name> are parsed variants of <name>=<value>

That seems unnecessarily cumbersome.  I can see why you'd want them to
be specifically identified, but why require the count rather than just
taking as many cmdline.foo arguments as are present?  I don't really see
what you're trying to achieve here.

(Also, cmdline.* is odd naming given that the whole thing arrives on a
command line anyway.)

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH 2/4] Accept environment variables on the command line for Xen.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 12.12.2013 20:12, Colin Watson wrote:
> On Thu, Dec 12, 2013 at 06:58:23PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> My response was terse and it seems that you misunderstood it, sorry. I
>> meant more along the lines that define:
>> cmdline.# as number of arguments
>> cmdline.1,..., cmdline.N are the raw arguments
>> cmdline.var.<name> are parsed variants of <name>=<value>
> 
> That seems unnecessarily cumbersome.  I can see why you'd want them to
> be specifically identified, but why require the count rather than just
> taking as many cmdline.foo arguments as are present?  I don't really see
> what you're trying to achieve here.
> 
> (Also, cmdline.* is odd naming given that the whole thing arrives on a
> command line anyway.)
> 
As discussed on IRD what I meant is how command line is exported to GRUB
environment.
Colin proposed on IRC to use already existing $*, $@ and $# but it poses
other problems with shadowing in source, configfile, menuentry and function.
@Jordan: your opinion since you do a lot of GRUB scripting?


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

Re: [PATCH 2/4] Accept environment variables on the command line for Xen.

[Vladimir 'phcoder' Serbinenko]

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

I was about to post similar remarks. I'm on phone so will tell just the key
points.
- Most of GRUB platforms have command lines. I.a xen, efi, loongson,
ieee1275, arc.
- Many platforms add their own command line arguments even if user didn't
ask for it. Those shouldn't interfere. In your patch root=sda1 from xen
will either be lost or will clobber GRUB root.
- IEEE1275 code already looks into command line. If it does sth sane it
could be used as base, otherwise it should be replaced with sth common.
- Perhaps we should use some prefix to avoid unintended clobbering.
- There should be a way to emulate pvgrub1 behaviour which took config name
from commandline. Perhaps pvgrub2 should even do it by default (by using
legacy_configfile)
On Dec 12, 2013 4:48 PM, "Andrey Borzenkov" <arvidjaar@gmail.com> wrote:

> В Thu, 12 Dec 2013 15:37:22 +0000
> Colin Watson <cjwatson@ubuntu.com> пишет:
>
> > * grub-core/kern/xen/init.c (fetch_command_line_word): New function.
> > (parse_command_line): Likewise.
> > (grub_machine_init): Call parse_command_line.
> > ---
> >  ChangeLog                 |  8 ++++++++
> >  grub-core/kern/xen/init.c | 44
> ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 52 insertions(+)
> >
> > diff --git a/ChangeLog b/ChangeLog
> > index 766fe4b..fc86601 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,5 +1,13 @@
> >  2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
> >
> > +     Accept environment variables on the command line for Xen.
> > +
>
> Thank you!
>
> It may make sense to generalize it to other archs. At least EFI
> definitely comes in mind. In this case platform specific part is only
> to fetch parameter string(s). What about
>
> - move fetch_command_line_word and parse_command_line to separate file
>   that is included for specific platforms only
> - make each platform that can accept fetch parameters in platform
>   specific way and call common code (parse_command_line)
>
> Does it make sense?
>
> I will add EFI part then later.
>
> > +     * grub-core/kern/xen/init.c (fetch_command_line_word): New
> function.
> > +     (parse_command_line): Likewise.
> > +     (grub_machine_init): Call parse_command_line.
> > +
> > +2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
> > +
> >       Add an option to exclude devices from search results.
> >
> >       * grub-core/commands/search.c (struct search_ctx): Add excludes and
> > diff --git a/grub-core/kern/xen/init.c b/grub-core/kern/xen/init.c
> > index 1d8eaec..eb9b8b3 100644
> > --- a/grub-core/kern/xen/init.c
> > +++ b/grub-core/kern/xen/init.c
> > @@ -525,6 +525,48 @@ map_all_pages (void)
> >    grub_mm_init_region ((void *) heap_start, heap_end - heap_start);
> >  }
> >
> > +static char *
> > + (char *pos, char **word)
> > +{
> > +  while (grub_isspace (*pos))
> > +    pos++;
> > +
> > +  if (!*pos)
> > +    return NULL;
> > +
> > +  *word = pos;
> > +  while (*pos && !grub_isspace (*pos))
> > +    pos++;
> > +  if (*pos)
> > +    *pos++ = '\0';
> > +  return pos;
> > +}
> > +
> > +static void
> > +parse_command_line (void)
> > +{
> > +  char *cmd_line;
> > +  char *pos, *word;
> > +
> > +  cmd_line = grub_malloc (MAX_GUEST_CMDLINE + 1);
> > +  grub_memcpy (cmd_line, grub_xen_start_page_addr->cmd_line,
> > +            MAX_GUEST_CMDLINE);
> > +  cmd_line[MAX_GUEST_CMDLINE] = '\0';
> > +  pos = cmd_line;
> > +  while ((pos = fetch_command_line_word (pos, &word)) != NULL)
> > +    {
> > +      char *equals;
> > +
> > +      equals = grub_strchr (word, '=');
> > +      if (!equals)
> > +     continue;
> > +
> > +      *equals = '\0';
> > +      grub_env_set (word, equals + 1);
> > +    }
> > +  grub_free (cmd_line);
> > +}
> > +
> >  extern char _end[];
> >
> >  void
> > @@ -547,6 +589,8 @@ grub_machine_init (void)
> >    grub_xendisk_init ();
> >
> >    grub_boot_init ();
> > +
> > +  parse_command_line ();
> >  }
> >
> >  void
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 5329 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 3/4] Build grub.xen.

[Colin Watson]

* .gitignore: Add xen.cfg.
* Makefile.am (grub-core/xen.cfg): New rule.
(grub.xen): Likewise.  Add to platform_DATA.
* configure.ac (COND_xen): New conditional.
* grub-core/Makefile.core.def (xen.cfg): New definition.
* grub-core/boot/xen/xen.cfg.in: New file.  This is suitable for
installation both in the dom0 filesystem (where it will find and
chainload a secondary grub.xen) and in the domU filesystem (where it
will load a matching grub.cfg).
---
 .gitignore                    |  1 +
 ChangeLog                     | 14 ++++++++++++++
 Makefile.am                   | 14 ++++++++++++++
 configure.ac                  |  1 +
 grub-core/Makefile.core.def   |  7 +++++++
 grub-core/boot/xen/xen.cfg.in | 31 +++++++++++++++++++++++++++++++
 6 files changed, 68 insertions(+)
 create mode 100644 grub-core/boot/xen/xen.cfg.in

diff --git a/.gitignore b/.gitignore
index 2292cc9..51aa8b4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -153,6 +153,7 @@ trigtables.c
 uhci_test
 update-grub_lib
 unidata.c
+xen.cfg
 xzcompress_test
 Makefile.in
 GPATH
diff --git a/ChangeLog b/ChangeLog
index fc86601..58304f7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,19 @@
 2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
 
+	Build grub.xen.
+
+	* .gitignore: Add xen.cfg.
+	* Makefile.am (grub-core/xen.cfg): New rule.
+	(grub.xen): Likewise.  Add to platform_DATA.
+	* configure.ac (COND_xen): New conditional.
+	* grub-core/Makefile.core.def (xen.cfg): New definition.
+	* grub-core/boot/xen/xen.cfg.in: New file.  This is suitable for
+	installation both in the dom0 filesystem (where it will find and
+	chainload a secondary grub.xen) and in the domU filesystem (where it
+	will load a matching grub.cfg).
+
+2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
+
 	Accept environment variables on the command line for Xen.
 
 	* grub-core/kern/xen/init.c (fetch_command_line_word): New function.
diff --git a/Makefile.am b/Makefile.am
index 0a2c099..e9792f6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -402,6 +402,20 @@ default_payload.elf: grub-mkstandalone grub-mkimage
 	pkgdatadir=. ./grub-mkstandalone --grub-mkimage=./grub-mkimage -O i386-coreboot -o $@ --modules='ahci pata ehci uhci ohci usb_keyboard usbms part_msdos xfs ext2 fat at_keyboard part_gpt usbserial_usbdebug cbfs' --install-modules='ls linux search configfile normal cbtime cbls memrw iorw minicmd lsmmap lspci halt reboot hexdump pcidump regexp setpci lsacpi chain test' --fonts= --themes= --locales= -d grub-core/ /boot/grub/grub.cfg=$(srcdir)/coreboot.cfg
 endif
 
+if COND_xen
+# The grub-core/util split means that we have to duplicate rules a little
+# bit here.
+grub-core/xen.cfg: grub-core/boot/xen/xen.cfg.in
+	$(MAKE) -C grub-core xen.cfg
+
+grub.xen: grub-mkstandalone grub-mkimage grub-core/xen.cfg
+	pkgdatadir=. ./grub-mkstandalone --grub=mkimage=./grub-mkimage \
+		   -o $@ -O $(target_cpu)-$(platform) -d grub-core/ \
+		   /boot/grub/grub.cfg=grub-core/xen.cfg
+
+platform_DATA += grub.xen
+endif
+
 windowsdir=$(top_builddir)/$(PACKAGE)-$(VERSION)-for-windows
 windowsdir: $(PROGRAMS) $(starfield_DATA) $(platform_DATA)
 	test -d $(windowsdir) && rm -rf $(windowsdir) || true
diff --git a/configure.ac b/configure.ac
index 0abbb99..08aa751 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1590,6 +1590,7 @@ AM_CONDITIONAL([COND_i386_ieee1275], [test x$target_cpu = xi386 -a x$platform =
 AM_CONDITIONAL([COND_i386_coreboot], [test x$target_cpu = xi386 -a x$platform = xcoreboot])
 AM_CONDITIONAL([COND_i386_multiboot], [test x$target_cpu = xi386 -a x$platform = xmultiboot])
 AM_CONDITIONAL([COND_x86_64_efi], [test x$target_cpu = xx86_64 -a x$platform = xefi])
+AM_CONDITIONAL([COND_xen], [test x$platform = xxen])
 AM_CONDITIONAL([COND_i386_xen], [test x$target_cpu = xi386 -a x$platform = xxen])
 AM_CONDITIONAL([COND_x86_64_xen], [test x$target_cpu = xx86_64 -a x$platform = xxen])
 AM_CONDITIONAL([COND_mips_loongson], [test x$target_cpu = xmipsel -a x$platform = xloongson])
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 060de44..63a8351 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -44,6 +44,13 @@ script = {
   enable = powerpc_ieee1275;
 };
 
+script = {
+  installdir = platform;
+  name = xen.cfg;
+  common = boot/xen/xen.cfg.in;
+  enable = xen;
+};
+
 kernel = {
   name = kernel;
 
diff --git a/grub-core/boot/xen/xen.cfg.in b/grub-core/boot/xen/xen.cfg.in
new file mode 100644
index 0000000..e2e590c
--- /dev/null
+++ b/grub-core/boot/xen/xen.cfg.in
@@ -0,0 +1,31 @@
+if [ -z "$grub_xen_guest" ]; then
+	# This is the copy of grub.xen installed in the dom0's filesystem.
+	# Look for a copy in the domU's filesystem and chainload that.  This
+	# allows us to guarantee that GRUB will be in sync with the
+	# configuration file in the domU.  The file locations here must not
+	# have any configure-generated substitutions applied, as the intent
+	# is that a single grub.xen should be able to cope with a variety of
+	# domU systems.
+	if search --set=root --file /boot/grub/grub.xen; then
+		linux /boot/grub/grub.xen grub_xen_guest=1
+		boot
+	elif search --set=root --file=/boot/grub2/grub.xen; then
+		linux /boot/grub2/grub.xen grub_xen_guest=1
+		boot
+	else
+		echo "No grub.xen found in guest filesystem.  Tried:"
+		echo "  /boot/grub/grub.xen"
+		echo "  /boot/grub2/grub.xen"
+		halt
+	fi
+else
+	# This is the copy of grub.xen installed in the domU's filesystem.
+	# Read its configuration file.
+	if search --set=root --exclude=memdisk --file \
+			/@bootdirname@/@grubdirname@/grub.cfg; then
+		configfile /@bootdirname@/@grubdirname@/grub.cfg
+	else
+		echo "No /@bootdirname@/@grubdirname@/grub.cfg found in guest filesystem."
+		halt
+	fi
+fi
-- 
1.8.4.4

Re: [PATCH 3/4] Build grub.xen.

[Vladimir 'phcoder' Serbinenko]

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

This config has a security problem. If a user has full acces to some
partition (e.g. fto server partition) he can put grub.xen there and load
his own code
On Dec 12, 2013 4:38 PM, "Colin Watson" <cjwatson@ubuntu.com> wrote:

> * .gitignore: Add xen.cfg.
> * Makefile.am (grub-core/xen.cfg): New rule.
> (grub.xen): Likewise.  Add to platform_DATA.
> * configure.ac (COND_xen): New conditional.
> * grub-core/Makefile.core.def (xen.cfg): New definition.
> * grub-core/boot/xen/xen.cfg.in: New file.  This is suitable for
> installation both in the dom0 filesystem (where it will find and
> chainload a secondary grub.xen) and in the domU filesystem (where it
> will load a matching grub.cfg).
> ---
>  .gitignore                    |  1 +
>  ChangeLog                     | 14 ++++++++++++++
>  Makefile.am                   | 14 ++++++++++++++
>  configure.ac                  |  1 +
>  grub-core/Makefile.core.def   |  7 +++++++
>  grub-core/boot/xen/xen.cfg.in | 31 +++++++++++++++++++++++++++++++
>  6 files changed, 68 insertions(+)
>  create mode 100644 grub-core/boot/xen/xen.cfg.in
>
> diff --git a/.gitignore b/.gitignore
> index 2292cc9..51aa8b4 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -153,6 +153,7 @@ trigtables.c
>  uhci_test
>  update-grub_lib
>  unidata.c
> +xen.cfg
>  xzcompress_test
>  Makefile.in
>  GPATH
> diff --git a/ChangeLog b/ChangeLog
> index fc86601..58304f7 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,19 @@
>  2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
>
> +       Build grub.xen.
> +
> +       * .gitignore: Add xen.cfg.
> +       * Makefile.am (grub-core/xen.cfg): New rule.
> +       (grub.xen): Likewise.  Add to platform_DATA.
> +       * configure.ac (COND_xen): New conditional.
> +       * grub-core/Makefile.core.def (xen.cfg): New definition.
> +       * grub-core/boot/xen/xen.cfg.in: New file.  This is suitable for
> +       installation both in the dom0 filesystem (where it will find and
> +       chainload a secondary grub.xen) and in the domU filesystem (where
> it
> +       will load a matching grub.cfg).
> +
> +2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
> +
>         Accept environment variables on the command line for Xen.
>
>         * grub-core/kern/xen/init.c (fetch_command_line_word): New
> function.
> diff --git a/Makefile.am b/Makefile.am
> index 0a2c099..e9792f6 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -402,6 +402,20 @@ default_payload.elf: grub-mkstandalone grub-mkimage
>         pkgdatadir=. ./grub-mkstandalone --grub-mkimage=./grub-mkimage -O
> i386-coreboot -o $@ --modules='ahci pata ehci uhci ohci usb_keyboard usbms
> part_msdos xfs ext2 fat at_keyboard part_gpt usbserial_usbdebug cbfs'
> --install-modules='ls linux search configfile normal cbtime cbls memrw iorw
> minicmd lsmmap lspci halt reboot hexdump pcidump regexp setpci lsacpi chain
> test' --fonts= --themes= --locales= -d grub-core/
> /boot/grub/grub.cfg=$(srcdir)/coreboot.cfg
>  endif
>
> +if COND_xen
> +# The grub-core/util split means that we have to duplicate rules a little
> +# bit here.
> +grub-core/xen.cfg: grub-core/boot/xen/xen.cfg.in
> +       $(MAKE) -C grub-core xen.cfg
> +
> +grub.xen: grub-mkstandalone grub-mkimage grub-core/xen.cfg
> +       pkgdatadir=. ./grub-mkstandalone --grub=mkimage=./grub-mkimage \
> +                  -o $@ -O $(target_cpu)-$(platform) -d grub-core/ \
> +                  /boot/grub/grub.cfg=grub-core/xen.cfg
> +
> +platform_DATA += grub.xen
> +endif
> +
>  windowsdir=$(top_builddir)/$(PACKAGE)-$(VERSION)-for-windows
>  windowsdir: $(PROGRAMS) $(starfield_DATA) $(platform_DATA)
>         test -d $(windowsdir) && rm -rf $(windowsdir) || true
> diff --git a/configure.ac b/configure.ac
> index 0abbb99..08aa751 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1590,6 +1590,7 @@ AM_CONDITIONAL([COND_i386_ieee1275], [test
> x$target_cpu = xi386 -a x$platform =
>  AM_CONDITIONAL([COND_i386_coreboot], [test x$target_cpu = xi386 -a
> x$platform = xcoreboot])
>  AM_CONDITIONAL([COND_i386_multiboot], [test x$target_cpu = xi386 -a
> x$platform = xmultiboot])
>  AM_CONDITIONAL([COND_x86_64_efi], [test x$target_cpu = xx86_64 -a
> x$platform = xefi])
> +AM_CONDITIONAL([COND_xen], [test x$platform = xxen])
>  AM_CONDITIONAL([COND_i386_xen], [test x$target_cpu = xi386 -a x$platform
> = xxen])
>  AM_CONDITIONAL([COND_x86_64_xen], [test x$target_cpu = xx86_64 -a
> x$platform = xxen])
>  AM_CONDITIONAL([COND_mips_loongson], [test x$target_cpu = xmipsel -a
> x$platform = xloongson])
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 060de44..63a8351 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -44,6 +44,13 @@ script = {
>    enable = powerpc_ieee1275;
>  };
>
> +script = {
> +  installdir = platform;
> +  name = xen.cfg;
> +  common = boot/xen/xen.cfg.in;
> +  enable = xen;
> +};
> +
>  kernel = {
>    name = kernel;
>
> diff --git a/grub-core/boot/xen/xen.cfg.in b/grub-core/boot/xen/xen.cfg.in
> new file mode 100644
> index 0000000..e2e590c
> --- /dev/null
> +++ b/grub-core/boot/xen/xen.cfg.in
> @@ -0,0 +1,31 @@
> +if [ -z "$grub_xen_guest" ]; then
> +       # This is the copy of grub.xen installed in the dom0's filesystem.
> +       # Look for a copy in the domU's filesystem and chainload that.
>  This
> +       # allows us to guarantee that GRUB will be in sync with the
> +       # configuration file in the domU.  The file locations here must not
> +       # have any configure-generated substitutions applied, as the intent
> +       # is that a single grub.xen should be able to cope with a variety
> of
> +       # domU systems.
> +       if search --set=root --file /boot/grub/grub.xen; then
> +               linux /boot/grub/grub.xen grub_xen_guest=1
> +               boot
> +       elif search --set=root --file=/boot/grub2/grub.xen; then
> +               linux /boot/grub2/grub.xen grub_xen_guest=1
> +               boot
> +       else
> +               echo "No grub.xen found in guest filesystem.  Tried:"
> +               echo "  /boot/grub/grub.xen"
> +               echo "  /boot/grub2/grub.xen"
> +               halt
> +       fi
> +else
> +       # This is the copy of grub.xen installed in the domU's filesystem.
> +       # Read its configuration file.
> +       if search --set=root --exclude=memdisk --file \
> +                       /@bootdirname@/@grubdirname@/grub.cfg; then
> +               configfile /@bootdirname@/@grubdirname@/grub.cfg
> +       else
> +               echo "No /@bootdirname@/@grubdirname@/grub.cfg found in
> guest filesystem."
> +               halt
> +       fi
> +fi
> --
> 1.8.4.4
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

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

Re: [PATCH 3/4] Build grub.xen.

[Colin Watson]

On Thu, Dec 12, 2013 at 05:24:50PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> This config has a security problem. If a user has full acces to some
> partition (e.g. fto server partition) he can put grub.xen there and load
> his own code

Only in the domU context, though.  If a user has full access to a guest
filesystem then of course they can run code in the domU.  This seems
unsurprising and not a problem?

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH 3/4] Build grub.xen.

[Vladimir 'phcoder' Serbinenko]

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

No I meant full access to just obe of guest partitions. E.g. FTP may be
using separate partition and non-admins may have control over it. If system
has some kind of automatic user creation and /home is separate someone may
register as boot or grub and put grub.xen in his directory. If /tmp is on
separate partition and not in RAM then everybody can put grub.xen to
/tmp/grub/grub.xen
On Dec 12, 2013 5:39 PM, "Colin Watson" <cjwatson@ubuntu.com> wrote:

> On Thu, Dec 12, 2013 at 05:24:50PM +0100, Vladimir 'phcoder' Serbinenko
> wrote:
> > This config has a security problem. If a user has full acces to some
> > partition (e.g. fto server partition) he can put grub.xen there and load
> > his own code
>
> Only in the domU context, though.  If a user has full access to a guest
> filesystem then of course they can run code in the domU.  This seems
> unsurprising and not a problem?
>
> --
> Colin Watson                                       [cjwatson@ubuntu.com]
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

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

Fwd: Re: [PATCH 3/4] Build grub.xen.

[Vladimir 'phcoder' Serbinenko]

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

---------- Forwarded message ----------
From: "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
Date: Dec 12, 2013 5:45 PM
Subject: Re: [PATCH 3/4] Build grub.xen.
To: "The development of GNU GRUB" <grub-devel@gnu.org>

No I meant full access to just obe of guest partitions. E.g. FTP may be
using separate partition and non-admins may have control over it. If system
has some kind of automatic user creation and /home is separate someone may
register as boot or grub and put grub.xen in his directory. If /tmp is on
separate partition and not in RAM then everybody can put grub.xen to
/tmp/grub/grub.xen
On Dec 12, 2013 5:39 PM, "Colin Watson" <cjwatson@ubuntu.com> wrote:

> On Thu, Dec 12, 2013 at 05:24:50PM +0100, Vladimir 'phcoder' Serbinenko
> wrote:
> > This config has a security problem. If a user has full acces to some
> > partition (e.g. fto server partition) he can put grub.xen there and load
> > his own code
>
> Only in the domU context, though.  If a user has full access to a guest
> filesystem then of course they can run code in the domU.  This seems
> unsurprising and not a problem?
>
> --
> Colin Watson                                       [cjwatson@ubuntu.com]
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

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

Fwd: Re: [PATCH 3/4] Build grub.xen.

[Vladimir 'phcoder' Serbinenko]

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

---------- Forwarded message ----------
From: "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
Date: Dec 12, 2013 5:45 PM
Subject: Re: [PATCH 3/4] Build grub.xen.
To: "The development of GNU GRUB" <grub-devel@gnu.org>

No I meant full access to just obe of guest partitions. E.g. FTP may be
using separate partition and non-admins may have control over it. If system
has some kind of automatic user creation and /home is separate someone may
register as boot or grub and put grub.xen in his directory. If /tmp is on
separate partition and not in RAM then everybody can put grub.xen to
/tmp/grub/grub.xen
On Dec 12, 2013 5:39 PM, "Colin Watson" <cjwatson@ubuntu.com> wrote:

> On Thu, Dec 12, 2013 at 05:24:50PM +0100, Vladimir 'phcoder' Serbinenko
> wrote:
> > This config has a security problem. If a user has full acces to some
> > partition (e.g. fto server partition) he can put grub.xen there and load
> > his own code
>
> Only in the domU context, though.  If a user has full access to a guest
> filesystem then of course they can run code in the domU.  This seems
> unsurprising and not a problem?
>
> --
> Colin Watson                                       [cjwatson@ubuntu.com]
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2080 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/4] Build grub.xen.

[Colin Watson]

On Thu, Dec 12, 2013 at 05:45:30PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> No I meant full access to just obe of guest partitions. E.g. FTP may be
> using separate partition and non-admins may have control over it. If system
> has some kind of automatic user creation and /home is separate someone may
> register as boot or grub and put grub.xen in his directory. If /tmp is on
> separate partition and not in RAM then everybody can put grub.xen to
> /tmp/grub/grub.xen

Oh, right.  Perhaps we could just look in a limited set of devices, e.g.
(xen/xvda) or (xen/xvda1)?  I'm not very familiar with how
multi-partition Xen guests are typically set up.  How is the root
partition generally designated at the moment?

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH 3/4] Build grub.xen.

[Andrey Borzenkov]

В Thu, 12 Dec 2013 17:36:43 +0000
Colin Watson <cjwatson@ubuntu.com> пишет:

> On Thu, Dec 12, 2013 at 05:45:30PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> > No I meant full access to just obe of guest partitions. E.g. FTP may be
> > using separate partition and non-admins may have control over it. If system
> > has some kind of automatic user creation and /home is separate someone may
> > register as boot or grub and put grub.xen in his directory. If /tmp is on
> > separate partition and not in RAM then everybody can put grub.xen to
> > /tmp/grub/grub.xen
> 
> Oh, right.  Perhaps we could just look in a limited set of devices, e.g.
> (xen/xvda) or (xen/xvda1)?

Is passing it as argument acceptable? I'm afraid that no automagic is
going to work for all cases.

>                            I'm not very familiar with how
> multi-partition Xen guests are typically set up.  How is the root
> partition generally designated at the moment?
> 

As I understand it is passed as argument to kernel (i.e. in VM
definition) or embedded in initrd as usual.

Re: [PATCH 3/4] Build grub.xen.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 12.12.2013 18:41, Andrey Borzenkov wrote:
> В Thu, 12 Dec 2013 17:36:43 +0000
> Colin Watson <cjwatson@ubuntu.com> пишет:
> 
>> On Thu, Dec 12, 2013 at 05:45:30PM +0100, Vladimir 'phcoder' Serbinenko wrote:
>>> No I meant full access to just obe of guest partitions. E.g. FTP may be
>>> using separate partition and non-admins may have control over it. If system
>>> has some kind of automatic user creation and /home is separate someone may
>>> register as boot or grub and put grub.xen in his directory. If /tmp is on
>>> separate partition and not in RAM then everybody can put grub.xen to
>>> /tmp/grub/grub.xen
>>
>> Oh, right.  Perhaps we could just look in a limited set of devices, e.g.
>> (xen/xvda) or (xen/xvda1)?
> 
> Is passing it as argument acceptable?
That's what pvgrub1 does. AFAIU it first tries configfile passed as
initrd, then configfile name passed on command line, failing both it
tries netboot.
Perhaps we need a parameter to make it sure that we won't try to
interpret old menu.lst or to load it. E.g.
grub.xenfile=(xen/xvda,1)/boot/grub/grubx64.xen
or
grub.config=(xen/xvda,1)/boot/grub/grub.cfg
> I'm afraid that no automagic is
> going to work for all cases.
> 
What we do here is essentially define "firmware" interface that would be
followed by installer. Ideally virtual nvram would be a solution but xen
has none.
A lazy possibility is to force creating of EFI system partition (in GPT
or MBR) and load /xen/boot/boot[x64|pae].xen but probably this is far
from ideal
>>                            I'm not very familiar with how
>> multi-partition Xen guests are typically set up.  How is the root
>> partition generally designated at the moment?
>>
> 
> As I understand it is passed as argument to kernel (i.e. in VM
> definition) or embedded in initrd as usual.
> 
There are also scenarios when guest admin has no right to modify machine
description but wants to install another OS. Idk if we'll need special
support for this as guest admin can always arrange for grub.xen to land
in right place by using tiny partition for this.


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

Re: [PATCH 3/4] Build grub.xen.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 12.12.2013 18:41, Andrey Borzenkov wrote:
> В Thu, 12 Dec 2013 17:36:43 +0000
> Colin Watson <cjwatson@ubuntu.com> пишет:
> 
>> On Thu, Dec 12, 2013 at 05:45:30PM +0100, Vladimir 'phcoder' Serbinenko wrote:
>>> No I meant full access to just obe of guest partitions. E.g. FTP may be
>>> using separate partition and non-admins may have control over it. If system
>>> has some kind of automatic user creation and /home is separate someone may
>>> register as boot or grub and put grub.xen in his directory. If /tmp is on
>>> separate partition and not in RAM then everybody can put grub.xen to
>>> /tmp/grub/grub.xen
>>
>> Oh, right.  Perhaps we could just look in a limited set of devices, e.g.
>> (xen/xvda) or (xen/xvda1)?
> 
> Is passing it as argument acceptable?
That's what pvgrub1 does. AFAIU it first tries configfile passed as
initrd, then configfile name passed on command line, failing both it
tries netboot.
Perhaps we need a parameter to make it sure that we won't try to
interpret old menu.lst or to load it. E.g.
grub.xenfile=(xen/xvda,1)/boot/grub/grubx64.xen
or
grub.config=(xen/xvda,1)/boot/grub/grub.cfg
> I'm afraid that no automagic is
> going to work for all cases.
> 
What we do here is essentially define "firmware" interface that would be
followed by installer. Ideally virtual nvram would be a solution but xen
has none.
A lazy possibility is to force creating of EFI system partition (in GPT
or MBR) and load /xen/boot/boot[x64|pae].xen but probably this is far
from ideal
>>                            I'm not very familiar with how
>> multi-partition Xen guests are typically set up.  How is the root
>> partition generally designated at the moment?
>>
> 
> As I understand it is passed as argument to kernel (i.e. in VM
> definition) or embedded in initrd as usual.
> 
There are also scenarios when guest admin has no right to modify machine
description but wants to install another OS. Idk if we'll need special
support for this as guest admin can always arrange for grub.xen to land
in right place by using tiny partition for this.


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

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/4] Build grub.xen.

[Colin Watson]

On Thu, Dec 12, 2013 at 03:37:41PM +0000, Colin Watson wrote:
> +if [ -z "$grub_xen_guest" ]; then
> +	# This is the copy of grub.xen installed in the dom0's filesystem.
> +	# Look for a copy in the domU's filesystem and chainload that.  This
> +	# allows us to guarantee that GRUB will be in sync with the
> +	# configuration file in the domU.  The file locations here must not
> +	# have any configure-generated substitutions applied, as the intent
> +	# is that a single grub.xen should be able to cope with a variety of
> +	# domU systems.
> +	if search --set=root --file /boot/grub/grub.xen; then
> +		linux /boot/grub/grub.xen grub_xen_guest=1
> +		boot
[...]

I talked about this with Ian Jackson in the pub last night.  We came to
the same conclusion more or less at the same time, that this is in fact
a new boot protocol; since it essentially just expects a bzImage here,
there's nothing to stop somebody for example putting a bare kernel
there.  We'd like people to be able to set up PV-GRUB2 in their dom0s
even for domUs that aren't new enough to have PV-GRUB2 inside them.
Furthermore we'd like to be able to arrange that PV-GRUB (Legacy) and
PyGRUB can at least in principle use the same boot protocol; in the case
of PV-GRUB that would presumably involve a stub menu.lst, but it
shouldn't take much more than that.  As such the file name in the domU's
filesystem shouldn't be GRUB-specific, although per Vladimir it'd be
good for it to be distinct for each architecture.

I'll put this patch series on hold for the time being (with the possible
exception of "search --exclude", which I think has been uncontroversial
so far and could perhaps be merged as a generally-useful gadget?) and
write this up as a proper protocol document for inclusion in xen.git.
Ian said he'd like to get this into Xen 4.4's documentation.  No changes
to Xen code are required as far as I know.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH 3/4] Build grub.xen.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 13.12.2013 12:56, Colin Watson wrote:
> On Thu, Dec 12, 2013 at 03:37:41PM +0000, Colin Watson wrote:
>> +if [ -z "$grub_xen_guest" ]; then
>> +	# This is the copy of grub.xen installed in the dom0's filesystem.
>> +	# Look for a copy in the domU's filesystem and chainload that.  This
>> +	# allows us to guarantee that GRUB will be in sync with the
>> +	# configuration file in the domU.  The file locations here must not
>> +	# have any configure-generated substitutions applied, as the intent
>> +	# is that a single grub.xen should be able to cope with a variety of
>> +	# domU systems.
>> +	if search --set=root --file /boot/grub/grub.xen; then
>> +		linux /boot/grub/grub.xen grub_xen_guest=1
>> +		boot
> [...]
> 
> I talked about this with Ian Jackson in the pub last night.

>  We came to
> the same conclusion more or less at the same time, that this is in fact
> a new boot protocol; since it essentially just expects a bzImage here,
Not a bzimage but ELF, optionally compressed/wrapped in bzimage.
> there's nothing to stop somebody for example putting a bare kernel
> there.
Yes, this is a possibility. You'd have to comile the options in it though.
> We'd like people to be able to set up PV-GRUB2 in their dom0s
> even for domUs that aren't new enough to have PV-GRUB2 inside them.
Yes, that's why I spoke about compatibility. But it's good that this
subject is explicitly discussed.
The main problem with this is security, as discussed in unprivileged
partition subthread. The best way is to replicate pvgrub1 behaviour for
such systems.
> Furthermore we'd like to be able to arrange that PV-GRUB (Legacy) and
> PyGRUB can at least in principle use the same boot protocol; in the case
> of PV-GRUB that would presumably involve a stub menu.lst, but it
> shouldn't take much more than that.
The legacy_configfile would need adjustments for that. PVGRUB1 used hdX
notation for virtual disks which legacy_configfile just maps to hdX.
This is a mess because even on runtime it's not obvious how disks are
mapped, it may even differ between pvgrub1 versions.
>  As such the file name in the domU's
> filesystem shouldn't be GRUB-specific, although per Vladimir it'd be
> good for it to be distinct for each architecture.
> 
Agreed. But if we save it, it should contain full xen device name,
partition specification (beware that partition numbering is ambiguous
and if you use partition number you have to specify how it's counted.
Otherwise you may opt to have some kind of static specification where to
locate the file. Using a special partition for this is an option and if
so, IMO, it's better to share it with EFI system partition.
> I'll put this patch series on hold for the time being (with the possible
> exception of "search --exclude", which I think has been uncontroversial
> so far and could perhaps be merged as a generally-useful gadget?)
I didn't review it for the reasons of clarifying intended usage. It has
problems, I'll answer in its thread.
> and
> write this up as a proper protocol document for inclusion in xen.git.
> Ian said he'd like to get this into Xen 4.4's documentation.  No changes
> to Xen code are required as far as I know.
>
Ok, could we be kept in the loop on the drafts? I'd hate to have to cope
with yet another ambiguous protocol specification.




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

Re: [PATCH 3/4] Build grub.xen.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 13.12.2013 12:56, Colin Watson wrote:
> On Thu, Dec 12, 2013 at 03:37:41PM +0000, Colin Watson wrote:
>> +if [ -z "$grub_xen_guest" ]; then
>> +	# This is the copy of grub.xen installed in the dom0's filesystem.
>> +	# Look for a copy in the domU's filesystem and chainload that.  This
>> +	# allows us to guarantee that GRUB will be in sync with the
>> +	# configuration file in the domU.  The file locations here must not
>> +	# have any configure-generated substitutions applied, as the intent
>> +	# is that a single grub.xen should be able to cope with a variety of
>> +	# domU systems.
>> +	if search --set=root --file /boot/grub/grub.xen; then
>> +		linux /boot/grub/grub.xen grub_xen_guest=1
>> +		boot
> [...]
> 
> I talked about this with Ian Jackson in the pub last night.

>  We came to
> the same conclusion more or less at the same time, that this is in fact
> a new boot protocol; since it essentially just expects a bzImage here,
Not a bzimage but ELF, optionally compressed/wrapped in bzimage.
> there's nothing to stop somebody for example putting a bare kernel
> there.
Yes, this is a possibility. You'd have to comile the options in it though.
> We'd like people to be able to set up PV-GRUB2 in their dom0s
> even for domUs that aren't new enough to have PV-GRUB2 inside them.
Yes, that's why I spoke about compatibility. But it's good that this
subject is explicitly discussed.
The main problem with this is security, as discussed in unprivileged
partition subthread. The best way is to replicate pvgrub1 behaviour for
such systems.
> Furthermore we'd like to be able to arrange that PV-GRUB (Legacy) and
> PyGRUB can at least in principle use the same boot protocol; in the case
> of PV-GRUB that would presumably involve a stub menu.lst, but it
> shouldn't take much more than that.
The legacy_configfile would need adjustments for that. PVGRUB1 used hdX
notation for virtual disks which legacy_configfile just maps to hdX.
This is a mess because even on runtime it's not obvious how disks are
mapped, it may even differ between pvgrub1 versions.
>  As such the file name in the domU's
> filesystem shouldn't be GRUB-specific, although per Vladimir it'd be
> good for it to be distinct for each architecture.
> 
Agreed. But if we save it, it should contain full xen device name,
partition specification (beware that partition numbering is ambiguous
and if you use partition number you have to specify how it's counted.
Otherwise you may opt to have some kind of static specification where to
locate the file. Using a special partition for this is an option and if
so, IMO, it's better to share it with EFI system partition.
> I'll put this patch series on hold for the time being (with the possible
> exception of "search --exclude", which I think has been uncontroversial
> so far and could perhaps be merged as a generally-useful gadget?)
I didn't review it for the reasons of clarifying intended usage. It has
problems, I'll answer in its thread.
> and
> write this up as a proper protocol document for inclusion in xen.git.
> Ian said he'd like to get this into Xen 4.4's documentation.  No changes
> to Xen code are required as far as I know.
>
Ok, could we be kept in the loop on the drafts? I'd hate to have to cope
with yet another ambiguous protocol specification.




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

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/4] Build grub.xen.

[Colin Watson]

On Thu, Dec 12, 2013 at 03:37:41PM +0000, Colin Watson wrote:
> +if [ -z "$grub_xen_guest" ]; then
> +	# This is the copy of grub.xen installed in the dom0's filesystem.
> +	# Look for a copy in the domU's filesystem and chainload that.  This
> +	# allows us to guarantee that GRUB will be in sync with the
> +	# configuration file in the domU.  The file locations here must not
> +	# have any configure-generated substitutions applied, as the intent
> +	# is that a single grub.xen should be able to cope with a variety of
> +	# domU systems.
> +	if search --set=root --file /boot/grub/grub.xen; then
> +		linux /boot/grub/grub.xen grub_xen_guest=1
> +		boot
[...]

I talked about this with Ian Jackson in the pub last night.  We came to
the same conclusion more or less at the same time, that this is in fact
a new boot protocol; since it essentially just expects a bzImage here,
there's nothing to stop somebody for example putting a bare kernel
there.  We'd like people to be able to set up PV-GRUB2 in their dom0s
even for domUs that aren't new enough to have PV-GRUB2 inside them.
Furthermore we'd like to be able to arrange that PV-GRUB (Legacy) and
PyGRUB can at least in principle use the same boot protocol; in the case
of PV-GRUB that would presumably involve a stub menu.lst, but it
shouldn't take much more than that.  As such the file name in the domU's
filesystem shouldn't be GRUB-specific, although per Vladimir it'd be
good for it to be distinct for each architecture.

I'll put this patch series on hold for the time being (with the possible
exception of "search --exclude", which I think has been uncontroversial
so far and could perhaps be merged as a generally-useful gadget?) and
write this up as a proper protocol document for inclusion in xen.git.
Ian said he'd like to get this into Xen 4.4's documentation.  No changes
to Xen code are required as far as I know.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

[PATCH 3/4] Build grub.xen.

[Colin Watson]

* .gitignore: Add xen.cfg.
* Makefile.am (grub-core/xen.cfg): New rule.
(grub.xen): Likewise.  Add to platform_DATA.
* configure.ac (COND_xen): New conditional.
* grub-core/Makefile.core.def (xen.cfg): New definition.
* grub-core/boot/xen/xen.cfg.in: New file.  This is suitable for
installation both in the dom0 filesystem (where it will find and
chainload a secondary grub.xen) and in the domU filesystem (where it
will load a matching grub.cfg).
---
 .gitignore                    |  1 +
 ChangeLog                     | 14 ++++++++++++++
 Makefile.am                   | 14 ++++++++++++++
 configure.ac                  |  1 +
 grub-core/Makefile.core.def   |  7 +++++++
 grub-core/boot/xen/xen.cfg.in | 31 +++++++++++++++++++++++++++++++
 6 files changed, 68 insertions(+)
 create mode 100644 grub-core/boot/xen/xen.cfg.in

diff --git a/.gitignore b/.gitignore
index 2292cc9..51aa8b4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -153,6 +153,7 @@ trigtables.c
 uhci_test
 update-grub_lib
 unidata.c
+xen.cfg
 xzcompress_test
 Makefile.in
 GPATH
diff --git a/ChangeLog b/ChangeLog
index fc86601..58304f7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,19 @@
 2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
 
+	Build grub.xen.
+
+	* .gitignore: Add xen.cfg.
+	* Makefile.am (grub-core/xen.cfg): New rule.
+	(grub.xen): Likewise.  Add to platform_DATA.
+	* configure.ac (COND_xen): New conditional.
+	* grub-core/Makefile.core.def (xen.cfg): New definition.
+	* grub-core/boot/xen/xen.cfg.in: New file.  This is suitable for
+	installation both in the dom0 filesystem (where it will find and
+	chainload a secondary grub.xen) and in the domU filesystem (where it
+	will load a matching grub.cfg).
+
+2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
+
 	Accept environment variables on the command line for Xen.
 
 	* grub-core/kern/xen/init.c (fetch_command_line_word): New function.
diff --git a/Makefile.am b/Makefile.am
index 0a2c099..e9792f6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -402,6 +402,20 @@ default_payload.elf: grub-mkstandalone grub-mkimage
 	pkgdatadir=. ./grub-mkstandalone --grub-mkimage=./grub-mkimage -O i386-coreboot -o $@ --modules='ahci pata ehci uhci ohci usb_keyboard usbms part_msdos xfs ext2 fat at_keyboard part_gpt usbserial_usbdebug cbfs' --install-modules='ls linux search configfile normal cbtime cbls memrw iorw minicmd lsmmap lspci halt reboot hexdump pcidump regexp setpci lsacpi chain test' --fonts= --themes= --locales= -d grub-core/ /boot/grub/grub.cfg=$(srcdir)/coreboot.cfg
 endif
 
+if COND_xen
+# The grub-core/util split means that we have to duplicate rules a little
+# bit here.
+grub-core/xen.cfg: grub-core/boot/xen/xen.cfg.in
+	$(MAKE) -C grub-core xen.cfg
+
+grub.xen: grub-mkstandalone grub-mkimage grub-core/xen.cfg
+	pkgdatadir=. ./grub-mkstandalone --grub=mkimage=./grub-mkimage \
+		   -o $@ -O $(target_cpu)-$(platform) -d grub-core/ \
+		   /boot/grub/grub.cfg=grub-core/xen.cfg
+
+platform_DATA += grub.xen
+endif
+
 windowsdir=$(top_builddir)/$(PACKAGE)-$(VERSION)-for-windows
 windowsdir: $(PROGRAMS) $(starfield_DATA) $(platform_DATA)
 	test -d $(windowsdir) && rm -rf $(windowsdir) || true
diff --git a/configure.ac b/configure.ac
index 0abbb99..08aa751 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1590,6 +1590,7 @@ AM_CONDITIONAL([COND_i386_ieee1275], [test x$target_cpu = xi386 -a x$platform =
 AM_CONDITIONAL([COND_i386_coreboot], [test x$target_cpu = xi386 -a x$platform = xcoreboot])
 AM_CONDITIONAL([COND_i386_multiboot], [test x$target_cpu = xi386 -a x$platform = xmultiboot])
 AM_CONDITIONAL([COND_x86_64_efi], [test x$target_cpu = xx86_64 -a x$platform = xefi])
+AM_CONDITIONAL([COND_xen], [test x$platform = xxen])
 AM_CONDITIONAL([COND_i386_xen], [test x$target_cpu = xi386 -a x$platform = xxen])
 AM_CONDITIONAL([COND_x86_64_xen], [test x$target_cpu = xx86_64 -a x$platform = xxen])
 AM_CONDITIONAL([COND_mips_loongson], [test x$target_cpu = xmipsel -a x$platform = xloongson])
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 060de44..63a8351 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -44,6 +44,13 @@ script = {
   enable = powerpc_ieee1275;
 };
 
+script = {
+  installdir = platform;
+  name = xen.cfg;
+  common = boot/xen/xen.cfg.in;
+  enable = xen;
+};
+
 kernel = {
   name = kernel;
 
diff --git a/grub-core/boot/xen/xen.cfg.in b/grub-core/boot/xen/xen.cfg.in
new file mode 100644
index 0000000..e2e590c
--- /dev/null
+++ b/grub-core/boot/xen/xen.cfg.in
@@ -0,0 +1,31 @@
+if [ -z "$grub_xen_guest" ]; then
+	# This is the copy of grub.xen installed in the dom0's filesystem.
+	# Look for a copy in the domU's filesystem and chainload that.  This
+	# allows us to guarantee that GRUB will be in sync with the
+	# configuration file in the domU.  The file locations here must not
+	# have any configure-generated substitutions applied, as the intent
+	# is that a single grub.xen should be able to cope with a variety of
+	# domU systems.
+	if search --set=root --file /boot/grub/grub.xen; then
+		linux /boot/grub/grub.xen grub_xen_guest=1
+		boot
+	elif search --set=root --file=/boot/grub2/grub.xen; then
+		linux /boot/grub2/grub.xen grub_xen_guest=1
+		boot
+	else
+		echo "No grub.xen found in guest filesystem.  Tried:"
+		echo "  /boot/grub/grub.xen"
+		echo "  /boot/grub2/grub.xen"
+		halt
+	fi
+else
+	# This is the copy of grub.xen installed in the domU's filesystem.
+	# Read its configuration file.
+	if search --set=root --exclude=memdisk --file \
+			/@bootdirname@/@grubdirname@/grub.cfg; then
+		configfile /@bootdirname@/@grubdirname@/grub.cfg
+	else
+		echo "No /@bootdirname@/@grubdirname@/grub.cfg found in guest filesystem."
+		halt
+	fi
+fi
-- 
1.8.4.4

[PATCH 4/4] Improve installation on Xen.

[Colin Watson]

* util/grub-install.c: Copy core image to GRUBDIR/grub.xen on Xen
platforms.
---
 ChangeLog           |  7 +++++++
 util/grub-install.c | 11 +++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 58304f7..f280b59 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
 
+	Improve installation on Xen.
+
+	* util/grub-install.c: Copy core image to GRUBDIR/grub.xen on Xen
+	platforms.
+
+2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
+
 	Build grub.xen.
 
 	* .gitignore: Add xen.cfg.
diff --git a/util/grub-install.c b/util/grub-install.c
index 5d22f90..2da05c2 100644
--- a/util/grub-install.c
+++ b/util/grub-install.c
@@ -1576,6 +1576,15 @@ main (int argc, char *argv[])
 	}
       break;
 
+    case GRUB_INSTALL_PLATFORM_I386_XEN:
+    case GRUB_INSTALL_PLATFORM_X86_64_XEN:
+      {
+	char *dst = grub_util_path_concat (2, grubdir, "grub.xen");
+	grub_install_copy_file (imgfile, dst, 1);
+	free (dst);
+      }
+      break;
+
     case GRUB_INSTALL_PLATFORM_MIPSEL_LOONGSON:
     case GRUB_INSTALL_PLATFORM_MIPSEL_QEMU_MIPS:
     case GRUB_INSTALL_PLATFORM_MIPS_QEMU_MIPS:
@@ -1584,8 +1593,6 @@ main (int argc, char *argv[])
     case GRUB_INSTALL_PLATFORM_MIPSEL_ARC:
     case GRUB_INSTALL_PLATFORM_ARM_UBOOT:
     case GRUB_INSTALL_PLATFORM_I386_QEMU:
-    case GRUB_INSTALL_PLATFORM_I386_XEN:
-    case GRUB_INSTALL_PLATFORM_X86_64_XEN:
       grub_util_warn ("%s",
 		      _("WARNING: no platform-specific install was performed"));
       break;
-- 
1.8.4.4

Re: [PATCH 4/4] Improve installation on Xen.

[Vladimir 'phcoder' Serbinenko]

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

This patch would make 32-bit and 64-bit GRUB conflict. This is a valid
usecase as it's possible to make biarch image by having both kernels and
32-bit userspace
On Dec 12, 2013 4:38 PM, "Colin Watson" <cjwatson@ubuntu.com> wrote:

> * util/grub-install.c: Copy core image to GRUBDIR/grub.xen on Xen
> platforms.
> ---
>  ChangeLog           |  7 +++++++
>  util/grub-install.c | 11 +++++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 58304f7..f280b59 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,12 @@
>  2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
>
> +       Improve installation on Xen.
> +
> +       * util/grub-install.c: Copy core image to GRUBDIR/grub.xen on Xen
> +       platforms.
> +
> +2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
> +
>         Build grub.xen.
>
>         * .gitignore: Add xen.cfg.
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 5d22f90..2da05c2 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -1576,6 +1576,15 @@ main (int argc, char *argv[])
>         }
>        break;
>
> +    case GRUB_INSTALL_PLATFORM_I386_XEN:
> +    case GRUB_INSTALL_PLATFORM_X86_64_XEN:
> +      {
> +       char *dst = grub_util_path_concat (2, grubdir, "grub.xen");
> +       grub_install_copy_file (imgfile, dst, 1);
> +       free (dst);
> +      }
> +      break;
> +
>      case GRUB_INSTALL_PLATFORM_MIPSEL_LOONGSON:
>      case GRUB_INSTALL_PLATFORM_MIPSEL_QEMU_MIPS:
>      case GRUB_INSTALL_PLATFORM_MIPS_QEMU_MIPS:
> @@ -1584,8 +1593,6 @@ main (int argc, char *argv[])
>      case GRUB_INSTALL_PLATFORM_MIPSEL_ARC:
>      case GRUB_INSTALL_PLATFORM_ARM_UBOOT:
>      case GRUB_INSTALL_PLATFORM_I386_QEMU:
> -    case GRUB_INSTALL_PLATFORM_I386_XEN:
> -    case GRUB_INSTALL_PLATFORM_X86_64_XEN:
>        grub_util_warn ("%s",
>                       _("WARNING: no platform-specific install was
> performed"));
>        break;
> --
> 1.8.4.4
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2765 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 4/4] Improve installation on Xen.

[Vladimir 'phcoder' Serbinenko]

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

This patch would make 32-bit and 64-bit GRUB conflict. This is a valid
usecase as it's possible to make biarch image by having both kernels and
32-bit userspace
On Dec 12, 2013 4:38 PM, "Colin Watson" <cjwatson@ubuntu.com> wrote:

> * util/grub-install.c: Copy core image to GRUBDIR/grub.xen on Xen
> platforms.
> ---
>  ChangeLog           |  7 +++++++
>  util/grub-install.c | 11 +++++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 58304f7..f280b59 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,12 @@
>  2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
>
> +       Improve installation on Xen.
> +
> +       * util/grub-install.c: Copy core image to GRUBDIR/grub.xen on Xen
> +       platforms.
> +
> +2013-12-12  Colin Watson  <cjwatson@ubuntu.com>
> +
>         Build grub.xen.
>
>         * .gitignore: Add xen.cfg.
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 5d22f90..2da05c2 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -1576,6 +1576,15 @@ main (int argc, char *argv[])
>         }
>        break;
>
> +    case GRUB_INSTALL_PLATFORM_I386_XEN:
> +    case GRUB_INSTALL_PLATFORM_X86_64_XEN:
> +      {
> +       char *dst = grub_util_path_concat (2, grubdir, "grub.xen");
> +       grub_install_copy_file (imgfile, dst, 1);
> +       free (dst);
> +      }
> +      break;
> +
>      case GRUB_INSTALL_PLATFORM_MIPSEL_LOONGSON:
>      case GRUB_INSTALL_PLATFORM_MIPSEL_QEMU_MIPS:
>      case GRUB_INSTALL_PLATFORM_MIPS_QEMU_MIPS:
> @@ -1584,8 +1593,6 @@ main (int argc, char *argv[])
>      case GRUB_INSTALL_PLATFORM_MIPSEL_ARC:
>      case GRUB_INSTALL_PLATFORM_ARM_UBOOT:
>      case GRUB_INSTALL_PLATFORM_I386_QEMU:
> -    case GRUB_INSTALL_PLATFORM_I386_XEN:
> -    case GRUB_INSTALL_PLATFORM_X86_64_XEN:
>        grub_util_warn ("%s",
>                       _("WARNING: no platform-specific install was
> performed"));
>        break;
> --
> 1.8.4.4
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

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

Re: [Xen-devel] [PATCH 4/4] Improve installation on Xen.

[Colin Watson]

On Thu, Dec 12, 2013 at 05:23:10PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> This patch would make 32-bit and 64-bit GRUB conflict. This is a valid
> usecase as it's possible to make biarch image by having both kernels and
> 32-bit userspace

Agreed.  Indeed the Debian packaging would naturally work out that way
by default as I've put both i386-xen and x86_64-xen in the same package
as previously discussed, so the postinst would want to run "grub-install
--target=i386-xen && grub-install --target=x86_64-xen".

I'll make sure that the next version of this patch series uses
architecture-specific paths.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [Xen-devel] [PATCH 0/4] Turn-key PV-GRUB2 installation

[Ian Campbell]

On Thu, 2013-12-12 at 15:36 +0000, Colin Watson wrote:
> This patch set should make it easier to maintain PV-GRUB2 installations.
> The general idea is based on discussions I had with Xen developers
> (mainly Ian Jackson) at the Ubuntu Developer Summit in May 2011; though
> I never did manage to get the core port done and Vladimir beat me to
> that, I think the configuration approach we discussed there is still
> valid and useful.
> 
> The idea here is that people maintaining dom0s, especially in big cloud
> installations, will probably not want to update their PV-GRUB2 image
> (which will be read from the dom0 filesystem but will run in the domU)
> very often; but on the other hand people maintaining distributions will
> want to be able to move on and make use of latest-and-greatest features.
> To make this work, we build a dom0-filesystem-suitable grub.xen image
> which just tries to chainload another grub.xen from a couple of
> reasonably standard locations in the domU's filesystem
> (/boot/grub/grub.xen and /boot/grub2/grub.xen).

This all sounds rather sensible. (actually, it sounds pretty awesome!)

A bit unrelated, and perhaps a bit far fetched, but I wonder if it might
be possible to chainload somehow from dom0's pvgrub1 to a pvgrub2 from
within the guest. I'm thinking of how long it might take cloud providers
to start providing pvgrub2 from their dom0s...

> 
> I've arranged that you can either use that very same grub.xen image in
> the domU by way of a new mechanism that passes GRUB environment
> variables via the Xen guest command line so that we can tell which
> context we're in, or you can run "grub-install --target=x86_64-xen" in
> the domU which will build and install a suitable one in more or less the
> usual way.
> 
> Colin Watson (4):
>   Add an option to exclude devices from search results.
>   Accept environment variables on the command line for Xen.
>   Build grub.xen.
>   Improve installation on Xen.
> 
>  .gitignore                       |  1 +
>  ChangeLog                        | 45 ++++++++++++++++++++++++++++++++++++++++
>  Makefile.am                      | 14 +++++++++++++
>  configure.ac                     |  1 +
>  docs/grub.texi                   |  7 ++++++-
>  grub-core/Makefile.core.def      |  7 +++++++
>  grub-core/boot/xen/xen.cfg.in    | 31 +++++++++++++++++++++++++++
>  grub-core/commands/search.c      | 15 ++++++++++++--
>  grub-core/commands/search_wrap.c | 27 +++++++++++++++++++-----
>  grub-core/kern/xen/init.c        | 44 +++++++++++++++++++++++++++++++++++++++
>  include/grub/search.h            |  9 +++++---
>  util/grub-install.c              | 11 ++++++++--
>  12 files changed, 199 insertions(+), 13 deletions(-)
>  create mode 100644 grub-core/boot/xen/xen.cfg.in
> 

Re: [Xen-devel] [PATCH 0/4] Turn-key PV-GRUB2 installation

[Samuel Thibault]

Ian Campbell, le Mon 16 Dec 2013 11:42:03 +0000, a écrit :
> I wonder if it might be possible to chainload somehow from dom0's
> pvgrub1 to a pvgrub2 from within the guest.

I don't see any ground reason why it shouldn't be possible. I don't have
a pvgrub2 image under my hand, but pvgrub1 is able to chainload itself,
for instance.

Samuel

Re: [PATCH 0/4] Turn-key PV-GRUB2 installation

[Samuel Thibault]

Ian Campbell, le Mon 16 Dec 2013 11:42:03 +0000, a écrit :
> I wonder if it might be possible to chainload somehow from dom0's
> pvgrub1 to a pvgrub2 from within the guest.

I don't see any ground reason why it shouldn't be possible. I don't have
a pvgrub2 image under my hand, but pvgrub1 is able to chainload itself,
for instance.

Samuel

Re: [PATCH 0/4] Turn-key PV-GRUB2 installation

[Ian Campbell]

On Thu, 2013-12-12 at 15:36 +0000, Colin Watson wrote:
> This patch set should make it easier to maintain PV-GRUB2 installations.
> The general idea is based on discussions I had with Xen developers
> (mainly Ian Jackson) at the Ubuntu Developer Summit in May 2011; though
> I never did manage to get the core port done and Vladimir beat me to
> that, I think the configuration approach we discussed there is still
> valid and useful.
> 
> The idea here is that people maintaining dom0s, especially in big cloud
> installations, will probably not want to update their PV-GRUB2 image
> (which will be read from the dom0 filesystem but will run in the domU)
> very often; but on the other hand people maintaining distributions will
> want to be able to move on and make use of latest-and-greatest features.
> To make this work, we build a dom0-filesystem-suitable grub.xen image
> which just tries to chainload another grub.xen from a couple of
> reasonably standard locations in the domU's filesystem
> (/boot/grub/grub.xen and /boot/grub2/grub.xen).

This all sounds rather sensible. (actually, it sounds pretty awesome!)

A bit unrelated, and perhaps a bit far fetched, but I wonder if it might
be possible to chainload somehow from dom0's pvgrub1 to a pvgrub2 from
within the guest. I'm thinking of how long it might take cloud providers
to start providing pvgrub2 from their dom0s...

> 
> I've arranged that you can either use that very same grub.xen image in
> the domU by way of a new mechanism that passes GRUB environment
> variables via the Xen guest command line so that we can tell which
> context we're in, or you can run "grub-install --target=x86_64-xen" in
> the domU which will build and install a suitable one in more or less the
> usual way.
> 
> Colin Watson (4):
>   Add an option to exclude devices from search results.
>   Accept environment variables on the command line for Xen.
>   Build grub.xen.
>   Improve installation on Xen.
> 
>  .gitignore                       |  1 +
>  ChangeLog                        | 45 ++++++++++++++++++++++++++++++++++++++++
>  Makefile.am                      | 14 +++++++++++++
>  configure.ac                     |  1 +
>  docs/grub.texi                   |  7 ++++++-
>  grub-core/Makefile.core.def      |  7 +++++++
>  grub-core/boot/xen/xen.cfg.in    | 31 +++++++++++++++++++++++++++
>  grub-core/commands/search.c      | 15 ++++++++++++--
>  grub-core/commands/search_wrap.c | 27 +++++++++++++++++++-----
>  grub-core/kern/xen/init.c        | 44 +++++++++++++++++++++++++++++++++++++++
>  include/grub/search.h            |  9 +++++---
>  util/grub-install.c              | 11 ++++++++--
>  12 files changed, 199 insertions(+), 13 deletions(-)
>  create mode 100644 grub-core/boot/xen/xen.cfg.in
>