Which partitioning schemes should be supported by GRUB?

Which partitioning schemes should be supported by GRUB?

[Grégoire Sutre]

Hi,

Tests of GRUB on NetBSD (and FreeBSD) have raised several issues (most
of them reported on the list) regarding partition detection.  However,
I have the feeling that some of these issues are not considered as real
issues since the test configuration is not supported by GRUB.  This
surprises me since I naively thought that most user configurations
should be supported.

So I ask the question: Which partitioning schemes are (or shall be)
supported by GRUB on i386-pc (with standard BIOS)?

To start the discussion, I'll focus on a few examples (the list is
surely not exhaustive).  Maybe some configurations simply cannot exist,
in which case please let me know.

1. A single top-level partition map
    (a) MS-DOS
    (b) GPT
    (c) BSD disklabel
    (d) Apple partition map
    (e) Sun label

2. Hybrid: top-level MS-DOS + another *top-level* partition map
    (a) MS-DOS + GPT
        (i.e. GPT + at-least one non 0xEE MS-DOS partition)
    (b) MS-DOS + BSD
    (c) ...

If I read the code correctly, grub-setup (on i386-pc) only supports
1(a) and 1(b).  However, on NetBSD, 1(c) is very common, and 2(b) is
not rare.  Also, some OSes are fine with 2(a), e.g. FreeBSD.

Personally, I would rather support all possible configurations, unless
some technical reason prevents it.  So grub-setup would not test for
some specific configurations, but would instead use a generic
(and simple) approach.  If it fails, it should be for a good reason,
and not because "No DOS-style partitions [were] found".

What's your opinion?

Grégoire

Re: Which partitioning schemes should be supported by GRUB?

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 06/06/2010 08:02 PM, Grégoire Sutre wrote:
> Hi,
>
> Tests of GRUB on NetBSD (and FreeBSD) have raised several issues (most
> of them reported on the list) regarding partition detection.  However,
> I have the feeling that some of these issues are not considered as real
> issues since the test configuration is not supported by GRUB.  This
> surprises me since I naively thought that most user configurations
> should be supported.
>
> So I ask the question: Which partitioning schemes are (or shall be)
> supported by GRUB on i386-pc (with standard BIOS)?
There are two parts of this question:
1) Which partition schemes should GRUB be able to read modules and
payloads from ? It's platform-indepedent and 2 conditions apply:
- Usage. There are OS which are able to boot from such OS and such
configuration isn't considered obscure by them.
- Non-confusability. The risk of false positive of this partition config
which would prevent normal function is small.
If at least one condition is met it's worth considering. If both
conditions are met it should be supported.
2) Support for embedding.
Embedding is a potentially dangerous operation so we have to be
cautious. Using a dedicated embedding partition if it can be
unambiguously identified as such is a sane solution.
>
> To start the discussion, I'll focus on a few examples (the list is
> surely not exhaustive).  Maybe some configurations simply cannot exist,
> in which case please let me know.
>
> 1. A single top-level partition map
>    (a) MS-DOS
>    (b) GPT
>    (c) BSD disklabel
>    (d) Apple partition map
>    (e) Sun label
>
> 2. Hybrid: top-level MS-DOS + another *top-level* partition map
>    (a) MS-DOS + GPT
>        (i.e. GPT + at-least one non 0xEE MS-DOS partition)
>    (b) MS-DOS + BSD
>    (c) ...
>
> If I read the code correctly, grub-setup (on i386-pc) only supports
> 1(a) and 1(b).  However, on NetBSD, 1(c) is very common, and 2(b) is
> not rare.  Also, some OSes are fine with 2(a), e.g. FreeBSD.
>
> Personally, I would rather support all possible configurations, unless
> some technical reason prevents it.  So grub-setup would not test for
> some specific configurations, but would instead use a generic
> (and simple) approach.  If it fails, it should be for a good reason,
> and not because "No DOS-style partitions [were] found".
>
> What's your opinion?
>
> Grégoire
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>


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



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

Re: Which partitioning schemes should be supported by GRUB?

[Grégoire Sutre]

On 06/07/2010 10:46 PM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:

> There are two parts of this question:
> 1) Which partition schemes should GRUB be able to read modules and
> payloads from ? It's platform-indepedent

Agreed.

> and 2 conditions apply:
> - Usage. There are OS which are able to boot from such OS and such
> configuration isn't considered obscure by them.
> - Non-confusability. The risk of false positive of this partition config
> which would prevent normal function is small.
> If at least one condition is met it's worth considering. If both
> conditions are met it should be supported.

Ok.  Regarding confusability, I can see potential problems in the
interpretation of offsets (absolute or relative?), such as for nested
BSD labels (discussed in another thread).  Do you see other potential
causes for confusion?


> 2) Support for embedding.
> Embedding is a potentially dangerous operation so we have to be
> cautious. Using a dedicated embedding partition if it can be
> unambiguously identified as such is a sane solution.

Sure.  As discussed on irc, this would require in-depth changes to
grub-setup, and it's worth another thread...

Grégoire

Re: Which partitioning schemes should be supported by GRUB?

[C. P. Ghost]

2010/6/10 Grégoire Sutre <gregoire.sutre@gmail.com>:
> On 06/07/2010 10:46 PM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>
>> There are two parts of this question:
>> 1) Which partition schemes should GRUB be able to read modules and
>> payloads from ? It's platform-indepedent
>
> Agreed.
>
>> and 2 conditions apply:
>> - Usage. There are OS which are able to boot from such OS and such
>> configuration isn't considered obscure by them.
>> - Non-confusability. The risk of false positive of this partition config
>> which would prevent normal function is small.
>> If at least one condition is met it's worth considering. If both
>> conditions are met it should be supported.
>
> Ok.  Regarding confusability, I can see potential problems in the
> interpretation of offsets (absolute or relative?), such as for nested
> BSD labels (discussed in another thread).  Do you see other potential
> causes for confusion?
>
>> 2) Support for embedding.
>> Embedding is a potentially dangerous operation so we have to be
>> cautious. Using a dedicated embedding partition if it can be
>> unambiguously identified as such is a sane solution.
>
> Sure.  As discussed on irc, this would require in-depth changes to
> grub-setup, and it's worth another thread...

Whatever you guys implement, as long as it just works, it's
fine with me.

I'm interested in GRUB2 because of its multiboot capability: I need
to boot L4Ka::Pistachio on top of a FreeBSD/UFS2 filesystem.
If it works inside DOS/bsdlabel and GPT partitions, that's ideal.
Of course, if NetBSD partitions are supported too, that would be
even better.

Thanks for the great support so far. ;-)

> Grégoire

-cpghost.

-- 
Cordula's Web. http://www.cordula.ws/

Re: Which partitioning schemes should be supported by GRUB?

[Grégoire Sutre]

On 06/07/2010 10:46 PM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:

> There are two parts of this question:
> 1) Which partition schemes should GRUB be able to read modules and
> payloads from ? It's platform-indepedent and 2 conditions apply:
> - Usage. There are OS which are able to boot from such OS and such
> configuration isn't considered obscure by them.
> - Non-confusability. The risk of false positive of this partition config
> which would prevent normal function is small.
> If at least one condition is met it's worth considering. If both
> conditions are met it should be supported.

According to these rules, hybrid msdos+gpt partitioning schemes should
be supported.  Grub should be able to read files from a GPT partition
even if the protective GPT entry in the MBR is not in the first slot.
It's also the conclusion of the thread [1], but I admit that this thread
is two years old.

Grégoire

[1] http://lists.gnu.org/archive/html/grub-devel/2008-09/msg00101.html

Re: Which partitioning schemes should be supported by GRUB?

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 06/12/2010 06:32 PM, Grégoire Sutre wrote:
> On 06/07/2010 10:46 PM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>
>> There are two parts of this question:
>> 1) Which partition schemes should GRUB be able to read modules and
>> payloads from ? It's platform-indepedent and 2 conditions apply:
>> - Usage. There are OS which are able to boot from such OS and such
>> configuration isn't considered obscure by them.
>> - Non-confusability. The risk of false positive of this partition config
>> which would prevent normal function is small.
>> If at least one condition is met it's worth considering. If both
>> conditions are met it should be supported.
>
> According to these rules, hybrid msdos+gpt partitioning schemes should
> be supported.  Grub should be able to read files from a GPT partition
> even if the protective GPT entry in the MBR is not in the first slot.
> It's also the conclusion of the thread [1], but I admit that this thread
> is two years old.
>
Any "hybrid" cofiguration fails the criteria of non confusability. Let's
consider a following situation:
- I format disk with scheme A and partitions A1, A2, A3
- I get bored and reformat with scheme B and partitios B1, B2, B3, B4.
When I did this filesystem on A2 may stay intact
- I use grub which supposes that it'shybrid system A+B and save_env's to
A2 since it's a valid partition on valid filesystem. But by a bad luck
save_env overlaps with superblock of B3 which becomes corrupted.
When installed grub sees both partitions A and B it makes sense to allow
user to see both of them for recovery purposes but in any case it should
be a recommended config. Desync is way too easy. And currently grub
isn't changed to new partition notation completely. E.g. during startup
prefix is calculated with old syntax and confusing A+B with either A or
B is likely to make user drop into rescue shell
Also save_env should ensure that no dangerous situation exist.
> Grégoire
>
> [1] http://lists.gnu.org/archive/html/grub-devel/2008-09/msg00101.html
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>


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



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

Re: Which partitioning schemes should be supported by GRUB?

[Grégoire Sutre]

On 06/12/2010 07:26 PM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:

> Any "hybrid" cofiguration fails the criteria of non confusability.

I was assuming the new partition notation.  The old notation is clearly
ambiguous when there are multiple partmaps, and AFAIR the new notation
was introduced precisely to solve that problem:

http://lists.gnu.org/archive/html/grub-devel/2010-01/msg00320.html

By the way, the old notation is worse than ambiguous when there are
multiple partmaps: the meaning of partition identifiers depends on the
partmap modules that are loaded, and on the order in which they are
loaded.

> Let's consider a following situation: - I format disk with scheme A
> and partitions A1, A2, A3 - I get bored and reformat with scheme B
> and partitios B1, B2, B3, B4. When I did this filesystem on A2 may
> stay intact - I use grub which supposes that it'shybrid system A+B
> and save_env's to A2 since it's a valid partition on valid
> filesystem. But by a bad luck save_env overlaps with superblock of B3
> which becomes corrupted.

If you save_env with -f then, with new notation, you know that you are
using the old scheme A.  If you didn't use -f, then it means that grub
modules were installed into A2 and survived the reformat, but then,
how could GRUB know that A is obsolete?  IMHO corrupting the superblock
of B3 is acceptable in that case.  An alternative would be to check
that partitions do not overlap (with the exception of identical
partitions).  But even this would work only if the partmap module for
B was loaded, which is likely not the case (as grub was installed at
the time A was used).

> And currently grub isn't changed to new partition notation
> completely. E.g. during startup prefix is calculated with old syntax
> and confusing A+B with either A or B is likely to make user drop into
> rescue shell

Is someone working on making the startup prefix use the new notation?

Grégoire

Re: Which partitioning schemes should be supported by GRUB?

[Colin Watson]

On Sun, Jun 13, 2010 at 06:16:13PM +0200, Grégoire Sutre wrote:
> On 06/12/2010 07:26 PM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> And currently grub isn't changed to new partition notation
>> completely. E.g. during startup prefix is calculated with old syntax
>> and confusing A+B with either A or B is likely to make user drop into
>> rescue shell
>
> Is someone working on making the startup prefix use the new notation?

I've only looked at the i386-pc startup sequence here, but the thing
that jumps out at me is that grub_install_dos_part and
grub_install_bsd_part are both longs.  I doubt we really need to support
2 billion partitions.  How about we split off, say, a byte of that to
hold the partition map type, and then it would be easy enough for the
startup code to turn that into appropriate prefixes?

There are two downsides that I can see:

  * We would need to maintain an enum of partition map identifiers; but
    there's precedent for this approach already, such as
    grub_disk_dev_id.

  * The names of all the partition maps would have to live in the kernel
    somewhere; packed as sequential strings, this comes to 42 bytes plus
    the code to use it, which isn't a lot by most measures but I know we
    never have much slack in the kernel.


I can think of an alternative.  We do still need grub_install_dos_part
and grub_install_bsd_part for the multiboot trampoline, which is in
assembly, so it's difficult to abandon them altogether.  However,
there's no reason we need to use them in make_install_device.  How about
we invent a way to encode most of the information in string form in
grub_prefix, while leaving a placeholder for make_install_device to fill
in the disk?  There are 64 bytes available for grub_prefix, which should
be plenty.  For example, how about the following (with \0 standing for
ASCII NUL):

  (\0,msdos1,bsd1)/boot/grub

It's then just a matter of spotting the "(\0," sequence and replacing
the \0 with the drive name.  I think this can probably be done using
less code than the first option above, and all told it feels a bit less
hacky to me.


What do people prefer?

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Which partitioning schemes should be supported by GRUB?

[richardvoigt]

> I can think of an alternative.  We do still need grub_install_dos_part
> and grub_install_bsd_part for the multiboot trampoline, which is in
> assembly, so it's difficult to abandon them altogether.  However,
> there's no reason we need to use them in make_install_device.  How about
> we invent a way to encode most of the information in string form in
> grub_prefix, while leaving a placeholder for make_install_device to fill
> in the disk?  There are 64 bytes available for grub_prefix, which should
> be plenty.  For example, how about the following (with \0 standing for
> ASCII NUL):
>
>  (\0,msdos1,bsd1)/boot/grub
>
> It's then just a matter of spotting the "(\0," sequence and replacing
> the \0 with the drive name.  I think this can probably be done using
> less code than the first option above, and all told it feels a bit less
> hacky to me.

Instead of doing string replacement, why not just define a
disk-relative partition format?

Also, the '\0' seems unnecessary.  Is having "(," meaningful in some
way already?


>
>
> What do people prefer?
>
> --
> Colin Watson                                       [cjwatson@ubuntu.com]
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>

Re: Which partitioning schemes should be supported by GRUB?

[Colin Watson]

On Mon, Jun 14, 2010 at 08:07:35AM -0500, richardvoigt@gmail.com wrote:
> Colin Watson wrote:
> > I can think of an alternative.  We do still need grub_install_dos_part
> > and grub_install_bsd_part for the multiboot trampoline, which is in
> > assembly, so it's difficult to abandon them altogether.  However,
> > there's no reason we need to use them in make_install_device.  How about
> > we invent a way to encode most of the information in string form in
> > grub_prefix, while leaving a placeholder for make_install_device to fill
> > in the disk?  There are 64 bytes available for grub_prefix, which should
> > be plenty.  For example, how about the following (with \0 standing for
> > ASCII NUL):
> >
> >  (\0,msdos1,bsd1)/boot/grub
> >
> > It's then just a matter of spotting the "(\0," sequence and replacing
> > the \0 with the drive name.  I think this can probably be done using
> > less code than the first option above, and all told it feels a bit less
> > hacky to me.
> 
> Instead of doing string replacement, why not just define a
> disk-relative partition format?

Simple string replacement would be much easier to implement, and
probably smaller.  Plus, we don't need disk-relative device naming
elsewhere, and I think it would require putting platform-specific code
(otherwise how do you know which disk to be relative to?) in places that
are otherwise pretty platform-independent.

> Also, the '\0' seems unnecessary.  Is having "(," meaningful in some
> way already?

Good point.  This would be sufficient.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Which partitioning schemes should be supported by GRUB?

[Colin Watson]

On Mon, Jun 14, 2010 at 02:25:39PM +0100, Colin Watson wrote:
> On Mon, Jun 14, 2010 at 08:07:35AM -0500, richardvoigt@gmail.com wrote:
> > Colin Watson wrote:
> > > I can think of an alternative.  We do still need grub_install_dos_part
> > > and grub_install_bsd_part for the multiboot trampoline, which is in
> > > assembly, so it's difficult to abandon them altogether.  However,
> > > there's no reason we need to use them in make_install_device.  How about
> > > we invent a way to encode most of the information in string form in
> > > grub_prefix, while leaving a placeholder for make_install_device to fill
> > > in the disk?  There are 64 bytes available for grub_prefix, which should
> > > be plenty.  For example, how about the following (with \0 standing for
> > > ASCII NUL):
> > >
> > >  (\0,msdos1,bsd1)/boot/grub
> > >
> > > It's then just a matter of spotting the "(\0," sequence and replacing
> > > the \0 with the drive name.  I think this can probably be done using
> > > less code than the first option above, and all told it feels a bit less
> > > hacky to me.
> > 
> > Instead of doing string replacement, why not just define a
> > disk-relative partition format?
> 
> Simple string replacement would be much easier to implement, and
> probably smaller.  Plus, we don't need disk-relative device naming
> elsewhere, and I think it would require putting platform-specific code
> (otherwise how do you know which disk to be relative to?) in places that
> are otherwise pretty platform-independent.
> 
> > Also, the '\0' seems unnecessary.  Is having "(," meaningful in some
> > way already?
> 
> Good point.  This would be sufficient.

How about the following patch, implementing this proposal?  I've tested this
with Debian GNU/kFreeBSD, and it's enough to make the boot loader work again
out of the box after grub-install.  The 'root' variable is still wrong, but
that isn't particularly urgent as UUIDs usually take care of that.

The kernel.img size increase is 84 bytes, yielding a core.img size
increase of 50 bytes in this configuration (22932 -> 22982 bytes).  Do I
need to work on making that smaller somehow?  It seems tolerable to me.

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

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

	* kern/i386/pc/init.c (make_install_device): 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.
	* 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 'kern/i386/pc/init.c'
--- kern/i386/pc/init.c	2010-05-21 18:08:48 +0000
+++ kern/i386/pc/init.c	2010-06-14 14:44:13 +0000
@@ -83,6 +83,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] == ',')
+    {
+      /* 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;
 }

=== 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 14:45:24 +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);
@@ -289,21 +292,45 @@ setup (const char *dir,
      override the current setting.  */
   if (*install_dos_part != -2)
     {
+      grub_partition_t root_part = root_dev->disk->partition;
+
       /* Embed information about the installed location.  */
-      if (root_dev->disk->partition)
+      if (root_part)
 	{
-	  if (root_dev->disk->partition->parent)
+	  char *new_prefix;
+
+	  if (root_part->parent)
  	    {
-	      if (root_dev->disk->partition->parent->parent)
+	      if (root_part->parent->parent)
 		grub_util_error ("Installing on doubly nested partitions is "
 				 "not supported");
-	      dos_part = root_dev->disk->partition->parent->number;
-	      bsd_part = root_dev->disk->partition->number;
+	      dos_part = root_part->parent->number;
+	      bsd_part = root_part->number;
+
+	      if (prefix[0] != '(')
+		{
+		  new_prefix = xasprintf ("(,%s%d,%s%d)%s",
+					  root_part->parent->partmap->name,
+					  root_part->parent->number + 1,
+					  root_part->partmap->name,
+					  root_part->number + 1,
+					  prefix);
+		  strcpy (prefix, new_prefix);
+		}
  	    }
 	  else
  	    {
-	      dos_part = root_dev->disk->partition->number;
+	      dos_part = root_part->number;
  	      bsd_part = -1;
+
+	      if (prefix[0] != '(')
+		{
+		  new_prefix = xasprintf ("(,%s%d)%s",
+					  root_part->partmap->name,
+					  root_part->number + 1,
+					  prefix);
+		  strcpy (prefix, new_prefix);
+		}
  	    }
 	}
       else

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Which partitioning schemes should be supported by GRUB?

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 06/14/2010 05:02 PM, Colin Watson wrote:
>
> 2010-06-14  Colin Watson  <cjwatson@ubuntu.com>
>
> 	Fix i386-pc prefix handling with nested partitions (Debian bug
> 	#585068).
>
> 	* kern/i386/pc/init.c (make_install_device): 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.
> 	* 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.
>
>   
That makes a regression for multiboot loading of image if it was moved
from one partition to another.
> === 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-14 14:44:13 +0000
> @@ -83,6 +83,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] == ',')
> +    {
> +      /* 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);
> +    }
>  
>   
Or grub_prefix[1] == ')' to allow ()/boot/grub syntax for unpartitioned
devices
>    return grub_prefix;
>  }
>
> === 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 14:45:24 +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);
> @@ -289,21 +292,45 @@ setup (const char *dir,
>       override the current setting.  */
>    if (*install_dos_part != -2)
>      {
> +      grub_partition_t root_part = root_dev->disk->partition;
> +
>        /* Embed information about the installed location.  */
> -      if (root_dev->disk->partition)
> +      if (root_part)
>  	{
> -	  if (root_dev->disk->partition->parent)
> +	  char *new_prefix;
> +
> +	  if (root_part->parent)
>   	    {
> -	      if (root_dev->disk->partition->parent->parent)
> +	      if (root_part->parent->parent)
>  		grub_util_error ("Installing on doubly nested partitions is "
>  				 "not supported");
>   
This error must go away with that new syntax.
> -	      dos_part = root_dev->disk->partition->parent->number;
> -	      bsd_part = root_dev->disk->partition->number;
> +	      dos_part = root_part->parent->number;
> +	      bsd_part = root_part->number;
> +
> +	      if (prefix[0] != '(')
> +		{
> +		  new_prefix = xasprintf ("(,%s%d,%s%d)%s",
> +					  root_part->parent->partmap->name,
> +					  root_part->parent->number + 1,
> +					  root_part->partmap->name,
> +					  root_part->number + 1,
> +					  prefix);
> +		  strcpy (prefix, new_prefix);
> +		}
>   
Please use grub_partition_get_name. It will greatly simplify this part.
>   	    }
>  	  else
>   	    {
> -	      dos_part = root_dev->disk->partition->number;
> +	      dos_part = root_part->number;
>   	      bsd_part = -1;
> +
> +	      if (prefix[0] != '(')
> +		{
> +		  new_prefix = xasprintf ("(,%s%d)%s",
> +					  root_part->partmap->name,
> +					  root_part->number + 1,
> +					  prefix);
> +		  strcpy (prefix, new_prefix);
> +		}
>   	    }
>  	}
>        else
>
> Thanks,
>
>   


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



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

Re: Which partitioning schemes should be supported by GRUB?

[Colin Watson]

On Mon, Jun 14, 2010 at 05:58:55PM +0200, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 06/14/2010 05:02 PM, Colin Watson wrote:
> > 2010-06-14  Colin Watson  <cjwatson@ubuntu.com>
> >
> > 	Fix i386-pc prefix handling with nested partitions (Debian bug
> > 	#585068).
> >
> > 	* kern/i386/pc/init.c (make_install_device): 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.
> > 	* 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.
> 
> That makes a regression for multiboot loading of image if it was moved
> from one partition to another.

Do you have any suggestions on how to deal with that?  I'm not familiar
with multiboot and need guidance.

> > === 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-14 14:44:13 +0000
> > @@ -83,6 +83,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] == ',')
> > +    {
> > +      /* 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);
> > +    }
> 
> Or grub_prefix[1] == ')' to allow ()/boot/grub syntax for unpartitioned
> devices

Makes sense.  I've updated my local tree.

> > +	  if (root_part->parent)
> >   	    {
> > -	      if (root_dev->disk->partition->parent->parent)
> > +	      if (root_part->parent->parent)
> >  		grub_util_error ("Installing on doubly nested partitions is "
> >  				 "not supported");
> >   
> 
> This error must go away with that new syntax.

I agree, but how should kern/i386/pc/startup.S:multiboot_trampoline
cope?  I left this there because I didn't know a straightforward way to
deal with that.

> > -	      dos_part = root_dev->disk->partition->parent->number;
> > -	      bsd_part = root_dev->disk->partition->number;
> > +	      dos_part = root_part->parent->number;
> > +	      bsd_part = root_part->number;
> > +
> > +	      if (prefix[0] != '(')
> > +		{
> > +		  new_prefix = xasprintf ("(,%s%d,%s%d)%s",
> > +					  root_part->parent->partmap->name,
> > +					  root_part->parent->number + 1,
> > +					  root_part->partmap->name,
> > +					  root_part->number + 1,
> > +					  prefix);
> > +		  strcpy (prefix, new_prefix);
> > +		}
> >   
> 
> Please use grub_partition_get_name. It will greatly simplify this part.

Done, thanks.  (I had a memory leak too.)

Updated patch follows; same ChangeLog.

=== 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-14 16:21:53 +0000
@@ -83,6 +83,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;
 }

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

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Which partitioning schemes should be supported by GRUB?

[Seth Goldberg]

>>> === 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-14 14:44:13 +0000
>>> @@ -83,6 +83,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] == ',')
>>> +    {
>>> +      /* 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);
>>> +    }

   What about CD devices?  Seems like you're limiting to hd and fd here (though 
CD may just be an alias for hd198 or something like that).  Just checking :).

  --S

Re: Which partitioning schemes should be supported by GRUB?

[Grégoire Sutre]

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.

Grégoire

p.s. This issue (with boot_device field) was discussed a bit in:

http://lists.gnu.org/archive/html/grub-devel/2010-01/msg00351.html
http://lists.gnu.org/archive/html/grub-devel/2010-02/msg00070.html

Re: Which partitioning schemes should be supported by GRUB?

[Colin Watson]

On Mon, Jun 14, 2010 at 09:55:09AM -0700, Seth Goldberg wrote:
>>>> === 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-14 14:44:13 +0000
>>>> @@ -83,6 +83,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] == ',')
>>>> +    {
>>>> +      /* 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);
>>>> +    }
>
>   What about CD devices?  Seems like you're limiting to hd and fd here 
> (though CD may just be an alias for hd198 or something like that).  Just 
> checking :).

That part of my patch was just copied from immediately above, so doesn't
really change the current state.

grub-mkrescue has special code for dealing with CDs and setting an
appropriate prefix at run-time from the output of a 'search' command.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

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

Re: Which partitioning schemes should be supported by GRUB?

[Grégoire Sutre]

On 06/15/2010 01:21 PM, Colin Watson wrote:

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

Ok.

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

This would be nice.

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

- 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).

>   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).

Grégoire

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  <cjwatson@ubuntu.com>

	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                                       [cjwatson@ubuntu.com]

Re: Which partitioning schemes should be supported by GRUB?

[Grégoire Sutre]

Hi,

I made several tests, and the patch works fine with standard boot.  When
multibooting core.img, the command-line is taken into account correctly.
However, if no multiboot command-line is given, the prefix is set as
before (old partition naming style).

This comes from the fact that the modifications to the prefix done in
grub-setup only apply to the embedded image, and not to the core.img
file.  The grub-mkimage command used by grub-install is:

grub-mkimage -O i386-pc --output=/path/to/core.img --prefix=/boot/grub

Would it make sense to put the full prefix here?


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

What I meant is: the assembly part could be restricted to the copy of
the relevant MBI information, i.e. flags, boot_device and cmdline.  And
leave the actual decisions regarding the contents of those fields to
grub_machine_init().  This would mean that the part dealing with
grub_install_dos/bsd_part (multiboot_trampoline) would be part of the C
code (probably in make_install_device()).


>   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 ());

You could avoid the variable found_multiboot_prefix and use
grub_env_find("prefix").  The intention would be that if someone
(multiboot or someone else) has already set the prefix in grub_env,
then we shouldn't override this choice here.  This would simplify
a bit grub_parse_multiboot_cmdline(), which would become purely
generic.  But this is essentially cosmetic...


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

The boot_device field should be used only if the MULTIBOOT_INFO_BOOTDEV
flag is set.  This problem is already present in trunk.

> +	/* 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

Is it safe to push?  Maybe we should start by setting %esp as is done a
few lines below.

I honestly do not understand all the details regarding the assembly code
or the memory management, so I can't provide useful feedback on that.
I just noticed that GRUB_MEMORY_MACHINE_MULTIBOOT_FLAGS is copied into a
``regular'' variable, but GRUB_MEMORY_MACHINE_MULTIBOOT_CMDLINE is not.

Grégoire

Re: Which partitioning schemes should be supported by GRUB?

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- 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 --]

Re: Which partitioning schemes should be supported by GRUB?

[Colin Watson]

On Thu, Jun 17, 2010 at 02:47:11AM +0200, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> 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)

OK, thanks.  I've committed the non-multiboot part as per
http://lists.gnu.org/archive/html/grub-devel/2010-06/msg00093.html.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]