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

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 at virtuozzo.com>
---
 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
-- 
2.31.1

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 at virtuozzo.com>
> ---
>  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_specification/
 
https://www.freedesktop.org/software/systemd/man/systemd-gpt-auto-generator.html

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_2%E2%80%9333)>
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
Laszlo