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

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? 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". 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. Third, while the commit message speaks about these flags, the code does not appear to deal with them at all. 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. (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. (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"). (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. 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