On 12/02/22 13:58, Richard W.M. Jones wrote:
> On Fri, Dec 02, 2022 at 01:44:09PM +0100, Laszlo Ersek wrote:
>> The "fallback" (or "default") boot behavior is described at great length
>> here:
>>
>> https://blog.uncooperative.org/uefi/linux/shim/efi%20system%20partition/2014/02/06/the-efi-system-partition.html
>>
>> The gist of it applies to all UEFI OSes, including Windows. For the
>> fallback boot behavior to work, the \EFI\BOOT\BOOTX64.efi boot loader on
>> the EFI system partition must match the installed operating system. We've
>> encountered a physical machine, during a virt-p2v conversion, where (a)
>> \EFI\BOOT\BOOTX64.efi belongs to a previously installed, but now wiped,
>> RHEL (hence shim+grub) deployment, and (b) the currently installed
>> operating system is Windows.
>>
>> Virt-v2v never transfers the UEFI variables (including the UEFI boot
>> options) of the source, therefore the converted VM always relies on the
>> default boot behavior when it is first started up. In the above scenario,
>> where \EFI\BOOT\BOOTX64.efi is actually "shim", the mismatch is triggered
>> at first boot after conversion, and a broken grub shell is reached instead
>> of the Windows boot loader.
>>
>> Detect this situation by investigating \EFI\BOOT\BOOTX64.efi on the EFI
>> system partition of a Windows disk image. If the file is missing, or is
>> not -- as expected -- a duplicate of \EFI\Microsoft\Boot\bootmgfw.efi,
>> then copy the latter to the former.
>>
>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2149629
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>>
>> Notes:
>> Tested with a freshly installed Win2019 guest whose
>> \EFI\BOOT\BOOTX64.efi binary I manually corrupted.
>>
>> convert/convert_windows.ml | 25 ++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml
>> index 34a5044dd338..57a7ff03398f 100644
>> --- a/convert/convert_windows.ml
>> +++ b/convert/convert_windows.ml
>> @@ -836,17 +836,42 @@ let convert (g : G.guestfs) _ inspect _ static_ips =
>> );
>> with
>> Not_found -> ()
>> +
>> + and fix_win_uefi_fallback esp_path uefi_arch =
>> + (* [esp_path] is on NTFS, and therefore it is considered
>> case-sensitive;
>> + * refer to
>> + *
>> <https://libguestfs.org/guestfs.3.html#guestfs_case_sensitive_path>.
>> + * However, the EFI system partition mounted under [esp_path] is
>> FAT32 per
>> + * UEFI spec, and the Linux vfat driver in the libguestfs appliance
>> treats
>> + * pathnames case-insensitively. Therefore, we're free to use any
>> case in
>> + * the ESP-relative pathnames below.
>
> It'd be nicer to keep the lines under 80 columns in length here for
> ease of reading. I think these are exactly 80 columns?
Yes, the longest two lines have exactly 80 chars each. For years I've
used 79, but I kept finding preexistent code lines in various projects
that were 80 chars.
The command
git grep -E '^.{80}$' -- '*.ml*'
returns a good number of lines in v2v as well, and the command
git grep -E '^.{80,}$' -- '*.ml*'
even more.
>
>> + *)
>> + let bootmgfw = sprintf "%s/efi/microsoft/boot/bootmgfw.efi" esp_path
>> in
>> + if g#is_file bootmgfw then
>> + let bootdir = sprintf "%s/efi/boot" esp_path in
>> + let fallback = sprintf "%s/boot%s.efi" bootdir uefi_arch in
>> + if not (g#is_file fallback) || not (g#equal fallback bootmgfw) then
>> (
>
> I'm going to say that you don't need the parens around (g#is_file ...)
> and (g#equal ...), but I'm actually not 100% certain about it.
> Normally function application binds tightest. Anyway if true, it's a
> matter of preference.
The parens are required:
# open Printf;;
# let f () = false;;
val f : unit -> bool = <fun>
# if not f () then printf "hello\n";;
Characters 3-6:
if not f () then printf "hello\n";;
^^^
Error: This function has type bool -> bool
It is applied to too many arguments; maybe you forgot a `;'.
# if not (f ()) then printf "hello\n";;
hello
- : unit = ()
>
>> + info (f_"Fixing UEFI bootloader.");
>> + g#rm_rf bootdir;
>> + g#mkdir_p bootdir;
>
> Is it safe to completely delete this directory or would it be better
> to only delete the errant BOOTX64.efi file? I'm just wondering if
> anything else important might be stored here.
Yes I think we *should* delete the \EFI\BOOT directory in this case.
We've determined that a Windows OS is installed on the disk, and I've
not seen any Windows version yet where \EFI\BOOT contained any other
file than BOOTX64.efi (or BOOTX32.efi), with the file being a copy of
bootmgfw.efi at that.
In the particular problem scenario
<https://bugzilla.redhat.com/show_bug.cgi?id=2149629#c6>, replacing only
the BOOTX64.efi file (= shim) with a copy of bootmgfw.efi would produce
a \EFI\BOOT directory containing two files:
- BOOTX64.efi (= bootmgfw.efi)
- fbx64.efi (from the shim installation)
While that might not necessarily cause runtime issues, it's certainly
confusing for any human reader (does not look like pristine ESP contents
with Windows installed).
>
>> + g#cp_a bootmgfw fallback
>> + )
>> in
>>
>> match inspect.i_firmware with
>> | I_BIOS -> ()
>> | I_UEFI esp_list ->
>> let esp_temp_path = g#mkdtemp "/Windows/Temp/ESP_XXXXXX" in
>> + let uefi_arch = get_uefi_arch_suffix inspect.i_arch in
>>
>> List.iter (
>> fun dev_path ->
>> g#mount dev_path esp_temp_path;
>> fix_win_uefi_bcd esp_temp_path;
>> + (match uefi_arch with
>> + | Some uefi_arch -> fix_win_uefi_fallback esp_temp_path uefi_arch
>> + | None -> ()
>> + );
>> g#umount esp_temp_path;
>> ) esp_list;
>
> Reviewed-by: Richard W.M. Jones <rjo...@redhat.com>
>
> If you can push this (optionally changed/fixed as you see fit) in the
> next hour then I can put it into C9S, since I'm doing a build today
> anyway.
Thanks, I'll push the series in a few minutes.
Laszlo
>
> Rich.
>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
