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