Re: Which partitioning schemes should be supported by GRUB?

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 06/15/2010 01:21 PM, Colin Watson wrote:
> 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.
>
>   
Apparently multiboot part has revealed to be a bit more tricky due to
heterogenity. Go ahead with non-multiboot part, I'll review multiboot
part separately (need to think about it a bit)
> 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,
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature