Merge ~ubuntu-core-dev/grub/+git/ubuntu:sil2100/auto-nvram into ~ubuntu-core-dev/grub/+git/ubuntu:ubuntu

[Łukasz Zemczak]

Add the auto-nvram functionality of only attempting the NVRAM variable update if the system has access to those. Useful for dual BIOS-EFI devices.

Add the auto-nvram functionality of only attempting the NVRAM variable update if the system has access to those. Useful for dual BIOS-EFI devices.

This branch's approach is to add a new parameter to the existing update procedures for EFI/IEEE1275. I like this approach since it's not adding any new functions to the namespace that need re-implementation for each platform. That being said, another MP will be following with the approach of simply adding new functions for doing the NVRAM existence checks which might be 'cleaner'.

This is the first time I'm submitting an MP for grub2 which is git-dpm based. I know this branch probably needs to have a git-dpm update-patches + dch run done, but for review's sake I'm not doing this step right now.

7b18c6e... by Łukasz Zemczak on 2018-03-20

Fix a typo due to cherry-picking. Add the missing auto-nvram check for ieee1275.

[Łukasz Zemczak (sil2100)]

The 'alternative' approach can be found here:

https://code.launchpad.net/~sil2100/grub2/+git/ubuntu/+ref/sil2100/auto-nvram-alternative

But as I said, I am more in favor of the one that's being proposed for merging here.

[Mathieu Trudel-Lapierre (cyphermox)]

What is the use of this new feature? How does it differ from --no-nvram?

I'm curious because it seems like this is simply "warning" and skipping setting variables, yet you usually do want to update variables in nearly all cases -- when you can't, this probably should be a critical bug and a proper failure.

I'm also unsure about the implementation, this looks like the behavior of "auto-nvram" and "no-nvram" probably could be merged to some degree.

This won't exactly help dual EFI/non-EFI devices much -- in the case where you're installing from a BIOS-installed system, you won't find the variables anyway to try to update them, and if you're booted in EFI mode, you're detecting whether the variables are there (and they should, as above, if they're not, this is potentially a critical issue; these special cases might well be best served with just --no-nvram). Maybe I've missed some important piece, but I see no logic handling the "I booted in BIOS mode, but I really want both things updated". I also have no idea how to handle such a case anyway.

review: Needs Information

[Łukasz Zemczak (sil2100)]

It differs from --no-nvram as it does attempt updating the EFI variables if present. So my understanding here, and I'm not an expert of the dual EFI/non-EFI installation, is that we would want grub-install for Ubuntu to use --auto-nvram by default for both BIOS and EFI (in all cases).

If we want to have both grub-pc and shim-efi installed, normally a grub-install --target=x86_64-efi from shim-efi would just fail on a BIOS booted system. We can't append --no-nvram to it unconditionally as then in cases when we're booted in EFI, we won't get the variables updated while we should. Instead of detecting from inside shim-efi if we're running in BIOS mode or not, Steve's proposition was to just add the --auto-nvram option which tells grub-install: "if your target is x86_64-efi but there is no access to the NVRAM, don't fail the install as it just means we're in BIOS mode and this is not a critical error as we're probably handling a dual EFI/non-EFI situation".
This would mean we'd just be doing grub-install --target=x86_64-efi --auto-nvram for shim-efi instead. This way, if I understand it correctly of course, we could have both packages co-installed and not erroring out. It's really just migrating the detection from the packaging side (like doing things in postinst) to grub-install getting slightly smarter.

That's my understanding of what I was working on - I might be terribly wrong of course! Steve can give a more detailed explanation probably as he's the author of the proposal.

[Mathieu Trudel-Lapierre (cyphermox)]

review: Approve

0ef4190... by Łukasz Zemczak on 2018-04-19

Upstream prefers ignoring the no-NVRAM access silently.

[Łukasz Zemczak (sil2100)]

Created a git-dpm clean branch for merging:

https://code.launchpad.net/~ubuntu-core-dev/grub/+git/ubuntu/+merge/343642

diff --git a/grub-core/osdep/basic/no_platform.c b/grub-core/osdep/basic/no_platform.c
index d76c34c..b39e97f 100644
--- a/grub-core/osdep/basic/no_platform.c
+++ b/grub-core/osdep/basic/no_platform.c
@@ -25,7 +25,7 @@
 
 void
 grub_install_register_ieee1275 (int is_prep, const char *install_device,
-				int partno, const char *relpath)
+				int partno, const char *relpath, int detect_nvram)
 {
   grub_util_error ("%s", _("no IEEE1275 routines are available for your platform"));
 }
@@ -33,7 +33,8 @@ grub_install_register_ieee1275 (int is_prep, const char *install_device,
 void
 grub_install_register_efi (grub_device_t efidir_grub_dev,
 			   const char *efifile_path,
-			   const char *efi_distributor)
+			   const char *efi_distributor,
+			   int detect_nvram)
 {
   grub_util_error ("%s", _("no EFI routines are available for your platform"));
 }
diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c
index f9c376c..ddfa4dd 100644
--- a/grub-core/osdep/unix/platform.c
+++ b/grub-core/osdep/unix/platform.c
@@ -134,7 +134,8 @@ grub_install_remove_efi_entries_by_distributor (const char *efi_distributor)
 int
 grub_install_register_efi (grub_device_t efidir_grub_dev,
 			   const char *efifile_path,
-			   const char *efi_distributor)
+			   const char *efi_distributor,
+			   int detect_nvram)
 {
   const char * efidir_disk;
   int efidir_part;
@@ -153,6 +154,18 @@ grub_install_register_efi (grub_device_t efidir_grub_dev,
 #ifdef __linux__
   grub_util_exec ((const char * []){ "modprobe", "-q", "efivars", NULL });
 #endif
+
+  /* If requested, we try to detect if NVRAM access is available and if not,
+     warn the user and resume normal operation.  */
+  if (detect_nvram)
+    {
+      error = grub_util_exec_redirect_null ((const char * []){ "efibootmgr", NULL });
+      if (error == 2)
+	return 0;
+      else if (error)
+	return error;
+    }
+
   /* Delete old entries from the same distributor.  */
   error = grub_install_remove_efi_entries_by_distributor (efi_distributor);
   if (error)
@@ -178,7 +191,7 @@ grub_install_register_efi (grub_device_t efidir_grub_dev,
 
 void
 grub_install_register_ieee1275 (int is_prep, const char *install_device,
-				int partno, const char *relpath)
+				int partno, const char *relpath, int detect_nvram)
 {
   char *boot_device;
 
@@ -189,6 +202,14 @@ grub_install_register_ieee1275 (int is_prep, const char *install_device,
       grub_util_error (_("%s: not found"), "ofpathname");
     }
 
+  /* If requested, we try to detect if NVRAM access is available and if not,
+     warn the user and resume normal operation.  */
+  if (detect_nvram && grub_util_exec_redirect_null ((const char * []){ "nvram", NULL }))
+    {
+      grub_util_warn ("%s", _("Auto-NVRAM selected and no IEEE1275 variable support detected on the system."));
+      return;
+    }
+
   /* Get the Open Firmware device tree path translation.  */
   if (!is_prep)
     {
diff --git a/grub-core/osdep/windows/platform.c b/grub-core/osdep/windows/platform.c
index c30025b..1c96959 100644
--- a/grub-core/osdep/windows/platform.c
+++ b/grub-core/osdep/windows/platform.c
@@ -210,7 +210,8 @@ set_efi_variable_bootn (grub_uint16_t n, void *in, grub_size_t len)
 void
 grub_install_register_efi (grub_device_t efidir_grub_dev,
 			   const char *efifile_path,
-			   const char *efi_distributor)
+			   const char *efi_distributor,
+			   int detect_nvram)
 {
   grub_uint16_t *boot_order, *new_boot_order;
   grub_uint16_t *distributor16;
@@ -417,7 +418,7 @@ grub_install_register_efi (grub_device_t efidir_grub_dev,
 
 void
 grub_install_register_ieee1275 (int is_prep, const char *install_device,
-				int partno, const char *relpath)
+				int partno, const char *relpath, int detect_nvram)
 {
   grub_util_error ("%s", _("no IEEE1275 routines are available for your platform"));
 }
diff --git a/include/grub/util/install.h b/include/grub/util/install.h
index 58648e2..1151883 100644
--- a/include/grub/util/install.h
+++ b/include/grub/util/install.h
@@ -212,11 +212,12 @@ grub_install_get_default_powerpc_machtype (void);
 int
 grub_install_register_efi (grub_device_t efidir_grub_dev,
 			   const char *efifile_path,
-			   const char *efi_distributor);
+			   const char *efi_distributor,
+			   int detect_nvram);
 
 void
 grub_install_register_ieee1275 (int is_prep, const char *install_device,
-				int partno, const char *relpath);
+				int partno, const char *relpath, int detect_nvram);
 
 void
 grub_install_sgi_setup (const char *install_device,
diff --git a/util/grub-install.c b/util/grub-install.c
index 2214e45..b8f9110 100644
--- a/util/grub-install.c
+++ b/util/grub-install.c
@@ -59,6 +59,7 @@ static int removable = 0;
 static int no_extra_removable = 0;
 static int recheck = 0;
 static int update_nvram = 1;
+static int auto_nvram = 0;
 static char *install_device = NULL;
 static char *debug_image = NULL;
 static char *rootdir = NULL;
@@ -115,7 +116,8 @@ enum
     OPTION_PRODUCT_VERSION,
     OPTION_UEFI_SECURE_BOOT,
     OPTION_NO_UEFI_SECURE_BOOT,
-    OPTION_NO_EXTRA_REMOVABLE
+    OPTION_NO_EXTRA_REMOVABLE,
+    OPTION_AUTO_NVRAM
   };
 
 static int fs_probe = 1;
@@ -206,6 +208,10 @@ argp_parser (int key, char *arg, struct argp_state *state)
       update_nvram = 0;
       return 0;
 
+    case OPTION_AUTO_NVRAM:
+      auto_nvram = 1;
+      return 0;
+
     case OPTION_FORCE:
       force = 1;
       return 0;
@@ -296,6 +302,9 @@ static struct argp_option options[] = {
   {"no-nvram", OPTION_NO_NVRAM, 0, 0,
    N_("don't update the `boot-device'/`Boot*' NVRAM variables. "
       "This option is only available on EFI and IEEE1275 targets."), 2},
+  {"auto-nvram", OPTION_AUTO_NVRAM, 0, 0,
+   N_("only update NVRAM variables if possible. "
+      "This option is only available on EFI and IEEE1275 targets."), 2},
   {"skip-fs-probe",'s',0,      0,
    N_("do not probe for filesystems in DEVICE"), 0},
   {"no-bootsector", OPTION_NO_BOOTSECTOR, 0, 0,
@@ -1924,7 +1933,7 @@ main (int argc, char *argv[])
 		? ins_dev->disk->partition->number + 1 : 0;
 	      dev = grub_util_get_os_disk (install_device);
 	      grub_install_register_ieee1275 (0, dev, partno,
-					      "\\\\BootX");
+					      "\\\\BootX", auto_nvram);
 	    }
 	  grub_device_close (ins_dev);
   	  free (grub_elf);
@@ -1956,7 +1965,7 @@ main (int argc, char *argv[])
 	  grub_device_close (ins_dev);
 	  if (update_nvram)
 	    grub_install_register_ieee1275 (1, grub_util_get_os_disk (install_device),
-					    0, NULL);
+					    0, NULL, auto_nvram);
 	  break;
       }
       /* fallthrough.  */
@@ -1971,7 +1980,7 @@ main (int argc, char *argv[])
 	    ? grub_dev->disk->partition->number + 1 : 0;
 	  dev = grub_util_get_os_disk (grub_devices[0]);
 	  grub_install_register_ieee1275 (0, dev,
-					  partno, relpath);
+					  partno, relpath, auto_nvram);
 	}
       break;
     case GRUB_INSTALL_PLATFORM_MIPS_ARC:
@@ -2020,7 +2029,7 @@ main (int argc, char *argv[])
 	      int error = 0;
 	      error = grub_install_register_efi (efidir_grub_dev,
 					 "\\System\\Library\\CoreServices",
-					 efi_distributor);
+					 efi_distributor, auto_nvram);
 	      if (error)
 	        grub_util_error (_("efibootmgr failed to register the boot entry: %s"),
 				 strerror (error));
@@ -2135,7 +2144,7 @@ main (int argc, char *argv[])
 			  (part ? ",": ""), (part ? : ""));
 	  grub_free (part);
 	  error = grub_install_register_efi (efidir_grub_dev,
-					     efifile_path, efi_distributor);
+					     efifile_path, efi_distributor, auto_nvram);
 	  if (error)
 	    grub_util_error (_("efibootmgr failed to register the boot entry: %s"),
 			     strerror (error));