Libguestfs - May 2020 - [PATCH] v2v: fix UEFI bootloader for linux guests

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.

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
+      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
@@ -36,6 +36,7 @@ class virtual bootloader = object
   method virtual configure_console : unit -> unit
   method virtual remove_console : unit -> unit
   method update () = ()
+  method virtual fix_efi_boot : unit -> string
 end
 
 (* Helper function for SUSE: remove (hdX,X) prefix from a path. *)
@@ -43,6 +44,115 @@ let remove_hd_prefix    let rex = PCRE.compile
"^\\(hd.*\\)" in
   PCRE.replace rex ""
 
+(* Standard uefi fallback path *)
+let uefi_fallback_path +  "/boot/efi/EFI/BOOT/"
+
+(* 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
+
+(* 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
+
+    let file_exists file +      if g#exists file then
+        true
+      else (
+        info (f_"Can't find file: '%s' needed for UEFI
bootloader fixing") file;
+        false ) in
+
+    let grub_path = String.sub grub_config 0 (String.rindex grub_config
'/') in
+    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 () )
+    else if g#exists uefi_fallback_name then
+      (* don't do anything if uefi fallback exists *)
+      ""
+    else if uefi_fallback_name = "" then
+      cant_fix_uefi ()
+    else (
+      info (f_"Fixing UEFI bootloader.");
+      match distro, distro_ver with
+      | "centos", 6 ->
+        (* to make a bootable uefi centos 6 we need to
+         * copy grub.efi and grub.conf to UEFI fallback path
+         * and rename them to BOOT<arch>.efi and BOOT<arch>.conf
+         * correspondingly *)
+        let uefi_grub_name = String.concat "" [grub_path;
"/grub.efi"] in
+        let uefi_grub_conf = String.concat "" [
+                               String.sub uefi_fallback_name 0
+                                 (String.rindex uefi_fallback_name
'.');
+                                 ".conf" ] in
+        if file_exists uefi_grub_name && file_exists grub_config then (
+          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 )
+        else
+          cant_fix_uefi ()
+      | "ubuntu", 14 ->
+        (* to make a bootable uefi ubuntu 14 we need to
+         * copy shim<arch>.efi to UEFI fallback path
+         * and rename it to BOOT<arch>.efi, also we copy
+         * grub.efi and grub.cfg to UEFI fallback path without renaming *)
+        let arch_suffix +          String.lowercase_ascii (get_uefi_arch_suffix
arch) in
+        let shim +          String.concat "" [grub_path;
"/shim"; arch_suffix; ".efi"] in
+        let uefi_grub_name +          String.concat "" [grub_path;
"/grub"; arch_suffix; ".efi"] in
+
+        if file_exists shim && file_exists uefi_grub_name
+                            && file_exists grub_config then (
+          g#mkdir_p uefi_fallback_path;
+          g#cp shim uefi_fallback_name;
+          g#cp uefi_grub_name uefi_fallback_path;
+          g#cp grub_config uefi_fallback_path;
+          (* if the shim is at the standard path, clean up uefi fixing
+           * if not, then just don't clean up and leave the temp loader
+           * at UEFI fallback path for simplicity
+           *)
+          if contains shim "/boot/efi/EFI/ubuntu/shim" then
+            sprintf
+"#!/bin/bash
+sudo efibootmgr -c -L ubuntu -l \\\\EFI\\\\ubuntu\\\\shim%s.efi
+rm -rf %s" arch_suffix uefi_fallback_path
+          else
+            "")
+        else
+          cant_fix_uefi ()
+      | _, _ ->
+        info (f_"No UEFI fix rule for %s %d") distro distro_ver;
+        cant_fix_uefi ()
+    )
+
 (* 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
 
 (** The method used to get and set the default kernel in Grub2. *)
@@ -193,7 +319,7 @@ type default_kernel_method    | MethodNone  (** No known
way. *)
 
 (* Grub2 representation. *)
-class bootloader_grub2 (g : G.guestfs) grub_config +class bootloader_grub2 (g :
G.guestfs) inspect grub_config
   let grub2_mkconfig_cmd      let elems = [
@@ -221,6 +347,15 @@ class bootloader_grub2 (g : G.guestfs) grub_config       
MethodNone
     ) 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 (self)
   inherit bootloader
 
@@ -340,8 +475,14 @@ object (self)
 
   method remove_console = self#grub2_update_console ~remove:true
 
-  method update () -    ignore (g#command [| grub2_mkconfig_cmd;
"-o"; grub_config |])
+  method update () = (
+    ignore (g#command [| grub2_mkconfig_cmd; "-o"; grub_config |]))
+
+  method fix_efi_boot () +    if uefi_active then
+      fix_uefi g distro distro_ver_major grub_config arch
+    else
+      ""
 end
 
 (* Helper type used in detect_bootloader. *)
@@ -390,6 +531,6 @@ let detect_bootloader (g : G.guestfs) inspect    let bl     
match typ with
     | Grub1 -> new bootloader_grub1 g inspect grub_config
-    | Grub2 -> new bootloader_grub2 g grub_config in
+    | Grub2 -> new bootloader_grub2 g inspect grub_config in
   debug "detected bootloader %s at %s" bl#name grub_config;
   bl
diff --git a/v2v/linux_bootloaders.mli b/v2v/linux_bootloaders.mli
index 30cdfe3..c4e1069 100644
--- a/v2v/linux_bootloaders.mli
+++ b/v2v/linux_bootloaders.mli
@@ -44,6 +44,8 @@ class virtual bootloader : object
   (** Update the bootloader: For grub2 only this runs the
       [grub2-mkconfig] command to rebuild the configuration.  This
       is not necessary for grub-legacy. *)
+  method virtual fix_efi_boot : unit -> string
+  (** fix UEFI bootloader and return a clean up script. *)
 end
 (** Encapsulates a Linux boot loader as object. *)
 
-- 
1.8.3.1

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/e73eca3b73f7d0a54615c5dc55eadd09dc170035/mlstdutils/std_utils.mli#L346

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