Re: Which partitioning schemes should be supported by GRUB?

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

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

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


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



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