2016-03-18 10:08:36

by Daniel Baluta

[permalink] [raw]
Subject: [PATCH v2] efi: Introduce EFI bootloader control driver

From: Matt Gumbel <[email protected]>

This driver intercepts system reboot requests and populates the
LoaderEntryOneShot EFI variable with the user-supplied reboot
argument. EFI bootloaders such as Gummiboot will consume this
variable and use it to control which OS is booted next.

We use this with Android where reboot() tells the kernel that
we want to boot into recovery or other non-default OS environment.

It is the bootloader's job to guard against this variable being
uninitialzed or containing invalid data, and just boot normally
if that is the case.

Signed-off-by: Matt Gumbel <[email protected]>
Signed-off-by: Mohamed Abbas <[email protected]>
Signed-off-by: Constantin Musca <[email protected]>
Signed-off-by: Daniel Baluta <[email protected]>
---
Changes since v1:
* updated Makefile after changing source name from efibc.c -> efi-bc.c
to comply with naming rules in drivers/firmware/efi/

drivers/firmware/efi/Kconfig | 11 ++
drivers/firmware/efi/Makefile | 1 +
drivers/firmware/efi/efi-bc.c | 251 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 263 insertions(+)
create mode 100644 drivers/firmware/efi/efi-bc.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index e1670d5..0f21af4 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -18,6 +18,17 @@ config EFI_VARS
Subsequent efibootmgr releases may be found at:
<http://github.com/vathpela/efibootmgr>

+config EFI_BOOTLOADER_CONTROL
+ tristate "EFI Bootloader Control module"
+ depends on EFI_VARS
+ default n
+ help
+ This driver installs a reboot hook, such that if reboot() is
+ invoked with a string argument NNN, "bootonce-NNN" is copied to
+ the 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.
+
config EFI_ESRT
bool
depends on EFI && !IA64
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 62e654f..50cfac2 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -10,6 +10,7 @@
KASAN_SANITIZE_runtime-wrappers.o := n

obj-$(CONFIG_EFI) += efi.o vars.o reboot.o
+obj-$(CONFIG_EFI_BOOTLOADER_CONTROL) += efi-bc.o
obj-$(CONFIG_EFI_VARS) += efivars.o
obj-$(CONFIG_EFI_ESRT) += esrt.o
obj-$(CONFIG_EFI_VARS_PSTORE) += efi-pstore.o
diff --git a/drivers/firmware/efi/efi-bc.c b/drivers/firmware/efi/efi-bc.c
new file mode 100644
index 0000000..7550fee
--- /dev/null
+++ b/drivers/firmware/efi/efi-bc.c
@@ -0,0 +1,251 @@
+/*
+ * efi-bc.c - EFI bootloader control
+ *
+ * Copyright (c) 2013, 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>
+
+#define LOADER_ENTRY_ONE_SHOT "LoaderEntryOneShot"
+#define LOADER_ENTRY_REBOOT "LoaderEntryRebootReason"
+#define LOADER_GUID EFI_GUID(0x4a67b082, 0x0a4c, 0x41cf, 0xb6, 0xc7, \
+ 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f)
+
+#define REBOOT_REASON_CRASH "kernel_panic"
+#define REBOOT_REASON_NORMAL "reboot"
+#define REBOOT_REASON_SHUTDOWN "shutdown"
+#define REBOOT_REASON_HALT "halt"
+#define REBOOT_REASON_WATCHDOG "watchdog"
+
+#define WATCHDOG_KERNEL_H "Watchdog"
+#define WATCHDOG_KERNEL_S "softlockup"
+#define WATCHDOG_KERNEL_D "Software Watchdog"
+
+/*
+ * Convert char string to efi_char16_t string. NULL byte at end is always
+ * preserved.
+ */
+static unsigned long
+efichar_from_char(efi_char16_t *dest, const char *src, size_t dest_len)
+{
+ int i;
+
+ for (i = 0; src[i] && i < (dest_len / sizeof(*dest)) - 1; i++)
+ dest[i] = src[i];
+ dest[i] = 0; /* Gummiboot does need null-terminated strings */
+ return i;
+}
+
+/*
+ * Returns required size of utf16 buffer for given char string. Space for
+ * trailing nul is included. str must not be NULL.
+ */
+static size_t efi_char16_bufsz(char *str)
+{
+ return (1 + strlen(str)) * sizeof(efi_char16_t);
+}
+
+/*
+ * Set EFI variable to specify next boot target for EFI bootloader. Meant to be
+ * called as a reboot notifier. DO NOT call this function without guaranteeing
+ * efi_enabled == true; it will crash.
+ */
+static int
+efibc_reboot_notifier_call(struct notifier_block *notifier, unsigned long what,
+ void *data)
+{
+ int ret = NOTIFY_DONE;
+ char *cmd = data;
+ efi_status_t status;
+ efi_char16_t *name_efichar = NULL, *cmd_efichar = NULL;
+ size_t name_efichar_blen, cmd_efichar_blen;
+
+ if (what != SYS_RESTART || !cmd)
+ goto out;
+
+ name_efichar_blen = efi_char16_bufsz(LOADER_ENTRY_ONE_SHOT);
+ cmd_efichar_blen = efi_char16_bufsz(cmd);
+
+ name_efichar = kzalloc(name_efichar_blen, GFP_KERNEL);
+ if (!name_efichar)
+ goto out;
+
+ cmd_efichar = kzalloc(cmd_efichar_blen, GFP_KERNEL);
+ if (!cmd_efichar)
+ goto out;
+
+ if (efichar_from_char(
+ name_efichar, LOADER_ENTRY_ONE_SHOT, name_efichar_blen)
+ != strlen(LOADER_ENTRY_ONE_SHOT)) {
+ pr_err(
+ "efibc: %s: Failed to convert char to efi_char16_t. length=%lu",
+ __func__, name_efichar_blen);
+ goto out;
+ }
+
+ if (efichar_from_char(cmd_efichar, cmd, cmd_efichar_blen)
+ != strlen(cmd)) {
+ pr_err(
+ "efibc: %s: Failed to convert char to efi_char16_t. length=%lu",
+ __func__, cmd_efichar_blen);
+ goto out;
+ }
+
+ status = efi.set_variable(name_efichar, &LOADER_GUID,
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ cmd_efichar_blen, cmd_efichar);
+ if (status != EFI_SUCCESS) {
+ pr_err("efibc: set_variable() failed. status=%lx\n", status);
+ goto out;
+ }
+
+ ret = NOTIFY_OK;
+out:
+ kfree(cmd_efichar);
+ kfree(name_efichar);
+ return ret;
+}
+
+static int efibc_reboot_reason(unsigned long what, char *cmd)
+{
+ int ret = NOTIFY_DONE;
+ efi_status_t status;
+ efi_char16_t name_efichar[40];
+ efi_char16_t cmd_efichar[20];
+ size_t name_efichar_blen, cmd_efichar_blen;
+
+ name_efichar_blen = efi_char16_bufsz(LOADER_ENTRY_REBOOT);
+ cmd_efichar_blen = efi_char16_bufsz(cmd);
+
+ if (efichar_from_char(
+ name_efichar, LOADER_ENTRY_REBOOT,
+ name_efichar_blen) != strlen(LOADER_ENTRY_REBOOT)) {
+ pr_err("efibc: %s: Failed to convert char to u16.", __func__);
+ goto out;
+ }
+
+ if (efichar_from_char(cmd_efichar, cmd, cmd_efichar_blen)
+ != strlen(cmd)) {
+ pr_err("efibc: %s: Failed to convert char to u16", __func__);
+ goto out;
+ }
+
+ status = efi.set_variable(name_efichar, &LOADER_GUID,
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ cmd_efichar_blen,
+ cmd_efichar);
+ if (status != EFI_SUCCESS) {
+ pr_err("efibc: set_variable() failed. status=%lx\n", status);
+ goto out;
+ }
+
+ ret = NOTIFY_OK;
+out:
+ return ret;
+}
+
+static int
+efibc_reboot_reason_notifier_call(struct notifier_block *notifier,
+ unsigned long what, void *data)
+{
+ int ret;
+
+ switch (what) {
+ case SYS_RESTART:
+ ret = efibc_reboot_reason(what, REBOOT_REASON_NORMAL);
+ break;
+ case SYS_HALT:
+ ret = efibc_reboot_reason(what, REBOOT_REASON_HALT);
+ break;
+ default:
+ ret = efibc_reboot_reason(what, REBOOT_REASON_SHUTDOWN);
+ break;
+ }
+
+ return ret;
+}
+
+static int
+efibc_panic_notifier_call(struct notifier_block *notifier, unsigned long what,
+ void *data)
+{
+ int ret;
+ char *str = data;
+
+ if (str &&
+ (!strncmp(str, WATCHDOG_KERNEL_H, strlen(WATCHDOG_KERNEL_H)) ||
+ !strncmp(str, WATCHDOG_KERNEL_S, strlen(WATCHDOG_KERNEL_S)) ||
+ !strncmp(str, WATCHDOG_KERNEL_D, strlen(WATCHDOG_KERNEL_D))))
+ ret = efibc_reboot_reason(what, REBOOT_REASON_WATCHDOG);
+ else
+ ret = efibc_reboot_reason(what, REBOOT_REASON_CRASH);
+
+ return ret;
+}
+
+static struct notifier_block efibc_reboot_notifier = {
+ .notifier_call = efibc_reboot_notifier_call,
+};
+
+static struct notifier_block efibc_reboot_reason_notifier = {
+ .notifier_call = efibc_reboot_reason_notifier_call,
+};
+
+static struct notifier_block paniced = {
+ .notifier_call = efibc_panic_notifier_call,
+};
+
+static int __init efibc_init(void)
+{
+ int ret;
+
+ if (!efi_enabled(EFI_RUNTIME_SERVICES))
+ return 0;
+
+ ret = register_reboot_notifier(&efibc_reboot_notifier);
+ if (ret) {
+ pr_err("efibc: unable to register reboot notifier\n");
+ return ret;
+ }
+
+ ret = register_reboot_notifier(&efibc_reboot_reason_notifier);
+ if (ret) {
+ pr_err("efibc: unable to register reboot notifier\n");
+ unregister_reboot_notifier(&efibc_reboot_notifier);
+ return ret;
+ }
+
+ atomic_notifier_chain_register(&panic_notifier_list, &paniced);
+
+ return 0;
+}
+module_init(efibc_init);
+
+static void __exit efibc_exit(void)
+{
+ unregister_reboot_notifier(&efibc_reboot_notifier);
+ unregister_reboot_notifier(&efibc_reboot_reason_notifier);
+ atomic_notifier_chain_unregister(&panic_notifier_list, &paniced);
+}
+module_exit(efibc_exit);
+
+MODULE_AUTHOR("Matt Gumbel <[email protected]");
+MODULE_DESCRIPTION("EFI bootloader communication module");
+MODULE_LICENSE("GPL v2");
--
2.5.0


2016-03-18 11:23:31

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v2] efi: Introduce EFI bootloader control driver

Hi Matt,

[auto build test WARNING on efi/next]
[also build test WARNING on v4.5 next-20160318]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url: https://github.com/0day-ci/linux/commits/Daniel-Baluta/efi-Introduce-EFI-bootloader-control-driver/20160318-181048
base: https://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git next
config: i386-allmodconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386

All warnings (new ones prefixed by >>):

In file included from include/linux/printk.h:6:0,
from include/linux/kernel.h:13,
from include/linux/list.h:8,
from include/linux/preempt.h:10,
from include/linux/spinlock.h:50,
from include/linux/seqlock.h:35,
from include/linux/time.h:5,
from include/linux/efi.h:16,
from drivers/firmware/efi/efi-bc.c:15:
drivers/firmware/efi/efi-bc.c: In function 'efibc_reboot_notifier_call':
include/linux/kern_levels.h:4:18: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'size_t {aka unsigned int}' [-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^
include/linux/kern_levels.h:10:18: note: in expansion of macro 'KERN_SOH'
#define KERN_ERR KERN_SOH "3" /* error conditions */
^
include/linux/printk.h:252:9: note: in expansion of macro 'KERN_ERR'
printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
^
>> drivers/firmware/efi/efi-bc.c:93:3: note: in expansion of macro 'pr_err'
pr_err(
^
include/linux/kern_levels.h:4:18: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'size_t {aka unsigned int}' [-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^
include/linux/kern_levels.h:10:18: note: in expansion of macro 'KERN_SOH'
#define KERN_ERR KERN_SOH "3" /* error conditions */
^
include/linux/printk.h:252:9: note: in expansion of macro 'KERN_ERR'
printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
^
drivers/firmware/efi/efi-bc.c:101:3: note: in expansion of macro 'pr_err'
pr_err(
^

vim +/pr_err +93 drivers/firmware/efi/efi-bc.c

77 goto out;
78
79 name_efichar_blen = efi_char16_bufsz(LOADER_ENTRY_ONE_SHOT);
80 cmd_efichar_blen = efi_char16_bufsz(cmd);
81
82 name_efichar = kzalloc(name_efichar_blen, GFP_KERNEL);
83 if (!name_efichar)
84 goto out;
85
86 cmd_efichar = kzalloc(cmd_efichar_blen, GFP_KERNEL);
87 if (!cmd_efichar)
88 goto out;
89
90 if (efichar_from_char(
91 name_efichar, LOADER_ENTRY_ONE_SHOT, name_efichar_blen)
92 != strlen(LOADER_ENTRY_ONE_SHOT)) {
> 93 pr_err(
94 "efibc: %s: Failed to convert char to efi_char16_t. length=%lu",
95 __func__, name_efichar_blen);
96 goto out;
97 }
98
99 if (efichar_from_char(cmd_efichar, cmd, cmd_efichar_blen)
100 != strlen(cmd)) {
101 pr_err(

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation


Attachments:
(No filename) (3.34 kB)
.config.gz (52.21 kB)
Download all attachments

2016-03-18 12:47:04

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v2] efi: Introduce EFI bootloader control driver

Hi Matt,

[auto build test WARNING on efi/next]
[also build test WARNING on v4.5 next-20160318]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url: https://github.com/0day-ci/linux/commits/Daniel-Baluta/efi-Introduce-EFI-bootloader-control-driver/20160318-181048
base: https://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git next
config: i386-allyesconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386

All warnings (new ones prefixed by >>):

drivers/firmware/efi/efi-bc.c: In function 'efibc_reboot_notifier_call':
>> drivers/firmware/efi/efi-bc.c:93:10: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'size_t {aka unsigned int}' [-Wformat=]
drivers/firmware/efi/efi-bc.c:101:10: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'size_t {aka unsigned int}' [-Wformat=]

vim +93 drivers/firmware/efi/efi-bc.c

77 goto out;
78
79 name_efichar_blen = efi_char16_bufsz(LOADER_ENTRY_ONE_SHOT);
80 cmd_efichar_blen = efi_char16_bufsz(cmd);
81
82 name_efichar = kzalloc(name_efichar_blen, GFP_KERNEL);
83 if (!name_efichar)
84 goto out;
85
86 cmd_efichar = kzalloc(cmd_efichar_blen, GFP_KERNEL);
87 if (!cmd_efichar)
88 goto out;
89
90 if (efichar_from_char(
91 name_efichar, LOADER_ENTRY_ONE_SHOT, name_efichar_blen)
92 != strlen(LOADER_ENTRY_ONE_SHOT)) {
> 93 pr_err(
94 "efibc: %s: Failed to convert char to efi_char16_t. length=%lu",
95 __func__, name_efichar_blen);
96 goto out;
97 }
98
99 if (efichar_from_char(cmd_efichar, cmd, cmd_efichar_blen)
100 != strlen(cmd)) {
101 pr_err(

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation


Attachments:
(No filename) (1.96 kB)
.config.gz (51.60 kB)
Download all attachments

2016-03-18 16:15:12

by Matt Fleming

[permalink] [raw]
Subject: Re: [PATCH v2] efi: Introduce EFI bootloader control driver

On Fri, 18 Mar, at 12:11:50PM, Daniel Baluta wrote:
> From: Matt Gumbel <[email protected]>
>
> This driver intercepts system reboot requests and populates the
> LoaderEntryOneShot EFI variable with the user-supplied reboot
> argument. EFI bootloaders such as Gummiboot will consume this
> variable and use it to control which OS is booted next.
>
> We use this with Android where reboot() tells the kernel that
> we want to boot into recovery or other non-default OS environment.
>
> It is the bootloader's job to guard against this variable being
> uninitialzed or containing invalid data, and just boot normally
> if that is the case.
>
> Signed-off-by: Matt Gumbel <[email protected]>
> Signed-off-by: Mohamed Abbas <[email protected]>
> Signed-off-by: Constantin Musca <[email protected]>
> Signed-off-by: Daniel Baluta <[email protected]>
> ---
> Changes since v1:
> * updated Makefile after changing source name from efibc.c -> efi-bc.c
> to comply with naming rules in drivers/firmware/efi/
>
> drivers/firmware/efi/Kconfig | 11 ++
> drivers/firmware/efi/Makefile | 1 +
> drivers/firmware/efi/efi-bc.c | 251 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 263 insertions(+)
> create mode 100644 drivers/firmware/efi/efi-bc.c

Why does this require a driver? Why is it not possible to solve this
problem by creating the variable in userspace before invoking
reboot(2)?

2016-03-18 19:18:24

by Stanacar, Stefan

[permalink] [raw]
Subject: Re: [PATCH v2] efi: Introduce EFI bootloader control driver

On Fri, 2016-03-18 at 16:15 +0000, Matt Fleming wrote:
> On Fri, 18 Mar, at 12:11:50PM, Daniel Baluta wrote:
> >
> > From: Matt Gumbel <[email protected]>
> >
> > This driver intercepts system reboot requests and populates the
> > LoaderEntryOneShot EFI variable with the user-supplied reboot
> > argument. EFI bootloaders such as Gummiboot will consume this
> > variable and use it to control which OS is booted next.
> >
> > We use this with Android where reboot() tells the kernel that
> > we want to boot into recovery or other non-default OS environment.
> >
> > It is the bootloader's job to guard against this variable being
> > uninitialzed or containing invalid data, and just boot normally
> > if that is the case.
> >
> > Signed-off-by: Matt Gumbel <[email protected]>
> > Signed-off-by: Mohamed Abbas <[email protected]>
> > Signed-off-by: Constantin Musca <[email protected]>
> > Signed-off-by: Daniel Baluta <[email protected]>
> > ---
> > Changes since v1:
> > * updated Makefile after changing source name from efibc.c ->
> > efi-bc.c
> > to comply with naming rules in drivers/firmware/efi/
> >
> >  drivers/firmware/efi/Kconfig  |  11 ++
> >  drivers/firmware/efi/Makefile |   1 +
> >  drivers/firmware/efi/efi-bc.c | 251
> > ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 263 insertions(+)
> >  create mode 100644 drivers/firmware/efi/efi-bc.c
> Why does this require a driver? Why is it not possible to solve this
> problem by creating the variable in userspace before invoking
> reboot(2)?


Hi Matt,

It is possible, but that means modifying those userspace apps :)
There are reboot implementations that do "reboot <reason>", such as
Android's reboot command [1] and Upstart's reboot replacement [2], which
pass the reason as an argument to the reboot syscall. 
Probably your first question will be - "Why don't you modify those
apps?" Well, I don't see platform-agnostic way how those could be
modified to pass the reason to the bootloader, regardless of platform or
bootloader.

Other non-EFI platforms use a driver as well:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/dri
vers/soc/tegra/pmc.c#n382



[1] https://android.googlesource.com/platform/system/core/+/master/libcu
tils/android_reboot.c#228

[2] https://bazaar.launchpad.net/~upstart-devel/upstart/trunk/view/head:
/util/reboot.c#L259


Cheers,
Stefan

2016-03-24 14:47:54

by Matt Fleming

[permalink] [raw]
Subject: Re: [PATCH v2] efi: Introduce EFI bootloader control driver

(Sorry for the delay)

On Fri, 18 Mar, at 07:18:17PM, Stanacar, Stefan wrote:
>
> Hi Matt,
>
> It is possible, but that means modifying those userspace apps :)
> There are reboot implementations that do "reboot <reason>", such as
> Android's reboot command [1] and Upstart's reboot replacement [2], which
> pass the reason as an argument to the reboot syscall.?
> Probably your first question will be - "Why don't you modify those
> apps?"

Your guess is correct ;)

> Well, I don't see platform-agnostic way how those could be
> modified to pass the reason to the bootloader, regardless of platform or
> bootloader.

This is true. But then again, what you're proposing isn't boot loader
or platform agnostic anyway. Yes it's transparent to both the app and
boot loader, but it's only going to work on EFI platforms running
gummiboot.

And because of that, if this is going to be merged upstream I think
something like drivers/power/reset/ would be a more appropriate place,
or drivers/platform/x86.

If this does get merged, please rework the patch to use the efivar API
instead of accessing efi.set_variable() directly. We've also got a
bunch of ucs2 string functions in lib/ucs2_string.c that you could
use. In fact, this version of the driver I found on the net is much
more like what I had in mind,

https://github.com/BORETS24/Kernel-for-Asus-Zenfone-2/blob/master/drivers/external_drivers/drivers/platform/x86/reboot_target_uefi.c

2016-03-24 16:16:15

by Stanacar, Stefan

[permalink] [raw]
Subject: Re: [PATCH v2] efi: Introduce EFI bootloader control driver

On Thu, 2016-03-24 at 14:47 +0000, Matt Fleming wrote:
> (Sorry for the delay)
>
> On Fri, 18 Mar, at 07:18:17PM, Stanacar, Stefan wrote:
> >
> >
> > Hi Matt,
> >
> > It is possible, but that means modifying those userspace apps :)
> > There are reboot implementations that do "reboot <reason>", such as
> > Android's reboot command [1] and Upstart's reboot replacement [2],
> > which
> > pass the reason as an argument to the reboot syscall. 
> > Probably your first question will be - "Why don't you modify those
> > apps?"
> Your guess is correct ;)
>
> >
> > Well, I don't see platform-agnostic way how those could be
> > modified to pass the reason to the bootloader, regardless of
> > platform or
> > bootloader.
> This is true. But then again, what you're proposing isn't boot loader
> or platform agnostic anyway. Yes it's transparent to both the app and
> boot loader, but it's only going to work on EFI platforms running
> gummiboot.

Yup, that's true. It's going to work only on EFI platforms. The
bootloader actually doesn't matter (any efi bootloader should work) as
long as it reads the reason and does what's supposed to do with it (boot
in recovery, etc)

>
> And because of that, if this is going to be merged upstream I think
> something like drivers/power/reset/ would be a more appropriate place,
> or drivers/platform/x86.
>

Agreed. It seems that drivers/power/reset is preferred by ARM boards:
https://git.linaro.org/people/john.stultz/flo.git/commit/f1e712be2be9f42
97215fc4af6194e0f75f05dfb


> If this does get merged, please rework the patch to use the efivar API
> instead of accessing efi.set_variable() directly. We've also got a
> bunch of ucs2 string functions in lib/ucs2_string.c that you could

Ok.

> use. In fact, this version of the driver I found on the net is much
> more like what I had in mind,
>
>   https://github.com/BORETS24/Kernel-for-Asus-Zenfone-2/blob/master/dr
> ivers/external_drivers/drivers/platform/x86/reboot_target_uefi.c

Ok, that's funny somehow... I wouldn't be surprised if I'd find 3 more
variations of the same driver in different vendor trees :(.
I'll ping the author of the patch, thanks!

Cheers,
Stefan

2016-03-29 12:53:50

by Matt Fleming

[permalink] [raw]
Subject: Re: [PATCH v2] efi: Introduce EFI bootloader control driver

On Thu, 24 Mar, at 04:15:56PM, Stanacar, Stefan wrote:
>
> Ok, that's funny somehow... I wouldn't be surprised if I'd find 3 more
> variations of the same driver in different vendor trees :(.

Right. And that does add weight to getting a version merged upstream;
to discourage people carrying their own.

> I'll ping the author of the patch, thanks!

I haven't reviewed the patch in detail, but it did look like the
correct general approach.