Re: Which partitioning schemes should be supported by GRUB?

[Colin Watson <cjwatson@ubuntu.com>]

On Mon, Jun 14, 2010 at 07:12:38PM +0200, Grégoire Sutre wrote:
> On 06/14/2010 18:43, Colin Watson wrote:
>> Do you have any suggestions on how to deal with that?  I'm not familiar
>> with multiboot and need guidance.
>
> A possible solution would be to use the multiboot-command line.  AFAIK,
> the boot_device field of the multiboot information structure is supposed
> to pass this kind of partition information, but you cannot specify the
> partmaps in this field, hence its interpretation is ambiguous.

That would potentially allow a user to override things, but doesn't help
with users who don't change their configuration.  Unless the user
explicitly configures things, boot_device is all we've got.

Thus, I guess we end up with a two-part fix:

  1) Honour key=value pairs in the multiboot command line when booting
     GRUB itself as a multiboot image.  These would simply become
     environment variables.  Presumably this goes in grub_machine_init,
     by analogy with kern/ieee1275/init.c.  This allows users to
     override the prefix on the command line as if they had changed the
     image itself.

  2) If multiboot_trampoline needs to change install_dos_part or
     install_bsd_part based on the value of boot_device in the MBI, then
     we know that the drive/partition part of prefix (which was
     calculated in the same way as install_dos_part and install_bsd_part
     when grub-setup was run) is now invalid and should be ignored.
     This fact needs to be passed on to make_install_device.

Something like this?  I'm afraid I have no idea how to test this (GRUB's
multiboot command doesn't seem to accept command-line arguments?), not
to mention that this is the first time I've been anywhere near most of
this code.  I would greatly appreciate advice and review.

2010-06-15  Colin Watson  <cjwatson@ubuntu.com>

	Fix i386-pc prefix handling with nested partitions (Debian bug
	#585068).

	* include/grub/i386/pc/kernel.h (grub_multiboot_change_prefix): New
	declaration.
	* kern/i386/pc/startup.S (multiboot_entry): Save a pointer to the
	MBI in startup_multiboot_info.
	(multiboot_trampoline): If the new partition numbers differ from the
	previous ones, then set grub_multiboot_change_prefix.
	(grub_multiboot_change_prefix): New definition.
	* kern/i386/pc/init.c (make_install_device): Invalidate any
	drive/partition part of the prefix if grub_multiboot_change_prefix
	is set.  After that, if the prefix starts with "(,", fill the boot
	drive in between those two characters, but expect that a full
	partition specification including partition map names will follow.
	(grub_parse_multiboot_cmdline): New function.
	(grub_machine_init): If we have an MBI, copy it, then call
	grub_parse_multiboot_cmdline.
	* util/i386/pc/grub-setup.c (setup): Unless an explicit prefix was
	specified, write a prefix without the drive name but including a
	full partition specification.

=== modified file 'include/grub/i386/pc/kernel.h'
--- include/grub/i386/pc/kernel.h	2010-04-26 19:11:16 +0000
+++ include/grub/i386/pc/kernel.h	2010-06-15 11:02:34 +0000
@@ -44,6 +44,10 @@ extern grub_int32_t grub_install_bsd_par
 /* The boot BIOS drive number.  */
 extern grub_uint8_t EXPORT_VAR(grub_boot_drive);
 
+/* Set if multiboot changed the partition numbers, so grub_prefix is now
+   invalid.  */
+extern grub_uint8_t grub_multiboot_change_prefix;
+
 #endif /* ! ASM_FILE */
 
 #endif /* ! KERNEL_MACHINE_HEADER */

=== modified file 'kern/i386/pc/init.c'
--- kern/i386/pc/init.c	2010-05-21 18:08:48 +0000
+++ kern/i386/pc/init.c	2010-06-15 11:06:20 +0000
@@ -32,6 +32,7 @@
 #include <grub/cache.h>
 #include <grub/time.h>
 #include <grub/cpu/tsc.h>
+#include <grub/multiboot.h>
 
 struct mem_region
 {
@@ -47,12 +48,29 @@ static int num_regions;
 grub_addr_t grub_os_area_addr;
 grub_size_t grub_os_area_size;
 
+/* A pointer to the MBI in its initial location.  */
+struct multiboot_info *startup_multiboot_info = NULL;
+
+/* The MBI has to be copied to our BSS so that it won't be
+   overwritten.  This is its final location.  */
+static struct multiboot_info kern_multiboot_info;
+
 static char *
 make_install_device (void)
 {
   /* XXX: This should be enough.  */
   char dev[100], *ptr = dev;
 
+  if (grub_prefix[0] == '(' && grub_multiboot_change_prefix)
+    {
+      /* The multiboot information invalidated our hardcoded prefix because
+         partition numbers differed.  Eliminate the drive/partition part of
+         the prefix, if possible.  */
+      char *ket = grub_strchr (grub_prefix, ')');
+      if (ket)
+	grub_memmove (grub_prefix, ket + 1, grub_strlen (ket));
+    }
+
   if (grub_prefix[0] != '(')
     {
       /* No hardcoded root partition - make it from the boot drive and the
@@ -83,6 +101,14 @@ make_install_device (void)
       grub_snprintf (ptr, sizeof (dev) - (ptr - dev), ")%s", grub_prefix);
       grub_strcpy (grub_prefix, dev);
     }
+  else if (grub_prefix[1] == ',' || grub_prefix[1] == ')')
+    {
+      /* We have a prefix, but still need to fill in the boot drive.  */
+      grub_snprintf (dev, sizeof (dev),
+		     "(%cd%u%s", (grub_boot_drive & 0x80) ? 'h' : 'f',
+		     grub_boot_drive & 0x7f, grub_prefix + 1);
+      grub_strcpy (grub_prefix, dev);
+    }
 
   return grub_prefix;
 }
@@ -134,6 +160,45 @@ compact_mem_regions (void)
       }
 }
 
+static void
+grub_parse_multiboot_cmdline (void)
+{
+  char *cmdline;
+  char *p;
+
+  if (! (kern_multiboot_info.flags & MULTIBOOT_INFO_CMDLINE))
+    return;
+
+  p = cmdline = grub_strdup ((char *) kern_multiboot_info.cmdline);
+
+  while (*p)
+    {
+      char *command = p;
+      char *end;
+      char *val;
+
+      end = grub_strchr (command, ' ');
+      if (end)
+	{
+	  *end = '\0';
+	  p = end + 1;
+	  while (*p == ' ')
+	    ++p;
+	}
+
+      /* Process command.  */
+      val = grub_strchr (command, '=');
+      if (val)
+	{
+	  *val = '\0';
+	  grub_env_set (command, val + 1);
+
+	  if (grub_strcmp (command, "prefix") == 0)
+	    grub_multiboot_change_prefix = 0;
+	}
+    }
+}
+
 void
 grub_machine_init (void)
 {
@@ -214,6 +279,15 @@ grub_machine_init (void)
   if (! grub_os_area_addr)
     grub_fatal ("no upper memory");
 
+  if (startup_multiboot_info)
+    {
+      /* Move MBI to a safe place.  */
+      grub_memmove (&kern_multiboot_info, startup_multiboot_info,
+		    sizeof (struct multiboot_info));
+
+      grub_parse_multiboot_cmdline ();
+    }
+
   grub_tsc_init ();
 }
 

=== modified file 'kern/i386/pc/startup.S'
--- kern/i386/pc/startup.S	2010-04-05 13:59:32 +0000
+++ kern/i386/pc/startup.S	2010-06-15 11:02:19 +0000
@@ -142,6 +142,8 @@ multiboot_header:
 
 multiboot_entry:
 	.code32
+	movl	%ebx, EXT_C(startup_multiboot_info)
+
 	/* obtain the boot device */
 	movl	12(%ebx), %edx
 
@@ -161,20 +163,38 @@ multiboot_entry:
 	jmp	*%eax
 
 multiboot_trampoline:
+	/* remember the original boot information */
+	movl	EXT_C(grub_install_dos_part), %eax
+	orl	%eax, %eax
+	jns	1f
+	movb	$0xFF, %al
+1:
+	movl	EXT_C(grub_install_bsd_part), %ebx
+	orl	%ebx, %ebx
+	jns	2f
+	movb	$0xFF, %bl
+2:
+	movb	%al, %bh
+
 	/* fill the boot information */
 	movl	%edx, %eax
 	shrl	$8, %eax
+	cmpw	%ax, %bx
+	je	3f
+	/* doesn't match the original */
+	movb	$1, EXT_C(grub_multiboot_change_prefix)
+3:
 	xorl	%ebx, %ebx
 	cmpb	$0xFF, %ah
-	je	1f
+	je	4f
 	movb	%ah, %bl
 	movl	%ebx, EXT_C(grub_install_dos_part)
-1:
+4:
 	cmpb	$0xFF, %al
-	je	2f
+	je	5f
 	movb	%al, %bl
 	movl	%ebx, EXT_C(grub_install_bsd_part)
-2:
+5:
 	shrl	$24, %edx
         movb    $0xFF, %dh
 	/* enter the usual booting */
@@ -285,6 +305,8 @@ LOCAL (codestart):
 
 VARIABLE(grub_boot_drive)
 	.byte	0
+VARIABLE(grub_multiboot_change_prefix)
+	.byte	0
 
 	.p2align	2	/* force 4-byte alignment */
 

=== modified file 'util/i386/pc/grub-setup.c'
--- util/i386/pc/grub-setup.c	2010-06-11 20:31:16 +0000
+++ util/i386/pc/grub-setup.c	2010-06-14 16:29:54 +0000
@@ -99,6 +99,7 @@ setup (const char *dir,
   struct grub_boot_blocklist *first_block, *block;
   grub_int32_t *install_dos_part, *install_bsd_part;
   grub_int32_t dos_part, bsd_part;
+  char *prefix;
   char *tmp_img;
   int i;
   grub_disk_addr_t first_sector;
@@ -230,6 +231,8 @@ setup (const char *dir,
 				       + GRUB_KERNEL_MACHINE_INSTALL_DOS_PART);
   install_bsd_part = (grub_int32_t *) (core_img + GRUB_DISK_SECTOR_SIZE
 				       + GRUB_KERNEL_MACHINE_INSTALL_BSD_PART);
+  prefix = (char *) (core_img + GRUB_DISK_SECTOR_SIZE +
+		     GRUB_KERNEL_MACHINE_PREFIX);
 
   /* Open the root device and the destination device.  */
   root_dev = grub_device_open (root);
@@ -305,6 +308,18 @@ setup (const char *dir,
 	      dos_part = root_dev->disk->partition->number;
  	      bsd_part = -1;
  	    }
+
+	  if (prefix[0] != '(')
+	    {
+	      char *root_part_name, *new_prefix;
+
+	      root_part_name =
+		grub_partition_get_name (root_dev->disk->partition);
+	      new_prefix = xasprintf ("(,%s)%s", root_part_name, prefix);
+	      strcpy (prefix, new_prefix);
+	      free (new_prefix);
+	      free (root_part_name);
+	    }
 	}
       else
 	dos_part = bsd_part = -1;

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]