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...
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 <lersek@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 <rjones@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.