On 12/21/22 17:38, Andrey Drobyshev wrote: > This patch is merely a refactoring and does not introduce any changes in > behaviour. It's used as a prelude for the next patch which prioritizes > BIOS boot partition when searching for the right firmware. > > Co-authored-by: Laszlo Ersek <lersek(a)redhat.com&gt; > Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com&gt; > --- > convert/inspect_source.ml | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) > > diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml > index 056d0bca..42a26f68 100644 > --- a/convert/inspect_source.ml > +++ b/convert/inspect_source.ml > @@ -225,26 +225,30 @@ 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 accumulate_partition esp_parts 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 > + let part_type_guid = g#part_get_gpt_type dev partnum in > + match part_type_guid with > + (* EFI system partition *) > + | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> part :: esp_parts > + | _ -> esp_parts > + else esp_parts > in > > - let partitions = Array.to_list (g#list_partitions ()) in > - let partitions = List.filter is_uefi_bootable_part partitions in > + let esp_partitions = > + Array.fold_left accumulate_partition [] (g#list_partitions ()) in > > - match partitions with > + match esp_partitions with > | [] -> I_BIOS > | partitions -> I_UEFI partitions > I've replaced "List.fold_left" with "Array.fold_left" in the commit message (subject line).

Merged as commit d04b9a05b2a5. Laszlo

If the guest uses BIOS firmware but GPT partitioned disk, v2v inspection might fail to correctly detect the firmware being used by the source mathine. Namely, consider the following GPT partition table: Number Start (sector) End (sector) Size Code Name 1 2048 4095 1024.0 KiB EF02 2 4096 528383 256.0 MiB EF00 3 528384 125827071 59.7 GiB 8300 4 125827072 134215679 4.0 GiB 8200 where partition 1 is a BIOS boot partition (code 0xEF02, GPT partition entry with GUID 21686148-6449-6E6F-744E-656564454649), and partition 2 is a EFI System partition (code 0xEF00, GPT partition entry with GUID C12A7328-F81F-11D2-BA4B-00A0C93EC93B). The former is commonly used to store the bootloader in BIOS+GPT machines. The latter is used by UEFI powered machines. Normally an OS installer wouldn't put those two together: if the disk is being partitioned in GPT, there's either BIOS boot partition (if BIOS firmware is used) or EFI System partition (if UEFI firmware is used). However, GRUB2 will deal with such "busted" layout just fine, so this configuration may exist. If it is the case, v2v inspection will detect the presence of an EFI system partition and mistakenly mark the system as UEFI. So let's prioritize Bios boot partition over ESP. As discussed in [1], this solution is not entirely bulletproof. It will work in case a GPT disk was first used in an UEFI machine, and then put into a BIOS machine with an ESP partition remaining on it. It won't work in the opposite case, i.e. when a GPT disk is moved from BIOS machine to UEFI machine with BIOS boot partition remaining on it. However, it's better to prioritize something, and the latter case is less probable than the former. [1] https://listman.redhat.com/archives/libguestfs/2022-December/030401.html Co-authored-by: Laszlo Ersek <lersek(a)redhat.com&gt; Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com&gt; --- convert/inspect_source.ml | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml index 42a26f68..a0d9f148 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 the guest has BIOS boot partition present, this is likely a BIOS+GPT + * setup, so [BIOS] is returned. + * * If it has ESP(s), then [UEFI devs] is returned where [devs] is the * list of at least one ESP. * @@ -233,24 +236,31 @@ and get_firmware_bootable_device g = debug "%s (ignored)" msg; false in - let accumulate_partition esp_parts part = + let accumulate_partition (esp_parts, bboot) 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 (* EFI system partition *) - | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> part :: esp_parts - | _ -> esp_parts - else esp_parts + | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> part :: esp_parts, bboot + (* BIOS boot partition *) + | "21686148-6449-6E6F-744E-656564454649" -> esp_parts, true + | _ -> esp_parts, bboot + else esp_parts, bboot in - let esp_partitions = - Array.fold_left accumulate_partition [] (g#list_partitions ()) in + let esp_partitions, bios_boot = + Array.fold_left accumulate_partition ([], false) (g#list_partitions ()) in - match esp_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. + * In this case we prioritize BIOS boot partition and detect BIOS firmware, + * no matter how many ESPs we've found. + *) + match esp_partitions, bios_boot with + | _ :: _, 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