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
=== 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_limoncie...@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 <phco...@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",
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
