Multiple partition maps

Multiple partition maps

[phcoder]

Hello. I was looking at the grub code and seen that if a disk has
multiple partition tables (e.g. macintel with bootcamp) then only first
one will be detected. In some cases it can lead to unreachable
partitions if for some reason partition is present only in one table.
Does anyone has an idea how theese cases may be treated compactly and
efficiently?
Vladimir 'phcoder' Serbinenko

Re: Multiple partition maps

[Robert Millan]

On Wed, Sep 03, 2008 at 08:08:50PM +0200, phcoder wrote:
> Hello. I was looking at the grub code and seen that if a disk has
> multiple partition tables (e.g. macintel with bootcamp) then only first
> one will be detected. In some cases it can lead to unreachable
> partitions if for some reason partition is present only in one table.
> Does anyone has an idea how theese cases may be treated compactly and
> efficiently?

Strictly speaking, GPT+MSDOS hybrid tables are a violation of the GPT
specification.  It's not clear what would be the "correct" way of handling
them.

Since we're not a legacy program, I suppose the sane thing to do would be
to abort MSDOS probing if a protective DOS partition (0xee) is found, and
then only GPT will be used.

Isn't this what GRUB does already?  I thought it would be...

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: Multiple partition maps

[phcoder]

Robert Millan wrote:
> On Wed, Sep 03, 2008 at 08:08:50PM +0200, phcoder wrote:
>> Hello. I was looking at the grub code and seen that if a disk has
>> multiple partition tables (e.g. macintel with bootcamp) then only first
>> one will be detected. In some cases it can lead to unreachable
>> partitions if for some reason partition is present only in one table.
>> Does anyone has an idea how theese cases may be treated compactly and
>> efficiently?
> 
> Strictly speaking, GPT+MSDOS hybrid tables are a violation of the GPT
> specification.  It's not clear what would be the "correct" way of handling
> them.
> 
> Since we're not a legacy program, I suppose the sane thing to do would be
> to abort MSDOS probing if a protective DOS partition (0xee) is found, and
> then only GPT will be used.

We're not a legacy application but some OS and partition tools are and
we have to work with them. I think just some maintenance tools included
in GRUB should be enough. I'll write them soon. (perhaps even tomorrow)
> 
> Isn't this what GRUB does already?  I thought it would be...
> 
It does. But I don't know if there are other cases similar to this one.

Vladimir 'phcoder' Serbinnenko

Re: Multiple partition maps

[Robert Millan]

On Thu, Sep 04, 2008 at 11:49:21PM +0200, phcoder wrote:
> Robert Millan wrote:
> > On Wed, Sep 03, 2008 at 08:08:50PM +0200, phcoder wrote:
> >> Hello. I was looking at the grub code and seen that if a disk has
> >> multiple partition tables (e.g. macintel with bootcamp) then only first
> >> one will be detected. In some cases it can lead to unreachable
> >> partitions if for some reason partition is present only in one table.
> >> Does anyone has an idea how theese cases may be treated compactly and
> >> efficiently?
> > 
> > Strictly speaking, GPT+MSDOS hybrid tables are a violation of the GPT
> > specification.  It's not clear what would be the "correct" way of handling
> > them.
> > 
> > Since we're not a legacy program, I suppose the sane thing to do would be
> > to abort MSDOS probing if a protective DOS partition (0xee) is found, and
> > then only GPT will be used.
> 
> We're not a legacy application but some OS and partition tools are and
> we have to work with them. I think just some maintenance tools included
> in GRUB should be enough. I'll write them soon. (perhaps even tomorrow)

I don't understand.  Which kind of maintenance do you mean?  GRUB isn't
designed to modify partition maps, only read them.  Perhaps what you want
is more related to Parted?

> > Isn't this what GRUB does already?  I thought it would be...
> > 
> It does. But I don't know if there are other cases similar to this one.

There's also grub-setup.  On an hybrid map the only option is to follow the
GPT install path and search for a BIOS boot partition (or otherwise use
blocklists).  Does it DTRT already?

I don't know of any other similar situation.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: Multiple partition maps

[phcoder]

BTW GPT module checks the protective MBR. In some cases when legay OS
modified the MBR it's no longer "protective MBR". And in theese cases
GRUB will refuse to boot. Isn't the magic number check enough?
Vladimir 'phcoder' Serbinenko
Robert Millan wrote:
> On Wed, Sep 03, 2008 at 08:08:50PM +0200, phcoder wrote:
>> Hello. I was looking at the grub code and seen that if a disk has
>> multiple partition tables (e.g. macintel with bootcamp) then only first
>> one will be detected. In some cases it can lead to unreachable
>> partitions if for some reason partition is present only in one table.
>> Does anyone has an idea how theese cases may be treated compactly and
>> efficiently?
> 
> Strictly speaking, GPT+MSDOS hybrid tables are a violation of the GPT
> specification.  It's not clear what would be the "correct" way of handling
> them.
> 
> Since we're not a legacy program, I suppose the sane thing to do would be
> to abort MSDOS probing if a protective DOS partition (0xee) is found, and
> then only GPT will be used.
> 
> Isn't this what GRUB does already?  I thought it would be...
> 

Re: Multiple partition maps

[Robert Millan]

On Thu, Sep 04, 2008 at 11:54:43PM +0200, phcoder wrote:
> BTW GPT module checks the protective MBR. In some cases when legay OS
> modified the MBR it's no longer "protective MBR". And in theese cases
> GRUB will refuse to boot. Isn't the magic number check enough?

If there's at least one protective GPT partition (0xee), I think this should
be considered enough to accept the partmap as GPT.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: Multiple partition maps

[phcoder]

Robert Millan wrote:
> On Thu, Sep 04, 2008 at 11:54:43PM +0200, phcoder wrote:
>> BTW GPT module checks the protective MBR. In some cases when legay OS
>> modified the MBR it's no longer "protective MBR". And in theese cases
>> GRUB will refuse to boot. Isn't the magic number check enough?
> 
> If there's at least one protective GPT partition (0xee), I think this should
> be considered enough to accept the partmap as GPT.
> 
In GPT module if first partition is not of type 0xee then it's
considered that no GPT is present. Is think that this check is
error-prone (with e.g. bootcamp) and unnecessary
Vladimir 'phcoder' Serbinenko

Re: Multiple partition maps

[Robert Millan]

On Mon, Sep 08, 2008 at 08:27:05PM +0200, phcoder wrote:
> Robert Millan wrote:
> > On Thu, Sep 04, 2008 at 11:54:43PM +0200, phcoder wrote:
> >> BTW GPT module checks the protective MBR. In some cases when legay OS
> >> modified the MBR it's no longer "protective MBR". And in theese cases
> >> GRUB will refuse to boot. Isn't the magic number check enough?
> > 
> > If there's at least one protective GPT partition (0xee), I think this should
> > be considered enough to accept the partmap as GPT.
> > 
> In GPT module if first partition is not of type 0xee then it's
> considered that no GPT is present. Is think that this check is
> error-prone (with e.g. bootcamp) and unnecessary

Agreed.  Can you fix this?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: Multiple partition maps

[phcoder]

Robert Millan wrote:
> On Mon, Sep 08, 2008 at 08:27:05PM +0200, phcoder wrote:
>> Robert Millan wrote:
>>> On Thu, Sep 04, 2008 at 11:54:43PM +0200, phcoder wrote:
>>>> BTW GPT module checks the protective MBR. In some cases when legay OS
>>>> modified the MBR it's no longer "protective MBR". And in theese cases
>>>> GRUB will refuse to boot. Isn't the magic number check enough?
>>> If there's at least one protective GPT partition (0xee), I think this should
>>> be considered enough to accept the partmap as GPT.
>>>
>> In GPT module if first partition is not of type 0xee then it's
>> considered that no GPT is present. Is think that this check is
>> error-prone (with e.g. bootcamp) and unnecessary
> 
> Agreed.  Can you fix this?
> 
OK.

[PATCH] Re: Multiple partition maps

[phcoder]

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

Robert Millan wrote:
> On Mon, Sep 08, 2008 at 08:27:05PM +0200, phcoder wrote:
>> Robert Millan wrote:
>>> On Thu, Sep 04, 2008 at 11:54:43PM +0200, phcoder wrote:
>>>> BTW GPT module checks the protective MBR. In some cases when legay OS
>>>> modified the MBR it's no longer "protective MBR". And in theese cases
>>>> GRUB will refuse to boot. Isn't the magic number check enough?
>>> If there's at least one protective GPT partition (0xee), I think this should
>>> be considered enough to accept the partmap as GPT.
>>>
>> In GPT module if first partition is not of type 0xee then it's
>> considered that no GPT is present. Is think that this check is
>> error-prone (with e.g. bootcamp) and unnecessary
> 
> Agreed.  Can you fix this?
> 

I send a patch for it. However I couldn't test it because of the bug in
the make system. About the bug I'll post in appropriate thread. In that
patch the pc module explicitely checks for the absence of GPT table. IMO
it's ugly. Another alternative would be to assign priorities to the
partition tables. I also tried this way and send patch for it. Again I
couldn't test it.

Vladimir 'phcoder' Serbinenko


[-- Attachment #2: parts.diff --]
[-- Type: text/x-diff, Size: 4704 bytes --]

Index: kern/partition.c
===================================================================
--- kern/partition.c	(revision 1861)
+++ kern/partition.c	(working copy)
@@ -25,8 +25,20 @@
 void
 grub_partition_map_register (grub_partition_map_t partmap)
 {
-  partmap->next = grub_partition_map_list;
-  grub_partition_map_list = partmap;
+  grub_partition_map_t cur, prev=0;
+  for (cur=grub_partition_map_list;cur && cur->priority>partmap->priority; 
+       cur=cur->next)
+    prev=cur;
+  if (!prev)
+    {
+      partmap->next = grub_partition_map_list;
+      grub_partition_map_list = partmap;
+    }
+  else
+    {
+      partmap->next=prev->next;
+      prev->next=partmap;
+    }
 }
 
 void
Index: include/grub/partition.h
===================================================================
--- include/grub/partition.h	(revision 1861)
+++ include/grub/partition.h	(working copy)
@@ -42,6 +42,9 @@
   
   /* Return the name of the partition PARTITION.  */
   char *(*get_name) (const grub_partition_t partition);
+
+  /* Partmaps with higher priorities are checked first */
+  int priority;
   
   /* The next partition map type.  */
   struct grub_partition_map *next;
Index: partmap/amiga.c
===================================================================
--- partmap/amiga.c	(revision 1861)
+++ partmap/amiga.c	(working copy)
@@ -205,7 +205,8 @@
     .name = "amiga_partition_map",
     .iterate = amiga_partition_map_iterate,
     .probe = amiga_partition_map_probe,
-    .get_name = amiga_partition_map_get_name
+    .get_name = amiga_partition_map_get_name,
+    .priority = 0
   };
 
 GRUB_MOD_INIT(amiga_partition_map)
Index: partmap/apple.c
===================================================================
--- partmap/apple.c	(revision 1861)
+++ partmap/apple.c	(working copy)
@@ -241,7 +241,8 @@
     .name = "apple_partition_map",
     .iterate = apple_partition_map_iterate,
     .probe = apple_partition_map_probe,
-    .get_name = apple_partition_map_get_name
+    .get_name = apple_partition_map_get_name,
+    .priority = 0
   };
 
 GRUB_MOD_INIT(apple_partition_map)
Index: partmap/acorn.c
===================================================================
--- partmap/acorn.c	(revision 1861)
+++ partmap/acorn.c	(working copy)
@@ -192,7 +192,8 @@
   .name = "Linux/ADFS partition map",
   .iterate = acorn_partition_map_iterate,
   .probe = acorn_partition_map_probe,
-  .get_name = acorn_partition_map_get_name
+  .get_name = acorn_partition_map_get_name,
+  .priority = 0
 };
 
 GRUB_MOD_INIT(acorn_partition_map)
Index: partmap/pc.c
===================================================================
--- partmap/pc.c	(revision 1861)
+++ partmap/pc.c	(working copy)
@@ -111,7 +111,7 @@
   pcdata.dos_part = -1;
   p.data = &pcdata;
   p.partmap = &grub_pc_partition_map;
-  
+
   while (1)
     {
       int i;
@@ -303,7 +303,8 @@
     .name = "pc_partition_map",
     .iterate = pc_partition_map_iterate,
     .probe = pc_partition_map_probe,
-    .get_name = pc_partition_map_get_name
+    .get_name = pc_partition_map_get_name,
+    .priority = -10
   };
 
 GRUB_MOD_INIT(pc_partition_map)
Index: partmap/sun.c
===================================================================
--- partmap/sun.c	(revision 1861)
+++ partmap/sun.c	(working copy)
@@ -205,7 +205,8 @@
     .name = "sun_partition_map",
     .iterate = sun_partition_map_iterate,
     .probe = sun_partition_map_probe,
-    .get_name = sun_partition_map_get_name
+    .get_name = sun_partition_map_get_name,
+    .priority = 0
   };
 
 GRUB_MOD_INIT(sun_partition_map)
Index: partmap/gpt.c
===================================================================
--- partmap/gpt.c	(revision 1861)
+++ partmap/gpt.c	(working copy)
@@ -58,18 +58,6 @@
   raw = *disk;
   raw.partition = 0;
 
-  /* Read the protective MBR.  */
-  if (grub_disk_read (&raw, 0, 0, sizeof (mbr), (char *) &mbr))
-    return grub_errno;
-
-  /* Check if it is valid.  */
-  if (mbr.signature != grub_cpu_to_le16 (GRUB_PC_PARTITION_SIGNATURE))
-    return grub_error (GRUB_ERR_BAD_PART_TABLE, "no signature");
-
-  /* Make sure the MBR is a protective MBR and not a normal MBR.  */
-  if (mbr.entries[0].type != GRUB_PC_PARTITION_TYPE_GPT_DISK)
-    return grub_error (GRUB_ERR_BAD_PART_TABLE, "no GPT partition map found");
-
   /* Read the GPT header.  */
   if (grub_disk_read (&raw, 1, 0, sizeof (gpt), (char *) &gpt))
     return grub_errno;
@@ -183,7 +171,8 @@
     .name = "gpt_partition_map",
     .iterate = gpt_partition_map_iterate,
     .probe = gpt_partition_map_probe,
-    .get_name = gpt_partition_map_get_name
+    .get_name = gpt_partition_map_get_name,
+    .priority = 0
   };
 
 GRUB_MOD_INIT(gpt_partition_map)

[-- Attachment #3: partsb.diff --]
[-- Type: text/x-diff, Size: 1764 bytes --]

Index: pc.c
===================================================================
--- pc.c        (revision 1861)
+++ pc.c        (working copy)
@@ -19,11 +19,17 @@

 #include <grub/partition.h>
 #include <grub/pc_partition.h>
+#include <grub/gpt_partition.h>
 #include <grub/disk.h>
 #include <grub/mm.h>
 #include <grub/misc.h>
 #include <grub/dl.h>

+static grub_uint8_t grub_gpt_magic[8] =
+  {
+    0x45, 0x46, 0x49, 0x20, 0x50, 0x41, 0x52, 0x54
+  };
+
 static struct grub_partition_map grub_pc_partition_map;

 #ifndef GRUB_UTIL
@@ -111,6 +117,13 @@
   pcdata.dos_part = -1;
   p.data = &pcdata;
   p.partmap = &grub_pc_partition_map;
+
+  /* Read the GPT header.  */
+  if (grub_disk_read (&raw, 1, 0, sizeof (gpt), (char *) &gpt))
+    return grub_errno;
+
+  if (!grub_memcmp (gpt.magic, grub_gpt_magic, sizeof (grub_gpt_magic)))
+    return grub_error (GRUB_ERR_BAD_PART_TABLE, "valid GPT header");

   while (1)
     {
Index: gpt.c
===================================================================
--- gpt.c       (revision 1861)
+++ gpt.c       (working copy)
@@ -58,18 +58,6 @@
   raw = *disk;
   raw.partition = 0;

-  /* Read the protective MBR.  */
-  if (grub_disk_read (&raw, 0, 0, sizeof (mbr), (char *) &mbr))
-    return grub_errno;
-
-  /* Check if it is valid.  */
-  if (mbr.signature != grub_cpu_to_le16 (GRUB_PC_PARTITION_SIGNATURE))
-    return grub_error (GRUB_ERR_BAD_PART_TABLE, "no signature");
-
-  /* Make sure the MBR is a protective MBR and not a normal MBR.  */
-  if (mbr.entries[0].type != GRUB_PC_PARTITION_TYPE_GPT_DISK)
-    return grub_error (GRUB_ERR_BAD_PART_TABLE, "no GPT partition map found");
-
   /* Read the GPT header.  */
   if (grub_disk_read (&raw, 1, 0, sizeof (gpt), (char *) &gpt))
     return grub_errno;

Re: [PATCH] Re: Multiple partition maps

[Robert Millan]

On Tue, Sep 09, 2008 at 02:47:11AM +0200, phcoder wrote:
> Robert Millan wrote:
> > On Mon, Sep 08, 2008 at 08:27:05PM +0200, phcoder wrote:
> >> Robert Millan wrote:
> >>> On Thu, Sep 04, 2008 at 11:54:43PM +0200, phcoder wrote:
> >>>> BTW GPT module checks the protective MBR. In some cases when legay OS
> >>>> modified the MBR it's no longer "protective MBR". And in theese cases
> >>>> GRUB will refuse to boot. Isn't the magic number check enough?
> >>> If there's at least one protective GPT partition (0xee), I think this should
> >>> be considered enough to accept the partmap as GPT.
> >>>
> >> In GPT module if first partition is not of type 0xee then it's
> >> considered that no GPT is present. Is think that this check is
> >> error-prone (with e.g. bootcamp) and unnecessary
> > 
> > Agreed.  Can you fix this?
> > 
> 
> I send a patch for it. However I couldn't test it because of the bug in
> the make system. About the bug I'll post in appropriate thread. In that
> patch the pc module explicitely checks for the absence of GPT table. IMO
> it's ugly. Another alternative would be to assign priorities to the
> partition tables. I also tried this way and send patch for it. Again I
> couldn't test it.

This patch looks overly complicated.  Fixing the discussed problem ought
to be much simpler...

> -  /* Make sure the MBR is a protective MBR and not a normal MBR.  */
> -  if (mbr.entries[0].type != GRUB_PC_PARTITION_TYPE_GPT_DISK)
> -    return grub_error (GRUB_ERR_BAD_PART_TABLE, "no GPT partition map found");

... here, it'd be just a matter of replacing this hardcoded '0' with an
iterator.

And maybe something similar for partmap/pc.c.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Re: Multiple partition maps

[phcoder]

Robert Millan wrote:
> On Tue, Sep 09, 2008 at 02:47:11AM +0200, phcoder wrote:
>> Robert Millan wrote:
>>> On Mon, Sep 08, 2008 at 08:27:05PM +0200, phcoder wrote:
>>>> Robert Millan wrote:
>>>>> On Thu, Sep 04, 2008 at 11:54:43PM +0200, phcoder wrote:
>>>>>> BTW GPT module checks the protective MBR. In some cases when legay OS
>>>>>> modified the MBR it's no longer "protective MBR". And in theese cases
>>>>>> GRUB will refuse to boot. Isn't the magic number check enough?
>>>>> If there's at least one protective GPT partition (0xee), I think this should
>>>>> be considered enough to accept the partmap as GPT.
>>>>>
>>>> In GPT module if first partition is not of type 0xee then it's
>>>> considered that no GPT is present. Is think that this check is
>>>> error-prone (with e.g. bootcamp) and unnecessary
>>> Agreed.  Can you fix this?
>>>
>> I send a patch for it. However I couldn't test it because of the bug in
>> the make system. About the bug I'll post in appropriate thread. In that
>> patch the pc module explicitely checks for the absence of GPT table. IMO
>> it's ugly. Another alternative would be to assign priorities to the
>> partition tables. I also tried this way and send patch for it. Again I
>> couldn't test it.
> 
> This patch looks overly complicated.  Fixing the discussed problem ought
> to be much simpler...
> 
>> -  /* Make sure the MBR is a protective MBR and not a normal MBR.  */
>> -  if (mbr.entries[0].type != GRUB_PC_PARTITION_TYPE_GPT_DISK)
>> -    return grub_error (GRUB_ERR_BAD_PART_TABLE, "no GPT partition map found");
> 
> ... here, it'd be just a matter of replacing this hardcoded '0' with an
> iterator.
> 
> And maybe something similar for partmap/pc.c.
> 
I don't think that this is enough since with bootcamp you can have a MBR
 without a protective entry. This leaves us with just two option:
priorities and ugly check in partmap/pc.c. I would prefer the first one
since the second one is unextensible. With first one I'll be able to do
something similar to what you can find in other bootmanagers: ability to
have more then one "primary" partition (like in http://symon.ru ).
Vladimir 'phcoder' Serbinenko

Re: [PATCH] Re: Multiple partition maps

[Robert Millan]

On Tue, Sep 09, 2008 at 03:16:26PM +0200, phcoder wrote:
> I don't think that this is enough since with bootcamp you can have a MBR
>  without a protective entry.

Then it looks like an MSDOS partition map to me.  Is this a normal use case,
or something weird?

Also, what do others (e.g. Linux) do?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."