Re: [Libguestfs] [PATCH] inspect: check presence of BIOS boot partition to handle BIOS+GPT setup

On 12/16/22 22:02, Andrey Drobyshev wrote: > On 12/16/22 13:13, Laszlo Ersek wrote: >> On 12/15/22 20:29, Andrey Drobyshev wrote: >>> If the guest uses BIOS firmware but GPT partitioned disk, and in >>> addition has "bios, esp" flag enabled on its "/boot" partition, then >>> inspection wrongly detects UEFI firmware and v2v ends up with the error: >>> "error: no bootloader detected". >>> >>> Let's fix this by checking for the presence of BIOS boot partition (0xef02 >>> type for gdisk, "bios_grub" flag for parted), which is used to store a >>> bootloader code in BIOS+GPT configurations. If such a partition is >>> present, then it's likely a BIOS+GPT setup. >>> >>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com&gt; >>> --- >>> convert/inspect_source.ml | 27 +++++++++++++++++++++++---- >>> 1 file changed, 23 insertions(+), 4 deletions(-) >>> >>> diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml >>> index 056d0bca..6650e136 100644 >>> --- a/convert/inspect_source.ml >>> +++ b/convert/inspect_source.ml >>> @@ -219,6 +219,9 @@ and list_applications g root = function >>> (* See if this guest could use UEFI to boot. It should use GPT and >>> * it should have an EFI System Partition (ESP). >>> * >>> + * If it has a BIOS boot partition (BIOS+GPT setup), then [BIOS] is >>> + * returned. >>> + * >>> * If it has ESP(s), then [UEFI devs] is returned where [devs] is the >>> * list of at least one ESP. >>> * >>> @@ -226,9 +229,13 @@ and list_applications g root = function >>> *) >>> and get_firmware_bootable_device g = >>> let rec uefi_ESP_guid = "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" >>> + and bios_boot_guid = "21686148-6449-6E6F-744E-656564454649" >>> and is_uefi_ESP dev part = >>> let partnum = g#part_to_partnum part in >>> g#part_get_gpt_type dev partnum = uefi_ESP_guid >>> + and is_bios_boot dev part = >>> + let partnum = g#part_to_partnum part in >>> + g#part_get_gpt_type dev partnum = bios_boot_guid >>> and parttype_is_gpt dev = >>> try g#part_get_parttype dev = "gpt" >>> with G.Error msg as exn -> >>> @@ -239,14 +246,26 @@ and get_firmware_bootable_device g = >>> and is_uefi_bootable_part part = >>> let dev = g#part_to_dev part in >>> parttype_is_gpt dev && is_uefi_ESP dev part >>> + and is_bios_gpt_part part = >>> + let dev = g#part_to_dev part in >>> + parttype_is_gpt dev && is_bios_boot dev part >>> in >>> >>> let partitions = Array.to_list (g#list_partitions ()) in >>> - let partitions = List.filter is_uefi_bootable_part partitions in >>> + let esp_partitions = List.filter is_uefi_bootable_part partitions in >>> >>> - match partitions with >>> - | [] -> I_BIOS >>> - | partitions -> I_UEFI partitions >>> + (* If there's a BIOS boot partition present (0xef02 type for gdisk, >>> + * "bios_grub" flag for parted), then this is likely a BIOS+GPT setup. >>> + * Note that if a source VM is using UEFI firmware and has a secondary >>> + * non-bootable disk attached which contains such a partition, the >>> + * firmware detection will detect I_BIOS wrongly. But this can only be >>> + * done manually, and we assume that there's no point doing it on purpose. >>> + *) >>> + if List.exists is_bios_gpt_part partitions then I_BIOS >>> + else >>> + match esp_partitions with >>> + | [] -> I_BIOS >>> + | esp_partitions -> I_UEFI esp_partitions >>> >>> (* If some inspection fields are "unknown", then that indicates a >>> * failure in inspection, and we shouldn't continue. For an example >> >> I've now re-read: >> >> https://en.wikipedia.org/wiki/GUID_Partition_Table >> https://en.wikipedia.org/wiki/BIOS_boot_partition >> >> and the proposed change seems to make sense. Effectively it adds an >> exception: in case we have at leasts one UEFI system partition (GPT >> partition entry with type GUID C12A7328-F81F-11D2-BA4B-00A0C93EC93B), >> but also a BIOS boot partition (GPT partition entry with type GUID >> 21686148-6449-6E6F-744E-656564454649), then state that the system >> under conversion uses BIOS and not UEFI. >> >> What I don't understand is what kind of system exposes such a >> partition scheme, requiring the above exception, for conversion. Can >> you describe that? > > Actually this is somewhat of a mystery for me as well, cause this fix > originated from a customer bug. There was a Debian 8 guest with the > following layout: > > # parted -l > Model: ATA harddisk (scsi) > Disk /dev/sda: 1288GB > Sector size (logical/physical): 512B/4096B > Partition Table: gpt > Disk Flags: pmbr_boot > > Number Start End Size File system Name Flags > 4 1049kB 2097kB 1049kB primary bios_grub > 1 2097kB 4297MB 4295MB linux-swap(v1) primary > 2 4297MB 4559MB 262MB ext4 primary boot, esp > 3 4559MB 1288GB 1284GB ext4 primary This GPT might even make sense, if not for the "esp" "flag", or more generally: partition 2. I have zero idea what parted intends to say with that. If you put part 2 aside for a minute: we have a BIOS computer with a GUID partition table; part of GRUB is in the BIOS Boot Partition (~1MB), we have about 4 GB of swap, and a root filesystem (1+ TB). Marking partition 2 as "esp" makes even less sense because it's really unexpected of any physical platform firmware to have an ext4 driver. In the wild, most ESPs are VFAT. The parted manual has the following to say: https://www.gnu.org/software/parted/manual/html_node/set.html > ‘boot’ > > (Mac, MS-DOS, PC98) - should be enabled if you want to boot off > the partition. The semantics vary between disk labels. For MS-DOS > disk labels, only one partition can be bootable. If you are > installing LILO on a partition that partition must be bootable. > For PC98 disk labels, all ext2 partitions must be bootable (this > is enforced by Parted). > > ‘esp’ > > (MS-DOS, GPT) - this flag identifies a UEFI System Partition. On > GPT it is an alias for boot. So I *guess* partition#2 qualifies as an EFI System Partition due to it having a partition type GUID C12A7328-F81F-11D2-BA4B-00A0C93EC93B, which parted translates to the flag "esp", which parted *further* translates to the flag "boot" due to the partitioning scheme being GPT. And, while part#2 has type C12A7328-F81F-11D2-BA4B-00A0C93EC93B, it is apparently also formatted to ext4. Honestly, this makes no sense to me. Back to your email: On 12/16/22 22:02, Andrey Drobyshev wrote: > AFAIU an OS installer wouldn't create a partition scheme that way, > even when dealing with GPT: there is either a BIOS boot partition > (BIOS firmware) or a EFI system partition (UEFI firmware). So I can't > really tell how we ended up with a scheme like this. But this > article: > > https://en.wikipedia.org/wiki/Boot_flag > > claims that >> Some BIOSes test if the boot flag of at least one partition is set, > otherwise they ignore the device in boot-order. But if this is a BIOS computer -- because if it weren't, why would we have a BIOS Boot partition in part#4 --, why not mark part#4 as bootable, using the "Legacy BIOS bootable" GPT entry attribute flag?

> > So could it be that this boot flag was manually set "just in case"? > > Another *possible* scenario to consider: > > - This disk image is initially used in an UEFI powered VM, hence > partition 2 has type "EFI system partition". No dedicated /boot > partition; > - The very same disk is then reused in a BIOS powered VM (without OS > installation); > - To be used in such a fashion, disk is re-partitioned so that > * Partition 2 is reused as /boot now, but parttype isn't changed; > * Swap partition is shrinked and BIOS boot partition is created in > the beginning of the disk (the fact that it's numbered 4 advocates > that > version). I think the disk (or disk image) must have indeed been in dual use, some way. FWIW, in your above description, partition#2 having been used as ESP at some point seems unlikely, because its filesystem is (apparently) ext4, and booting that even with virtual (guest) UEFI firmware is unlikely. It assumes that the guest firmware has an ext4 driver built-in. It is not impossible: https://github.com/tianocore/tianocore.github.io/wiki/Tasks-ext2-file-sys... But unlikely.

> In any case, I think it's safe to assume that firmware detection was > manually and artificially "broken" in this case. Moreover, even with > this patch applied it can be manually broken as well -- just as I've > mentioned in the code comments, by adding a secondary non-bootable > disk with BIOS boot partition on it. But IMO it's also safe to assume > that the latter scenario is less probable to happen accidentally. Right, so whichever way this (inconsistent!) partition table was created, what should virt-v2v do about it? If you convert it to a UEFI guest, it will not boot (because in the conversion output, we'll use OVMF, and OVMF will not be able to use the ext4 formatted ESP). > >> >> Furthermore, the commit message says, >> >> has "bios, esp" flag enabled on its "/boot" partition >> >> and I totally don't understand that. >> >> First, the /boot partition is orthogonal to both the UEFI system >> partition and the BIOS boot partition. The above wikipedia article, >> and various *new* specs: >> >> https://uapi-group.org/specifications/specs/discoverable_partitions_speci... >> https://www.freedesktop.org/software/systemd/man/systemd-gpt-auto-generat... >> >> try to make us believe that /boot is supposed to be described with >> type GUID bc13c2ff-59e6-4262-a352-b275fd6f7172. But in fact, on my >> just-installed RHEL-9.1 system, the /boot partition has type GUID >> 0FC63DAF-8483-4772-8E79-3D69D8477DE4 "Linux filesystem data". > > You're right, "bios" and "esp" have nothing to do with each other, > that is a typo. I meant to say "boot, esp" (see below). > >> >> Second, I don't understand the "bios" and "esp" references as >> *flags*. The Partition attributes at >> <https://en.wikipedia.org/wiki/GUID_Partition_Table#Partition_entries_(LBA... >> includes "Legacy BIOS bootable", but "esp" does not seem like a flag. > > This is the terminology used by parted: > https://www.gnu.org/software/parted/manual/html_node/set.html > > Namely, the docs say: >> ‘esp’ >> (MS-DOS, GPT) - this flag identifies a UEFI System Partition. On GPT > it is an alias for boot. > > So in case of a GPT partitioned disk it treats those as complete > synonyms, as such: > > # parted -l /dev/sda | egrep '^\s*2' > 2 2097kB 271MB 268MB ext4 > # parted /dev/sda "set 2 esp on" > # parted -l /dev/sda | egrep '^\s*2' > 2 2097kB 271MB 268MB ext4 boot, esp > # parted /dev/sda "set 2 boot off" > # parted -l /dev/sda | egrep '^\s*2' > 2 2097kB 271MB 268MB ext4 > > But then this flags aren't being treated by partition attributes, but > rather being interpreted by the partition tool to decide which > parttype to write in the partition table for that particular entry: > > # parted -l /dev/sda | egrep '^\s*2' > 2 2097kB 271MB 268MB ext4 <-- (no flags) > # lsblk -p -o NAME,PARTTYPE /dev/sda2 > NAME PARTTYPE > /dev/sda2 0fc63daf-8483-4772-8e79-3d69d8477de4 <-- (linux filesystem) > # parted /dev/sda "set 2 boot on" > # lsblk -p -o NAME,PARTTYPE /dev/sda2 > NAME PARTTYPE > /dev/sda2 c12a7328-f81f-11d2-ba4b-00a0c93ec93b <-- (EFI partition) > >> >> Third, while the commit message speaks about these flags, the code >> does not appear to deal with them at all. > > Again, in this context, as you can see, flags and parttypes are pretty > much interchangeable, and we actually are dealing with them. But I > agree that it could've been stated more clearly. OK, thanks; this seems to be aligned with my understanding of the parted docs. >> >> So here's what I suggest: >> >> (1) In the commit message, describe *precisely* a GPT partition table >> that currently breaks inspection. Best provide an actual example >> dump, with partition type GUIDs and partition attributes. > > Can specify the partition scheme from above. > >> >> (2) Furthermore, please describe what concrete system or partitioning >> utility the partition table comes from. >> >> (3) The "bios, esp" paragraph looks like a distraction, so I suggest >> dropping it. > > Should probably substitute that with the particular parttype GUIDs. > >> >> (4) Please clarify the second paragraph by saying what the code will >> do -- if the GPT has a partition entry with type GUID >> 21686148-6449-6E6F-744E-656564454649 ("BIOS boot partition"), then >> we'll give priority to that and mark the system as BIOS, even if >> there's another partition entry with type GUID >> C12A7328-F81F-11D2-BA4B-00A0C93EC93B ("EFI system partition"). > > Sure. ... I guess if you can include the parted listing and explain that this is actually a broken (inconsistent) partition table that we're just trying to cope with, and that giving priority to the BIOS Boot partition is the better -- but still not foolproof -- choice, that should suffice.

> >> >> (5) I don't really like the runtime logic duplication that seems to >> be introduced by the patch. By iterating over the list of partitions >> twice, we make twice as many libguestfs calls. I think we should >> iterate only once over the list of partitions. > > Agreed, iterating just once is obviously better. I'll try to come up > with the code which is less obscure. For instance, List.map with a > custom function, taking partition as its argument and returning a > tuple (Some part * bool). This way we'd get a list of tuples which > you referred to in your other comment. Can you try out the patches I proposed below? I agree that List.map + List.fold would work, but then (a) you don't need a separate mapping step just so you can fold the mapped list: you can push the mapping down into the folder callback; (b) I honestly doubt in this case that the purely functional implementation would be easier to read. Anyway, if you prefer that, I'm certainly OK with it.

> diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml index 056d0bca..5743c234 100644 --- a/convert/inspect_source.ml +++ b/convert/inspect_source.ml @@ -225,28 +225,39 @@ and list_applications g root = function * Otherwise, [BIOS] is returned. *) and get_firmware_bootable_device g = - let rec uefi_ESP_guid = "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" - and is_uefi_ESP dev part = - let partnum = g#part_to_partnum part in - g#part_get_gpt_type dev partnum = uefi_ESP_guid - and parttype_is_gpt dev = + let parttype_is_gpt dev = try g#part_get_parttype dev = "gpt" with G.Error msg as exn -> (* If it's _not_ "unrecognised disk label" then re-raise it. *) if g#last_errno () <> G.Errno.errno_EINVAL then raise exn; debug "%s (ignored)" msg; false - and is_uefi_bootable_part part = + in + let part_filter_fun part = let dev = g#part_to_dev part in - parttype_is_gpt dev && is_uefi_ESP dev part + if parttype_is_gpt dev then ( + let partnum = g#part_to_partnum part in + match g#part_get_gpt_type dev partnum with + | "21686148-6449-6E6F-744E-656564454649" -> (true, None) + | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> (false, Some part) + | _ -> (false, None) + ) else ( + (false, None) + ) in let partitions = Array.to_list (g#list_partitions ()) in - let partitions = List.filter is_uefi_bootable_part partitions in + let part_tuples = List.map part_filter_fun partitions in + let bios_boot = List.exists (fun (a, b) -> a) part_tuples in + let esp_parts = List.filter_map (fun (a, b) -> b) part_tuples in - match partitions with - | [] -> I_BIOS - | partitions -> I_UEFI partitions + if bios_boot then ( + I_BIOS + ) else ( + match esp_parts with + | [] -> I_BIOS + | esp_parts -> I_UEFI esp_parts + ) (* If some inspection fields are "unknown", then that indicates a * failure in inspection, and we shouldn't continue. For an example

Cheers Laszlo >> I recommend a refactoring patch first: >> >>> diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml >>> index 056d0bca62e6..0dad9fc56572 100644 >>> --- a/convert/inspect_source.ml >>> +++ b/convert/inspect_source.ml >>> @@ -224,30 +224,32 @@ and list_applications g root = function >>> * >>> * Otherwise, [BIOS] is returned. >>> *) >>> and get_firmware_bootable_device g = >>> - let rec uefi_ESP_guid = "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" >>> - and is_uefi_ESP dev part = >>> - let partnum = g#part_to_partnum part in >>> - g#part_get_gpt_type dev partnum = uefi_ESP_guid >>> + let esp_partitions = ref [] >>> and parttype_is_gpt dev = >>> try g#part_get_parttype dev = "gpt" >>> with G.Error msg as exn -> >>> (* If it's _not_ "unrecognised disk label" then re-raise it. *) >>> if g#last_errno () <> G.Errno.errno_EINVAL then raise exn; >>> debug "%s (ignored)" msg; >>> false >>> - and is_uefi_bootable_part part = >>> - let dev = g#part_to_dev part in >>> - parttype_is_gpt dev && is_uefi_ESP dev part >>> in >>> >>> - let partitions = Array.to_list (g#list_partitions ()) in >>> - let partitions = List.filter is_uefi_bootable_part partitions in >>> + Array.iter (fun part -> >>> + let dev = g#part_to_dev part in >>> + if (parttype_is_gpt dev) then >>> + let partnum = g#part_to_partnum part in >>> + let part_type_guid = g#part_get_gpt_type dev partnum in >>> + match part_type_guid with >>> + | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> >>> + esp_partitions := part :: !esp_partitions >>> + | _ -> () >>> + ) (g#list_partitions ()); >>> >>> - match partitions with >>> + match !esp_partitions with >>> | [] -> I_BIOS >>> - | partitions -> I_UEFI partitions >>> + | esp_partitions -> I_UEFI esp_partitions >>> >>> (* If some inspection fields are "unknown", then that indicates a >>> * failure in inspection, and we shouldn't continue. For an example >>> * of this, see RHBZ#1278371. However don't "assert" here, since >> >> and then a simpler extension: >> >>> diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml >>> index 0dad9fc56572..a8dece3d4b5f 100644 >>> --- a/convert/inspect_source.ml >>> +++ b/convert/inspect_source.ml >>> @@ -226,6 +226,7 @@ and list_applications g root = function >>> *) >>> and get_firmware_bootable_device g = >>> let esp_partitions = ref [] >>> + and bios_boot = ref false >>> and parttype_is_gpt dev = >>> try g#part_get_parttype dev = "gpt" >>> with G.Error msg as exn -> >>> @@ -243,12 +244,14 @@ and get_firmware_bootable_device g = >>> match part_type_guid with >>> | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> >>> esp_partitions := part :: !esp_partitions >>> + | "21686148-6449-6E6F-744E-656564454649" -> >>> + bios_boot := true >>> | _ -> () >>> ) (g#list_partitions ()); >>> >>> - match !esp_partitions with >>> - | [] -> I_BIOS >>> - | esp_partitions -> I_UEFI esp_partitions >>> + match !esp_partitions, !bios_boot with >>> + | _ :: _ as esp_partitions, false -> I_UEFI esp_partitions >>> + | _ -> I_BIOS >>> >>> (* If some inspection fields are "unknown", then that indicates a >>> * failure in inspection, and we shouldn't continue. For an example >> >> Laszlo >> >