Re: Which partitioning schemes should be supported by GRUB?

[Colin Watson]

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  <address@hidden>

        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                                       address@hidden