[PATCH v2] bootstd: Make efi_mgr bootmeth work for non-sandbox setups
Enable the bootflow based on this bootmeth if the BootOrder EFI variable is set.
Signed-off-by: Mark Kettenis kettenis@openbsd.org ---
ChangeLog:
v2: - Initialize EFI subsystem to populate EFI variables
boot/bootmeth_efi_mgr.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c index e9d973429f..e6c42d41fb 100644 --- a/boot/bootmeth_efi_mgr.c +++ b/boot/bootmeth_efi_mgr.c @@ -14,6 +14,8 @@ #include <bootmeth.h> #include <command.h> #include <dm.h> +#include <efi_loader.h> +#include <efi_variable.h>
/** * struct efi_mgr_priv - private info for the efi-mgr driver @@ -46,13 +48,26 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter) static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow) { struct efi_mgr_priv *priv = dev_get_priv(dev); + efi_status_t ret; + efi_uintn_t size; + u16 *bootorder;
if (priv->fake_dev) { bflow->state = BOOTFLOWST_READY; return 0; }
- /* To be implemented */ + ret = efi_init_obj_list(); + if (ret) + return log_msg_ret("init", ret); + + /* Enable this method if the "BootOrder" UEFI exists. */ + bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, + &size); + if (bootorder) { + bflow->state = BOOTFLOWST_READY; + return 0; + }
return -EINVAL; }
Hi Mark,
On Sun, 3 Sept 2023 at 14:40, Mark Kettenis kettenis@openbsd.org wrote:
Enable the bootflow based on this bootmeth if the BootOrder EFI variable is set.
Signed-off-by: Mark Kettenis kettenis@openbsd.org
ChangeLog:
v2: - Initialize EFI subsystem to populate EFI variables
boot/bootmeth_efi_mgr.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c index e9d973429f..e6c42d41fb 100644 --- a/boot/bootmeth_efi_mgr.c +++ b/boot/bootmeth_efi_mgr.c @@ -14,6 +14,8 @@ #include <bootmeth.h> #include <command.h> #include <dm.h> +#include <efi_loader.h> +#include <efi_variable.h>
/**
- struct efi_mgr_priv - private info for the efi-mgr driver
@@ -46,13 +48,26 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter) static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow) { struct efi_mgr_priv *priv = dev_get_priv(dev);
efi_status_t ret;efi_uintn_t size;u16 *bootorder; if (priv->fake_dev) { bflow->state = BOOTFLOWST_READY; return 0; }
/* To be implemented */
ret = efi_init_obj_list();
Doesn't this start up the whole EFI system? Is there a cheaper way to see if the variable subsystem exists, or can work?
if (ret)return log_msg_ret("init", ret);/* Enable this method if the "BootOrder" UEFI exists. */bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid,&size);if (bootorder) {bflow->state = BOOTFLOWST_READY;return 0;} return -EINVAL;}
2.42.0
Regards, Simon
From: Simon Glass sjg@chromium.org Date: Thu, 7 Sep 2023 06:23:10 -0600
Hi Mark,
On Sun, 3 Sept 2023 at 14:40, Mark Kettenis kettenis@openbsd.org wrote:
Enable the bootflow based on this bootmeth if the BootOrder EFI variable is set.
Signed-off-by: Mark Kettenis kettenis@openbsd.org
ChangeLog:
v2: - Initialize EFI subsystem to populate EFI variables
boot/bootmeth_efi_mgr.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c index e9d973429f..e6c42d41fb 100644 --- a/boot/bootmeth_efi_mgr.c +++ b/boot/bootmeth_efi_mgr.c @@ -14,6 +14,8 @@ #include <bootmeth.h> #include <command.h> #include <dm.h> +#include <efi_loader.h> +#include <efi_variable.h>
/**
- struct efi_mgr_priv - private info for the efi-mgr driver
@@ -46,13 +48,26 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter) static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow) { struct efi_mgr_priv *priv = dev_get_priv(dev);
efi_status_t ret;efi_uintn_t size;u16 *bootorder; if (priv->fake_dev) { bflow->state = BOOTFLOWST_READY; return 0; }
/* To be implemented */
ret = efi_init_obj_list();Doesn't this start up the whole EFI system? Is there a cheaper way to see if the variable subsystem exists, or can work?
A fair bit of it at least. With the possible exception of launching EFI capsules (which shouldn't happen for a "normal" boot), I don't think this is very expensive though.
There currently isn't a way to only initialize the variable subsystem; we can't just call efi_init_variables() here since we have to also call efi_disks_register() and a later efi_init_obj_list() call would call efi_init_variables() again, which would leak memory.
Alternatively we could just unconditionally declare the efi_mgr bootmeth as "ready" (like you already do for sandbox). That would postpone initialization of the EFI subsystem until we actually execute this bootmeth.
But we need to do something before we switch more targets to standard boot since otherwise booting through the EFI boot manager is just plain broken.
if (ret)return log_msg_ret("init", ret);/* Enable this method if the "BootOrder" UEFI exists. */bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid,&size);if (bootorder) {bflow->state = BOOTFLOWST_READY;return 0;} return -EINVAL;}
2.42.0
Regards, Simon
Hi Mark,
On Thu, 7 Sept 2023 at 07:08, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Thu, 7 Sep 2023 06:23:10 -0600
Hi Mark,
On Sun, 3 Sept 2023 at 14:40, Mark Kettenis kettenis@openbsd.org wrote:
Enable the bootflow based on this bootmeth if the BootOrder EFI variable is set.
Signed-off-by: Mark Kettenis kettenis@openbsd.org
ChangeLog:
v2: - Initialize EFI subsystem to populate EFI variables
boot/bootmeth_efi_mgr.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c index e9d973429f..e6c42d41fb 100644 --- a/boot/bootmeth_efi_mgr.c +++ b/boot/bootmeth_efi_mgr.c @@ -14,6 +14,8 @@ #include <bootmeth.h> #include <command.h> #include <dm.h> +#include <efi_loader.h> +#include <efi_variable.h>
/**
- struct efi_mgr_priv - private info for the efi-mgr driver
@@ -46,13 +48,26 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter) static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow) { struct efi_mgr_priv *priv = dev_get_priv(dev);
efi_status_t ret;efi_uintn_t size;u16 *bootorder; if (priv->fake_dev) { bflow->state = BOOTFLOWST_READY; return 0; }
/* To be implemented */
ret = efi_init_obj_list();Doesn't this start up the whole EFI system? Is there a cheaper way to see if the variable subsystem exists, or can work?
A fair bit of it at least. With the possible exception of launching EFI capsules (which shouldn't happen for a "normal" boot), I don't think this is very expensive though.
There currently isn't a way to only initialize the variable subsystem; we can't just call efi_init_variables() here since we have to also call efi_disks_register() and a later efi_init_obj_list() call would call efi_init_variables() again, which would leak memory.
Alternatively we could just unconditionally declare the efi_mgr bootmeth as "ready" (like you already do for sandbox). That would postpone initialization of the EFI subsystem until we actually execute this bootmeth.
But we need to do something before we switch more targets to standard boot since otherwise booting through the EFI boot manager is just plain broken.
Yes...
I don't have any great ideas on this. If we could get the EFI code better integrated into U-Boot then perhaps we would not need this 'monolithm' init?
Regards, Simon
From: Simon Glass sjg@chromium.org Date: Thu, 7 Sep 2023 09:57:34 -0600
Hi Mark,
On Thu, 7 Sept 2023 at 07:08, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Thu, 7 Sep 2023 06:23:10 -0600
Hi Mark,
On Sun, 3 Sept 2023 at 14:40, Mark Kettenis kettenis@openbsd.org wrote:
Enable the bootflow based on this bootmeth if the BootOrder EFI variable is set.
Signed-off-by: Mark Kettenis kettenis@openbsd.org
ChangeLog:
v2: - Initialize EFI subsystem to populate EFI variables
boot/bootmeth_efi_mgr.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c index e9d973429f..e6c42d41fb 100644 --- a/boot/bootmeth_efi_mgr.c +++ b/boot/bootmeth_efi_mgr.c @@ -14,6 +14,8 @@ #include <bootmeth.h> #include <command.h> #include <dm.h> +#include <efi_loader.h> +#include <efi_variable.h>
/**
- struct efi_mgr_priv - private info for the efi-mgr driver
@@ -46,13 +48,26 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter) static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow) { struct efi_mgr_priv *priv = dev_get_priv(dev);
efi_status_t ret;efi_uintn_t size;u16 *bootorder; if (priv->fake_dev) { bflow->state = BOOTFLOWST_READY; return 0; }
/* To be implemented */
ret = efi_init_obj_list();Doesn't this start up the whole EFI system? Is there a cheaper way to see if the variable subsystem exists, or can work?
A fair bit of it at least. With the possible exception of launching EFI capsules (which shouldn't happen for a "normal" boot), I don't think this is very expensive though.
There currently isn't a way to only initialize the variable subsystem; we can't just call efi_init_variables() here since we have to also call efi_disks_register() and a later efi_init_obj_list() call would call efi_init_variables() again, which would leak memory.
Alternatively we could just unconditionally declare the efi_mgr bootmeth as "ready" (like you already do for sandbox). That would postpone initialization of the EFI subsystem until we actually execute this bootmeth.
But we need to do something before we switch more targets to standard boot since otherwise booting through the EFI boot manager is just plain broken.
Yes...
I don't have any great ideas on this. If we could get the EFI code better integrated into U-Boot then perhaps we would not need this 'monolithm' init?
You'll have to take that up with the maintainers of the EFI loader subsystem. I just want to repair the breakage that was introduced with bootstd.
Hi Mark,
On Sun, 10 Sept 2023 at 08:57, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Thu, 7 Sep 2023 09:57:34 -0600
Hi Mark,
On Thu, 7 Sept 2023 at 07:08, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Thu, 7 Sep 2023 06:23:10 -0600
Hi Mark,
On Sun, 3 Sept 2023 at 14:40, Mark Kettenis kettenis@openbsd.org wrote:
Enable the bootflow based on this bootmeth if the BootOrder EFI variable is set.
Signed-off-by: Mark Kettenis kettenis@openbsd.org
ChangeLog:
v2: - Initialize EFI subsystem to populate EFI variables
boot/bootmeth_efi_mgr.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c index e9d973429f..e6c42d41fb 100644 --- a/boot/bootmeth_efi_mgr.c +++ b/boot/bootmeth_efi_mgr.c @@ -14,6 +14,8 @@ #include <bootmeth.h> #include <command.h> #include <dm.h> +#include <efi_loader.h> +#include <efi_variable.h>
/**
- struct efi_mgr_priv - private info for the efi-mgr driver
@@ -46,13 +48,26 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter) static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow) { struct efi_mgr_priv *priv = dev_get_priv(dev);
efi_status_t ret;efi_uintn_t size;u16 *bootorder; if (priv->fake_dev) { bflow->state = BOOTFLOWST_READY; return 0; }
/* To be implemented */
ret = efi_init_obj_list();Doesn't this start up the whole EFI system? Is there a cheaper way to see if the variable subsystem exists, or can work?
A fair bit of it at least. With the possible exception of launching EFI capsules (which shouldn't happen for a "normal" boot), I don't think this is very expensive though.
There currently isn't a way to only initialize the variable subsystem; we can't just call efi_init_variables() here since we have to also call efi_disks_register() and a later efi_init_obj_list() call would call efi_init_variables() again, which would leak memory.
Alternatively we could just unconditionally declare the efi_mgr bootmeth as "ready" (like you already do for sandbox). That would postpone initialization of the EFI subsystem until we actually execute this bootmeth.
But we need to do something before we switch more targets to standard boot since otherwise booting through the EFI boot manager is just plain broken.
Yes...
I don't have any great ideas on this. If we could get the EFI code better integrated into U-Boot then perhaps we would not need this 'monolithm' init?
You'll have to take that up with the maintainers of the EFI loader subsystem. I just want to repair the breakage that was introduced with bootstd.
Yes, understood. Do you know how long it takes to do this step?
The control for this feature is CMD_BOOTEFI_BOOTMGR...I think it would be better to have BOOTEFI_BOOTMGR as well as CMD_BOOTEFI_BOOTMGR, since it is strange to have a CMD Kconfig in lib/efi_loader
I am OK with this patch as I don't see any alternative. We need to do something and we can always improve it if it causes problems.
Heinrich?
Regards, Simon
From: Simon Glass sjg@chromium.org Date: Sun, 10 Sep 2023 13:13:16 -0600
Hi Mark,
On Sun, 10 Sept 2023 at 08:57, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Thu, 7 Sep 2023 09:57:34 -0600
Hi Mark,
On Thu, 7 Sept 2023 at 07:08, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Thu, 7 Sep 2023 06:23:10 -0600
Hi Mark,
On Sun, 3 Sept 2023 at 14:40, Mark Kettenis kettenis@openbsd.org wrote:
Enable the bootflow based on this bootmeth if the BootOrder EFI variable is set.
Signed-off-by: Mark Kettenis kettenis@openbsd.org
ChangeLog:
v2: - Initialize EFI subsystem to populate EFI variables
boot/bootmeth_efi_mgr.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c index e9d973429f..e6c42d41fb 100644 --- a/boot/bootmeth_efi_mgr.c +++ b/boot/bootmeth_efi_mgr.c @@ -14,6 +14,8 @@ #include <bootmeth.h> #include <command.h> #include <dm.h> +#include <efi_loader.h> +#include <efi_variable.h>
/**
- struct efi_mgr_priv - private info for the efi-mgr driver
@@ -46,13 +48,26 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter) static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow) { struct efi_mgr_priv *priv = dev_get_priv(dev);
efi_status_t ret;efi_uintn_t size;u16 *bootorder; if (priv->fake_dev) { bflow->state = BOOTFLOWST_READY; return 0; }
/* To be implemented */
ret = efi_init_obj_list();Doesn't this start up the whole EFI system? Is there a cheaper way to see if the variable subsystem exists, or can work?
A fair bit of it at least. With the possible exception of launching EFI capsules (which shouldn't happen for a "normal" boot), I don't think this is very expensive though.
There currently isn't a way to only initialize the variable subsystem; we can't just call efi_init_variables() here since we have to also call efi_disks_register() and a later efi_init_obj_list() call would call efi_init_variables() again, which would leak memory.
Alternatively we could just unconditionally declare the efi_mgr bootmeth as "ready" (like you already do for sandbox). That would postpone initialization of the EFI subsystem until we actually execute this bootmeth.
But we need to do something before we switch more targets to standard boot since otherwise booting through the EFI boot manager is just plain broken.
Yes...
I don't have any great ideas on this. If we could get the EFI code better integrated into U-Boot then perhaps we would not need this 'monolithm' init?
You'll have to take that up with the maintainers of the EFI loader subsystem. I just want to repair the breakage that was introduced with bootstd.
Yes, understood. Do you know how long it takes to do this step?
It is instant. But then this is on a fast machine with the EFI variables stored on the internal NVMe disk, which is plenty fast as well. And I haven't enabled any of the capsule or TPM stuff hich may take more time.
The control for this feature is CMD_BOOTEFI_BOOTMGR...I think it would be better to have BOOTEFI_BOOTMGR as well as CMD_BOOTEFI_BOOTMGR, since it is strange to have a CMD Kconfig in lib/efi_loader
I am OK with this patch as I don't see any alternative. We need to do something and we can always improve it if it causes problems.
Heinrich?
Regards, Simon
On Tue, 12 Sept 2023 at 14:06, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Sun, 10 Sep 2023 13:13:16 -0600
Hi Mark,
On Sun, 10 Sept 2023 at 08:57, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Thu, 7 Sep 2023 09:57:34 -0600
Hi Mark,
On Thu, 7 Sept 2023 at 07:08, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Thu, 7 Sep 2023 06:23:10 -0600
Hi Mark,
On Sun, 3 Sept 2023 at 14:40, Mark Kettenis kettenis@openbsd.org wrote: > > Enable the bootflow based on this bootmeth if the BootOrder EFI > variable is set. > > Signed-off-by: Mark Kettenis kettenis@openbsd.org > --- > > ChangeLog: > > v2: - Initialize EFI subsystem to populate EFI variables > > > boot/bootmeth_efi_mgr.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/boot/bootmeth_efi_mgr.c b/boot/bootmeth_efi_mgr.c > index e9d973429f..e6c42d41fb 100644 > --- a/boot/bootmeth_efi_mgr.c > +++ b/boot/bootmeth_efi_mgr.c > @@ -14,6 +14,8 @@ > #include <bootmeth.h> > #include <command.h> > #include <dm.h> > +#include <efi_loader.h> > +#include <efi_variable.h> > > /** > * struct efi_mgr_priv - private info for the efi-mgr driver > @@ -46,13 +48,26 @@ static int efi_mgr_check(struct udevice *dev, struct bootflow_iter *iter) > static int efi_mgr_read_bootflow(struct udevice *dev, struct bootflow *bflow) > { > struct efi_mgr_priv *priv = dev_get_priv(dev); > + efi_status_t ret; > + efi_uintn_t size; > + u16 *bootorder; > > if (priv->fake_dev) { > bflow->state = BOOTFLOWST_READY; > return 0; > } > > - /* To be implemented */ > + ret = efi_init_obj_list();
Doesn't this start up the whole EFI system? Is there a cheaper way to see if the variable subsystem exists, or can work?
A fair bit of it at least. With the possible exception of launching EFI capsules (which shouldn't happen for a "normal" boot), I don't think this is very expensive though.
There currently isn't a way to only initialize the variable subsystem; we can't just call efi_init_variables() here since we have to also call efi_disks_register() and a later efi_init_obj_list() call would call efi_init_variables() again, which would leak memory.
Alternatively we could just unconditionally declare the efi_mgr bootmeth as "ready" (like you already do for sandbox). That would postpone initialization of the EFI subsystem until we actually execute this bootmeth.
But we need to do something before we switch more targets to standard boot since otherwise booting through the EFI boot manager is just plain broken.
Yes...
I don't have any great ideas on this. If we could get the EFI code better integrated into U-Boot then perhaps we would not need this 'monolithm' init?
You'll have to take that up with the maintainers of the EFI loader subsystem. I just want to repair the breakage that was introduced with bootstd.
Yes, understood. Do you know how long it takes to do this step?
It is instant. But then this is on a fast machine with the EFI variables stored on the internal NVMe disk, which is plenty fast as well. And I haven't enabled any of the capsule or TPM stuff hich may take more time.
The control for this feature is CMD_BOOTEFI_BOOTMGR...I think it would be better to have BOOTEFI_BOOTMGR as well as CMD_BOOTEFI_BOOTMGR, since it is strange to have a CMD Kconfig in lib/efi_loader
I am OK with this patch as I don't see any alternative. We need to do something and we can always improve it if it causes problems.
Heinrich?
Reviewed-by: Simon Glass sjg@chromium.org
On Sun, Sep 03, 2023 at 10:40:00PM +0200, Mark Kettenis wrote:
Enable the bootflow based on this bootmeth if the BootOrder EFI variable is set.
Signed-off-by: Mark Kettenis kettenis@openbsd.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
participants (4)
-
Mark Kettenis -
Mark Kettenis -
Simon Glass -
Tom Rini