Re: Which partitioning schemes should be supported by GRUB?

[Colin Watson]

On Tue, Jun 15, 2010 at 11:07:42PM +0200, Grégoire Sutre wrote:
> On 06/15/2010 01:21 PM, Colin Watson wrote:
>>    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.
>
> Since the command-line processing of the MBI is done after startup.S (in  
> grub_machine_init()),
>
> - the MBI (only the relevant portions for simplicity) needs to be copied
>   to a safe place.  The patch does it at the end of grub_machine_init(),
>   but I'm afraid this might be too late: the MBI may have been
>   overwritten before we reach that point.  Or is the code (startup.S and
>   grub_machine_init()) designed to guarantee that all memory writes are
>   in safe locations, outside of the whole MBI?

You're right.  I was copying this from coreboot, but it does much less
in its startup.S - in particular it doesn't relocate and decompress the
GRUB kernel!

> - couldn't the complete processing of the MBI be performed in the same
>   place, i.e. grub_machine_init()?  The assembly multiboot part would
>   only check whether GRUB was booted by multiboot, and set (or copy)
>   the MBI in that case.  Then the procedure to set grub_prefix would be
>   coded in one place.  This would require though, for multiboot, to get
>   grub_boot_drive from the boot_device field (the current assembly code
>   takes care of this).

My investigations suggest that this is very difficult.  Relocating the
GRUB kernel, which is almost the first thing multiboot_entry does, is
liable to overwrite the MBI, and you can't get at C code until you've
done that.  That's why multiboot_entry has to copy out the boot device
field even before it relocates the kernel.

>>   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));
>
> Moving the MBI is more complex, since the structure contains pointers
> to other structures (themselves containing pointers etc.).  But in our
> case it's not too painful since we only need to copy the struct
> multiboot_info and the string pointed to by its cmdline field (and set
> the field to the new address).

Yes, you're right that we do need to copy the cmdline string itself.

Here's an updated patch, which I've tested to the point of it actually
being able to make use of prefix environment variables passed on the
multiboot command line.  Please review this again?

2010-06-16  Colin Watson  <address@hidden>

        Fix i386-pc prefix handling with nested partitions (Debian bug
        #585068).  Take some care to avoid regressing multiboot, including
        adding command-line environment variable support on i386-pc.

        * include/grub/i386/pc/kernel.h (grub_multiboot_flags): New
        declaration.
        (grub_multiboot_change_prefix): Likewise.
        * include/grub/i386/pc/memory.h
        (GRUB_MEMORY_MACHINE_MULTIBOOT_FLAGS): Reserve space after the
        initial struct grub_machine_mmap_entry in the scratch area.
        (GRUB_MEMORY_MACHINE_MULTIBOOT_CMDLINE): Likewise.
        * include/grub/offsets.h (GRUB_KERNEL_I386_PC_RAW_SIZE): Increase to
        GRUB_KERNEL_I386_PC_DATA_END + 0x620.
        * kern/i386/pc/startup.S (multiboot_entry): Copy the MBI flags and
        command line (if set) to the scratch area.
        (multiboot_trampoline): Copy the MBI flags to grub_multiboot_flags
        now that we're relocated.  If the new partition numbers differ from
        the previous ones, then set grub_multiboot_change_prefix.
        (grub_multiboot_change_prefix): New definition.
        (grub_multiboot_flags): Likewise.
        * kern/i386/pc/init.c (make_install_device): Invalidate any
        drive/partition part of the prefix if grub_multiboot_change_prefix
        is set and an explicit prefix was not set on the multiboot command
        line.  After that, if the prefix starts with "(," or "()", 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 the MULTIBOOT_INFO_CMDLINE flag is set, call
        grub_parse_multiboot_cmdline.
        (grub_machine_set_prefix): Don't overwrite an explicit prefix set on
        the multiboot command line.
        * 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-16 12:53:46 +0000
@@ -44,6 +44,13 @@ extern grub_int32_t grub_install_bsd_par
 /* The boot BIOS drive number.  */
 extern grub_uint8_t EXPORT_VAR(grub_boot_drive);
 
+/* The multiboot flags.  Zero if not booted using multiboot.  */
+extern grub_uint32_t grub_multiboot_flags;
+
+/* 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 'include/grub/i386/pc/memory.h'
--- include/grub/i386/pc/memory.h       2010-04-26 08:56:12 +0000
+++ include/grub/i386/pc/memory.h       2010-06-16 12:32:34 +0000
@@ -54,6 +54,15 @@
 #define GRUB_MEMORY_MACHINE_RESERVED_END       \
        (GRUB_MEMORY_MACHINE_PROT_STACK + 0x10)
 
+/* The area where multiboot information is copied at early startup.  These
+   need to live in the scratch area rather than in normal variables because
+   they are copied away for safekeeping before GRUB relocates its own code.
+   We leave space for the heap memory allocator first.  */
+#define GRUB_MEMORY_MACHINE_MULTIBOOT_FLAGS    \
+       (GRUB_MEMORY_MACHINE_SCRATCH_ADDR + 0x20)
+#define GRUB_MEMORY_MACHINE_MULTIBOOT_CMDLINE  \
+       (GRUB_MEMORY_MACHINE_MULTIBOOT_FLAGS + 0x4)
+
 /* The area where GRUB is decompressed at early startup.  */
 #define GRUB_MEMORY_MACHINE_DECOMPRESSION_ADDR 0x100000
 

=== modified file 'include/grub/offsets.h'
--- include/grub/offsets.h      2010-04-26 19:11:16 +0000
+++ include/grub/offsets.h      2010-06-16 12:53:07 +0000
@@ -41,7 +41,7 @@
 #define GRUB_KERNEL_I386_PC_DATA_END           0x5c
 
 /* The size of the first region which won't be compressed.  */
-#define GRUB_KERNEL_I386_PC_RAW_SIZE           (GRUB_KERNEL_I386_PC_DATA_END + 
0x5F0)
+#define GRUB_KERNEL_I386_PC_RAW_SIZE           (GRUB_KERNEL_I386_PC_DATA_END + 
0x620)
 
 /* The segment where the kernel is loaded.  */
 #define GRUB_BOOT_I386_PC_KERNEL_SEG   0x800

=== 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-16 12:55:50 +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,25 @@ static int num_regions;
 grub_addr_t grub_os_area_addr;
 grub_size_t grub_os_area_size;
 
+static int found_multiboot_prefix;
+
 static char *
 make_install_device (void)
 {
   /* XXX: This should be enough.  */
   char dev[100], *ptr = dev;
 
+  if (grub_prefix[0] == '(' && grub_multiboot_change_prefix &&
+      !found_multiboot_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 +97,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 +156,49 @@ compact_mem_regions (void)
       }
 }
 
+static void
+grub_parse_multiboot_cmdline (void)
+{
+  char *cmdline = (char *) GRUB_MEMORY_MACHINE_MULTIBOOT_CMDLINE;
+  char *p;
+
+  if (!*cmdline)
+    return;
+
+  p = cmdline = grub_strdup (cmdline);
+
+  while (*p)
+    {
+      char *command = p;
+      char *end;
+      char *val;
+
+      end = grub_strchr (command, ' ');
+      if (end)
+       {
+         *end = '\0';
+         p = end + 1;
+         while (*p == ' ')
+           ++p;
+       }
+      else
+       p = grub_strchr (command, '\0');
+
+      /* Process command.  */
+      val = grub_strchr (command, '=');
+      if (val)
+       {
+         *val = '\0';
+         grub_env_set (command, val + 1);
+
+         /* If "prefix" is explicitly on the command line, then don't override
+            it later.  */
+         if (grub_strcmp (command, "prefix") == 0)
+           found_multiboot_prefix = 1;
+       }
+    }
+}
+
 void
 grub_machine_init (void)
 {
@@ -214,6 +279,9 @@ grub_machine_init (void)
   if (! grub_os_area_addr)
     grub_fatal ("no upper memory");
 
+  if (grub_multiboot_flags & MULTIBOOT_INFO_CMDLINE)
+    grub_parse_multiboot_cmdline ();
+
   grub_tsc_init ();
 }
 
@@ -221,7 +289,8 @@ void
 grub_machine_set_prefix (void)
 {
   /* Initialize the prefix.  */
-  grub_env_set ("prefix", make_install_device ());
+  if (!found_multiboot_prefix)
+    grub_env_set ("prefix", make_install_device ());
 }
 
 void

=== 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-16 12:55:05 +0000
@@ -145,6 +145,32 @@ multiboot_entry:
        /* obtain the boot device */
        movl    12(%ebx), %edx
 
+       cld
+
+       /* do we have a multiboot command line? */
+       movl    (%ebx), %eax
+       movl    $GRUB_MEMORY_MACHINE_MULTIBOOT_FLAGS, %edi
+       movl    %eax, (%edi)
+       testl   $MULTIBOOT_INFO_CMDLINE, %eax
+       jz      1f
+
+       /* if so, copy it to a safe place; do this before relocating code to
+          make sure that we don't lose it */
+       movl    16(%ebx), %edi
+       pushl   %edi
+       xorb    %al, %al
+       xorl    %ecx, %ecx
+       decl    %ecx
+       repne
+       scasb
+       incl    %ecx
+       negl    %ecx
+       popl    %esi
+       movl    $GRUB_MEMORY_MACHINE_MULTIBOOT_CMDLINE, %edi
+       rep
+       movsb
+
+1:
        movl    $GRUB_MEMORY_MACHINE_PROT_STACK, %ebp
        movl    %ebp, %esp
 
@@ -153,7 +179,6 @@ multiboot_entry:
        addl    EXT_C(grub_compressed_size) - _start + 0x100000 + 0x200, %ecx
        movl    $0x100000, %esi
        movl    $GRUB_BOOT_MACHINE_KERNEL_ADDR, %edi
-       cld
        rep
        movsb
        /* jump to the real address */
@@ -161,20 +186,44 @@ multiboot_entry:
        jmp     *%eax
 
 multiboot_trampoline:
+       /* move the multiboot flags to a local variable now that we've
+          relocated ourselves; this lets us guarantee that multiboot_flags
+          will be zero when not booted using multiboot */
+       movl    $GRUB_MEMORY_MACHINE_MULTIBOOT_FLAGS, %eax
+       movl    (%eax), %eax
+       movl    %eax, EXT_C(grub_multiboot_flags)
+
+       movl    EXT_C(grub_install_dos_part), %eax
+       testl   %eax, %eax
+       jns     1f
+       movb    $0xFF, %al
+1:
+       movl    EXT_C(grub_install_bsd_part), %ebx
+       testl   %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 */
+       incb    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,9 +334,14 @@ LOCAL (codestart):
 
 VARIABLE(grub_boot_drive)
        .byte   0
+VARIABLE(grub_multiboot_change_prefix)
+       .byte   0
 
        .p2align        2       /* force 4-byte alignment */
 
+VARIABLE(grub_multiboot_flags)
+       .long   0
+
 #include "../realmode.S"
 
 /*

=== 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