On Fri, May 15, 2020 at 10:44:11AM +0300, Denis Plotnikov wrote:
Not all UEFI guests can survive conversion, because of lost bootloader information in UEFI NVRAM. But some guest can cope with this because they have a fallback bootloader and use UEFI Removable Media Boot Behavior. (see https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_A_Feb14.pdf 3.5.1.1 Removable Media Boot Behavior) to load. If UEFI firmware can't find a bootloader in its settings it uses the removable media boot behavior to find a bootloader.
We can fix the guests which don't have such a fallback loader by providing a temporary one. This bootloader is used for the first boot only, then the conversion script restores the initial bootloader settings and removes the temporary loader.
There's a bunch of both general and specific issues here which I'll try to cover. The biggest general problem is that the code is in the wrong place - it should all be in v2v/convert_linux.ml. I guess it's in v2v/linux_bootloaders.ml because it needed access to grub_config, but you could make a tiny bootloader method bootloader#config_file which returns that value.
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> --- v2v/convert_linux.ml | 15 +++++ v2v/linux_bootloaders.ml | 149 ++++++++++++++++++++++++++++++++++++++++++++-- v2v/linux_bootloaders.mli | 2 + 3 files changed, 162 insertions(+), 4 deletions(-)
diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml index e91ae12..77a2555 100644 --- a/v2v/convert_linux.ml +++ b/v2v/convert_linux.ml @@ -1122,6 +1122,21 @@ shutdown -h -P +0 Linux.augeas_reload g );
+ (* Some linux uefi setups can't boot after conversion because of + lost uefi boot entries. The uefi boot entries are stored in uefi + NVRAM. The NVRAM content isn't a part of vm disk content and + usualy can't be converted with a vm. If a vm doesn't have uefi + fallback path (/EFI/BOOT/BOOT<arch>.efi), the vm is unbootable + after conversion. The following function will try to make an uefi + fallback path if the vm being converted is an uefi setup. + *) + + let efi_fix_script = bootloader#fix_efi_boot () in + + if efi_fix_script <> "" then
As written, the bootloader#fix_efi_boot method should be returning an option type, ie. Some "firstboot script" or None if one isn't needed. However if this whole function was moved into v2v/convert_linux.ml then it would not need to return anything at all, it could simply install the firstboot script if it was needed, or not install it.
+ Firstboot.add_firstboot_script g inspect.i_root + "fix uefi boot" efi_fix_script; + (* Delete blkid caches if they exist, since they will refer to the old * device names. blkid will rebuild these on demand. * diff --git a/v2v/linux_bootloaders.ml b/v2v/linux_bootloaders.ml index de3d107..cdab7bf 100644 --- a/v2v/linux_bootloaders.ml +++ b/v2v/linux_bootloaders.ml ... +(* Helper function checks if 'source' contains 's' *) +let contains source s = + let re = Str.regexp_string s in + try + ignore (Str.search_forward re source 0); + true + with Not_found -> false
+(* Helper function to get architecture suffixes for uefi files *) +let get_uefi_arch_suffix arch = + let arch_suffix = + if contains arch "x86_64" then "X64" + else if contains arch "x86_32" then "X32" + else "" in + arch_suffix
I don't think you're actually using substrings here? So how about: let get_uefi_arch_suffix = function | "x86_64" -> Some "X64" | "x86_32" -> Some "X32" | None Note again use of the option type.
+ +(* Function fixes uefi boot. It's used in both cases: legacy grub and grub2 *) +let fix_uefi g distro distro_ver grub_config arch = + let cant_fix_uefi () = ( + info (f_"Can't fix UEFI bootloader. VM may not boot."); + "" ) in
You might want to look at with_return which acts like a return statement: https://github.com/libguestfs/libguestfs-common/blob/e73eca3b73f7d0a54615c5d... The function would become: let fix_uefi ... = with_return (fun { return } -> if it_can_be_done then ( cant_fix_uefi (); return (); ); if it_can_be_done_for_another_reason then ( cant_fix_uefi (); return (); ); Firstboot.add_firstboot_script ... )
+ let uefi_fallback_name = + let arch_suffix = get_uefi_arch_suffix arch in + if arch_suffix <> "" then + String.concat "" [uefi_fallback_path; "BOOT"; arch_suffix; ".EFI"] + else + "" in + + if uefi_fallback_name = "" then ( + info (f_"Can't determine UEFI fallback path."); + cant_fix_uefi () )
With the return statement and the option typed version of get_uefi_arch_suffix this would become this briefer and much more type-safe code: let uefi_fallback_name = match get_uefi_arch_suffix arch with | None -> cant_fix_uefi (); return () | Some suffix -> sprintf "%sBOOT%s.EFI" uefi_fallback_path suffix in ...
+ if file_exists uefi_grub_name && file_exists grub_config then (
This would be a negative test followed by return, ie: if not (file_exists uefi_grub_name) || not (file_exists grub_config) then ( cant_fix_uefi (); return () );
+ g#mkdir_p uefi_fallback_path; + g#cp uefi_grub_name uefi_fallback_name; + g#cp grub_config uefi_grub_conf; + let script = sprintf +"#!/bin/bash +efibootmgr -c -L \"CentOS 6\" +rm -rf %s" uefi_fallback_path in + script )
Here's an example of a place where you'd simply install the firstboot script.
(* Grub1 (AKA grub-legacy) representation. *) class bootloader_grub1 (g : G.guestfs) inspect grub_config = let () = @@ -60,6 +170,16 @@ class bootloader_grub1 (g : G.guestfs) inspect grub_config = fun path -> List.mem_assoc path mounts ) [ "/boot/grub"; "/boot" ] with Not_found -> "" in + + let uefi_active = + match inspect.i_firmware with + | I_UEFI _ -> true + | _ -> false in + + let arch = inspect.i_arch in + let distro = inspect.i_distro in + let distro_ver_major = inspect.i_major_version in + object inherit bootloader
@@ -184,6 +304,12 @@ object loop paths;
g#aug_save () + + method fix_efi_boot () = + if uefi_active then + fix_uefi g distro distro_ver_major grub_config arch + else + "" end
Although you don't need any of this code any longer, I will note that splitting the two parts of this function is odd. A simpler way to have written would have been: method fix_efi_boot inspect = match inspect.i_firmware with | I_UEFI -> fix_uefi g inspect.i_distro inspect.i_major_version grub_config inspect.i_arch | I_BIOS -> () Also it's generally better to avoid “| _ ->” where possible since it bypasses type-safety. If we add an extra firmware case in future then the compiler won't tell us that we need to go and check this code to see if it needs an update. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top