[PATCH] efibc: EFI Bootloader Control

[PATCH] efibc: EFI Bootloader Control

[Compostella, Jeremy]

This module installs a reboot hook, such that if reboot() is invoked
with a string argument NNN, "NNN" is copied to the
"LoaderEntryOneShot" EFI variable, to be read by the bootloader. If
the string matches one of the boot labels defined in its
configuration, the bootloader will boot once to that label.  The
"LoaderEntryRebootReason" EFI variable is set with the reboot reason:
"reboot", "shutdown", "kernel_panic" or "watchdog".  The bootloader
reads this reboot reason and takes particular action according to its
policy.

Signed-off-by: Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 arch/x86/Kconfig               |  16 ++++
 arch/x86/platform/efi/Makefile |   1 +
 arch/x86/platform/efi/efibc.c  | 164 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 181 insertions(+)
 create mode 100644 arch/x86/platform/efi/efibc.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2dc18605..cd4b6e4 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -432,6 +432,22 @@ config X86_EXTENDED_PLATFORM
 	  generic distribution kernel, say Y here - otherwise say N.
 endif
 
+config EFI_BOOTLOADER_CONTROL
+	tristate "EFI Bootloader Control"
+	depends on EFI_VARS
+	default n
+	---help---
+	  This module installs a reboot hook, such that if reboot() is
+	  invoked with a string argument NNN, "NNN" is copied to the
+	  "LoaderEntryOneShot" EFI variable, to be read by the
+	  bootloader. If the string matches one of the boot labels
+	  defined in its configuration, the bootloader will boot once
+	  to that label. The "LoaderEntryRebootReason" EFI variable is
+	  set with the reboot reason: "reboot", "shutdown",
+	  "kernel_panic" or "watchdog". The bootloader reads this
+	  reboot reason and takes particular action according to its
+	  policy.
+
 if X86_64
 config X86_EXTENDED_PLATFORM
 	bool "Support for extended (non-PC) x86 platforms"
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index 066619b..05cf769 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
 obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
 obj-$(CONFIG_EARLY_PRINTK_EFI)	+= early_printk.o
 obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
+obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
diff --git a/arch/x86/platform/efi/efibc.c b/arch/x86/platform/efi/efibc.c
new file mode 100644
index 0000000..8ca8b43
--- /dev/null
+++ b/arch/x86/platform/efi/efibc.c
@@ -0,0 +1,164 @@
+/*
+ * efibc: control EFI bootloaders which obey LoaderEntryOneShot var
+ * Copyright (c) 2013-2016, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/efi.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/reboot.h>
+#include <linux/kexec.h>
+#include <linux/slab.h>
+#include <linux/nls.h>
+
+#define MODULE_NAME "efibc"
+
+static const efi_guid_t LOADER_GUID =
+	EFI_GUID(0x4a67b082, 0x0a4c, 0x41cf,
+		 0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f);
+
+static const char REBOOT_TARGET[] = "LoaderEntryOneShot";
+static const char REBOOT_REASON[] = "LoaderEntryRebootReason";
+
+static const char * const WDT_SOURCE_PREFIX[] = {
+	"Kernel Watchdog", "Watchdog", "softlockup", "Software Watchdog"
+};
+
+static void efibc_str_to_str16(const char *str, wchar_t *str16)
+{
+	size_t size = strlen(str) + 1;
+
+	utf8s_to_utf16s(str, size, UTF16_LITTLE_ENDIAN, str16,
+			size * sizeof(*str16) / sizeof(*str));
+	str16[size - 1] = '\0';
+}
+
+static struct efivar_entry *efibc_get_var_entry(const char *name)
+{
+	wchar_t name16[strlen(name) + 1];
+	struct efivar_entry *entry;
+
+	efibc_str_to_str16(name, name16);
+
+	efivar_entry_iter_begin();
+	entry = efivar_entry_find(name16, LOADER_GUID,
+				  &efivar_sysfs_list, false);
+	efivar_entry_iter_end();
+
+	return entry;
+}
+
+static void efibc_set_variable(const char *name, const char *value)
+{
+	u32 attributes = EFI_VARIABLE_NON_VOLATILE
+		| EFI_VARIABLE_BOOTSERVICE_ACCESS
+		| EFI_VARIABLE_RUNTIME_ACCESS;
+	wchar_t name16[strlen(name) + 1];
+	wchar_t value16[strlen(value) + 1];
+	int ret;
+
+	efibc_str_to_str16(name, name16);
+	efibc_str_to_str16(value, value16);
+
+	ret = efivar_entry_set_safe(name16, LOADER_GUID, attributes, true,
+				    sizeof(value16), value16);
+	if (ret)
+		pr_err(MODULE_NAME ": failed to set %s EFI variable: 0x%x\n",
+		       name, ret);
+}
+
+static int efibc_reboot_notifier_call(struct notifier_block *notifier,
+				      unsigned long what, void *data)
+{
+	char *reason = what == SYS_RESTART ? "reboot" : "shutdown";
+
+	efibc_set_variable(REBOOT_REASON, reason);
+
+	if (!data)
+		return NOTIFY_DONE;
+
+	efibc_set_variable(REBOOT_TARGET, (char *)data);
+
+	return NOTIFY_DONE;
+}
+
+static bool is_watchdog_source(const char *str)
+{
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(WDT_SOURCE_PREFIX); i++)
+		if (!strncmp(str, WDT_SOURCE_PREFIX[i],
+			     strlen(WDT_SOURCE_PREFIX[i])))
+			return true;
+
+	return false;
+}
+
+static int efibc_panic_notifier_call(struct notifier_block *notifier,
+				     unsigned long what, void *data)
+{
+	char *reason;
+
+	/* If the reboot reason has already been supplied, keep it. */
+	if (efibc_get_var_entry(REBOOT_REASON))
+		return NOTIFY_DONE;
+
+	if (data && is_watchdog_source((char *)data))
+		reason = "watchdog";
+	else
+		reason = "kernel_panic";
+
+	efibc_set_variable(REBOOT_REASON, reason);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block efibc_reboot_notifier = {
+	.notifier_call = efibc_reboot_notifier_call,
+};
+
+static struct notifier_block efibc_panic_notifier = {
+	.notifier_call  = efibc_panic_notifier_call,
+};
+
+static int __init efibc_init(void)
+{
+	int ret;
+
+	if (!efi_enabled(EFI_RUNTIME_SERVICES))
+		return -ENODEV;
+
+	ret = register_reboot_notifier(&efibc_reboot_notifier);
+	if (ret) {
+		pr_err(MODULE_NAME ": unable to register reboot notifier\n");
+		return ret;
+	}
+
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &efibc_panic_notifier);
+
+	return 0;
+}
+module_init(efibc_init);
+
+static void __exit efibc_exit(void)
+{
+	unregister_reboot_notifier(&efibc_reboot_notifier);
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &efibc_panic_notifier);
+}
+module_exit(efibc_exit);
+
+MODULE_AUTHOR("Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
+MODULE_AUTHOR("Matt Gumbel <matthew.k.gumbel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org");
+MODULE_DESCRIPTION("EFI Bootloader Control");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH] efibc: EFI Bootloader Control

[Matt Fleming]

(Sorry for the delay)

On Fri, 01 Apr, at 03:09:51PM, Compostella, Jeremy wrote:
> This module installs a reboot hook, such that if reboot() is invoked
> with a string argument NNN, "NNN" is copied to the
> "LoaderEntryOneShot" EFI variable, to be read by the bootloader. If
> the string matches one of the boot labels defined in its
> configuration, the bootloader will boot once to that label.  The
> "LoaderEntryRebootReason" EFI variable is set with the reboot reason:
> "reboot", "shutdown", "kernel_panic" or "watchdog".  The bootloader
> reads this reboot reason and takes particular action according to its
> policy.
 
This commit message is missing some info. In particular, I'd like see
to which boot loaders support this protocol and why this feature
should be a kernel driver and cannot be done with the userspace EFI
variable API.

Yes we've already discussed these bits on the mailing list, but
recording it for posterity in the commit log is very worthwhile.

> Signed-off-by: Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  arch/x86/Kconfig               |  16 ++++
>  arch/x86/platform/efi/Makefile |   1 +
>  arch/x86/platform/efi/efibc.c  | 164 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 181 insertions(+)
>  create mode 100644 arch/x86/platform/efi/efibc.c
 
Hehe, I suspect I was a little unclear in my previous comments about
moving this out of drivers/firmware/efi.

arch/x86/platform/efi is not the right place for this kind of stuff
either, since it contains core x86 EFI support.

That directory is no longer named appropriately, as EFI should not be
considered a feature of specific platforms - it's the standard
firmware technology nowadays on x86. I've been meaning to rename it.

But let's ignore the issue of finding the right home for now. I've got
some reservations about the panic notifier that I've outlined below.

> diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
> index 066619b..05cf769 100644
> --- a/arch/x86/platform/efi/Makefile
> +++ b/arch/x86/platform/efi/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
>  obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
>  obj-$(CONFIG_EARLY_PRINTK_EFI)	+= early_printk.o
>  obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
> +obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
> diff --git a/arch/x86/platform/efi/efibc.c b/arch/x86/platform/efi/efibc.c
> new file mode 100644
> index 0000000..8ca8b43
> --- /dev/null
> +++ b/arch/x86/platform/efi/efibc.c
> @@ -0,0 +1,164 @@
> +/*
> + * efibc: control EFI bootloaders which obey LoaderEntryOneShot var
> + * Copyright (c) 2013-2016, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/efi.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/reboot.h>
> +#include <linux/kexec.h>
> +#include <linux/slab.h>
> +#include <linux/nls.h>
> +
> +#define MODULE_NAME "efibc"
> +
> +static const efi_guid_t LOADER_GUID =
> +	EFI_GUID(0x4a67b082, 0x0a4c, 0x41cf,
> +		 0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f);

Please move this guid to include/linux/efi.h.

> +
> +static const char REBOOT_TARGET[] = "LoaderEntryOneShot";
> +static const char REBOOT_REASON[] = "LoaderEntryRebootReason";
> +
> +static const char * const WDT_SOURCE_PREFIX[] = {
> +	"Kernel Watchdog", "Watchdog", "softlockup", "Software Watchdog"
> +};

Please use lowercase for names, unless you're using #define.
Additionally, because REBOOT_* are only used in one place you could
just use the strings directly instead of making them file-global.

> +static void efibc_str_to_str16(const char *str, wchar_t *str16)
> +{

s/wchar_t/efi_char16_t/

> +	size_t size = strlen(str) + 1;
> +
> +	utf8s_to_utf16s(str, size, UTF16_LITTLE_ENDIAN, str16,
> +			size * sizeof(*str16) / sizeof(*str));
> +	str16[size - 1] = '\0';

	for (i = 0; i < strlen(str); i++)
		str16[i] = str[i];

	str16[i] = '\0';

right? Are there some scenarios where utf8s_to_utf16s() works and the
above loop doesn't? We have to do this conversion in the efivarfs file
system too, so does that need updating?

> +static struct efivar_entry *efibc_get_var_entry(const char *name)
> +{
> +	wchar_t name16[strlen(name) + 1];
> +	struct efivar_entry *entry;
> +
> +	efibc_str_to_str16(name, name16);
> +
> +	efivar_entry_iter_begin();
> +	entry = efivar_entry_find(name16, LOADER_GUID,
> +				  &efivar_sysfs_list, false);

Hmm... there's an interesting consequence of using efivar_sysfs_list
here, and that is that if someone creates this variable manually via
the efivarfs file system you won't see it on efivar_sysfs_list.

Of course, when you restart the machine you will see it then, so it's
probably not a big deal for this driver that only cares about reboot;
I just wanted to make you aware.

> +	efivar_entry_iter_end();
> +
> +	return entry;
> +}
> +
> +static void efibc_set_variable(const char *name, const char *value)
> +{
> +	u32 attributes = EFI_VARIABLE_NON_VOLATILE
> +		| EFI_VARIABLE_BOOTSERVICE_ACCESS
> +		| EFI_VARIABLE_RUNTIME_ACCESS;
> +	wchar_t name16[strlen(name) + 1];
> +	wchar_t value16[strlen(value) + 1];
> +	int ret;

Please put the attribute value on a separate line from the
declaration.

> +
> +	efibc_str_to_str16(name, name16);
> +	efibc_str_to_str16(value, value16);
> +
> +	ret = efivar_entry_set_safe(name16, LOADER_GUID, attributes, true,
> +				    sizeof(value16), value16);

If it is always OK to block when calling SetVariable() you don't want
to be using efivar_entry_set_safe(). The regular efivar_entry_set()
will suffice.

The *_safe() version is special because it will handle being called in
a context where it is not allowed to block.

Oh but I just realised you call this from a panic handler, so it's not
safe to always block.

In which case, yes, using efivar_entry_set_safe() is correct but you
cannot always pass 'true' for the @block parameter.

> +	if (ret)
> +		pr_err(MODULE_NAME ": failed to set %s EFI variable: 0x%x\n",
> +		       name, ret);
> +}

You can redefine pr_fmt to avoid having to specify MODULE_NAME here,
e.g.

#define pr_fmt(fmt)	"efifbc: "

> +
> +static int efibc_reboot_notifier_call(struct notifier_block *notifier,
> +				      unsigned long what, void *data)
> +{
> +	char *reason = what == SYS_RESTART ? "reboot" : "shutdown";
> +

Let's rename 'what' to 'event' to provide an idea of the kind of data
it contains. This line needs breaking down too.

How about,

	const char *reason = "shutdown";

	if (event == SYS_RESTART)
		reason = "reboot";

Which makes it a little clearer that we treat the SYS_RESTART event as
being special and lump all other events under "shutdown".

> +}
> +
> +static bool is_watchdog_source(const char *str)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < ARRAY_SIZE(WDT_SOURCE_PREFIX); i++)
> +		if (!strncmp(str, WDT_SOURCE_PREFIX[i],
> +			     strlen(WDT_SOURCE_PREFIX[i])))
> +			return true;
> +
> +	return false;
> +}
> +
> +static int efibc_panic_notifier_call(struct notifier_block *notifier,
> +				     unsigned long what, void *data)
> +{
> +	char *reason;
> +
> +	/* If the reboot reason has already been supplied, keep it. */
> +	if (efibc_get_var_entry(REBOOT_REASON))
> +		return NOTIFY_DONE;

I'm not sure this is the best idea. If, in the process of doing a
normal reboot, we panic, wouldn't the user rather see the panic
reason and not the reboot reason?

> +
> +	if (data && is_watchdog_source((char *)data))
> +		reason = "watchdog";
> +	else
> +		reason = "kernel_panic";
> +
> +	efibc_set_variable(REBOOT_REASON, reason);

Checking the panic string prefix for known watchdog strings is a bit
hacky. For example, this won't work with the hard lockup detector.
What happens if someone adds a new watchdog driver?

On second thought, I'm not at all sure of the value of this panic
notifier. As I understood it, the proposition of this driver was that
it allows the user to pass information via the kernel to the boot
loader upon reboot. This data is strictly opaque to the kernel, and
I'm OK with that.

However, with this panic handler, you're doing something different.
You're passing kernel strings directly (albeit translated to
"watchdog" or "kernel_panic") to the boot loader. You're creating an
ABI between the kernel and boot loaders.

Besides which, we already have code to store information in EFI
variables on panic; the efi-pstore driver.

Sorry, but NAK on this part of the patch.

Re: [PATCH] efibc: EFI Bootloader Control

[Compostella, Jeremy]

Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> writes:

> (Sorry for the delay)
No worries.

> On Fri, 01 Apr, at 03:09:51PM, Compostella, Jeremy wrote:
>> This module installs a reboot hook, such that if reboot() is invoked
>> with a string argument NNN, "NNN" is copied to the
>> "LoaderEntryOneShot" EFI variable, to be read by the bootloader. If
>> the string matches one of the boot labels defined in its
>> configuration, the bootloader will boot once to that label.  The
>> "LoaderEntryRebootReason" EFI variable is set with the reboot reason:
>> "reboot", "shutdown", "kernel_panic" or "watchdog".  The bootloader
>> reads this reboot reason and takes particular action according to its
>> policy.
>  
> This commit message is missing some info. In particular, I'd like see
> to which boot loaders support this protocol and why this feature
> should be a kernel driver and cannot be done with the userspace EFI
> variable API.
>
> Yes we've already discussed these bits on the mailing list, but
> recording it for posterity in the commit log is very worthwhile.
Done.

>> Signed-off-by: Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>  arch/x86/Kconfig               |  16 ++++
>>  arch/x86/platform/efi/Makefile |   1 +
>>  arch/x86/platform/efi/efibc.c  | 164 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 181 insertions(+)
>>  create mode 100644 arch/x86/platform/efi/efibc.c
>  
> Hehe, I suspect I was a little unclear in my previous comments about
> moving this out of drivers/firmware/efi.
>
> arch/x86/platform/efi is not the right place for this kind of stuff
> either, since it contains core x86 EFI support.
>
> That directory is no longer named appropriately, as EFI should not be
> considered a feature of specific platforms - it's the standard
> firmware technology nowadays on x86. I've been meaning to rename it.
I moved it to driver/firmware/efi/.

> But let's ignore the issue of finding the right home for now. I've got
> some reservations about the panic notifier that I've outlined below.
>
>> diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
>> index 066619b..05cf769 100644
>> --- a/arch/x86/platform/efi/Makefile
>> +++ b/arch/x86/platform/efi/Makefile
>> @@ -4,3 +4,4 @@ obj-$(CONFIG_EFI) 		+= quirks.o efi.o efi_$(BITS).o efi_stub_$(BITS).o
>>  obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
>>  obj-$(CONFIG_EARLY_PRINTK_EFI)	+= early_printk.o
>>  obj-$(CONFIG_EFI_MIXED)		+= efi_thunk_$(BITS).o
>> +obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
>> diff --git a/arch/x86/platform/efi/efibc.c b/arch/x86/platform/efi/efibc.c
>> new file mode 100644
>> index 0000000..8ca8b43
>> --- /dev/null
>> +++ b/arch/x86/platform/efi/efibc.c
>> @@ -0,0 +1,164 @@
>> +/*
>> + * efibc: control EFI bootloaders which obey LoaderEntryOneShot var
>> + * Copyright (c) 2013-2016, Intel Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + */
>> +
>> +#include <linux/efi.h>
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
>> +#include <linux/reboot.h>
>> +#include <linux/kexec.h>
>> +#include <linux/slab.h>
>> +#include <linux/nls.h>
>> +
>> +#define MODULE_NAME "efibc"
>> +
>> +static const efi_guid_t LOADER_GUID =
>> +	EFI_GUID(0x4a67b082, 0x0a4c, 0x41cf,
>> +		 0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f);
>
> Please move this guid to include/linux/efi.h.
Done.  But I'm not sure if EFI_LOADER_GUID constant name is
appropriate.  However, I'd like to avoid naming it after Kernelflinger
or Gummiboot.

>> +
>> +static const char REBOOT_TARGET[] = "LoaderEntryOneShot";
>> +static const char REBOOT_REASON[] = "LoaderEntryRebootReason";
>> +
>> +static const char * const WDT_SOURCE_PREFIX[] = {
>> +	"Kernel Watchdog", "Watchdog", "softlockup", "Software Watchdog"
>> +};
>
> Please use lowercase for names, unless you're using #define.
> Additionally, because REBOOT_* are only used in one place you could
> just use the strings directly instead of making them file-global.
Done.

>> +static void efibc_str_to_str16(const char *str, wchar_t *str16)
>> +{
>
> s/wchar_t/efi_char16_t/
>
>> +	size_t size = strlen(str) + 1;
>> +
>> +	utf8s_to_utf16s(str, size, UTF16_LITTLE_ENDIAN, str16,
>> +			size * sizeof(*str16) / sizeof(*str));
>> +	str16[size - 1] = '\0';
>
> 	for (i = 0; i < strlen(str); i++)
> 		str16[i] = str[i];
>
> 	str16[i] = '\0';
>
> right? Are there some scenarios where utf8s_to_utf16s() works and the
> above loop doesn't? We have to do this conversion in the efivarfs file
> system too, so does that need updating?
Done.  I do not see any scenario where ASCII is not enough.

>> +static struct efivar_entry *efibc_get_var_entry(const char *name)
>> +{
>> +	wchar_t name16[strlen(name) + 1];
>> +	struct efivar_entry *entry;
>> +
>> +	efibc_str_to_str16(name, name16);
>> +
>> +	efivar_entry_iter_begin();
>> +	entry = efivar_entry_find(name16, LOADER_GUID,
>> +				  &efivar_sysfs_list, false);
>
> Hmm... there's an interesting consequence of using efivar_sysfs_list
> here, and that is that if someone creates this variable manually via
> the efivarfs file system you won't see it on efivar_sysfs_list.
>
> Of course, when you restart the machine you will see it then, so it's
> probably not a big deal for this driver that only cares about reboot;
> I just wanted to make you aware.
I removed this function (not needed anymore).

>> +	efivar_entry_iter_end();
>> +
>> +	return entry;
>> +}
>> +
>> +static void efibc_set_variable(const char *name, const char *value)
>> +{
>> +	u32 attributes = EFI_VARIABLE_NON_VOLATILE
>> +		| EFI_VARIABLE_BOOTSERVICE_ACCESS
>> +		| EFI_VARIABLE_RUNTIME_ACCESS;
>> +	wchar_t name16[strlen(name) + 1];
>> +	wchar_t value16[strlen(value) + 1];
>> +	int ret;
>
> Please put the attribute value on a separate line from the
> declaration.
Done.

>> +
>> +	efibc_str_to_str16(name, name16);
>> +	efibc_str_to_str16(value, value16);
>> +
>> +	ret = efivar_entry_set_safe(name16, LOADER_GUID, attributes, true,
>> +				    sizeof(value16), value16);
>
> If it is always OK to block when calling SetVariable() you don't want
> to be using efivar_entry_set_safe(). The regular efivar_entry_set()
> will suffice.
>
> The *_safe() version is special because it will handle being called in
> a context where it is not allowed to block.


> Oh but I just realised you call this from a panic handler, so it's not
> safe to always block.
>
> In which case, yes, using efivar_entry_set_safe() is correct but you
> cannot always pass 'true' for the @block parameter.
>
>> +	if (ret)
>> +		pr_err(MODULE_NAME ": failed to set %s EFI variable: 0x%x\n",
>> +		       name, ret);
>> +}
>
> You can redefine pr_fmt to avoid having to specify MODULE_NAME here,
> e.g.
>
> #define pr_fmt(fmt)	"efifbc: "
Done.

>> +
>> +static int efibc_reboot_notifier_call(struct notifier_block *notifier,
>> +				      unsigned long what, void *data)
>> +{
>> +	char *reason = what == SYS_RESTART ? "reboot" : "shutdown";
>> +
>
> Let's rename 'what' to 'event' to provide an idea of the kind of data
> it contains. This line needs breaking down too.
>
> How about,
>
> 	const char *reason = "shutdown";
>
> 	if (event == SYS_RESTART)
> 		reason = "reboot";
>
> Which makes it a little clearer that we treat the SYS_RESTART event as
> being special and lump all other events under "shutdown".
Done.

>> +}
>> +
>> +static bool is_watchdog_source(const char *str)
>> +{
>> +	size_t i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(WDT_SOURCE_PREFIX); i++)
>> +		if (!strncmp(str, WDT_SOURCE_PREFIX[i],
>> +			     strlen(WDT_SOURCE_PREFIX[i])))
>> +			return true;
>> +
>> +	return false;
>> +}
>> +
>> +static int efibc_panic_notifier_call(struct notifier_block *notifier,
>> +				     unsigned long what, void *data)
>> +{
>> +	char *reason;
>> +
>> +	/* If the reboot reason has already been supplied, keep it. */
>> +	if (efibc_get_var_entry(REBOOT_REASON))
>> +		return NOTIFY_DONE;
>
> I'm not sure this is the best idea. If, in the process of doing a
> normal reboot, we panic, wouldn't the user rather see the panic
> reason and not the reboot reason?
Not needed anymore.

>> +
>> +	if (data && is_watchdog_source((char *)data))
>> +		reason = "watchdog";
>> +	else
>> +		reason = "kernel_panic";
>> +
>> +	efibc_set_variable(REBOOT_REASON, reason);
>
> Checking the panic string prefix for known watchdog strings is a bit
> hacky. For example, this won't work with the hard lockup detector.
> What happens if someone adds a new watchdog driver?
>
> On second thought, I'm not at all sure of the value of this panic
> notifier. As I understood it, the proposition of this driver was that
> it allows the user to pass information via the kernel to the boot
> loader upon reboot. This data is strictly opaque to the kernel, and
> I'm OK with that.
>
> However, with this panic handler, you're doing something different.
> You're passing kernel strings directly (albeit translated to
> "watchdog" or "kernel_panic") to the boot loader. You're creating an
> ABI between the kernel and boot loaders.
>
> Besides which, we already have code to store information in EFI
> variables on panic; the efi-pstore driver.
>
> Sorry, but NAK on this part of the patch.
Agreed.  I removed all the panic handler related code.  Though, I'd
like to keep the LoaderEntryRebootReason filled with "shutdown" or
"reboot" if that's okay with you.

Thanks,

Jérémy
-- 
One Emacs to rule them all

Re: [PATCH] efibc: EFI Bootloader Control

[Compostella, Jeremy]

This module installs a reboot hook, such that if reboot() is invoked
with a string argument NNN, "NNN" is copied to the
"LoaderEntryOneShot" EFI variable, to be read by the bootloader. If
the string matches one of the boot labels defined in its
configuration, the bootloader will boot once to that label.  The
"LoaderEntryRebootReason" EFI variable is set with the reboot reason:
"reboot", "shutdown".  The bootloader reads this reboot reason and
takes particular action according to its policy.

There are reboot implementations that do "reboot <reason>", such as
Android's reboot command and Upstart's reboot replacement, which pass
the reason as an argument to the reboot syscall.  There is no
platform-agnostic way how those could be modified to pass the reason
to the bootloader, regardless of platform or bootloader.

Signed-off-by: Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/Kconfig  |  15 +++++++
 drivers/firmware/efi/Makefile |   1 +
 drivers/firmware/efi/efibc.c  | 101 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/efi.h           |   4 ++
 4 files changed, 121 insertions(+)
 create mode 100644 drivers/firmware/efi/efibc.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index e1670d5..0b0b635 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -87,6 +87,21 @@ config EFI_RUNTIME_WRAPPERS
 config EFI_ARMSTUB
 	bool
 
+config EFI_BOOTLOADER_CONTROL
+	tristate "EFI Bootloader Control"
+	depends on EFI_VARS
+	default n
+	---help---
+	  This module installs a reboot hook, such that if reboot() is
+	  invoked with a string argument NNN, "NNN" is copied to the
+	  "LoaderEntryOneShot" EFI variable, to be read by the
+	  bootloader. If the string matches one of the boot labels
+	  defined in its configuration, the bootloader will boot once
+	  to that label. The "LoaderEntryRebootReason" EFI variable is
+	  set with the reboot reason: "reboot" or "shutdown". The
+	  bootloader reads this reboot reason and takes particular
+	  action according to its policy.
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 62e654f..c35e218 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
 obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
 obj-$(CONFIG_EFI_STUB)			+= libstub/
 obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_mem.o
+obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
 
 arm-obj-$(CONFIG_EFI)			:= arm-init.o arm-runtime.o
 obj-$(CONFIG_ARM)			+= $(arm-obj-y)
diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
new file mode 100644
index 0000000..9833ef5
--- /dev/null
+++ b/drivers/firmware/efi/efibc.c
@@ -0,0 +1,101 @@
+/*
+ * efibc: control EFI bootloaders which obey LoaderEntryOneShot var
+ * Copyright (c) 2013-2016, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#define pr_fmt(fmt) "efibc: " fmt
+
+#include <linux/efi.h>
+#include <linux/module.h>
+#include <linux/reboot.h>
+
+static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
+{
+	size_t i;
+
+	for (i = 0; i < strlen(str); i++)
+		str16[i] = str[i];
+
+	str16[i] = '\0';
+}
+
+static void efibc_set_variable(const char *name, const char *value)
+{
+	int ret;
+	efi_guid_t guid = EFI_LOADER_GUID;
+	struct efivar_entry entry;
+	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
+
+	if (size > sizeof(entry.var.Data))
+		pr_err("value is too large");
+
+	efibc_str_to_str16(name, entry.var.VariableName);
+	efibc_str_to_str16(value, (efi_char16_t *)entry.var.Data);
+	memcpy(&entry.var.VendorGuid, &guid, sizeof(guid));
+
+	ret = efivar_entry_set(&entry,
+			       EFI_VARIABLE_NON_VOLATILE
+			       | EFI_VARIABLE_BOOTSERVICE_ACCESS
+			       | EFI_VARIABLE_RUNTIME_ACCESS,
+			       size, entry.var.Data, NULL);
+	if (ret)
+		pr_err("failed to set %s EFI variable: 0x%x\n",
+		       name, ret);
+}
+
+static int efibc_reboot_notifier_call(struct notifier_block *notifier,
+				      unsigned long event, void *data)
+{
+	char *reason = "shutdown";
+
+	if (event == SYS_RESTART)
+		reason = "reboot";
+
+	efibc_set_variable("LoaderEntryRebootReason", reason);
+
+	if (!data)
+		return NOTIFY_DONE;
+
+	efibc_set_variable("LoaderEntryOneShot", (char *)data);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block efibc_reboot_notifier = {
+	.notifier_call = efibc_reboot_notifier_call,
+};
+
+static int __init efibc_init(void)
+{
+	int ret;
+
+	if (!efi_enabled(EFI_RUNTIME_SERVICES))
+		return -ENODEV;
+
+	ret = register_reboot_notifier(&efibc_reboot_notifier);
+	if (ret)
+		pr_err("unable to register reboot notifier\n");
+
+	return ret;
+}
+module_init(efibc_init);
+
+static void __exit efibc_exit(void)
+{
+	unregister_reboot_notifier(&efibc_reboot_notifier);
+}
+module_exit(efibc_exit);
+
+MODULE_AUTHOR("Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
+MODULE_AUTHOR("Matt Gumbel <matthew.k.gumbel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org");
+MODULE_DESCRIPTION("EFI Bootloader Control");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 1626474..8dbf034 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -623,6 +623,10 @@ void efi_native_runtime_setup(void);
 	EFI_GUID(0x3152bca5, 0xeade, 0x433d, \
 		 0x86, 0x2e, 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44)
 
+#define EFI_LOADER_GUID \
+	EFI_GUID(0x4a67b082, 0x0a4c, 0x41cf, \
+		 0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f)
+
 typedef struct {
 	efi_guid_t guid;
 	u64 table;
-- 
1.9.1

Re: [PATCH] efibc: EFI Bootloader Control

[Matt Fleming]

On Mon, 18 Apr, at 03:04:44PM, Compostella, Jeremy wrote:
> Done.  But I'm not sure if EFI_LOADER_GUID constant name is
> appropriate.  However, I'd like to avoid naming it after Kernelflinger
> or Gummiboot.

Yeah. What about EFI_LOADER_ENTRY_GUID ?

> Agreed.  I removed all the panic handler related code.  Though, I'd
> like to keep the LoaderEntryRebootReason filled with "shutdown" or
> "reboot" if that's okay with you.

Fine by me!

Re: [PATCH] efibc: EFI Bootloader Control

[Matt Fleming]

On Mon, 18 Apr, at 03:05:42PM, Jeremy Compostella wrote:
> This module installs a reboot hook, such that if reboot() is invoked
> with a string argument NNN, "NNN" is copied to the
> "LoaderEntryOneShot" EFI variable, to be read by the bootloader. If
> the string matches one of the boot labels defined in its
> configuration, the bootloader will boot once to that label.  The
> "LoaderEntryRebootReason" EFI variable is set with the reboot reason:
> "reboot", "shutdown".  The bootloader reads this reboot reason and
> takes particular action according to its policy.
> 
> There are reboot implementations that do "reboot <reason>", such as
> Android's reboot command and Upstart's reboot replacement, which pass
> the reason as an argument to the reboot syscall.  There is no
> platform-agnostic way how those could be modified to pass the reason
> to the bootloader, regardless of platform or bootloader.
> 
> Signed-off-by: Jeremy Compostella <jeremy.compostella-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/firmware/efi/Kconfig  |  15 +++++++
>  drivers/firmware/efi/Makefile |   1 +
>  drivers/firmware/efi/efibc.c  | 101 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/efi.h           |   4 ++
>  4 files changed, 121 insertions(+)
>  create mode 100644 drivers/firmware/efi/efibc.c
 
It's difficult to have any concerns with a driver that's so small, so
I've applied this. Please note a couple of things I fixed up,

 - Add 'const' qualifier to char *reason
 - Change EFI_LOADER_GUID to LINUX_EFI_LOADER_ENTRY_GUID

Shout if you've issues with the change of name for the guid. It is
important to make it clear that this guid is not found in the UEFI
spec, which we've done in the past by prefixing guid names with LINUX_
(though presumably the Loader Entry GUID isn't necessarily Linux
specific).

Re: [PATCH] efibc: EFI Bootloader Control

[Matt Fleming]

On Mon, 18 Apr, at 03:05:42PM, Jeremy Compostella wrote:
> +static void efibc_set_variable(const char *name, const char *value)
> +{
> +	int ret;
> +	efi_guid_t guid = EFI_LOADER_GUID;
> +	struct efivar_entry entry;
> +	size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);

Putting 'entry' on the stack leads to the following build warning,

 drivers/firmware/efi/efibc.c: In function ‘efibc_set_variable’:
 drivers/firmware/efi/efibc.c:53:1: warning: the frame size of 2192 bytes is larger than 2048 bytes [-Wframe-larger-than=]

That's a big object.

Given that efibc_reboot_notifier_call() can only be invoked once,
could we just statically allocate 'entry' instead?