Re: [PATCH v3 2/2] Minimise writes to EFI variable storage
No individual comments, but... Overall, I'm thinking it would be nice
to see some more function-level comments to explain what's going on,
but that doesn't seem to match the prevailing style in the grub
source. :-/
On Sat, Mar 23, 2019 at 01:42:30PM +, Colin Watson wrote:
>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 =
[PATCH v3 2/2] Minimise writes to EFI variable storage
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;
