Richard W.M. Jones
2022-Dec-02 12:58 UTC
[Libguestfs] [v2v PATCH 2/2] convert_windows: fix up the UEFI fallback boot loader if broken
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 <lersek at 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?> + *) > + 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.> + 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.> + 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 at 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. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Laszlo Ersek
2022-Dec-02 13:25 UTC
[Libguestfs] [v2v PATCH 2/2] convert_windows: fix up the UEFI fallback boot loader if broken
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 <lersek at 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 at 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. >
Laszlo Ersek
2022-Dec-02 13:34 UTC
[Libguestfs] [v2v PATCH 2/2] convert_windows: fix up the UEFI fallback boot loader if broken
On 12/02/22 13:58, Richard W.M. Jones wrote:> 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.Commit range 7b49177e2b0c..9d4b58dcecc4. Cheers Laszlo