Re: [PATCH v2 2/2] Minimise writes to EFI variable storage

2019-03-23 Thread Colin Watson
On Fri, Mar 22, 2019 at 11:31:18PM +, Colin Watson wrote:
> grub-install now reuses existing boot entries where possible, and avoids
> writing to variables when the new contents are the same as the old
> contents.  In the common upgrade case where nothing needs to change, it
> no longer writes to NVRAM at all.  It's also now slightly faster, since
> using libefivar is faster than forking efibootmgr.

After uploading this to Debian I found that it fails to build on ARM due
to -Wcast-align.  Sorry about that.  I'll post a v3 ASAP ...

-- 
Colin Watson   [cjwat...@debian.org]

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


[PATCH v3 0/2] Minimise writes to EFI variable storage

2019-03-23 Thread Colin Watson
Some UEFI firmware is easily provoked into running out of space in its
variable storage.  This is usually due to certain kernel drivers (e.g.
pstore), but regardless of the cause it can cause grub-install to fail
because it currently asks efibootmgr to delete and re-add entries, and
the deletion often doesn't result in an immediate garbage collection.
Writing variables frequently also increases wear on the NVRAM which may
have limited write cycles.  For these reasons, it's desirable to find a
way to minimise writes while still allowing grub-install to ensure that
a suitable boot entry exists.

This short patch series does so by using the efivar and efiboot
libraries directly.

v2: simplify grub_lltoa; use free_efi_variable in more error paths

v3: avoid -Wcast-align diagnostics on ARM

Colin Watson (2):
  Add %X to grub_vsnprintf_real and friends
  Minimise writes to EFI variable storage

 INSTALL |   5 +
 Makefile.util.def   |  20 ++
 configure.ac|  12 +
 grub-core/kern/misc.c   |   7 +-
 grub-core/osdep/efivar.c|   3 +
 grub-core/osdep/unix/efivar.c   | 508 
 grub-core/osdep/unix/platform.c | 100 +--
 include/grub/util/install.h |   5 +
 util/grub-install.c |   4 +-
 9 files changed, 567 insertions(+), 97 deletions(-)
 create mode 100644 grub-core/osdep/efivar.c
 create mode 100644 grub-core/osdep/unix/efivar.c

-- 
2.17.1

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


[PATCH v3 2/2] Minimise writes to EFI variable storage

2019-03-23 Thread Colin Watson
Some UEFI firmware is easily provoked into running out of space in its
variable storage.  This is usually due to certain kernel drivers (e.g.
pstore), but regardless of the cause it can cause grub-install to fail
because it currently asks efibootmgr to delete and re-add entries, and
the deletion often doesn't result in an immediate garbage collection.
Writing variables frequently also increases wear on the NVRAM which may
have limited write cycles.  For these reasons, it's desirable to find a
way to minimise writes while still allowing grub-install to ensure that
a suitable boot entry exists.

Unfortunately, efibootmgr doesn't offer an interface that would let
grub-install do this.  It doesn't in general make very much effort to
minimise writes; it doesn't allow modifying an existing Boot* variable
entry, except in certain limited ways; and current versions don't have a
way to export the expected variable data so that grub-install can
compare it to the current data.  While it would be possible (and perhaps
desirable?) to add at least some of this to efibootmgr, that would still
leave the problem that there isn't a good upstreamable way for
grub-install to guarantee that it has a new enough version of
efibootmgr.  In any case, it's cumbersome and slow for grub-install to
have to fork efibootmgr to get things done.

Fortunately, a few years ago Peter Jones helpfully factored out a
substantial part of efibootmgr to the efivar and efiboot libraries, and
so it's now possible to have grub-install use those directly.  We still
have to use some code from efibootmgr, but much less than would
previously have been necessary.

grub-install now reuses existing boot entries where possible, and avoids
writing to variables when the new contents are the same as the old
contents.  In the common upgrade case where nothing needs to change, it
no longer writes to NVRAM at all.  It's also now slightly faster, since
using libefivar is faster than forking efibootmgr.

Fixes Debian bug #891434.

Signed-off-by: Colin Watson 
---
 INSTALL |   5 +
 Makefile.util.def   |  20 ++
 configure.ac|  12 +
 grub-core/osdep/efivar.c|   3 +
 grub-core/osdep/unix/efivar.c   | 508 
 grub-core/osdep/unix/platform.c | 100 +--
 include/grub/util/install.h |   5 +
 util/grub-install.c |   4 +-
 8 files changed, 562 insertions(+), 95 deletions(-)
 create mode 100644 grub-core/osdep/efivar.c
 create mode 100644 grub-core/osdep/unix/efivar.c

diff --git a/INSTALL b/INSTALL
index 8acb40902..342c158e9 100644
--- a/INSTALL
+++ b/INSTALL
@@ -41,6 +41,11 @@ configuring the GRUB.
 * Other standard GNU/Unix tools
 * a libc with large file support (e.g. glibc 2.1 or later)
 
+On Unix-based systems, you also need:
+
+* libefivar (recommended)
+* libefiboot (recommended; your OS may ship this together with libefivar)
+
 On GNU/Linux, you also need:
 
 * libdevmapper 1.02.34 or later (recommended)
diff --git a/Makefile.util.def b/Makefile.util.def
index 969d32f00..60ec4d593 100644
--- a/Makefile.util.def
+++ b/Makefile.util.def
@@ -535,6 +535,8 @@ program = {
   common = grub-core/osdep/compress.c;
   extra_dist = grub-core/osdep/unix/compress.c;
   extra_dist = grub-core/osdep/basic/compress.c;
+  common = grub-core/osdep/efivar.c;
+  extra_dist = grub-core/osdep/unix/efivar.c;
   common = util/editenv.c;
   common = grub-core/osdep/blocklist.c;
   common = grub-core/osdep/config.c;
@@ -548,12 +550,15 @@ program = {
   common = grub-core/kern/emu/argp_common.c;
   common = grub-core/osdep/init.c;
 
+  cflags = '$(EFIVAR_CFLAGS)';
+
   ldadd = '$(LIBLZMA)';
   ldadd = libgrubmods.a;
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/lib/gnulib/libgnu.a;
   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) 
$(LIBGEOM)';
+  ldadd = '$(EFIVAR_LIBS)';
 
   condition = COND_HAVE_EXEC;
 };
@@ -582,6 +587,8 @@ program = {
   extra_dist = grub-core/osdep/basic/no_platform.c;
   extra_dist = grub-core/osdep/unix/platform.c;
   common = grub-core/osdep/compress.c;
+  common = grub-core/osdep/efivar.c;
+  extra_dist = grub-core/osdep/unix/efivar.c;
   common = util/editenv.c;
   common = grub-core/osdep/blocklist.c;
   common = grub-core/osdep/config.c;
@@ -595,12 +602,15 @@ program = {
   common = grub-core/kern/emu/argp_common.c;
   common = grub-core/osdep/init.c;
 
+  cflags = '$(EFIVAR_CFLAGS)';
+
   ldadd = '$(LIBLZMA)';
   ldadd = libgrubmods.a;
   ldadd = libgrubgcry.a;
   ldadd = libgrubkern.a;
   ldadd = grub-core/lib/gnulib/libgnu.a;
   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) 
$(LIBGEOM)';
+  ldadd = '$(EFIVAR_LIBS)';
 };
 
 program = {
@@ -622,6 +632,8 @@ program = {
   common = grub-core/osdep/platform.c;
   common = grub-core/osdep/platform_unix.c;
   common = grub-core/osdep/compress.c;
+  common = grub-core/osdep/efivar.c;
+  extra_dist = grub-core/osdep/unix/efivar.c;
   co

[PATCH v3 1/2] Add %X to grub_vsnprintf_real and friends

2019-03-23 Thread Colin Watson
This is needed for UEFI Boot* variables, which the standard says are
named using upper-case hexadecimal.

Signed-off-by: Colin Watson 
---
 grub-core/kern/misc.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 3b633d51f..18cad5803 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -588,7 +588,7 @@ grub_divmod64 (grub_uint64_t n, grub_uint64_t d, 
grub_uint64_t *r)
 static inline char *
 grub_lltoa (char *str, int c, unsigned long long n)
 {
-  unsigned base = (c == 'x') ? 16 : 10;
+  unsigned base = (c == 'x' || c == 'X') ? 16 : 10;
   char *p;
 
   if ((long long) n < 0 && c == 'd')
@@ -603,7 +603,7 @@ grub_lltoa (char *str, int c, unsigned long long n)
 do
   {
unsigned d = (unsigned) (n & 0xf);
-   *p++ = (d > 9) ? d + 'a' - 10 : d + '0';
+   *p++ = (d > 9) ? d + ((c == 'x') ? 'a' : 'A') - 10 : d + '0';
   }
 while (n >>= 4);
   else
@@ -676,6 +676,7 @@ parse_printf_args (const char *fmt0, struct printf_args 
*args,
{
case 'p':
case 'x':
+   case 'X':
case 'u':
case 'd':
case 'c':
@@ -762,6 +763,7 @@ parse_printf_args (const char *fmt0, struct printf_args 
*args,
   switch (c)
{
case 'x':
+   case 'X':
case 'u':
  args->ptr[curn].type = UNSIGNED_INT + longfmt;
  break;
@@ -900,6 +902,7 @@ grub_vsnprintf_real (char *str, grub_size_t max_len, const 
char *fmt0,
  c = 'x';
  /* Fall through. */
case 'x':
+   case 'X':
case 'u':
case 'd':
  {
-- 
2.17.1

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel