3:02 p.m.
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>
> ---
> 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
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.
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).
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.
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.
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.
(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.
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