From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1Q3HUD-0000Bf-8g for mharc-grub-devel@gnu.org; Fri, 25 Mar 2011 20:36:13 -0400 Received: from [140.186.70.92] (port=41083 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q3HU9-0000AH-QR for grub-devel@gnu.org; Fri, 25 Mar 2011 20:36:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q3HU8-00034B-ES for grub-devel@gnu.org; Fri, 25 Mar 2011 20:36:09 -0400 Received: from ausc60pc101.us.dell.com ([143.166.85.206]:54091) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q3HU7-00033U-U2 for grub-devel@gnu.org; Fri, 25 Mar 2011 20:36:08 -0400 X-Loopcount0: from 10.9.160.253 Message-ID: <4D8D3534.5040407@dell.com> Date: Fri, 25 Mar 2011 19:37:08 -0500 From: Mario Limonciello Organization: Dell Inc. User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.8) Gecko/20100802 Thunderbird/3.1.2 MIME-Version: 1.0 To: Colin Watson References: <4D8D1397.5030802@dell.com> <4D8D1D64.2040801@gmail.com> <20110325233122.GC9163@riva.ucam.org> In-Reply-To: <20110325233122.GC9163@riva.ucam.org> X-Enigmail-Version: 1.1.1 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig269228ABE38C837CE084883C" X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 143.166.85.206 Cc: =?UTF-8?B?VmxhZGltaXIgJ8+GLWNvZGVyL3BoY29kZXInIFNlcmJpbmVua28=?= , "grub-devel@gnu.org" Subject: Re: [PATCH] grub-setup Modify the conditionality of the copy of the partition table X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 26 Mar 2011 00:36:11 -0000 This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig269228ABE38C837CE084883C Content-Type: multipart/mixed; boundary="------------090101010303030304010808" This is a multi-part message in MIME format. --------------090101010303030304010808 Content-Type: multipart/alternative; boundary="------------050808030403000306030400" --------------050808030403000306030400 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hi Colin & Vladimir: On 03/25/2011 06:31 PM, Colin Watson wrote: > On Fri, Mar 25, 2011 at 11:55:32PM +0100, Vladimir '=CF=86-coder/phcode= r' Serbinenko wrote: >> On 25.03.2011 23:13, Mario Limonciello wrote: >>> + * util/grub-setup.c: Conditionalize the partition map copy o= n floppy >>> + support, not on whether the target contains partitions. >>> + >>> + Otherwise, the BIOS on Dell Latitude E series laptops will= freeze=20 >>> + during POST if an invalid partition table is contained in = the PBR >>> + of the active partition when GRUB is installed to a partit= ion. >> It's wrong to assume that no floppies contain partition tables. Also >> --allow-floppy is usually used for USB sticks which sometimes appear a= s >> 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 sens= e > 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_flopp= y (dest_dev->disk)) or !allow_floppy && !grub_util_biosdisk_is_floppy (de= st_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 partitionles= s 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. --=20 *Mario Limonciello* Linux Engineer *Dell* | OS Engineering --------------050808030403000306030400 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hi Colin & Vladimir:

On 03/25/2011 06:31 PM, Colin Watson wrote: 
On Fri, Mar 25, 2011 at 11:55:32PM +0100, Vladimir '=
=CF=86-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 fre=
eze=20
+          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_M=
AGIC);

?


But it's ok to preserve this contents if floppy su=
pport 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_fl=
oppy (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.=C2=A0 I agree fully with Colin's comments.=C2= =A0 Here's an updated patch.

--
Mario Limonciello
 Linux Engineer
Dell | OS Engineering
--------------050808030403000306030400-- --------------090101010303030304010808 Content-Type: text/x-diff; name="always_copy_partition_table.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="always_copy_partition_table.patch" =3D=3D=3D 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 + * util/grub-setup.c: Conditionalize the partition map copy on fl= oppy + support, not on whether the target contains partitions. + + Otherwise, the BIOS on Dell Latitude E series laptops will fre= eze=20 + 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 =20 * grub-core/term/gfxterm.c (calculate_normal_character_width): Return 8= =3D=3D=3D 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 =20 - if (! dest_partmap) - { - grub_util_warn (_("Attempting to install GRUB to a partitionless disk o= r 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 p= artition labels or both partition label and filesystem. This is not supp= orted 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);= =20 free (tmp_img); =20 + if (! dest_partmap) + { + grub_util_warn (_("Attempting to install GRUB to a partitionless disk o= r 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 p= artition labels or both partition label and filesystem. This is not supp= orted yet.")); + goto unable_to_embed; + } + if (!dest_partmap->embed) { grub_util_warn ("Partition style '%s' doesn't support embeding", --------------090101010303030304010808-- --------------enig269228ABE38C837CE084883C Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk2NNTQACgkQ2CrZjkA73Yti1gCeP+Dhi+09SrohZwk8Njixzrri Vk8AnRYgc24O+XLUTqOgdS5Ei9kucLhQ =lvyC -----END PGP SIGNATURE----- --------------enig269228ABE38C837CE084883C--