[PATCH] grub-setup Modify the conditionality of the copy of the partition table

[PATCH] grub-setup Modify the conditionality of the copy of the partition table

[Mario Limonciello]

[-- Attachment #1.1: Type: text/plain, Size: 928 bytes --]

 Hi:

I've discovered that in snapshots of GRUB2 in Ubuntu natty, you are no longer able to install GRUB2 to a partition and successfully boot on certain Dell E-series Latitudes.  This is a regression from the GRUB2 that was included in Ubuntu maverick.  The BIOS makes an assertion based on the content of the partition table section of the PBR and content that doesn't resemble a partition table will cause the the BIOS to get stuck in a loop and never complete POST.

I've identified the changeset that caused this to be r2751.  I've created a patch that modifies the conditionality of the partition table copy to not check for a partition table, but instead to verify it's not operating on a floppy disk.  This restores the previous behavior when installing to a partition, and still allows the new behavior for floppy disk installations.

Thanks,

-- 
*Mario Limonciello*
Linux Engineer
*Dell* | OS Engineering

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: always_copy_partition_table.patch --]
[-- Type: text/x-diff; name="always_copy_partition_table.patch", Size: 2238 bytes --]

=== modified file 'ChangeLog'
--- ChangeLog	2011-03-23 19:29:17 +0000
+++ ChangeLog	2011-03-25 21:56:23 +0000
@@ -1,3 +1,11 @@
+2011-03-24  Mario Limonciello    <Mario_Limonciello@Dell.com>
+        * util/grub-setup.c: Conditionalize the partition map copy on floppy
+          support, not on whether the target contains partitions.
+
+          Otherwise, the BIOS on Dell Latitude E series laptops will freeze 
+          during POST if an invalid partition table is contained in the PBR
+          of the active partition when GRUB is installed to a partition.
+
 2011-03-23  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	* grub-core/term/gfxterm.c (calculate_normal_character_width): Return 8

=== modified file 'util/grub-setup.c'
--- util/grub-setup.c	2011-01-07 12:27:34 +0000
+++ util/grub-setup.c	2011-03-25 21:51:56 +0000
@@ -399,25 +399,25 @@
       }
 #endif
 
-    if (! dest_partmap)
-      {
-	grub_util_warn (_("Attempting to install GRUB to a partitionless disk or to a partition.  This is a BAD idea."));
-	goto unable_to_embed;
-      }
-    if (multiple_partmaps || fs)
-      {
-	grub_util_warn (_("Attempting to install GRUB to a disk with multiple partition labels or both partition label and filesystem.  This is not supported yet."));
-	goto unable_to_embed;
-      }
-
     /* Copy the partition table.  */
-    if (dest_partmap)
+    if (!allow_floppy || !grub_util_biosdisk_is_floppy (dest_dev->disk))
       memcpy (boot_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
 	      tmp_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
 	      GRUB_BOOT_MACHINE_PART_END - GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC);
 
     free (tmp_img);
     
+    if (! dest_partmap)
+      {
+	grub_util_warn (_("Attempting to install GRUB to a partitionless disk or to a partition.  This is a BAD idea."));
+	goto unable_to_embed;
+      }
+    if (multiple_partmaps || fs)
+      {
+	grub_util_warn (_("Attempting to install GRUB to a disk with multiple partition labels or both partition label and filesystem.  This is not supported yet."));
+	goto unable_to_embed;
+      }
+
     if (!dest_partmap->embed)
       {
 	grub_util_warn ("Partition style '%s' doesn't support embeding",


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

Re: [PATCH] grub-setup Modify the conditionality of the copy of the partition table

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 25.03.2011 23:13, Mario Limonciello wrote:
> +        * util/grub-setup.c: Conditionalize the partition map copy on floppy
> +          support, not on whether the target contains partitions.
> +
> +          Otherwise, the BIOS on Dell Latitude E series laptops will freeze 
> +          during POST if an invalid partition table is contained in the PBR
> +          of the active partition when GRUB is installed to a partition.
It's wrong to assume that no floppies contain partition tables. Also
--allow-floppy is usually used for USB sticks which sometimes appear as
floppies on some BIOSes and they pretty much contain the partition
table. Bottom line is: if you see a partition table: preserve it. But
it's ok to preserve this contents if floppy support is disabled even if
no partition table is discovered. A small logic lecture:

Floppy support is: allow_floppy || grub_util_biosdisk_is_floppy (dest_dev->disk)
Negation of it is either !(allow_floppy || grub_util_biosdisk_is_floppy (dest_dev->disk)) or !allow_floppy && !grub_util_biosdisk_is_floppy (dest_dev->disk), not what you wrote.

Please don't move around unrelated parts.


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



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

Re: [PATCH] grub-setup Modify the conditionality of the copy of the partition table

[Colin Watson]

On Fri, Mar 25, 2011 at 11:55:32PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 25.03.2011 23:13, Mario Limonciello wrote:
> > +        * util/grub-setup.c: Conditionalize the partition map copy on floppy
> > +          support, not on whether the target contains partitions.
> > +
> > +          Otherwise, the BIOS on Dell Latitude E series laptops will freeze 
> > +          during POST if an invalid partition table is contained in the PBR
> > +          of the active partition when GRUB is installed to a partition.
> 
> It's wrong to assume that no floppies contain partition tables. Also
> --allow-floppy is usually used for USB sticks which sometimes appear as
> floppies on some BIOSes and they pretty much contain the partition
> table. Bottom line is: if you see a partition table: preserve it.

Reading from floppies requires the floppy_probe function in boot.S,
doesn't it, which collides with the partition table?  But it makes sense
to keep dest_partmap in the condition for safety anyway (and it saves a
call to grub_util_biosdisk_is_floppy), so we'd end up with:

    if (dest_partmap ||
        (!allow_floppy && !grub_util_biosdisk_is_floppy (dest_dev->disk)))
      memcpy (boot_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
              tmp_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
              GRUB_BOOT_MACHINE_PART_END - GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC);

?

> But it's ok to preserve this contents if floppy support is disabled
> even if no partition table is discovered.

In the case Mario discovered, there was indeed no partition table (all
zeroes).  Writing the floppy-probing code into that area caused it to
fail.

> A small logic lecture:
> 
> Floppy support is: allow_floppy || grub_util_biosdisk_is_floppy (dest_dev->disk)
> Negation of it is either !(allow_floppy || grub_util_biosdisk_is_floppy (dest_dev->disk)) or !allow_floppy && !grub_util_biosdisk_is_floppy (dest_dev->disk), not what you wrote.

Agreed.  The latter form is found elsewhere in grub-setup.

> Please don't move around unrelated parts.

The parts Mario moved around weren't unrelated.  Since the case at hand
is that of installing to a partition boot record, this if block will
run:

    if (! dest_partmap)
      {
        grub_util_warn (_("Attempting to install GRUB to a partitionless disk or to a partition.  This is a BAD idea."));
        goto unable_to_embed;
      }

In order to preserve the partition table (or lack thereof) in this case,
it was necessary to move the memcpy up above that test.

Cheers,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] grub-setup Modify the conditionality of the copy of the partition table

[Mario Limonciello]

[-- Attachment #1.1.1: Type: text/plain, Size: 2886 bytes --]

 Hi Colin & Vladimir:

On 03/25/2011 06:31 PM, Colin Watson wrote:
> On Fri, Mar 25, 2011 at 11:55:32PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> On 25.03.2011 23:13, Mario Limonciello wrote:
>>> +        * util/grub-setup.c: Conditionalize the partition map copy on floppy
>>> +          support, not on whether the target contains partitions.
>>> +
>>> +          Otherwise, the BIOS on Dell Latitude E series laptops will freeze 
>>> +          during POST if an invalid partition table is contained in the PBR
>>> +          of the active partition when GRUB is installed to a partition.
>> It's wrong to assume that no floppies contain partition tables. Also
>> --allow-floppy is usually used for USB sticks which sometimes appear as
>> floppies on some BIOSes and they pretty much contain the partition
>> table. Bottom line is: if you see a partition table: preserve it.
> Reading from floppies requires the floppy_probe function in boot.S,
> doesn't it, which collides with the partition table?  But it makes sense
> to keep dest_partmap in the condition for safety anyway (and it saves a
> call to grub_util_biosdisk_is_floppy), so we'd end up with:
>
>     if (dest_partmap ||
>         (!allow_floppy && !grub_util_biosdisk_is_floppy (dest_dev->disk)))
>       memcpy (boot_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
>               tmp_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
>               GRUB_BOOT_MACHINE_PART_END - GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC);
>
> ?
>
>> But it's ok to preserve this contents if floppy support is disabled
>> even if no partition table is discovered.
> In the case Mario discovered, there was indeed no partition table (all
> zeroes).  Writing the floppy-probing code into that area caused it to
> fail.
>
>> A small logic lecture:
>>
>> Floppy support is: allow_floppy || grub_util_biosdisk_is_floppy (dest_dev->disk)
>> Negation of it is either !(allow_floppy || grub_util_biosdisk_is_floppy (dest_dev->disk)) or !allow_floppy && !grub_util_biosdisk_is_floppy (dest_dev->disk), not what you wrote.
> Agreed.  The latter form is found elsewhere in grub-setup.
>
>> Please don't move around unrelated parts.
> The parts Mario moved around weren't unrelated.  Since the case at hand
> is that of installing to a partition boot record, this if block will
> run:
>
>     if (! dest_partmap)
>       {
>         grub_util_warn (_("Attempting to install GRUB to a partitionless disk or to a partition.  This is a BAD idea."));
>         goto unable_to_embed;
>       }
>
> In order to preserve the partition table (or lack thereof) in this case,
> it was necessary to move the memcpy up above that test.
>
> Cheers,
>
Thanks for the comments.  I agree fully with Colin's comments.  Here's an updated patch.

-- 
*Mario Limonciello*
Linux Engineer
*Dell* | OS Engineering

[-- Attachment #1.1.2: Type: text/html, Size: 4743 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: always_copy_partition_table.patch --]
[-- Type: text/x-diff; name="always_copy_partition_table.patch", Size: 2266 bytes --]

=== modified file 'ChangeLog'
--- ChangeLog	2011-03-23 19:29:17 +0000
+++ ChangeLog	2011-03-25 21:56:23 +0000
@@ -1,3 +1,11 @@
+2011-03-24  Mario Limonciello    <Mario_Limonciello@Dell.com>
+        * util/grub-setup.c: Conditionalize the partition map copy on floppy
+          support, not on whether the target contains partitions.
+
+          Otherwise, the BIOS on Dell Latitude E series laptops will freeze 
+          during POST if an invalid partition table is contained in the PBR
+          of the active partition when GRUB is installed to a partition.
+
 2011-03-23  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	* grub-core/term/gfxterm.c (calculate_normal_character_width): Return 8

=== modified file 'util/grub-setup.c'
--- util/grub-setup.c	2011-01-07 12:27:34 +0000
+++ util/grub-setup.c	2011-03-26 00:31:58 +0000
@@ -399,25 +399,26 @@
       }
 #endif
 
-    if (! dest_partmap)
-      {
-	grub_util_warn (_("Attempting to install GRUB to a partitionless disk or to a partition.  This is a BAD idea."));
-	goto unable_to_embed;
-      }
-    if (multiple_partmaps || fs)
-      {
-	grub_util_warn (_("Attempting to install GRUB to a disk with multiple partition labels or both partition label and filesystem.  This is not supported yet."));
-	goto unable_to_embed;
-      }
-
     /* Copy the partition table.  */
-    if (dest_partmap)
+    if (dest_partmap ||
+        (!allow_floppy && !grub_util_biosdisk_is_floppy (dest_dev->disk)))
       memcpy (boot_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
 	      tmp_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
 	      GRUB_BOOT_MACHINE_PART_END - GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC);
 
     free (tmp_img);
     
+    if (! dest_partmap)
+      {
+	grub_util_warn (_("Attempting to install GRUB to a partitionless disk or to a partition.  This is a BAD idea."));
+	goto unable_to_embed;
+      }
+    if (multiple_partmaps || fs)
+      {
+	grub_util_warn (_("Attempting to install GRUB to a disk with multiple partition labels or both partition label and filesystem.  This is not supported yet."));
+	goto unable_to_embed;
+      }
+
     if (!dest_partmap->embed)
       {
 	grub_util_warn ("Partition style '%s' doesn't support embeding",


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