[U-Boot] [PATCH v2 0/5] efi_loader: run a specific efi application more easily
This patch is a result from re-organizing my previous patches; a combination of [1] and part of [2] so as to solely provide several ways of executing a specific efi application explicitly. * bootmanager via BootNext variable * bootefi with boot id * run -e with BootXXXX variable
Changes in v2 (Jan 15, 2019) * not specify any attributes when deleting BootNext variable in efi_bootmgr_load() * introduce EFI_BOOTMGR_DEFAULT_ORDER macro on behalf of BootOrder for use in efi_bootmgr_load() * simplify the code around efi_handle_fdt() in do_bootefi() * add do_bootefi_run() and call it at "run -e" so that we don't have to export internal efi helper functions
[1] https://lists.denx.de/pipermail/u-boot/2018-November/349281.html [2] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
AKASHI Takahiro (5): efi_loader: bootmgr: support BootNext and BootCurrent variable behavior efi_loader: bootmgr: allow for running a given load option cmd: bootefi: carve out fdt parameter handling cmd: bootefi: run an EFI application of a specific load option cmd: run: add "-e" option to run an EFI application
cmd/bootefi.c | 114 +++++++++++++++++++++++++++-------- cmd/nvedit.c | 9 ++- common/cli.c | 10 +++ include/command.h | 3 + include/efi_loader.h | 5 +- lib/efi_loader/efi_bootmgr.c | 43 ++++++++++++- 6 files changed, 156 insertions(+), 28 deletions(-)
See UEFI v2.7, section 3.1.2 for details of the specification.
With my efitool command, you can try as the following: => efi boot add 1 SHELL ... => efi boot add 2 HELLO ... => efi boot order 1 2 => efi bootmgr (starting SHELL ...) => efi boot next 2 => efi bootmgr (starting HELLO ...) => efi dumpvar <snip ...> BootCurrent: {boot,run}(blob) 00000000: 02 00 .. BootOrder: {boot,run}(blob) 00000000: 01 00 02 00 ....
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index a095df3f540b..6c5303736dc6 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, efi_deserialize_load_option(&lo, load_option);
if (lo.attributes & LOAD_OPTION_ACTIVE) { + u32 attributes; efi_status_t ret;
debug("%s: trying to load "%ls" from %pD\n", __func__, lo.label, lo.file_path);
+ attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS; + size = sizeof(n); + ret = rs->set_variable(L"BootCurrent", + (efi_guid_t *)&efi_global_variable_guid, + attributes, size, &n); + if (ret != EFI_SUCCESS) + goto error; + ret = efi_load_image_from_path(lo.file_path, &image);
if (ret != EFI_SUCCESS) @@ -173,16 +183,38 @@ error: void *efi_bootmgr_load(struct efi_device_path **device_path, struct efi_device_path **file_path) { - uint16_t *bootorder; + u16 bootnext, *bootorder; efi_uintn_t size; void *image = NULL; int i, num; + efi_status_t ret;
__efi_entry_check();
bs = systab.boottime; rs = systab.runtime;
+ /* get BootNext */ + size = sizeof(bootnext); + ret = rs->get_variable(L"BootNext", + (efi_guid_t *)&efi_global_variable_guid, + NULL, &size, &bootnext); + if (!bootnext) + goto run_list; + + /* delete BootNext */ + ret = rs->set_variable(L"BootNext", + (efi_guid_t *)&efi_global_variable_guid, + 0, 0, &bootnext); + if (ret != EFI_SUCCESS) + goto error; + + image = try_load_entry(bootnext, device_path, file_path); + if (image) + goto error; + +run_list: + /* BootOrder */ bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size); if (!bootorder) goto error;
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
See UEFI v2.7, section 3.1.2 for details of the specification.
With my efitool command, you can try as the following: => efi boot add 1 SHELL ... => efi boot add 2 HELLO ... => efi boot order 1 2 => efi bootmgr (starting SHELL ...) => efi boot next 2 => efi bootmgr (starting HELLO ...) => efi dumpvar <snip ...> BootCurrent: {boot,run}(blob) 00000000: 02 00 .. BootOrder: {boot,run}(blob) 00000000: 01 00 02 00 ....
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index a095df3f540b..6c5303736dc6 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, efi_deserialize_load_option(&lo, load_option);
if (lo.attributes & LOAD_OPTION_ACTIVE) {
u32 attributes;efi_status_t ret;
debug("%s: trying to load "%ls" from %pD\n", __func__, lo.label, lo.file_path);
attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS;size = sizeof(n);ret = rs->set_variable(L"BootCurrent",(efi_guid_t *)&efi_global_variable_guid,
Use EFI_CALL(). Instead of dereferencing you could directly call efi_set_variable().
attributes, size, &n);if (ret != EFI_SUCCESS)goto error;ret = efi_load_image_from_path(lo.file_path, &image);
if (ret != EFI_SUCCESS)
@@ -173,16 +183,38 @@ error: void *efi_bootmgr_load(struct efi_device_path **device_path, struct efi_device_path **file_path) {
- uint16_t *bootorder;
u16 bootnext, *bootorder; efi_uintn_t size; void *image = NULL; int i, num;
efi_status_t ret;
__efi_entry_check();
bs = systab.boottime; rs = systab.runtime;
/* get BootNext */
size = sizeof(bootnext);
ret = rs->get_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,NULL, &size, &bootnext);
You could call efi_get_variable() directly instead of dereferencing rs. But anyway you have to use EFI_CALL().
- if (!bootnext)
goto run_list;
Goto is acceptable for error handling. But otherwise I would rather avoid it.
- /* delete BootNext */
- ret = rs->set_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,0, 0, &bootnext);
EFI_CALL().
Best regards
Heinrich
- if (ret != EFI_SUCCESS)
goto error;- image = try_load_entry(bootnext, device_path, file_path);
- if (image)
goto error;+run_list:
- /* BootOrder */ bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size); if (!bootorder) goto error;
On Tue, Feb 26, 2019 at 07:57:26PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
See UEFI v2.7, section 3.1.2 for details of the specification.
With my efitool command, you can try as the following: => efi boot add 1 SHELL ... => efi boot add 2 HELLO ... => efi boot order 1 2 => efi bootmgr (starting SHELL ...) => efi boot next 2 => efi bootmgr (starting HELLO ...) => efi dumpvar <snip ...> BootCurrent: {boot,run}(blob) 00000000: 02 00 .. BootOrder: {boot,run}(blob) 00000000: 01 00 02 00 ....
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index a095df3f540b..6c5303736dc6 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, efi_deserialize_load_option(&lo, load_option);
if (lo.attributes & LOAD_OPTION_ACTIVE) {
u32 attributes;efi_status_t ret;
debug("%s: trying to load "%ls" from %pD\n", __func__, lo.label, lo.file_path);
attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS;size = sizeof(n);ret = rs->set_variable(L"BootCurrent",(efi_guid_t *)&efi_global_variable_guid,Use EFI_CALL().
Okay But as I said somewhere else, it's quite annoying to me that some efi_xxx requires EFI_CALL(), and others not. There should have been consistent naming rules.
Instead of dereferencing you could directly call efi_set_variable().
Yeah, given that this code is under lib/efi_loader, it may be natural to use efi_set_variable(). But existing get_var() uses the same style of coding.
Do you want to change all of the call sites including get_var()?
attributes, size, &n);if (ret != EFI_SUCCESS)goto error;ret = efi_load_image_from_path(lo.file_path, &image);
if (ret != EFI_SUCCESS)
@@ -173,16 +183,38 @@ error: void *efi_bootmgr_load(struct efi_device_path **device_path, struct efi_device_path **file_path) {
- uint16_t *bootorder;
u16 bootnext, *bootorder; efi_uintn_t size; void *image = NULL; int i, num;
efi_status_t ret;
__efi_entry_check();
bs = systab.boottime; rs = systab.runtime;
/* get BootNext */
size = sizeof(bootnext);
ret = rs->get_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,NULL, &size, &bootnext);You could call efi_get_variable() directly instead of dereferencing rs. But anyway you have to use EFI_CALL().
Ditto
- if (!bootnext)
goto run_list;Goto is acceptable for error handling. But otherwise I would rather avoid it.
Okay with another indentation.
- /* delete BootNext */
- ret = rs->set_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,0, 0, &bootnext);EFI_CALL().
Thanks, -Takahiro Akashi
Best regards
Heinrich
- if (ret != EFI_SUCCESS)
goto error;- image = try_load_entry(bootnext, device_path, file_path);
- if (image)
goto error;+run_list:
- /* BootOrder */ bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size); if (!bootorder) goto error;
On 2/27/19 6:47 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 07:57:26PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
See UEFI v2.7, section 3.1.2 for details of the specification.
With my efitool command, you can try as the following: => efi boot add 1 SHELL ... => efi boot add 2 HELLO ... => efi boot order 1 2 => efi bootmgr (starting SHELL ...) => efi boot next 2 => efi bootmgr (starting HELLO ...) => efi dumpvar <snip ...> BootCurrent: {boot,run}(blob) 00000000: 02 00 .. BootOrder: {boot,run}(blob) 00000000: 01 00 02 00 ....
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index a095df3f540b..6c5303736dc6 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, efi_deserialize_load_option(&lo, load_option);
if (lo.attributes & LOAD_OPTION_ACTIVE) {
u32 attributes;efi_status_t ret;
debug("%s: trying to load "%ls" from %pD\n", __func__, lo.label, lo.file_path);
attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS;size = sizeof(n);ret = rs->set_variable(L"BootCurrent",(efi_guid_t *)&efi_global_variable_guid,Use EFI_CALL().
Okay But as I said somewhere else, it's quite annoying to me that some efi_xxx requires EFI_CALL(), and others not. There should have been consistent naming rules.
We started with having separate functions like efi_allocate_pages_ext() and efi_allocate_pages(). Then Rob Clark came along and introduced EFI_CALL() in a095aadffa96 and I stopped creating _ext() functions.
When running with DEBUG 1 it sometimes is helpful to see which function is calling which other and where errors are originally reported.
But I am open to changes in this area.
Instead of dereferencing you could directly call efi_set_variable().
Yeah, given that this code is under lib/efi_loader, it may be natural to use efi_set_variable(). But existing get_var() uses the same style of coding.
Do you want to change all of the call sites including get_var()?
Calling efi_set_variable() directly uses less bytes of code than rs->get_variable() which makes it preferable.
I have seen that iPXE modifies system->boottime to intercept system calls. The same could be done by an EFI driver to the runtime vectors.
In the light of your work on secure boot I think we should not allow an EFI driver to intercept the reading and changing of variables here.
We should also rethink it for efidebug.c
Best regards
Heinrich
attributes, size, &n);if (ret != EFI_SUCCESS)goto error;ret = efi_load_image_from_path(lo.file_path, &image);
if (ret != EFI_SUCCESS)
@@ -173,16 +183,38 @@ error: void *efi_bootmgr_load(struct efi_device_path **device_path, struct efi_device_path **file_path) {
- uint16_t *bootorder;
u16 bootnext, *bootorder; efi_uintn_t size; void *image = NULL; int i, num;
efi_status_t ret;
__efi_entry_check();
bs = systab.boottime; rs = systab.runtime;
/* get BootNext */
size = sizeof(bootnext);
ret = rs->get_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,NULL, &size, &bootnext);You could call efi_get_variable() directly instead of dereferencing rs. But anyway you have to use EFI_CALL().
Ditto
- if (!bootnext)
goto run_list;Goto is acceptable for error handling. But otherwise I would rather avoid it.
Okay with another indentation.
- /* delete BootNext */
- ret = rs->set_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,0, 0, &bootnext);EFI_CALL().
Thanks, -Takahiro Akashi
Best regards
Heinrich
- if (ret != EFI_SUCCESS)
goto error;- image = try_load_entry(bootnext, device_path, file_path);
- if (image)
goto error;+run_list:
- /* BootOrder */ bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size); if (!bootorder) goto error;
On Wed, Feb 27, 2019 at 07:14:10AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 6:47 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 07:57:26PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
See UEFI v2.7, section 3.1.2 for details of the specification.
With my efitool command, you can try as the following: => efi boot add 1 SHELL ... => efi boot add 2 HELLO ... => efi boot order 1 2 => efi bootmgr (starting SHELL ...) => efi boot next 2 => efi bootmgr (starting HELLO ...) => efi dumpvar <snip ...> BootCurrent: {boot,run}(blob) 00000000: 02 00 .. BootOrder: {boot,run}(blob) 00000000: 01 00 02 00 ....
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index a095df3f540b..6c5303736dc6 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, efi_deserialize_load_option(&lo, load_option);
if (lo.attributes & LOAD_OPTION_ACTIVE) {
u32 attributes;efi_status_t ret;
debug("%s: trying to load "%ls" from %pD\n", __func__, lo.label, lo.file_path);
attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS;size = sizeof(n);ret = rs->set_variable(L"BootCurrent",(efi_guid_t *)&efi_global_variable_guid,Use EFI_CALL().
Okay But as I said somewhere else, it's quite annoying to me that some efi_xxx requires EFI_CALL(), and others not. There should have been consistent naming rules.
We started with having separate functions like efi_allocate_pages_ext() and efi_allocate_pages(). Then Rob Clark came along and introduced EFI_CALL() in a095aadffa96 and I stopped creating _ext() functions.
When running with DEBUG 1 it sometimes is helpful to see which function is calling which other and where errors are originally reported.
But I am open to changes in this area.
Instead of dereferencing you could directly call efi_set_variable().
Yeah, given that this code is under lib/efi_loader, it may be natural to use efi_set_variable(). But existing get_var() uses the same style of coding.
Do you want to change all of the call sites including get_var()?
Calling efi_set_variable() directly uses less bytes of code than rs->get_variable() which makes it preferable.
So is your answer yes, or no?
I have seen that iPXE modifies system->boottime to intercept system calls. The same could be done by an EFI driver to the runtime vectors.
In the light of your work on secure boot I think we should not allow an EFI driver to intercept the reading and changing of variables here.
We should also rethink it for efidebug.c
I'm not sure about your concern here, but no doubt efidebug should be disabled on production system with secure boot.
Thanks, -Takahiro Akashi
Best regards
Heinrich
attributes, size, &n);if (ret != EFI_SUCCESS)goto error;ret = efi_load_image_from_path(lo.file_path, &image);
if (ret != EFI_SUCCESS)
@@ -173,16 +183,38 @@ error: void *efi_bootmgr_load(struct efi_device_path **device_path, struct efi_device_path **file_path) {
- uint16_t *bootorder;
u16 bootnext, *bootorder; efi_uintn_t size; void *image = NULL; int i, num;
efi_status_t ret;
__efi_entry_check();
bs = systab.boottime; rs = systab.runtime;
/* get BootNext */
size = sizeof(bootnext);
ret = rs->get_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,NULL, &size, &bootnext);You could call efi_get_variable() directly instead of dereferencing rs. But anyway you have to use EFI_CALL().
Ditto
- if (!bootnext)
goto run_list;Goto is acceptable for error handling. But otherwise I would rather avoid it.
Okay with another indentation.
- /* delete BootNext */
- ret = rs->set_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,0, 0, &bootnext);EFI_CALL().
Thanks, -Takahiro Akashi
Best regards
Heinrich
- if (ret != EFI_SUCCESS)
goto error;- image = try_load_entry(bootnext, device_path, file_path);
- if (image)
goto error;+run_list:
- /* BootOrder */ bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size); if (!bootorder) goto error;
On 2/27/19 7:27 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 07:14:10AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 6:47 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 07:57:26PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
See UEFI v2.7, section 3.1.2 for details of the specification.
With my efitool command, you can try as the following: => efi boot add 1 SHELL ... => efi boot add 2 HELLO ... => efi boot order 1 2 => efi bootmgr (starting SHELL ...) => efi boot next 2 => efi bootmgr (starting HELLO ...) => efi dumpvar <snip ...> BootCurrent: {boot,run}(blob) 00000000: 02 00 .. BootOrder: {boot,run}(blob) 00000000: 01 00 02 00 ....
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index a095df3f540b..6c5303736dc6 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, efi_deserialize_load_option(&lo, load_option);
if (lo.attributes & LOAD_OPTION_ACTIVE) {
u32 attributes;efi_status_t ret;
debug("%s: trying to load "%ls" from %pD\n", __func__, lo.label, lo.file_path);
attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS;size = sizeof(n);ret = rs->set_variable(L"BootCurrent",(efi_guid_t *)&efi_global_variable_guid,Use EFI_CALL().
Okay But as I said somewhere else, it's quite annoying to me that some efi_xxx requires EFI_CALL(), and others not. There should have been consistent naming rules.
We started with having separate functions like efi_allocate_pages_ext() and efi_allocate_pages(). Then Rob Clark came along and introduced EFI_CALL() in a095aadffa96 and I stopped creating _ext() functions.
When running with DEBUG 1 it sometimes is helpful to see which function is calling which other and where errors are originally reported.
But I am open to changes in this area.
Instead of dereferencing you could directly call efi_set_variable().
Yeah, given that this code is under lib/efi_loader, it may be natural to use efi_set_variable(). But existing get_var() uses the same style of coding.
Do you want to change all of the call sites including get_var()?
Calling efi_set_variable() directly uses less bytes of code than rs->get_variable() which makes it preferable.
So is your answer yes, or no?
I would prefer calling efi_get_variable() directly and not to use rs->get_variable().
I have seen that iPXE modifies system->boottime to intercept system calls. The same could be done by an EFI driver to the runtime vectors.
In the light of your work on secure boot I think we should not allow an EFI driver to intercept the reading and changing of variables here.
We should also rethink it for efidebug.c
I'm not sure about your concern here, but no doubt efidebug should be disabled on production system with secure boot.
Also in efidebug we are creating more runtime code bytes than needed by using system->runtime->efi_something() or system->boottime->efi_something().
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
attributes, size, &n);if (ret != EFI_SUCCESS)goto error;ret = efi_load_image_from_path(lo.file_path, &image);
if (ret != EFI_SUCCESS)
@@ -173,16 +183,38 @@ error: void *efi_bootmgr_load(struct efi_device_path **device_path, struct efi_device_path **file_path) {
- uint16_t *bootorder;
u16 bootnext, *bootorder; efi_uintn_t size; void *image = NULL; int i, num;
efi_status_t ret;
__efi_entry_check();
bs = systab.boottime; rs = systab.runtime;
/* get BootNext */
size = sizeof(bootnext);
ret = rs->get_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,NULL, &size, &bootnext);You could call efi_get_variable() directly instead of dereferencing rs. But anyway you have to use EFI_CALL().
Ditto
- if (!bootnext)
goto run_list;Goto is acceptable for error handling. But otherwise I would rather avoid it.
Okay with another indentation.
- /* delete BootNext */
- ret = rs->set_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,0, 0, &bootnext);EFI_CALL().
Thanks, -Takahiro Akashi
Best regards
Heinrich
- if (ret != EFI_SUCCESS)
goto error;- image = try_load_entry(bootnext, device_path, file_path);
- if (image)
goto error;+run_list:
- /* BootOrder */ bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size); if (!bootorder) goto error;
On Wed, Feb 27, 2019 at 07:39:50AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:27 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 07:14:10AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 6:47 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 07:57:26PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
See UEFI v2.7, section 3.1.2 for details of the specification.
With my efitool command, you can try as the following: => efi boot add 1 SHELL ... => efi boot add 2 HELLO ... => efi boot order 1 2 => efi bootmgr (starting SHELL ...) => efi boot next 2 => efi bootmgr (starting HELLO ...) => efi dumpvar <snip ...> BootCurrent: {boot,run}(blob) 00000000: 02 00 .. BootOrder: {boot,run}(blob) 00000000: 01 00 02 00 ....
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index a095df3f540b..6c5303736dc6 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, efi_deserialize_load_option(&lo, load_option);
if (lo.attributes & LOAD_OPTION_ACTIVE) {
u32 attributes;efi_status_t ret;
debug("%s: trying to load "%ls" from %pD\n", __func__, lo.label, lo.file_path);
attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |EFI_VARIABLE_RUNTIME_ACCESS;size = sizeof(n);ret = rs->set_variable(L"BootCurrent",(efi_guid_t *)&efi_global_variable_guid,Use EFI_CALL().
Okay But as I said somewhere else, it's quite annoying to me that some efi_xxx requires EFI_CALL(), and others not. There should have been consistent naming rules.
We started with having separate functions like efi_allocate_pages_ext() and efi_allocate_pages(). Then Rob Clark came along and introduced EFI_CALL() in a095aadffa96 and I stopped creating _ext() functions.
When running with DEBUG 1 it sometimes is helpful to see which function is calling which other and where errors are originally reported.
But I am open to changes in this area.
Instead of dereferencing you could directly call efi_set_variable().
Yeah, given that this code is under lib/efi_loader, it may be natural to use efi_set_variable(). But existing get_var() uses the same style of coding.
Do you want to change all of the call sites including get_var()?
Calling efi_set_variable() directly uses less bytes of code than rs->get_variable() which makes it preferable.
So is your answer yes, or no?
I would prefer calling efi_get_variable() directly and not to use rs->get_variable().
My point is "including get_var()" or not. I have never touched that function in my patch.
I have seen that iPXE modifies system->boottime to intercept system calls. The same could be done by an EFI driver to the runtime vectors.
In the light of your work on secure boot I think we should not allow an EFI driver to intercept the reading and changing of variables here.
We should also rethink it for efidebug.c
I'm not sure about your concern here, but no doubt efidebug should be disabled on production system with secure boot.
Also in efidebug we are creating more runtime code bytes than needed by using system->runtime->efi_something() or system->boottime->efi_something().
I think we discussed in the past. I prefer to calling boot time/run time services via system table as this command is expected to be implemented as an (embedded) EFI application sometime in the future.
Thanks, -Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
attributes, size, &n);if (ret != EFI_SUCCESS)goto error;ret = efi_load_image_from_path(lo.file_path, &image);
if (ret != EFI_SUCCESS)
@@ -173,16 +183,38 @@ error: void *efi_bootmgr_load(struct efi_device_path **device_path, struct efi_device_path **file_path) {
- uint16_t *bootorder;
u16 bootnext, *bootorder; efi_uintn_t size; void *image = NULL; int i, num;
efi_status_t ret;
__efi_entry_check();
bs = systab.boottime; rs = systab.runtime;
/* get BootNext */
size = sizeof(bootnext);
ret = rs->get_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,NULL, &size, &bootnext);You could call efi_get_variable() directly instead of dereferencing rs. But anyway you have to use EFI_CALL().
Ditto
- if (!bootnext)
goto run_list;Goto is acceptable for error handling. But otherwise I would rather avoid it.
Okay with another indentation.
- /* delete BootNext */
- ret = rs->set_variable(L"BootNext",
(efi_guid_t *)&efi_global_variable_guid,0, 0, &bootnext);EFI_CALL().
Thanks, -Takahiro Akashi
Best regards
Heinrich
- if (ret != EFI_SUCCESS)
goto error;- image = try_load_entry(bootnext, device_path, file_path);
- if (image)
goto error;+run_list:
- /* BootOrder */ bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size); if (!bootorder) goto error;
With an extra argument, efi_bootmgr_load() can now load an efi binary based on a "BootXXXX" variable specified.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- cmd/bootefi.c | 3 ++- include/efi_loader.h | 5 ++++- lib/efi_loader/efi_bootmgr.c | 9 ++++++++- 3 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 7012d72ab50d..76171ab679c3 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -452,7 +452,8 @@ static int do_bootefi_bootmgr_exec(void) void *addr; efi_status_t r;
- addr = efi_bootmgr_load(&device_path, &file_path); + addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER, + &device_path, &file_path); if (!addr) return 1;
diff --git a/include/efi_loader.h b/include/efi_loader.h index dd68cfce5c65..3077a1e9d58b 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -549,9 +549,12 @@ struct efi_load_option { u8 *optional_data; };
+#define EFI_BOOTMGR_DEFAULT_ORDER (-1) + void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data); unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data); -void *efi_bootmgr_load(struct efi_device_path **device_path, +void *efi_bootmgr_load(int boot_id, + struct efi_device_path **device_path, struct efi_device_path **file_path);
#else /* CONFIG_IS_ENABLED(EFI_LOADER) */ diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 6c5303736dc6..7ce51de4dfb0 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -180,7 +180,8 @@ error: * available load-options, finding and returning the first one that can * be loaded successfully. */ -void *efi_bootmgr_load(struct efi_device_path **device_path, +void *efi_bootmgr_load(int boot_id, + struct efi_device_path **device_path, struct efi_device_path **file_path) { u16 bootnext, *bootorder; @@ -194,6 +195,12 @@ void *efi_bootmgr_load(struct efi_device_path **device_path, bs = systab.boottime; rs = systab.runtime;
+ /* specified boot option */ + if (boot_id != -1) { + image = try_load_entry(boot_id, device_path, file_path); + goto error; + } + /* get BootNext */ size = sizeof(bootnext); ret = rs->get_variable(L"BootNext",
The current way how command parameters, particularly "fdt addr," are handled makes it a bit complicated to add a subcommand-specific parameter. So just refactor the code and extract efi_handle_fdt().
This commit is a preparatory change for enhancing bootmgr sub-command.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- cmd/bootefi.c | 49 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 16 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 76171ab679c3..3be01b49b589 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -279,6 +279,31 @@ static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj, efi_delete_handle(&image_obj->header); }
+static int efi_handle_fdt(char *fdt_opt) +{ + unsigned long fdt_addr; + efi_status_t r; + + if (fdt_opt) { + fdt_addr = simple_strtoul(fdt_opt, NULL, 16); + if (!fdt_addr && *fdt_opt != '0') + return CMD_RET_USAGE; + + /* Install device tree */ + r = efi_install_fdt(fdt_addr); + if (r != EFI_SUCCESS) { + printf("ERROR: failed to install device tree\n"); + return CMD_RET_FAILURE; + } + } else { + /* Remove device tree. EFI_NOT_FOUND can be ignored here */ + efi_install_configuration_table(&efi_guid_fdt, NULL); + printf("WARNING: booting without device tree\n"); + } + + return CMD_RET_SUCCESS; +} + /** * do_bootefi_exec() - execute EFI binary * @@ -474,7 +499,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) unsigned long addr; char *saddr; efi_status_t r; - unsigned long fdt_addr;
/* Allow unaligned memory access */ allow_unaligned(); @@ -490,21 +514,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
- if (argc > 2) { - fdt_addr = simple_strtoul(argv[2], NULL, 16); - if (!fdt_addr && *argv[2] != '0') - return CMD_RET_USAGE; - /* Install device tree */ - r = efi_install_fdt(fdt_addr); - if (r != EFI_SUCCESS) { - printf("ERROR: failed to install device tree\n"); - return CMD_RET_FAILURE; - } - } else { - /* Remove device tree. EFI_NOT_FOUND can be ignored here */ - efi_install_configuration_table(&efi_guid_fdt, NULL); - printf("WARNING: booting without device tree\n"); - } #ifdef CONFIG_CMD_BOOTEFI_HELLO if (!strcmp(argv[1], "hello")) { ulong size = __efi_helloworld_end - __efi_helloworld_begin; @@ -522,6 +531,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) struct efi_loaded_image_obj *image_obj; struct efi_loaded_image *loaded_image_info;
+ if (efi_handle_fdt(argc > 2 ? argv[2] : NULL)) + return CMD_RET_FAILURE; + if (bootefi_test_prepare(&image_obj, &loaded_image_info, "\selftest", (uintptr_t)&efi_selftest, "efi_selftest")) @@ -534,6 +546,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } else #endif if (!strcmp(argv[1], "bootmgr")) { + if (efi_handle_fdt(argc > 2 ? argv[2] : NULL)) + return CMD_RET_FAILURE; + return do_bootefi_bootmgr_exec(); } else { saddr = argv[1]; @@ -543,6 +558,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (!addr && *saddr != '0') return CMD_RET_USAGE;
+ if (efi_handle_fdt(argc > 2 ? argv[2] : NULL)) + return CMD_RET_FAILURE; }
printf("## Starting EFI application at %08lx ...\n", addr);
With this patch applied, we will be able to selectively execute an EFI application by specifying a load option, say "1" for Boot0001, "2" for Boot0002 and so on.
=> bootefi bootmgr <fdt addr> 1, or bootefi bootmgr - 1
Please note that BootXXXX need not be included in "BootOrder".
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3be01b49b589..241fd0f987ab 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare
#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
-static int do_bootefi_bootmgr_exec(void) +static int do_bootefi_bootmgr_exec(int boot_id) { struct efi_device_path *device_path, *file_path; void *addr; efi_status_t r;
- addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER, - &device_path, &file_path); + addr = efi_bootmgr_load(boot_id, &device_path, &file_path); if (!addr) - return 1; + return CMD_RET_FAILURE;
printf("## Starting EFI application at %p ...\n", addr); r = do_bootefi_exec(addr, device_path, file_path); @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) r & ~EFI_ERROR_MASK);
if (r != EFI_SUCCESS) - return 1; + return CMD_RET_FAILURE;
- return 0; + return CMD_RET_SUCCESS; }
/* Interpreter command to boot an arbitrary EFI image from memory */ @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } else #endif if (!strcmp(argv[1], "bootmgr")) { - if (efi_handle_fdt(argc > 2 ? argv[2] : NULL)) - return CMD_RET_FAILURE; + char *fdtstr, *endp; + int boot_id = EFI_BOOTMGR_DEFAULT_ORDER; + + if (argc > 2) { + fdtstr = argv[2]; + /* Special address "-" means no device tree */ + if (fdtstr[0] == '-') + fdtstr = NULL; + + r = efi_handle_fdt(fdtstr); + if (r) + return CMD_RET_FAILURE; + } + + if (argc > 3) { + boot_id = (int)simple_strtoul(argv[3], &endp, 0); + if ((argv[3] + strlen(argv[3]) != endp) || + boot_id > 0xffff) + return CMD_RET_USAGE; + }
- return do_bootefi_bootmgr_exec(); + return do_bootefi_bootmgr_exec(boot_id); } else { saddr = argv[1];
@@ -590,7 +607,7 @@ static char bootefi_help_text[] = " Use environment variable efi_selftest to select a single test.\n" " Use 'setenv efi_selftest list' to enumerate all tests.\n" #endif - "bootefi bootmgr [fdt addr]\n" + "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" "\n" " If specified, the device tree located at <fdt address> gets\n" @@ -598,7 +615,7 @@ static char bootefi_help_text[] = #endif
U_BOOT_CMD( - bootefi, 3, 0, do_bootefi, + bootefi, 5, 0, do_bootefi, "Boots an EFI payload from memory", bootefi_help_text );
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
With this patch applied, we will be able to selectively execute an EFI application by specifying a load option, say "1" for Boot0001, "2" for Boot0002 and so on.
=> bootefi bootmgr <fdt addr> 1, or bootefi bootmgr - 1
You already introduced the support for BootNext. So is there a real benefit?
Please note that BootXXXX need not be included in "BootOrder".
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3be01b49b589..241fd0f987ab 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare
#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
-static int do_bootefi_bootmgr_exec(void) +static int do_bootefi_bootmgr_exec(int boot_id) { struct efi_device_path *device_path, *file_path; void *addr; efi_status_t r;
- addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER,
&device_path, &file_path);
- addr = efi_bootmgr_load(boot_id, &device_path, &file_path); if (!addr)
return 1;
return CMD_RET_FAILURE;printf("## Starting EFI application at %p ...\n", addr); r = do_bootefi_exec(addr, device_path, file_path);
@@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) r & ~EFI_ERROR_MASK);
if (r != EFI_SUCCESS)
return 1;
return CMD_RET_FAILURE;
- return 0;
- return CMD_RET_SUCCESS;
}
/* Interpreter command to boot an arbitrary EFI image from memory */ @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } else #endif if (!strcmp(argv[1], "bootmgr")) {
if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))return CMD_RET_FAILURE;
char *fdtstr, *endp;int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;if (argc > 2) {fdtstr = argv[2];/* Special address "-" means no device tree */if (fdtstr[0] == '-')fdtstr = NULL;r = efi_handle_fdt(fdtstr);if (r)return CMD_RET_FAILURE;}if (argc > 3) {boot_id = (int)simple_strtoul(argv[3], &endp, 0);if ((argv[3] + strlen(argv[3]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;}
return do_bootefi_bootmgr_exec();
return do_bootefi_bootmgr_exec(boot_id);
Why not communicate via the BootNext variable?
} else { saddr = argv[1];
@@ -590,7 +607,7 @@ static char bootefi_help_text[] = " Use environment variable efi_selftest to select a single test.\n" " Use 'setenv efi_selftest list' to enumerate all tests.\n" #endif
- "bootefi bootmgr [fdt addr]\n"
- "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" "\n" " If specified, the device tree located at <fdt address> gets\n"
@@ -598,7 +615,7 @@ static char bootefi_help_text[] = #endif
U_BOOT_CMD(
- bootefi, 3, 0, do_bootefi,
- bootefi, 5, 0, do_bootefi,
Why 5?
Best regards
Heinrich
"Boots an EFI payload from memory", bootefi_help_text );
On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
With this patch applied, we will be able to selectively execute an EFI application by specifying a load option, say "1" for Boot0001, "2" for Boot0002 and so on.
=> bootefi bootmgr <fdt addr> 1, or bootefi bootmgr - 1
You already introduced the support for BootNext. So is there a real benefit?
This is a convenient way of running EFI application directly, but I already removed this feature from the next version.
Please note that BootXXXX need not be included in "BootOrder".
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3be01b49b589..241fd0f987ab 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare
#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
-static int do_bootefi_bootmgr_exec(void) +static int do_bootefi_bootmgr_exec(int boot_id) { struct efi_device_path *device_path, *file_path; void *addr; efi_status_t r;
- addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER,
&device_path, &file_path);
- addr = efi_bootmgr_load(boot_id, &device_path, &file_path); if (!addr)
return 1;
return CMD_RET_FAILURE;printf("## Starting EFI application at %p ...\n", addr); r = do_bootefi_exec(addr, device_path, file_path);
@@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) r & ~EFI_ERROR_MASK);
if (r != EFI_SUCCESS)
return 1;
return CMD_RET_FAILURE;
- return 0;
- return CMD_RET_SUCCESS;
}
/* Interpreter command to boot an arbitrary EFI image from memory */ @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } else #endif if (!strcmp(argv[1], "bootmgr")) {
if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))return CMD_RET_FAILURE;
char *fdtstr, *endp;int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;if (argc > 2) {fdtstr = argv[2];/* Special address "-" means no device tree */if (fdtstr[0] == '-')fdtstr = NULL;r = efi_handle_fdt(fdtstr);if (r)return CMD_RET_FAILURE;}if (argc > 3) {boot_id = (int)simple_strtoul(argv[3], &endp, 0);if ((argv[3] + strlen(argv[3]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;}
return do_bootefi_bootmgr_exec();
return do_bootefi_bootmgr_exec(boot_id);Why not communicate via the BootNext variable?
I don't get your point. BootNext and BootOrder are both defined by UEFI specification.
} else { saddr = argv[1];
@@ -590,7 +607,7 @@ static char bootefi_help_text[] = " Use environment variable efi_selftest to select a single test.\n" " Use 'setenv efi_selftest list' to enumerate all tests.\n" #endif
- "bootefi bootmgr [fdt addr]\n"
- "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" "\n" " If specified, the device tree located at <fdt address> gets\n"
@@ -598,7 +615,7 @@ static char bootefi_help_text[] = #endif
U_BOOT_CMD(
- bootefi, 3, 0, do_bootefi,
- bootefi, 5, 0, do_bootefi,
Why 5?
For additional/optional '-' and <boot id>. But I removed this feature from bootefi.
Thanks, -Takahiro Akashi
Best regards
Heinrich
"Boots an EFI payload from memory", bootefi_help_text );
On 2/27/19 6:58 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
With this patch applied, we will be able to selectively execute an EFI application by specifying a load option, say "1" for Boot0001, "2" for Boot0002 and so on.
=> bootefi bootmgr <fdt addr> 1, or bootefi bootmgr - 1
You already introduced the support for BootNext. So is there a real benefit?
This is a convenient way of running EFI application directly, but I already removed this feature from the next version.
Please, remove 'run -e' instead because it cannot specify the device tree needed for booting ARM boards.
Please note that BootXXXX need not be included in "BootOrder".
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3be01b49b589..241fd0f987ab 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare
#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
-static int do_bootefi_bootmgr_exec(void) +static int do_bootefi_bootmgr_exec(int boot_id) { struct efi_device_path *device_path, *file_path; void *addr; efi_status_t r;
- addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER,
&device_path, &file_path);
- addr = efi_bootmgr_load(boot_id, &device_path, &file_path); if (!addr)
return 1;
return CMD_RET_FAILURE;printf("## Starting EFI application at %p ...\n", addr); r = do_bootefi_exec(addr, device_path, file_path);
@@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) r & ~EFI_ERROR_MASK);
if (r != EFI_SUCCESS)
return 1;
return CMD_RET_FAILURE;
- return 0;
- return CMD_RET_SUCCESS;
}
/* Interpreter command to boot an arbitrary EFI image from memory */ @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } else #endif if (!strcmp(argv[1], "bootmgr")) {
if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))return CMD_RET_FAILURE;
char *fdtstr, *endp;int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;if (argc > 2) {fdtstr = argv[2];/* Special address "-" means no device tree */if (fdtstr[0] == '-')fdtstr = NULL;r = efi_handle_fdt(fdtstr);if (r)return CMD_RET_FAILURE;}if (argc > 3) {boot_id = (int)simple_strtoul(argv[3], &endp, 0);if ((argv[3] + strlen(argv[3]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;}
return do_bootefi_bootmgr_exec();
return do_bootefi_bootmgr_exec(boot_id);Why not communicate via the BootNext variable?
I don't get your point. BootNext and BootOrder are both defined by UEFI specification.
Instead of changing the interface of do_bootefi_bootmgr_exec() you could simply set BootNext. Then the boot manager would pick up the option from the variable and finally delete the variable. This would result in less code.
Best regards
Heinrich
} else { saddr = argv[1];
@@ -590,7 +607,7 @@ static char bootefi_help_text[] = " Use environment variable efi_selftest to select a single test.\n" " Use 'setenv efi_selftest list' to enumerate all tests.\n" #endif
- "bootefi bootmgr [fdt addr]\n"
- "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" "\n" " If specified, the device tree located at <fdt address> gets\n"
@@ -598,7 +615,7 @@ static char bootefi_help_text[] = #endif
U_BOOT_CMD(
- bootefi, 3, 0, do_bootefi,
- bootefi, 5, 0, do_bootefi,
Why 5?
For additional/optional '-' and <boot id>. But I removed this feature from bootefi.
Thanks, -Takahiro Akashi
Best regards
Heinrich
"Boots an EFI payload from memory", bootefi_help_text );
On Wed, Feb 27, 2019 at 07:31:06AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 6:58 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
With this patch applied, we will be able to selectively execute an EFI application by specifying a load option, say "1" for Boot0001, "2" for Boot0002 and so on.
=> bootefi bootmgr <fdt addr> 1, or bootefi bootmgr - 1
You already introduced the support for BootNext. So is there a real benefit?
This is a convenient way of running EFI application directly, but I already removed this feature from the next version.
Please, remove 'run -e' instead because it cannot specify the device tree needed for booting ARM boards.
See my comment for patch#5 first.
Please note that BootXXXX need not be included in "BootOrder".
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3be01b49b589..241fd0f987ab 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare
#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
-static int do_bootefi_bootmgr_exec(void) +static int do_bootefi_bootmgr_exec(int boot_id) { struct efi_device_path *device_path, *file_path; void *addr; efi_status_t r;
- addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER,
&device_path, &file_path);
- addr = efi_bootmgr_load(boot_id, &device_path, &file_path); if (!addr)
return 1;
return CMD_RET_FAILURE;printf("## Starting EFI application at %p ...\n", addr); r = do_bootefi_exec(addr, device_path, file_path);
@@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) r & ~EFI_ERROR_MASK);
if (r != EFI_SUCCESS)
return 1;
return CMD_RET_FAILURE;
- return 0;
- return CMD_RET_SUCCESS;
}
/* Interpreter command to boot an arbitrary EFI image from memory */ @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } else #endif if (!strcmp(argv[1], "bootmgr")) {
if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))return CMD_RET_FAILURE;
char *fdtstr, *endp;int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;if (argc > 2) {fdtstr = argv[2];/* Special address "-" means no device tree */if (fdtstr[0] == '-')fdtstr = NULL;r = efi_handle_fdt(fdtstr);if (r)return CMD_RET_FAILURE;}if (argc > 3) {boot_id = (int)simple_strtoul(argv[3], &endp, 0);if ((argv[3] + strlen(argv[3]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;}
return do_bootefi_bootmgr_exec();
return do_bootefi_bootmgr_exec(boot_id);Why not communicate via the BootNext variable?
I don't get your point. BootNext and BootOrder are both defined by UEFI specification.
Instead of changing the interface of do_bootefi_bootmgr_exec()
Who care changing an *internal* function?
you could simply set BootNext. Then the boot manager would pick up the option from the variable and finally delete the variable. This would result in less code.
No. Even with "run -e," BootNext will disappear after execution. This is a requirement by UEFI spec.
Thanks, -Takahiro Akashi
Best regards
Heinrich
} else { saddr = argv[1];
@@ -590,7 +607,7 @@ static char bootefi_help_text[] = " Use environment variable efi_selftest to select a single test.\n" " Use 'setenv efi_selftest list' to enumerate all tests.\n" #endif
- "bootefi bootmgr [fdt addr]\n"
- "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" "\n" " If specified, the device tree located at <fdt address> gets\n"
@@ -598,7 +615,7 @@ static char bootefi_help_text[] = #endif
U_BOOT_CMD(
- bootefi, 3, 0, do_bootefi,
- bootefi, 5, 0, do_bootefi,
Why 5?
For additional/optional '-' and <boot id>. But I removed this feature from bootefi.
Thanks, -Takahiro Akashi
Best regards
Heinrich
"Boots an EFI payload from memory", bootefi_help_text );
On 2/27/19 7:47 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 07:31:06AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 6:58 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
With this patch applied, we will be able to selectively execute an EFI application by specifying a load option, say "1" for Boot0001, "2" for Boot0002 and so on.
=> bootefi bootmgr <fdt addr> 1, or bootefi bootmgr - 1
You already introduced the support for BootNext. So is there a real benefit?
This is a convenient way of running EFI application directly, but I already removed this feature from the next version.
Please, remove 'run -e' instead because it cannot specify the device tree needed for booting ARM boards.
See my comment for patch#5 first.
Please note that BootXXXX need not be included in "BootOrder".
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3be01b49b589..241fd0f987ab 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare
#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
-static int do_bootefi_bootmgr_exec(void) +static int do_bootefi_bootmgr_exec(int boot_id) { struct efi_device_path *device_path, *file_path; void *addr; efi_status_t r;
- addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER,
&device_path, &file_path);
- addr = efi_bootmgr_load(boot_id, &device_path, &file_path); if (!addr)
return 1;
return CMD_RET_FAILURE;printf("## Starting EFI application at %p ...\n", addr); r = do_bootefi_exec(addr, device_path, file_path);
@@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) r & ~EFI_ERROR_MASK);
if (r != EFI_SUCCESS)
return 1;
return CMD_RET_FAILURE;
- return 0;
- return CMD_RET_SUCCESS;
}
/* Interpreter command to boot an arbitrary EFI image from memory */ @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } else #endif if (!strcmp(argv[1], "bootmgr")) {
if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))return CMD_RET_FAILURE;
char *fdtstr, *endp;int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;if (argc > 2) {fdtstr = argv[2];/* Special address "-" means no device tree */if (fdtstr[0] == '-')fdtstr = NULL;r = efi_handle_fdt(fdtstr);if (r)return CMD_RET_FAILURE;}if (argc > 3) {boot_id = (int)simple_strtoul(argv[3], &endp, 0);if ((argv[3] + strlen(argv[3]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;}
return do_bootefi_bootmgr_exec();
return do_bootefi_bootmgr_exec(boot_id);Why not communicate via the BootNext variable?
I don't get your point. BootNext and BootOrder are both defined by UEFI specification.
Instead of changing the interface of do_bootefi_bootmgr_exec()
Who care changing an *internal* function?
you could simply set BootNext. Then the boot manager would pick up the option from the variable and finally delete the variable. This would result in less code.
No. Even with "run -e," BootNext will disappear after execution. This is a requirement by UEFI spec.
Shouldn't BootNext always be reset when executing bootefi no matter whether the boot manager is used or not?
Regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
} else { saddr = argv[1];
@@ -590,7 +607,7 @@ static char bootefi_help_text[] = " Use environment variable efi_selftest to select a single test.\n" " Use 'setenv efi_selftest list' to enumerate all tests.\n" #endif
- "bootefi bootmgr [fdt addr]\n"
- "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" "\n" " If specified, the device tree located at <fdt address> gets\n"
@@ -598,7 +615,7 @@ static char bootefi_help_text[] = #endif
U_BOOT_CMD(
- bootefi, 3, 0, do_bootefi,
- bootefi, 5, 0, do_bootefi,
Why 5?
For additional/optional '-' and <boot id>. But I removed this feature from bootefi.
Thanks, -Takahiro Akashi
Best regards
Heinrich
"Boots an EFI payload from memory", bootefi_help_text );
On Wed, Feb 27, 2019 at 08:33:17PM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:47 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 07:31:06AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 6:58 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
With this patch applied, we will be able to selectively execute an EFI application by specifying a load option, say "1" for Boot0001, "2" for Boot0002 and so on.
=> bootefi bootmgr <fdt addr> 1, or bootefi bootmgr - 1
You already introduced the support for BootNext. So is there a real benefit?
This is a convenient way of running EFI application directly, but I already removed this feature from the next version.
Please, remove 'run -e' instead because it cannot specify the device tree needed for booting ARM boards.
See my comment for patch#5 first.
Please note that BootXXXX need not be included in "BootOrder".
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3be01b49b589..241fd0f987ab 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare
#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
-static int do_bootefi_bootmgr_exec(void) +static int do_bootefi_bootmgr_exec(int boot_id) { struct efi_device_path *device_path, *file_path; void *addr; efi_status_t r;
- addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER,
&device_path, &file_path);
- addr = efi_bootmgr_load(boot_id, &device_path, &file_path); if (!addr)
return 1;
return CMD_RET_FAILURE;printf("## Starting EFI application at %p ...\n", addr); r = do_bootefi_exec(addr, device_path, file_path);
@@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) r & ~EFI_ERROR_MASK);
if (r != EFI_SUCCESS)
return 1;
return CMD_RET_FAILURE;
- return 0;
- return CMD_RET_SUCCESS;
}
/* Interpreter command to boot an arbitrary EFI image from memory */ @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } else #endif if (!strcmp(argv[1], "bootmgr")) {
if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))return CMD_RET_FAILURE;
char *fdtstr, *endp;int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;if (argc > 2) {fdtstr = argv[2];/* Special address "-" means no device tree */if (fdtstr[0] == '-')fdtstr = NULL;r = efi_handle_fdt(fdtstr);if (r)return CMD_RET_FAILURE;}if (argc > 3) {boot_id = (int)simple_strtoul(argv[3], &endp, 0);if ((argv[3] + strlen(argv[3]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;}
return do_bootefi_bootmgr_exec();
return do_bootefi_bootmgr_exec(boot_id);Why not communicate via the BootNext variable?
I don't get your point. BootNext and BootOrder are both defined by UEFI specification.
Instead of changing the interface of do_bootefi_bootmgr_exec()
Who care changing an *internal* function?
So do you agree?
you could simply set BootNext. Then the boot manager would pick up the option from the variable and finally delete the variable. This would result in less code.
No. Even with "run -e," BootNext will disappear after execution. This is a requirement by UEFI spec.
Shouldn't BootNext always be reset when executing bootefi no matter whether the boot manager is used or not?
Didn't I say the same thing? Or do you expect that BootNext remain after "run -e"?
-Takahiro Akashi
Regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
} else { saddr = argv[1];
@@ -590,7 +607,7 @@ static char bootefi_help_text[] = " Use environment variable efi_selftest to select a single test.\n" " Use 'setenv efi_selftest list' to enumerate all tests.\n" #endif
- "bootefi bootmgr [fdt addr]\n"
- "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" "\n" " If specified, the device tree located at <fdt address> gets\n"
@@ -598,7 +615,7 @@ static char bootefi_help_text[] = #endif
U_BOOT_CMD(
- bootefi, 3, 0, do_bootefi,
- bootefi, 5, 0, do_bootefi,
Why 5?
For additional/optional '-' and <boot id>. But I removed this feature from bootefi.
Thanks, -Takahiro Akashi
Best regards
Heinrich
"Boots an EFI payload from memory", bootefi_help_text );
On 2/28/19 5:28 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 08:33:17PM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:47 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 07:31:06AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 6:58 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote: > With this patch applied, we will be able to selectively execute > an EFI application by specifying a load option, say "1" for Boot0001, > "2" for Boot0002 and so on. > > => bootefi bootmgr <fdt addr> 1, or > bootefi bootmgr - 1
You already introduced the support for BootNext. So is there a real benefit?
This is a convenient way of running EFI application directly, but I already removed this feature from the next version.
Please, remove 'run -e' instead because it cannot specify the device tree needed for booting ARM boards.
See my comment for patch#5 first.
> > Please note that BootXXXX need not be included in "BootOrder". > > Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org > --- > cmd/bootefi.c | 39 ++++++++++++++++++++++++++++----------- > 1 file changed, 28 insertions(+), 11 deletions(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index 3be01b49b589..241fd0f987ab 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -471,16 +471,15 @@ static efi_status_t bootefi_test_prepare > > #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */ > > -static int do_bootefi_bootmgr_exec(void) > +static int do_bootefi_bootmgr_exec(int boot_id) > { > struct efi_device_path *device_path, *file_path; > void *addr; > efi_status_t r; > > - addr = efi_bootmgr_load(EFI_BOOTMGR_DEFAULT_ORDER, > - &device_path, &file_path); > + addr = efi_bootmgr_load(boot_id, &device_path, &file_path); > if (!addr) > - return 1; > + return CMD_RET_FAILURE; > > printf("## Starting EFI application at %p ...\n", addr); > r = do_bootefi_exec(addr, device_path, file_path); > @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void) > r & ~EFI_ERROR_MASK); > > if (r != EFI_SUCCESS) > - return 1; > + return CMD_RET_FAILURE; > > - return 0; > + return CMD_RET_SUCCESS; > } > > /* Interpreter command to boot an arbitrary EFI image from memory */ > @@ -546,10 +545,28 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > } else > #endif > if (!strcmp(argv[1], "bootmgr")) { > - if (efi_handle_fdt(argc > 2 ? argv[2] : NULL)) > - return CMD_RET_FAILURE; > + char *fdtstr, *endp; > + int boot_id = EFI_BOOTMGR_DEFAULT_ORDER; > + > + if (argc > 2) { > + fdtstr = argv[2]; > + /* Special address "-" means no device tree */ > + if (fdtstr[0] == '-') > + fdtstr = NULL; > + > + r = efi_handle_fdt(fdtstr); > + if (r) > + return CMD_RET_FAILURE; > + } > + > + if (argc > 3) { > + boot_id = (int)simple_strtoul(argv[3], &endp, 0); > + if ((argv[3] + strlen(argv[3]) != endp) || > + boot_id > 0xffff) > + return CMD_RET_USAGE; > + } > > - return do_bootefi_bootmgr_exec(); > + return do_bootefi_bootmgr_exec(boot_id);
Why not communicate via the BootNext variable?
I don't get your point. BootNext and BootOrder are both defined by UEFI specification.
Instead of changing the interface of do_bootefi_bootmgr_exec()
Who care changing an *internal* function?
So do you agree?
What is wrong about calling efi_set_variable(L"BootNext", ) instead? Wouldn't that result in less code?
you could simply set BootNext. Then the boot manager would pick up the option from the variable and finally delete the variable. This would result in less code.
No. Even with "run -e," BootNext will disappear after execution. This is a requirement by UEFI spec.
Shouldn't BootNext always be reset when executing bootefi no matter whether the boot manager is used or not?
Didn't I say the same thing? Or do you expect that BootNext remain after "run -e"?
As described in the response to patch 5/5 'run -e' is not usable with ARM given the current state of device trees in Linux.
Patch 1/5 only deletes variable BootNext if the boot manager is called. Is this really correct? Shouldn't we delete the BootNext variable when executing `bootefi $kernel_addr_r $fdt_addr_r`, too?
Best regards
Heinrich
-Takahiro Akashi
Regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
> } else { > saddr = argv[1]; > > @@ -590,7 +607,7 @@ static char bootefi_help_text[] = > " Use environment variable efi_selftest to select a single test.\n" > " Use 'setenv efi_selftest list' to enumerate all tests.\n" > #endif > - "bootefi bootmgr [fdt addr]\n" > + "bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n" > " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" > "\n" > " If specified, the device tree located at <fdt address> gets\n" > @@ -598,7 +615,7 @@ static char bootefi_help_text[] = > #endif > > U_BOOT_CMD( > - bootefi, 3, 0, do_bootefi, > + bootefi, 5, 0, do_bootefi,
Why 5?
For additional/optional '-' and <boot id>. But I removed this feature from bootefi.
Thanks, -Takahiro Akashi
Best regards
Heinrich
> "Boots an EFI payload from memory", > bootefi_help_text > ); >
"run -e" allows for executing EFI application with a specific "BootXXXX" variable. If no "BootXXXX" is specified or "BootOrder" is specified, it tries to run an EFI application specified in the order of "bootOrder."
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ cmd/nvedit.c | 9 ++++++++- common/cli.c | 10 ++++++++++ include/command.h | 3 +++ 4 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 241fd0f987ab..ebe149dffa1f 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) return CMD_RET_SUCCESS; }
+/* Called by "run" command */ +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + int boot_id = -1; + char *endp; + + if (argc > 2) + return CMD_RET_USAGE; + + if (argc == 2) { + if (!strcmp(argv[1], "BootOrder")) { + boot_id = -1; + } else if (!strncmp(argv[2], "Boot", 4)) { + boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0); + if ((argv[2] + strlen(argv[2]) != endp) || + boot_id > 0xffff) + return CMD_RET_USAGE; + } else { + return CMD_RET_USAGE; + } + } + + if (efi_init_obj_list()) + return CMD_RET_FAILURE; + + if (efi_handle_fdt(NULL)) + return CMD_RET_FAILURE; + + return do_bootefi_bootmgr_exec(boot_id); +} + /* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { diff --git a/cmd/nvedit.c b/cmd/nvedit.c index de16c72c23f2..ce746bbf1b3e 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( run, CONFIG_SYS_MAXARGS, 1, do_run, "run commands in an environment variable", "var [...]\n" - " - run the commands in the environment variable(s) 'var'", + " - run the commands in the environment variable(s) 'var'" +#if defined(CONFIG_CMD_BOOTEFI) + "\n" + "run -e [BootXXXX]\n" + " - load and run UEFI app based on 'BootXXXX' UEFI variable", +#else + , +#endif var_complete ); #endif diff --git a/common/cli.c b/common/cli.c index 51b8d5f85cbb..fbb09d5049a4 100644 --- a/common/cli.c +++ b/common/cli.c @@ -12,6 +12,7 @@ #include <cli.h> #include <cli_hush.h> #include <console.h> +#include <efi_loader.h> #include <fdtdec.h> #include <malloc.h>
@@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
+#ifdef CONFIG_CMD_BOOTEFI + if (!strcmp(argv[1], "-e")) { + argc--; + argv++; + + return do_bootefi_run(cmdtp, flag, argc, argv); + } +#endif + for (i = 1; i < argc; ++i) { char *arg;
diff --git a/include/command.h b/include/command.h index 200c7a5e9f4e..feddef300ccc 100644 --- a/include/command.h +++ b/include/command.h @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; #if defined(CONFIG_CMD_RUN) extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); #endif +#if defined(CONFIG_CMD_BOOTEFI) +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); +#endif
/* common/command.c */ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
"run -e" allows for executing EFI application with a specific "BootXXXX" variable. If no "BootXXXX" is specified or "BootOrder" is specified, it tries to run an EFI application specified in the order of "bootOrder."
If we cannot specify the device tree what would be the use of this for ARM processors?
Why do you add the option to run and not to bootefi?
You already introduced the capability to set BootNext. Why isn't that enough?
Best regards
Heinrich
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ cmd/nvedit.c | 9 ++++++++- common/cli.c | 10 ++++++++++ include/command.h | 3 +++ 4 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 241fd0f987ab..ebe149dffa1f 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) return CMD_RET_SUCCESS; }
+/* Called by "run" command */ +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{
- int boot_id = -1;
- char *endp;
- if (argc > 2)
return CMD_RET_USAGE;- if (argc == 2) {
if (!strcmp(argv[1], "BootOrder")) {boot_id = -1;} else if (!strncmp(argv[2], "Boot", 4)) {boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0);if ((argv[2] + strlen(argv[2]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;} else {return CMD_RET_USAGE;}- }
- if (efi_init_obj_list())
return CMD_RET_FAILURE;- if (efi_handle_fdt(NULL))
return CMD_RET_FAILURE;- return do_bootefi_bootmgr_exec(boot_id);
+}
/* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { diff --git a/cmd/nvedit.c b/cmd/nvedit.c index de16c72c23f2..ce746bbf1b3e 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( run, CONFIG_SYS_MAXARGS, 1, do_run, "run commands in an environment variable", "var [...]\n"
- " - run the commands in the environment variable(s) 'var'",
- " - run the commands in the environment variable(s) 'var'"
+#if defined(CONFIG_CMD_BOOTEFI)
- "\n"
- "run -e [BootXXXX]\n"
- " - load and run UEFI app based on 'BootXXXX' UEFI variable",
+#else
- ,
+#endif var_complete ); #endif diff --git a/common/cli.c b/common/cli.c index 51b8d5f85cbb..fbb09d5049a4 100644 --- a/common/cli.c +++ b/common/cli.c @@ -12,6 +12,7 @@ #include <cli.h> #include <cli_hush.h> #include <console.h> +#include <efi_loader.h> #include <fdtdec.h> #include <malloc.h>
@@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
+#ifdef CONFIG_CMD_BOOTEFI
- if (!strcmp(argv[1], "-e")) {
argc--;argv++;return do_bootefi_run(cmdtp, flag, argc, argv);- }
+#endif
- for (i = 1; i < argc; ++i) { char *arg;
diff --git a/include/command.h b/include/command.h index 200c7a5e9f4e..feddef300ccc 100644 --- a/include/command.h +++ b/include/command.h @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; #if defined(CONFIG_CMD_RUN) extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); #endif +#if defined(CONFIG_CMD_BOOTEFI) +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); +#endif
/* common/command.c */ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
"run -e" allows for executing EFI application with a specific "BootXXXX" variable. If no "BootXXXX" is specified or "BootOrder" is specified, it tries to run an EFI application specified in the order of "bootOrder."
If we cannot specify the device tree what would be the use of this for ARM processors?
I don't get your point. What's the matter with device tree?
Why do you add the option to run and not to bootefi?
You already introduced the capability to set BootNext. Why isn't that enough?
Simple.
=> run -e Boot0001
versus
=> efidebug boot next 1 => bootefi bootmgr
First of all, efidebug is only recognized as a *debugging* tool. I believe that the former syntax is more intuitive, and it looks a natural extension to "run" command semantics akin to "env -e".
As a result, we don't have to know about bootefi for normal operations.
Thanks, -Takahiro Akashi
Best regards
Heinrich
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ cmd/nvedit.c | 9 ++++++++- common/cli.c | 10 ++++++++++ include/command.h | 3 +++ 4 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 241fd0f987ab..ebe149dffa1f 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) return CMD_RET_SUCCESS; }
+/* Called by "run" command */ +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{
- int boot_id = -1;
- char *endp;
- if (argc > 2)
return CMD_RET_USAGE;- if (argc == 2) {
if (!strcmp(argv[1], "BootOrder")) {boot_id = -1;} else if (!strncmp(argv[2], "Boot", 4)) {boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0);if ((argv[2] + strlen(argv[2]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;} else {return CMD_RET_USAGE;}- }
- if (efi_init_obj_list())
return CMD_RET_FAILURE;- if (efi_handle_fdt(NULL))
return CMD_RET_FAILURE;- return do_bootefi_bootmgr_exec(boot_id);
+}
/* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { diff --git a/cmd/nvedit.c b/cmd/nvedit.c index de16c72c23f2..ce746bbf1b3e 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( run, CONFIG_SYS_MAXARGS, 1, do_run, "run commands in an environment variable", "var [...]\n"
- " - run the commands in the environment variable(s) 'var'",
- " - run the commands in the environment variable(s) 'var'"
+#if defined(CONFIG_CMD_BOOTEFI)
- "\n"
- "run -e [BootXXXX]\n"
- " - load and run UEFI app based on 'BootXXXX' UEFI variable",
+#else
- ,
+#endif var_complete ); #endif diff --git a/common/cli.c b/common/cli.c index 51b8d5f85cbb..fbb09d5049a4 100644 --- a/common/cli.c +++ b/common/cli.c @@ -12,6 +12,7 @@ #include <cli.h> #include <cli_hush.h> #include <console.h> +#include <efi_loader.h> #include <fdtdec.h> #include <malloc.h>
@@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
+#ifdef CONFIG_CMD_BOOTEFI
- if (!strcmp(argv[1], "-e")) {
argc--;argv++;return do_bootefi_run(cmdtp, flag, argc, argv);- }
+#endif
- for (i = 1; i < argc; ++i) { char *arg;
diff --git a/include/command.h b/include/command.h index 200c7a5e9f4e..feddef300ccc 100644 --- a/include/command.h +++ b/include/command.h @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; #if defined(CONFIG_CMD_RUN) extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); #endif +#if defined(CONFIG_CMD_BOOTEFI) +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); +#endif
/* common/command.c */ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
"run -e" allows for executing EFI application with a specific "BootXXXX" variable. If no "BootXXXX" is specified or "BootOrder" is specified, it tries to run an EFI application specified in the order of "bootOrder."
If we cannot specify the device tree what would be the use of this for ARM processors?
I don't get your point. What's the matter with device tree?
To boot an ARM board on Linux or BSD you need a device tree.
So run -e Boot0001 will not allow you to boot into Linux because it does not specify a device tree.
Why do you add the option to run and not to bootefi?
You already introduced the capability to set BootNext. Why isn't that enough?
Simple.
=> run -e Boot00> versus
=> efidebug boot next 1 => bootefi bootmgr
In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
So there is no need to go through efidebug.
I think we should avoid alternative commands.
First of all, efidebug is only recognized as a *debugging* tool. I believe that the former syntax is more intuitive, and it looks a natural extension to "run" command semantics akin to "env -e".
As a result, we don't have to know about bootefi for normal operations.
But you have to know about 'run' which you might not need otherwise.
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ cmd/nvedit.c | 9 ++++++++- common/cli.c | 10 ++++++++++ include/command.h | 3 +++ 4 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 241fd0f987ab..ebe149dffa1f 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) return CMD_RET_SUCCESS; }
+/* Called by "run" command */ +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{
- int boot_id = -1;
- char *endp;
- if (argc > 2)
return CMD_RET_USAGE;- if (argc == 2) {
if (!strcmp(argv[1], "BootOrder")) {boot_id = -1;} else if (!strncmp(argv[2], "Boot", 4)) {boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0);if ((argv[2] + strlen(argv[2]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;} else {return CMD_RET_USAGE;}- }
- if (efi_init_obj_list())
return CMD_RET_FAILURE;- if (efi_handle_fdt(NULL))
return CMD_RET_FAILURE;- return do_bootefi_bootmgr_exec(boot_id);
+}
/* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { diff --git a/cmd/nvedit.c b/cmd/nvedit.c index de16c72c23f2..ce746bbf1b3e 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( run, CONFIG_SYS_MAXARGS, 1, do_run, "run commands in an environment variable", "var [...]\n"
- " - run the commands in the environment variable(s) 'var'",
- " - run the commands in the environment variable(s) 'var'"
+#if defined(CONFIG_CMD_BOOTEFI)
- "\n"
- "run -e [BootXXXX]\n"
- " - load and run UEFI app based on 'BootXXXX' UEFI variable",
+#else
- ,
+#endif var_complete ); #endif diff --git a/common/cli.c b/common/cli.c index 51b8d5f85cbb..fbb09d5049a4 100644 --- a/common/cli.c +++ b/common/cli.c @@ -12,6 +12,7 @@ #include <cli.h> #include <cli_hush.h> #include <console.h> +#include <efi_loader.h> #include <fdtdec.h> #include <malloc.h>
@@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
+#ifdef CONFIG_CMD_BOOTEFI
- if (!strcmp(argv[1], "-e")) {
argc--;argv++;return do_bootefi_run(cmdtp, flag, argc, argv);- }
+#endif
- for (i = 1; i < argc; ++i) { char *arg;
diff --git a/include/command.h b/include/command.h index 200c7a5e9f4e..feddef300ccc 100644 --- a/include/command.h +++ b/include/command.h @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; #if defined(CONFIG_CMD_RUN) extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); #endif +#if defined(CONFIG_CMD_BOOTEFI) +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); +#endif
/* common/command.c */ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
"run -e" allows for executing EFI application with a specific "BootXXXX" variable. If no "BootXXXX" is specified or "BootOrder" is specified, it tries to run an EFI application specified in the order of "bootOrder."
If we cannot specify the device tree what would be the use of this for ARM processors?
I don't get your point. What's the matter with device tree?
To boot an ARM board on Linux or BSD you need a device tree.
When I discussed with Alex about Boot Manager (and distro_bootcmd?), he suggested that we should not allow users to specify fdt at a command line. I believe that it would apply to my case above.
IMO, we should always provide system's fdt to EFI applications.
So run -e Boot0001 will not allow you to boot into Linux because it does not specify a device tree.
Why do you add the option to run and not to bootefi?
You already introduced the capability to set BootNext. Why isn't that enough?
Simple.
=> run -e Boot00> versus
=> efidebug boot next 1 => bootefi bootmgr
In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
So there is no need to go through efidebug.
I think we should avoid alternative commands.
As I said, I already removed this feature from bootefi.
First of all, efidebug is only recognized as a *debugging* tool. I believe that the former syntax is more intuitive, and it looks a natural extension to "run" command semantics akin to "env -e".
As a result, we don't have to know about bootefi for normal operations.
But you have to know about 'run' which you might not need otherwise.
"run" is much better known to U-Boot users than bootefi.
Thanks, -Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ cmd/nvedit.c | 9 ++++++++- common/cli.c | 10 ++++++++++ include/command.h | 3 +++ 4 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 241fd0f987ab..ebe149dffa1f 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) return CMD_RET_SUCCESS; }
+/* Called by "run" command */ +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{
- int boot_id = -1;
- char *endp;
- if (argc > 2)
return CMD_RET_USAGE;- if (argc == 2) {
if (!strcmp(argv[1], "BootOrder")) {boot_id = -1;} else if (!strncmp(argv[2], "Boot", 4)) {boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0);if ((argv[2] + strlen(argv[2]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;} else {return CMD_RET_USAGE;}- }
- if (efi_init_obj_list())
return CMD_RET_FAILURE;- if (efi_handle_fdt(NULL))
return CMD_RET_FAILURE;- return do_bootefi_bootmgr_exec(boot_id);
+}
/* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { diff --git a/cmd/nvedit.c b/cmd/nvedit.c index de16c72c23f2..ce746bbf1b3e 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( run, CONFIG_SYS_MAXARGS, 1, do_run, "run commands in an environment variable", "var [...]\n"
- " - run the commands in the environment variable(s) 'var'",
- " - run the commands in the environment variable(s) 'var'"
+#if defined(CONFIG_CMD_BOOTEFI)
- "\n"
- "run -e [BootXXXX]\n"
- " - load and run UEFI app based on 'BootXXXX' UEFI variable",
+#else
- ,
+#endif var_complete ); #endif diff --git a/common/cli.c b/common/cli.c index 51b8d5f85cbb..fbb09d5049a4 100644 --- a/common/cli.c +++ b/common/cli.c @@ -12,6 +12,7 @@ #include <cli.h> #include <cli_hush.h> #include <console.h> +#include <efi_loader.h> #include <fdtdec.h> #include <malloc.h>
@@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
+#ifdef CONFIG_CMD_BOOTEFI
- if (!strcmp(argv[1], "-e")) {
argc--;argv++;return do_bootefi_run(cmdtp, flag, argc, argv);- }
+#endif
- for (i = 1; i < argc; ++i) { char *arg;
diff --git a/include/command.h b/include/command.h index 200c7a5e9f4e..feddef300ccc 100644 --- a/include/command.h +++ b/include/command.h @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; #if defined(CONFIG_CMD_RUN) extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); #endif +#if defined(CONFIG_CMD_BOOTEFI) +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); +#endif
/* common/command.c */ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
"run -e" allows for executing EFI application with a specific "BootXXXX" variable. If no "BootXXXX" is specified or "BootOrder" is specified, it tries to run an EFI application specified in the order of "bootOrder."
If we cannot specify the device tree what would be the use of this for ARM processors?
I don't get your point. What's the matter with device tree?
To boot an ARM board on Linux or BSD you need a device tree.
When I discussed with Alex about Boot Manager (and distro_bootcmd?), he suggested that we should not allow users to specify fdt at a command line. I believe that it would apply to my case above.
IMO, we should always provide system's fdt to EFI applications.
With current Linux development practice this unfortunately does not work. Linux device trees sometimes see incompatible changes between versions. Booting may fail with a device tree that is either too old or too new for your Linux version.
E.g. for the Odroid C2 some reserved memory regions were removed from the device tree and replaced by a logic that determines them on the fly due to changes in ARM trusted firmware location.
The Wandboard rev B1 device tree was moved to a new file when a new board revision appeared and the new revision changed the old file (sic).
U-Boot is also not perfect at keeping its device tree in sync with the newest Linux device tree.
So run -e Boot0001 will not allow you to boot into Linux because it does not specify a device tree.
Why do you add the option to run and not to bootefi?
You already introduced the capability to set BootNext. Why isn't that enough?
Simple.
=> run -e Boot00> versus
=> efidebug boot next 1 => bootefi bootmgr
In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
So there is no need to go through efidebug.
I think we should avoid alternative commands.
As I said, I already removed this feature from bootefi.
First of all, efidebug is only recognized as a *debugging* tool. I believe that the former syntax is more intuitive, and it looks a natural extension to "run" command semantics akin to "env -e".
As a result, we don't have to know about bootefi for normal operations.
But you have to know about 'run' which you might not need otherwise.
"run" is much better known to U-Boot users than bootefi.
Do you have a statistic ;)
Up to now booting always required a command starting on boot...
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ cmd/nvedit.c | 9 ++++++++- common/cli.c | 10 ++++++++++ include/command.h | 3 +++ 4 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 241fd0f987ab..ebe149dffa1f 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) return CMD_RET_SUCCESS; }
+/* Called by "run" command */ +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{
- int boot_id = -1;
- char *endp;
- if (argc > 2)
return CMD_RET_USAGE;- if (argc == 2) {
if (!strcmp(argv[1], "BootOrder")) {boot_id = -1;} else if (!strncmp(argv[2], "Boot", 4)) {boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0);if ((argv[2] + strlen(argv[2]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;} else {return CMD_RET_USAGE;}- }
- if (efi_init_obj_list())
return CMD_RET_FAILURE;- if (efi_handle_fdt(NULL))
return CMD_RET_FAILURE;- return do_bootefi_bootmgr_exec(boot_id);
+}
/* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { diff --git a/cmd/nvedit.c b/cmd/nvedit.c index de16c72c23f2..ce746bbf1b3e 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( run, CONFIG_SYS_MAXARGS, 1, do_run, "run commands in an environment variable", "var [...]\n"
- " - run the commands in the environment variable(s) 'var'",
- " - run the commands in the environment variable(s) 'var'"
+#if defined(CONFIG_CMD_BOOTEFI)
- "\n"
- "run -e [BootXXXX]\n"
- " - load and run UEFI app based on 'BootXXXX' UEFI variable",
+#else
- ,
+#endif var_complete ); #endif diff --git a/common/cli.c b/common/cli.c index 51b8d5f85cbb..fbb09d5049a4 100644 --- a/common/cli.c +++ b/common/cli.c @@ -12,6 +12,7 @@ #include <cli.h> #include <cli_hush.h> #include <console.h> +#include <efi_loader.h> #include <fdtdec.h> #include <malloc.h>
@@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
+#ifdef CONFIG_CMD_BOOTEFI
- if (!strcmp(argv[1], "-e")) {
argc--;argv++;return do_bootefi_run(cmdtp, flag, argc, argv);- }
+#endif
- for (i = 1; i < argc; ++i) { char *arg;
diff --git a/include/command.h b/include/command.h index 200c7a5e9f4e..feddef300ccc 100644 --- a/include/command.h +++ b/include/command.h @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; #if defined(CONFIG_CMD_RUN) extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); #endif +#if defined(CONFIG_CMD_BOOTEFI) +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); +#endif
/* common/command.c */ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
"run -e" allows for executing EFI application with a specific "BootXXXX" variable. If no "BootXXXX" is specified or "BootOrder" is specified, it tries to run an EFI application specified in the order of "bootOrder."
If we cannot specify the device tree what would be the use of this for ARM processors?
I don't get your point. What's the matter with device tree?
To boot an ARM board on Linux or BSD you need a device tree.
When I discussed with Alex about Boot Manager (and distro_bootcmd?), he suggested that we should not allow users to specify fdt at a command line. I believe that it would apply to my case above.
IMO, we should always provide system's fdt to EFI applications.
With current Linux development practice this unfortunately does not work. Linux device trees sometimes see incompatible changes between versions. Booting may fail with a device tree that is either too old or too new for your Linux version.
E.g. for the Odroid C2 some reserved memory regions were removed from the device tree and replaced by a logic that determines them on the fly due to changes in ARM trusted firmware location.
The Wandboard rev B1 device tree was moved to a new file when a new board revision appeared and the new revision changed the old file (sic).
U-Boot is also not perfect at keeping its device tree in sync with the newest Linux device tree.
Why don't you use "fdt" command in that case? IMO, we don't need <fdt> argument at bootefi (and run -e). Obviously, I have one assumption that we need change the code to utilize "fdtaddr" variable in do_bootefi().
Under the current implementation, a similar behavior is achieved only via distro_bootcmd. IMO, this should be corrected. If you agree, I will add another patch to my current patchset for this purpose.
So run -e Boot0001 will not allow you to boot into Linux because it does not specify a device tree.
Why do you add the option to run and not to bootefi?
You already introduced the capability to set BootNext. Why isn't that enough?
Simple.
=> run -e Boot00> versus
=> efidebug boot next 1 => bootefi bootmgr
In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
So there is no need to go through efidebug.
I think we should avoid alternative commands.
As I said, I already removed this feature from bootefi.
First of all, efidebug is only recognized as a *debugging* tool. I believe that the former syntax is more intuitive, and it looks a natural extension to "run" command semantics akin to "env -e".
As a result, we don't have to know about bootefi for normal operations.
But you have to know about 'run' which you might not need otherwise.
"run" is much better known to U-Boot users than bootefi.
Do you have a statistic ;)
Up to now booting always required a command starting on boot...
What I meant is that people need not learn more commands.
# Relating to secure boot, I'm now thinking of pulling bootmgr out of # bootefi and making it as a single command. In that case, # bootefi does exist only for hello and selftest.
-Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ cmd/nvedit.c | 9 ++++++++- common/cli.c | 10 ++++++++++ include/command.h | 3 +++ 4 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 241fd0f987ab..ebe149dffa1f 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) return CMD_RET_SUCCESS; }
+/* Called by "run" command */ +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{
- int boot_id = -1;
- char *endp;
- if (argc > 2)
return CMD_RET_USAGE;- if (argc == 2) {
if (!strcmp(argv[1], "BootOrder")) {boot_id = -1;} else if (!strncmp(argv[2], "Boot", 4)) {boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0);if ((argv[2] + strlen(argv[2]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;} else {return CMD_RET_USAGE;}- }
- if (efi_init_obj_list())
return CMD_RET_FAILURE;- if (efi_handle_fdt(NULL))
return CMD_RET_FAILURE;- return do_bootefi_bootmgr_exec(boot_id);
+}
/* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { diff --git a/cmd/nvedit.c b/cmd/nvedit.c index de16c72c23f2..ce746bbf1b3e 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( run, CONFIG_SYS_MAXARGS, 1, do_run, "run commands in an environment variable", "var [...]\n"
- " - run the commands in the environment variable(s) 'var'",
- " - run the commands in the environment variable(s) 'var'"
+#if defined(CONFIG_CMD_BOOTEFI)
- "\n"
- "run -e [BootXXXX]\n"
- " - load and run UEFI app based on 'BootXXXX' UEFI variable",
+#else
- ,
+#endif var_complete ); #endif diff --git a/common/cli.c b/common/cli.c index 51b8d5f85cbb..fbb09d5049a4 100644 --- a/common/cli.c +++ b/common/cli.c @@ -12,6 +12,7 @@ #include <cli.h> #include <cli_hush.h> #include <console.h> +#include <efi_loader.h> #include <fdtdec.h> #include <malloc.h>
@@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
+#ifdef CONFIG_CMD_BOOTEFI
- if (!strcmp(argv[1], "-e")) {
argc--;argv++;return do_bootefi_run(cmdtp, flag, argc, argv);- }
+#endif
- for (i = 1; i < argc; ++i) { char *arg;
diff --git a/include/command.h b/include/command.h index 200c7a5e9f4e..feddef300ccc 100644 --- a/include/command.h +++ b/include/command.h @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; #if defined(CONFIG_CMD_RUN) extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); #endif +#if defined(CONFIG_CMD_BOOTEFI) +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); +#endif
/* common/command.c */ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
On 2/28/19 5:45 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote: > "run -e" allows for executing EFI application with a specific "BootXXXX" > variable. If no "BootXXXX" is specified or "BootOrder" is specified, > it tries to run an EFI application specified in the order of "bootOrder." >
If we cannot specify the device tree what would be the use of this for ARM processors?
I don't get your point. What's the matter with device tree?
To boot an ARM board on Linux or BSD you need a device tree.
When I discussed with Alex about Boot Manager (and distro_bootcmd?), he suggested that we should not allow users to specify fdt at a command line. I believe that it would apply to my case above.
IMO, we should always provide system's fdt to EFI applications.
With current Linux development practice this unfortunately does not work. Linux device trees sometimes see incompatible changes between versions. Booting may fail with a device tree that is either too old or too new for your Linux version.
E.g. for the Odroid C2 some reserved memory regions were removed from the device tree and replaced by a logic that determines them on the fly due to changes in ARM trusted firmware location.
The Wandboard rev B1 device tree was moved to a new file when a new board revision appeared and the new revision changed the old file (sic).
U-Boot is also not perfect at keeping its device tree in sync with the newest Linux device tree.
Why don't you use "fdt" command in that case? IMO, we don't need <fdt> argument at bootefi (and run -e). Obviously, I have one assumption that we need change the code to utilize "fdtaddr" variable in do_bootefi().
Such a change would mean that after an upgrade of U-Boot all boards running on Suse and Fedora suddenly will not boot again.
We should not change existing commands in an incompatible way.
Under the current implementation, a similar behavior is achieved only via distro_bootcmd. IMO, this should be corrected. If you agree, I will add another patch to my current patchset for this purpose.
I suggest to drop patch 5/5 from your series.
Best regards
Heinrich
So run -e Boot0001 will not allow you to boot into Linux because it does not specify a device tree.
Why do you add the option to run and not to bootefi?
You already introduced the capability to set BootNext. Why isn't that enough?
Simple.
=> run -e Boot00> versus
=> efidebug boot next 1 => bootefi bootmgr
In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
So there is no need to go through efidebug.
I think we should avoid alternative commands.
As I said, I already removed this feature from bootefi.
First of all, efidebug is only recognized as a *debugging* tool. I believe that the former syntax is more intuitive, and it looks a natural extension to "run" command semantics akin to "env -e".
As a result, we don't have to know about bootefi for normal operations.
But you have to know about 'run' which you might not need otherwise.
"run" is much better known to U-Boot users than bootefi.
Do you have a statistic ;)
Up to now booting always required a command starting on boot...
What I meant is that people need not learn more commands.
# Relating to secure boot, I'm now thinking of pulling bootmgr out of # bootefi and making it as a single command. In that case, # bootefi does exist only for hello and selftest.
-Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org > --- > cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ > cmd/nvedit.c | 9 ++++++++- > common/cli.c | 10 ++++++++++ > include/command.h | 3 +++ > 4 files changed, 52 insertions(+), 1 deletion(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index 241fd0f987ab..ebe149dffa1f 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) > return CMD_RET_SUCCESS; > } > > +/* Called by "run" command */ > +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > +{ > + int boot_id = -1; > + char *endp; > + > + if (argc > 2) > + return CMD_RET_USAGE; > + > + if (argc == 2) { > + if (!strcmp(argv[1], "BootOrder")) { > + boot_id = -1; > + } else if (!strncmp(argv[2], "Boot", 4)) { > + boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0); > + if ((argv[2] + strlen(argv[2]) != endp) || > + boot_id > 0xffff) > + return CMD_RET_USAGE; > + } else { > + return CMD_RET_USAGE; > + } > + } > + > + if (efi_init_obj_list()) > + return CMD_RET_FAILURE; > + > + if (efi_handle_fdt(NULL)) > + return CMD_RET_FAILURE; > + > + return do_bootefi_bootmgr_exec(boot_id); > +} > + > /* Interpreter command to boot an arbitrary EFI image from memory */ > static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > { > diff --git a/cmd/nvedit.c b/cmd/nvedit.c > index de16c72c23f2..ce746bbf1b3e 100644 > --- a/cmd/nvedit.c > +++ b/cmd/nvedit.c > @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( > run, CONFIG_SYS_MAXARGS, 1, do_run, > "run commands in an environment variable", > "var [...]\n" > - " - run the commands in the environment variable(s) 'var'", > + " - run the commands in the environment variable(s) 'var'" > +#if defined(CONFIG_CMD_BOOTEFI) > + "\n" > + "run -e [BootXXXX]\n" > + " - load and run UEFI app based on 'BootXXXX' UEFI variable", > +#else > + , > +#endif > var_complete > ); > #endif > diff --git a/common/cli.c b/common/cli.c > index 51b8d5f85cbb..fbb09d5049a4 100644 > --- a/common/cli.c > +++ b/common/cli.c > @@ -12,6 +12,7 @@ > #include <cli.h> > #include <cli_hush.h> > #include <console.h> > +#include <efi_loader.h> > #include <fdtdec.h> > #include <malloc.h> > > @@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > if (argc < 2) > return CMD_RET_USAGE; > > +#ifdef CONFIG_CMD_BOOTEFI > + if (!strcmp(argv[1], "-e")) { > + argc--; > + argv++; > + > + return do_bootefi_run(cmdtp, flag, argc, argv); > + } > +#endif > + > for (i = 1; i < argc; ++i) { > char *arg; > > diff --git a/include/command.h b/include/command.h > index 200c7a5e9f4e..feddef300ccc 100644 > --- a/include/command.h > +++ b/include/command.h > @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; > #if defined(CONFIG_CMD_RUN) > extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); > #endif > +#if defined(CONFIG_CMD_BOOTEFI) > +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); > +#endif > > /* common/command.c */ > int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int >
On Thu, Feb 28, 2019 at 05:53:17AM +0100, Heinrich Schuchardt wrote:
On 2/28/19 5:45 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote: > On 1/15/19 3:54 AM, AKASHI Takahiro wrote: >> "run -e" allows for executing EFI application with a specific "BootXXXX" >> variable. If no "BootXXXX" is specified or "BootOrder" is specified, >> it tries to run an EFI application specified in the order of "bootOrder." >> > > If we cannot specify the device tree what would be the use of this for > ARM processors?
I don't get your point. What's the matter with device tree?
To boot an ARM board on Linux or BSD you need a device tree.
When I discussed with Alex about Boot Manager (and distro_bootcmd?), he suggested that we should not allow users to specify fdt at a command line. I believe that it would apply to my case above.
IMO, we should always provide system's fdt to EFI applications.
With current Linux development practice this unfortunately does not work. Linux device trees sometimes see incompatible changes between versions. Booting may fail with a device tree that is either too old or too new for your Linux version.
E.g. for the Odroid C2 some reserved memory regions were removed from the device tree and replaced by a logic that determines them on the fly due to changes in ARM trusted firmware location.
The Wandboard rev B1 device tree was moved to a new file when a new board revision appeared and the new revision changed the old file (sic).
U-Boot is also not perfect at keeping its device tree in sync with the newest Linux device tree.
Why don't you use "fdt" command in that case? IMO, we don't need <fdt> argument at bootefi (and run -e). Obviously, I have one assumption that we need change the code to utilize "fdtaddr" variable in do_bootefi().
Such a change would mean that after an upgrade of U-Boot all boards running on Suse and Fedora suddenly will not boot again.
Why do you think so? Unless people intentionally run "fdt" command before bootefi, the system will behave in the exact same way.
How many people really expect that, in the case below, => load ... <addr> <file> => fdt addr <addr> => bootefi bootmgr bootefi will start EFI application *without* fdt?
-Takahiro Akashi
We should not change existing commands in an incompatible way.
Under the current implementation, a similar behavior is achieved only via distro_bootcmd. IMO, this should be corrected. If you agree, I will add another patch to my current patchset for this purpose.
I suggest to drop patch 5/5 from your series.
Best regards
Heinrich
So run -e Boot0001 will not allow you to boot into Linux because it does not specify a device tree.
> Why do you add the option to run and not to bootefi? > > You already introduced the capability to set BootNext. Why isn't that > enough?
Simple.
=> run -e Boot00> versus
=> efidebug boot next 1 => bootefi bootmgr
In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
So there is no need to go through efidebug.
I think we should avoid alternative commands.
As I said, I already removed this feature from bootefi.
First of all, efidebug is only recognized as a *debugging* tool. I believe that the former syntax is more intuitive, and it looks a natural extension to "run" command semantics akin to "env -e".
As a result, we don't have to know about bootefi for normal operations.
But you have to know about 'run' which you might not need otherwise.
"run" is much better known to U-Boot users than bootefi.
Do you have a statistic ;)
Up to now booting always required a command starting on boot...
What I meant is that people need not learn more commands.
# Relating to secure boot, I'm now thinking of pulling bootmgr out of # bootefi and making it as a single command. In that case, # bootefi does exist only for hello and selftest.
-Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
> Best regards > > Heinrich > >> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >> --- >> cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ >> cmd/nvedit.c | 9 ++++++++- >> common/cli.c | 10 ++++++++++ >> include/command.h | 3 +++ >> 4 files changed, 52 insertions(+), 1 deletion(-) >> >> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >> index 241fd0f987ab..ebe149dffa1f 100644 >> --- a/cmd/bootefi.c >> +++ b/cmd/bootefi.c >> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) >> return CMD_RET_SUCCESS; >> } >> >> +/* Called by "run" command */ >> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> +{ >> + int boot_id = -1; >> + char *endp; >> + >> + if (argc > 2) >> + return CMD_RET_USAGE; >> + >> + if (argc == 2) { >> + if (!strcmp(argv[1], "BootOrder")) { >> + boot_id = -1; >> + } else if (!strncmp(argv[2], "Boot", 4)) { >> + boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0); >> + if ((argv[2] + strlen(argv[2]) != endp) || >> + boot_id > 0xffff) >> + return CMD_RET_USAGE; >> + } else { >> + return CMD_RET_USAGE; >> + } >> + } >> + >> + if (efi_init_obj_list()) >> + return CMD_RET_FAILURE; >> + >> + if (efi_handle_fdt(NULL)) >> + return CMD_RET_FAILURE; >> + >> + return do_bootefi_bootmgr_exec(boot_id); >> +} >> + >> /* Interpreter command to boot an arbitrary EFI image from memory */ >> static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> { >> diff --git a/cmd/nvedit.c b/cmd/nvedit.c >> index de16c72c23f2..ce746bbf1b3e 100644 >> --- a/cmd/nvedit.c >> +++ b/cmd/nvedit.c >> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( >> run, CONFIG_SYS_MAXARGS, 1, do_run, >> "run commands in an environment variable", >> "var [...]\n" >> - " - run the commands in the environment variable(s) 'var'", >> + " - run the commands in the environment variable(s) 'var'" >> +#if defined(CONFIG_CMD_BOOTEFI) >> + "\n" >> + "run -e [BootXXXX]\n" >> + " - load and run UEFI app based on 'BootXXXX' UEFI variable", >> +#else >> + , >> +#endif >> var_complete >> ); >> #endif >> diff --git a/common/cli.c b/common/cli.c >> index 51b8d5f85cbb..fbb09d5049a4 100644 >> --- a/common/cli.c >> +++ b/common/cli.c >> @@ -12,6 +12,7 @@ >> #include <cli.h> >> #include <cli_hush.h> >> #include <console.h> >> +#include <efi_loader.h> >> #include <fdtdec.h> >> #include <malloc.h> >> >> @@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> if (argc < 2) >> return CMD_RET_USAGE; >> >> +#ifdef CONFIG_CMD_BOOTEFI >> + if (!strcmp(argv[1], "-e")) { >> + argc--; >> + argv++; >> + >> + return do_bootefi_run(cmdtp, flag, argc, argv); >> + } >> +#endif >> + >> for (i = 1; i < argc; ++i) { >> char *arg; >> >> diff --git a/include/command.h b/include/command.h >> index 200c7a5e9f4e..feddef300ccc 100644 >> --- a/include/command.h >> +++ b/include/command.h >> @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; >> #if defined(CONFIG_CMD_RUN) >> extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); >> #endif >> +#if defined(CONFIG_CMD_BOOTEFI) >> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); >> +#endif >> >> /* common/command.c */ >> int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int >> >
On 2/28/19 6:06 AM, AKASHI Takahiro wrote:
On Thu, Feb 28, 2019 at 05:53:17AM +0100, Heinrich Schuchardt wrote:
On 2/28/19 5:45 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:12 AM, AKASHI Takahiro wrote: > On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote: >> On 1/15/19 3:54 AM, AKASHI Takahiro wrote: >>> "run -e" allows for executing EFI application with a specific "BootXXXX" >>> variable. If no "BootXXXX" is specified or "BootOrder" is specified, >>> it tries to run an EFI application specified in the order of "bootOrder." >>> >> >> If we cannot specify the device tree what would be the use of this for >> ARM processors? > > I don't get your point. What's the matter with device tree?
To boot an ARM board on Linux or BSD you need a device tree.
When I discussed with Alex about Boot Manager (and distro_bootcmd?), he suggested that we should not allow users to specify fdt at a command line. I believe that it would apply to my case above.
IMO, we should always provide system's fdt to EFI applications.
With current Linux development practice this unfortunately does not work. Linux device trees sometimes see incompatible changes between versions. Booting may fail with a device tree that is either too old or too new for your Linux version.
E.g. for the Odroid C2 some reserved memory regions were removed from the device tree and replaced by a logic that determines them on the fly due to changes in ARM trusted firmware location.
The Wandboard rev B1 device tree was moved to a new file when a new board revision appeared and the new revision changed the old file (sic).
U-Boot is also not perfect at keeping its device tree in sync with the newest Linux device tree.
Why don't you use "fdt" command in that case? IMO, we don't need <fdt> argument at bootefi (and run -e). Obviously, I have one assumption that we need change the code to utilize "fdtaddr" variable in do_bootefi().
Such a change would mean that after an upgrade of U-Boot all boards running on Suse and Fedora suddenly will not boot again.
Why do you think so? Unless people intentionally run "fdt" command before bootefi, the system will behave in the exact same way.
How many people really expect that, in the case below, => load ... <addr> <file> => fdt addr <addr> => bootefi bootmgr bootefi will start EFI application *without* fdt?
-Takahiro Akashi
Your previous mail sounded to me as if you wanted to drop the possibility to specify an FDT address in the bootefi command. But maybe I got you wrong.
If your idea is that we should use the address specified in command fdt and $fdtcontroladdr as fallbacks if no FDT address is specified, that is another story.
Best regards
Heinrich
We should not change existing commands in an incompatible way.
Under the current implementation, a similar behavior is achieved only via distro_bootcmd. IMO, this should be corrected. If you agree, I will add another patch to my current patchset for this purpose.
I suggest to drop patch 5/5 from your series.
Best regards
Heinrich
So run -e Boot0001 will not allow you to boot into Linux because it does not specify a device tree.
> > >> Why do you add the option to run and not to bootefi? >> >> You already introduced the capability to set BootNext. Why isn't that >> enough? > > Simple. > > => run -e Boot00> > versus > > => efidebug boot next 1 > => bootefi bootmgr
In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
So there is no need to go through efidebug.
I think we should avoid alternative commands.
As I said, I already removed this feature from bootefi.
> > First of all, efidebug is only recognized as a *debugging* tool. > I believe that the former syntax is more intuitive, and it looks > a natural extension to "run" command semantics akin to "env -e". > > As a result, we don't have to know about bootefi for normal operations.
But you have to know about 'run' which you might not need otherwise.
"run" is much better known to U-Boot users than bootefi.
Do you have a statistic ;)
Up to now booting always required a command starting on boot...
What I meant is that people need not learn more commands.
# Relating to secure boot, I'm now thinking of pulling bootmgr out of # bootefi and making it as a single command. In that case, # bootefi does exist only for hello and selftest.
-Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
Best regards
Heinrich
> > Thanks, > -Takahiro Akashi > > >> Best regards >> >> Heinrich >> >>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >>> --- >>> cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ >>> cmd/nvedit.c | 9 ++++++++- >>> common/cli.c | 10 ++++++++++ >>> include/command.h | 3 +++ >>> 4 files changed, 52 insertions(+), 1 deletion(-) >>> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>> index 241fd0f987ab..ebe149dffa1f 100644 >>> --- a/cmd/bootefi.c >>> +++ b/cmd/bootefi.c >>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) >>> return CMD_RET_SUCCESS; >>> } >>> >>> +/* Called by "run" command */ >>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>> +{ >>> + int boot_id = -1; >>> + char *endp; >>> + >>> + if (argc > 2) >>> + return CMD_RET_USAGE; >>> + >>> + if (argc == 2) { >>> + if (!strcmp(argv[1], "BootOrder")) { >>> + boot_id = -1; >>> + } else if (!strncmp(argv[2], "Boot", 4)) { >>> + boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0); >>> + if ((argv[2] + strlen(argv[2]) != endp) || >>> + boot_id > 0xffff) >>> + return CMD_RET_USAGE; >>> + } else { >>> + return CMD_RET_USAGE; >>> + } >>> + } >>> + >>> + if (efi_init_obj_list()) >>> + return CMD_RET_FAILURE; >>> + >>> + if (efi_handle_fdt(NULL)) >>> + return CMD_RET_FAILURE; >>> + >>> + return do_bootefi_bootmgr_exec(boot_id); >>> +} >>> + >>> /* Interpreter command to boot an arbitrary EFI image from memory */ >>> static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>> { >>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c >>> index de16c72c23f2..ce746bbf1b3e 100644 >>> --- a/cmd/nvedit.c >>> +++ b/cmd/nvedit.c >>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( >>> run, CONFIG_SYS_MAXARGS, 1, do_run, >>> "run commands in an environment variable", >>> "var [...]\n" >>> - " - run the commands in the environment variable(s) 'var'", >>> + " - run the commands in the environment variable(s) 'var'" >>> +#if defined(CONFIG_CMD_BOOTEFI) >>> + "\n" >>> + "run -e [BootXXXX]\n" >>> + " - load and run UEFI app based on 'BootXXXX' UEFI variable", >>> +#else >>> + , >>> +#endif >>> var_complete >>> ); >>> #endif >>> diff --git a/common/cli.c b/common/cli.c >>> index 51b8d5f85cbb..fbb09d5049a4 100644 >>> --- a/common/cli.c >>> +++ b/common/cli.c >>> @@ -12,6 +12,7 @@ >>> #include <cli.h> >>> #include <cli_hush.h> >>> #include <console.h> >>> +#include <efi_loader.h> >>> #include <fdtdec.h> >>> #include <malloc.h> >>> >>> @@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>> if (argc < 2) >>> return CMD_RET_USAGE; >>> >>> +#ifdef CONFIG_CMD_BOOTEFI >>> + if (!strcmp(argv[1], "-e")) { >>> + argc--; >>> + argv++; >>> + >>> + return do_bootefi_run(cmdtp, flag, argc, argv); >>> + } >>> +#endif >>> + >>> for (i = 1; i < argc; ++i) { >>> char *arg; >>> >>> diff --git a/include/command.h b/include/command.h >>> index 200c7a5e9f4e..feddef300ccc 100644 >>> --- a/include/command.h >>> +++ b/include/command.h >>> @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; >>> #if defined(CONFIG_CMD_RUN) >>> extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); >>> #endif >>> +#if defined(CONFIG_CMD_BOOTEFI) >>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); >>> +#endif >>> >>> /* common/command.c */ >>> int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int >>> >> >
On Thu, Feb 28, 2019 at 06:13:39AM +0100, Heinrich Schuchardt wrote:
On 2/28/19 6:06 AM, AKASHI Takahiro wrote:
On Thu, Feb 28, 2019 at 05:53:17AM +0100, Heinrich Schuchardt wrote:
On 2/28/19 5:45 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote: > On 2/27/19 7:12 AM, AKASHI Takahiro wrote: >> On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote: >>> On 1/15/19 3:54 AM, AKASHI Takahiro wrote: >>>> "run -e" allows for executing EFI application with a specific "BootXXXX" >>>> variable. If no "BootXXXX" is specified or "BootOrder" is specified, >>>> it tries to run an EFI application specified in the order of "bootOrder." >>>> >>> >>> If we cannot specify the device tree what would be the use of this for >>> ARM processors? >> >> I don't get your point. What's the matter with device tree? > > To boot an ARM board on Linux or BSD you need a device tree.
When I discussed with Alex about Boot Manager (and distro_bootcmd?), he suggested that we should not allow users to specify fdt at a command line. I believe that it would apply to my case above.
IMO, we should always provide system's fdt to EFI applications.
With current Linux development practice this unfortunately does not work. Linux device trees sometimes see incompatible changes between versions. Booting may fail with a device tree that is either too old or too new for your Linux version.
E.g. for the Odroid C2 some reserved memory regions were removed from the device tree and replaced by a logic that determines them on the fly due to changes in ARM trusted firmware location.
The Wandboard rev B1 device tree was moved to a new file when a new board revision appeared and the new revision changed the old file (sic).
U-Boot is also not perfect at keeping its device tree in sync with the newest Linux device tree.
Why don't you use "fdt" command in that case? IMO, we don't need <fdt> argument at bootefi (and run -e). Obviously, I have one assumption that we need change the code to utilize "fdtaddr" variable in do_bootefi().
Such a change would mean that after an upgrade of U-Boot all boards running on Suse and Fedora suddenly will not boot again.
Why do you think so? Unless people intentionally run "fdt" command before bootefi, the system will behave in the exact same way.
How many people really expect that, in the case below, => load ... <addr> <file> => fdt addr <addr> => bootefi bootmgr bootefi will start EFI application *without* fdt?
-Takahiro Akashi
Your previous mail sounded to me as if you wanted to drop the possibility to specify an FDT address in the bootefi command. But maybe I got you wrong.
No, even in v2, I didn't change any semantics in do_bootefi() and hence bootefi command.
If your idea is that we should use the address specified in command fdt and $fdtcontroladdr as fallbacks if no FDT address is specified, that is another story.
Exactly for "run -e" case (and probably a new command, efibootmgr, too).
The only concern is an incompatibility with distro_bootcmd. It has yet another logic regarding fdt.
-Takahiro Akashi
Best regards
Heinrich
We should not change existing commands in an incompatible way.
Under the current implementation, a similar behavior is achieved only via distro_bootcmd. IMO, this should be corrected. If you agree, I will add another patch to my current patchset for this purpose.
I suggest to drop patch 5/5 from your series.
Best regards
Heinrich
> So run -e Boot0001 will not allow you to boot into Linux because it does > not specify a device tree. > >> >> >>> Why do you add the option to run and not to bootefi? >>> >>> You already introduced the capability to set BootNext. Why isn't that >>> enough? >> >> Simple. >> >> => run -e Boot00> >> versus >> >> => efidebug boot next 1 >> => bootefi bootmgr > > In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001' > > So there is no need to go through efidebug. > > I think we should avoid alternative commands.
As I said, I already removed this feature from bootefi.
>> >> First of all, efidebug is only recognized as a *debugging* tool. >> I believe that the former syntax is more intuitive, and it looks >> a natural extension to "run" command semantics akin to "env -e". >> >> As a result, we don't have to know about bootefi for normal operations. > > But you have to know about 'run' which you might not need otherwise.
"run" is much better known to U-Boot users than bootefi.
Do you have a statistic ;)
Up to now booting always required a command starting on boot...
What I meant is that people need not learn more commands.
# Relating to secure boot, I'm now thinking of pulling bootmgr out of # bootefi and making it as a single command. In that case, # bootefi does exist only for hello and selftest.
-Takahiro Akashi
Best regards
Heinrich
Thanks, -Takahiro Akashi
> Best regards > > Heinrich > >> >> Thanks, >> -Takahiro Akashi >> >> >>> Best regards >>> >>> Heinrich >>> >>>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >>>> --- >>>> cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ >>>> cmd/nvedit.c | 9 ++++++++- >>>> common/cli.c | 10 ++++++++++ >>>> include/command.h | 3 +++ >>>> 4 files changed, 52 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>>> index 241fd0f987ab..ebe149dffa1f 100644 >>>> --- a/cmd/bootefi.c >>>> +++ b/cmd/bootefi.c >>>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) >>>> return CMD_RET_SUCCESS; >>>> } >>>> >>>> +/* Called by "run" command */ >>>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>> +{ >>>> + int boot_id = -1; >>>> + char *endp; >>>> + >>>> + if (argc > 2) >>>> + return CMD_RET_USAGE; >>>> + >>>> + if (argc == 2) { >>>> + if (!strcmp(argv[1], "BootOrder")) { >>>> + boot_id = -1; >>>> + } else if (!strncmp(argv[2], "Boot", 4)) { >>>> + boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0); >>>> + if ((argv[2] + strlen(argv[2]) != endp) || >>>> + boot_id > 0xffff) >>>> + return CMD_RET_USAGE; >>>> + } else { >>>> + return CMD_RET_USAGE; >>>> + } >>>> + } >>>> + >>>> + if (efi_init_obj_list()) >>>> + return CMD_RET_FAILURE; >>>> + >>>> + if (efi_handle_fdt(NULL)) >>>> + return CMD_RET_FAILURE; >>>> + >>>> + return do_bootefi_bootmgr_exec(boot_id); >>>> +} >>>> + >>>> /* Interpreter command to boot an arbitrary EFI image from memory */ >>>> static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>> { >>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c >>>> index de16c72c23f2..ce746bbf1b3e 100644 >>>> --- a/cmd/nvedit.c >>>> +++ b/cmd/nvedit.c >>>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( >>>> run, CONFIG_SYS_MAXARGS, 1, do_run, >>>> "run commands in an environment variable", >>>> "var [...]\n" >>>> - " - run the commands in the environment variable(s) 'var'", >>>> + " - run the commands in the environment variable(s) 'var'" >>>> +#if defined(CONFIG_CMD_BOOTEFI) >>>> + "\n" >>>> + "run -e [BootXXXX]\n" >>>> + " - load and run UEFI app based on 'BootXXXX' UEFI variable", >>>> +#else >>>> + , >>>> +#endif >>>> var_complete >>>> ); >>>> #endif >>>> diff --git a/common/cli.c b/common/cli.c >>>> index 51b8d5f85cbb..fbb09d5049a4 100644 >>>> --- a/common/cli.c >>>> +++ b/common/cli.c >>>> @@ -12,6 +12,7 @@ >>>> #include <cli.h> >>>> #include <cli_hush.h> >>>> #include <console.h> >>>> +#include <efi_loader.h> >>>> #include <fdtdec.h> >>>> #include <malloc.h> >>>> >>>> @@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>> if (argc < 2) >>>> return CMD_RET_USAGE; >>>> >>>> +#ifdef CONFIG_CMD_BOOTEFI >>>> + if (!strcmp(argv[1], "-e")) { >>>> + argc--; >>>> + argv++; >>>> + >>>> + return do_bootefi_run(cmdtp, flag, argc, argv); >>>> + } >>>> +#endif >>>> + >>>> for (i = 1; i < argc; ++i) { >>>> char *arg; >>>> >>>> diff --git a/include/command.h b/include/command.h >>>> index 200c7a5e9f4e..feddef300ccc 100644 >>>> --- a/include/command.h >>>> +++ b/include/command.h >>>> @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; >>>> #if defined(CONFIG_CMD_RUN) >>>> extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); >>>> #endif >>>> +#if defined(CONFIG_CMD_BOOTEFI) >>>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); >>>> +#endif >>>> >>>> /* common/command.c */ >>>> int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int >>>> >>> >> >
Heinrich,
On Fri, Mar 01, 2019 at 10:22:13AM +0900, AKASHI Takahiro wrote:
On Thu, Feb 28, 2019 at 06:13:39AM +0100, Heinrich Schuchardt wrote:
On 2/28/19 6:06 AM, AKASHI Takahiro wrote:
On Thu, Feb 28, 2019 at 05:53:17AM +0100, Heinrich Schuchardt wrote:
On 2/28/19 5:45 AM, AKASHI Takahiro wrote:
On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
On 2/27/19 7:37 AM, AKASHI Takahiro wrote: > On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote: >> On 2/27/19 7:12 AM, AKASHI Takahiro wrote: >>> On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote: >>>> On 1/15/19 3:54 AM, AKASHI Takahiro wrote: >>>>> "run -e" allows for executing EFI application with a specific "BootXXXX" >>>>> variable. If no "BootXXXX" is specified or "BootOrder" is specified, >>>>> it tries to run an EFI application specified in the order of "bootOrder." >>>>> >>>> >>>> If we cannot specify the device tree what would be the use of this for >>>> ARM processors? >>> >>> I don't get your point. What's the matter with device tree? >> >> To boot an ARM board on Linux or BSD you need a device tree. > > When I discussed with Alex about Boot Manager (and distro_bootcmd?), > he suggested that we should not allow users to specify fdt at a command line. > I believe that it would apply to my case above. > > IMO, we should always provide system's fdt to EFI applications.
With current Linux development practice this unfortunately does not work. Linux device trees sometimes see incompatible changes between versions. Booting may fail with a device tree that is either too old or too new for your Linux version.
E.g. for the Odroid C2 some reserved memory regions were removed from the device tree and replaced by a logic that determines them on the fly due to changes in ARM trusted firmware location.
The Wandboard rev B1 device tree was moved to a new file when a new board revision appeared and the new revision changed the old file (sic).
U-Boot is also not perfect at keeping its device tree in sync with the newest Linux device tree.
Why don't you use "fdt" command in that case? IMO, we don't need <fdt> argument at bootefi (and run -e). Obviously, I have one assumption that we need change the code to utilize "fdtaddr" variable in do_bootefi().
Such a change would mean that after an upgrade of U-Boot all boards running on Suse and Fedora suddenly will not boot again.
Why do you think so? Unless people intentionally run "fdt" command before bootefi, the system will behave in the exact same way.
How many people really expect that, in the case below, => load ... <addr> <file> => fdt addr <addr> => bootefi bootmgr bootefi will start EFI application *without* fdt?
-Takahiro Akashi
Your previous mail sounded to me as if you wanted to drop the possibility to specify an FDT address in the bootefi command. But maybe I got you wrong.
No, even in v2, I didn't change any semantics in do_bootefi() and hence bootefi command.
If your idea is that we should use the address specified in command fdt and $fdtcontroladdr as fallbacks if no FDT address is specified, that is another story.
Exactly for "run -e" case (and probably a new command, efibootmgr, too).
The only concern is an incompatibility with distro_bootcmd. It has yet another logic regarding fdt.
I changed my mind a bit. I dropped "run -e" patch, and posted "BootNext" patch on its own. I'd like to focus more on re-working do_bootefi(). See my next RFC.
Thanks, -Takahiro Akashi
-Takahiro Akashi
Best regards
Heinrich
We should not change existing commands in an incompatible way.
Under the current implementation, a similar behavior is achieved only via distro_bootcmd. IMO, this should be corrected. If you agree, I will add another patch to my current patchset for this purpose.
I suggest to drop patch 5/5 from your series.
Best regards
Heinrich
> >> So run -e Boot0001 will not allow you to boot into Linux because it does >> not specify a device tree. >> >>> >>> >>>> Why do you add the option to run and not to bootefi? >>>> >>>> You already introduced the capability to set BootNext. Why isn't that >>>> enough? >>> >>> Simple. >>> >>> => run -e Boot00> >>> versus >>> >>> => efidebug boot next 1 >>> => bootefi bootmgr >> >> In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001' >> >> So there is no need to go through efidebug. >> >> I think we should avoid alternative commands. > > As I said, I already removed this feature from bootefi. > >>> >>> First of all, efidebug is only recognized as a *debugging* tool. >>> I believe that the former syntax is more intuitive, and it looks >>> a natural extension to "run" command semantics akin to "env -e". >>> >>> As a result, we don't have to know about bootefi for normal operations. >> >> But you have to know about 'run' which you might not need otherwise. > > "run" is much better known to U-Boot users than bootefi.
Do you have a statistic ;)
Up to now booting always required a command starting on boot...
What I meant is that people need not learn more commands.
# Relating to secure boot, I'm now thinking of pulling bootmgr out of # bootefi and making it as a single command. In that case, # bootefi does exist only for hello and selftest.
-Takahiro Akashi
Best regards
Heinrich
> > Thanks, > -Takahiro Akashi > >> Best regards >> >> Heinrich >> >>> >>> Thanks, >>> -Takahiro Akashi >>> >>> >>>> Best regards >>>> >>>> Heinrich >>>> >>>>> Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org >>>>> --- >>>>> cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ >>>>> cmd/nvedit.c | 9 ++++++++- >>>>> common/cli.c | 10 ++++++++++ >>>>> include/command.h | 3 +++ >>>>> 4 files changed, 52 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>>>> index 241fd0f987ab..ebe149dffa1f 100644 >>>>> --- a/cmd/bootefi.c >>>>> +++ b/cmd/bootefi.c >>>>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) >>>>> return CMD_RET_SUCCESS; >>>>> } >>>>> >>>>> +/* Called by "run" command */ >>>>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>>> +{ >>>>> + int boot_id = -1; >>>>> + char *endp; >>>>> + >>>>> + if (argc > 2) >>>>> + return CMD_RET_USAGE; >>>>> + >>>>> + if (argc == 2) { >>>>> + if (!strcmp(argv[1], "BootOrder")) { >>>>> + boot_id = -1; >>>>> + } else if (!strncmp(argv[2], "Boot", 4)) { >>>>> + boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0); >>>>> + if ((argv[2] + strlen(argv[2]) != endp) || >>>>> + boot_id > 0xffff) >>>>> + return CMD_RET_USAGE; >>>>> + } else { >>>>> + return CMD_RET_USAGE; >>>>> + } >>>>> + } >>>>> + >>>>> + if (efi_init_obj_list()) >>>>> + return CMD_RET_FAILURE; >>>>> + >>>>> + if (efi_handle_fdt(NULL)) >>>>> + return CMD_RET_FAILURE; >>>>> + >>>>> + return do_bootefi_bootmgr_exec(boot_id); >>>>> +} >>>>> + >>>>> /* Interpreter command to boot an arbitrary EFI image from memory */ >>>>> static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>>> { >>>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c >>>>> index de16c72c23f2..ce746bbf1b3e 100644 >>>>> --- a/cmd/nvedit.c >>>>> +++ b/cmd/nvedit.c >>>>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( >>>>> run, CONFIG_SYS_MAXARGS, 1, do_run, >>>>> "run commands in an environment variable", >>>>> "var [...]\n" >>>>> - " - run the commands in the environment variable(s) 'var'", >>>>> + " - run the commands in the environment variable(s) 'var'" >>>>> +#if defined(CONFIG_CMD_BOOTEFI) >>>>> + "\n" >>>>> + "run -e [BootXXXX]\n" >>>>> + " - load and run UEFI app based on 'BootXXXX' UEFI variable", >>>>> +#else >>>>> + , >>>>> +#endif >>>>> var_complete >>>>> ); >>>>> #endif >>>>> diff --git a/common/cli.c b/common/cli.c >>>>> index 51b8d5f85cbb..fbb09d5049a4 100644 >>>>> --- a/common/cli.c >>>>> +++ b/common/cli.c >>>>> @@ -12,6 +12,7 @@ >>>>> #include <cli.h> >>>>> #include <cli_hush.h> >>>>> #include <console.h> >>>>> +#include <efi_loader.h> >>>>> #include <fdtdec.h> >>>>> #include <malloc.h> >>>>> >>>>> @@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>>> if (argc < 2) >>>>> return CMD_RET_USAGE; >>>>> >>>>> +#ifdef CONFIG_CMD_BOOTEFI >>>>> + if (!strcmp(argv[1], "-e")) { >>>>> + argc--; >>>>> + argv++; >>>>> + >>>>> + return do_bootefi_run(cmdtp, flag, argc, argv); >>>>> + } >>>>> +#endif >>>>> + >>>>> for (i = 1; i < argc; ++i) { >>>>> char *arg; >>>>> >>>>> diff --git a/include/command.h b/include/command.h >>>>> index 200c7a5e9f4e..feddef300ccc 100644 >>>>> --- a/include/command.h >>>>> +++ b/include/command.h >>>>> @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; >>>>> #if defined(CONFIG_CMD_RUN) >>>>> extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); >>>>> #endif >>>>> +#if defined(CONFIG_CMD_BOOTEFI) >>>>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); >>>>> +#endif >>>>> >>>>> /* common/command.c */ >>>>> int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int >>>>> >>>> >>> >> >
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
"run -e" allows for executing EFI application with a specific "BootXXXX" variable. If no "BootXXXX" is specified or "BootOrder" is specified, it tries to run an EFI application specified in the order of "bootOrder."
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ cmd/nvedit.c | 9 ++++++++- common/cli.c | 10 ++++++++++ include/command.h | 3 +++ 4 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 241fd0f987ab..ebe149dffa1f 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) return CMD_RET_SUCCESS; }
+/* Called by "run" command */ +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{
- int boot_id = -1;
- char *endp;
- if (argc > 2)
return CMD_RET_USAGE;- if (argc == 2) {
if (!strcmp(argv[1], "BootOrder")) {boot_id = -1;} else if (!strncmp(argv[2], "Boot", 4)) {boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0);if ((argv[2] + strlen(argv[2]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;} else {return CMD_RET_USAGE;}- }
- if (efi_init_obj_list())
return CMD_RET_FAILURE;- if (efi_handle_fdt(NULL))
return CMD_RET_FAILURE;- return do_bootefi_bootmgr_exec(boot_id);
This function does not invoke:
allow_unaligned(); switch_to_non_secure_mode();
Probably these calls should be moved to efi_init_obj_list() anyway.
Best regards
Heinrich
+}
/* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { diff --git a/cmd/nvedit.c b/cmd/nvedit.c index de16c72c23f2..ce746bbf1b3e 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( run, CONFIG_SYS_MAXARGS, 1, do_run, "run commands in an environment variable", "var [...]\n"
- " - run the commands in the environment variable(s) 'var'",
- " - run the commands in the environment variable(s) 'var'"
+#if defined(CONFIG_CMD_BOOTEFI)
- "\n"
- "run -e [BootXXXX]\n"
- " - load and run UEFI app based on 'BootXXXX' UEFI variable",
+#else
- ,
+#endif var_complete ); #endif diff --git a/common/cli.c b/common/cli.c index 51b8d5f85cbb..fbb09d5049a4 100644 --- a/common/cli.c +++ b/common/cli.c @@ -12,6 +12,7 @@ #include <cli.h> #include <cli_hush.h> #include <console.h> +#include <efi_loader.h> #include <fdtdec.h> #include <malloc.h>
@@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
+#ifdef CONFIG_CMD_BOOTEFI
- if (!strcmp(argv[1], "-e")) {
argc--;argv++;return do_bootefi_run(cmdtp, flag, argc, argv);- }
+#endif
- for (i = 1; i < argc; ++i) { char *arg;
diff --git a/include/command.h b/include/command.h index 200c7a5e9f4e..feddef300ccc 100644 --- a/include/command.h +++ b/include/command.h @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; #if defined(CONFIG_CMD_RUN) extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); #endif +#if defined(CONFIG_CMD_BOOTEFI) +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); +#endif
/* common/command.c */ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
On Thu, Feb 28, 2019 at 09:59:14PM +0100, Heinrich Schuchardt wrote:
On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
"run -e" allows for executing EFI application with a specific "BootXXXX" variable. If no "BootXXXX" is specified or "BootOrder" is specified, it tries to run an EFI application specified in the order of "bootOrder."
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++ cmd/nvedit.c | 9 ++++++++- common/cli.c | 10 ++++++++++ include/command.h | 3 +++ 4 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 241fd0f987ab..ebe149dffa1f 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id) return CMD_RET_SUCCESS; }
+/* Called by "run" command */ +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{
- int boot_id = -1;
- char *endp;
- if (argc > 2)
return CMD_RET_USAGE;- if (argc == 2) {
if (!strcmp(argv[1], "BootOrder")) {boot_id = -1;} else if (!strncmp(argv[2], "Boot", 4)) {boot_id = (int)simple_strtoul(&argv[2][4], &endp, 0);if ((argv[2] + strlen(argv[2]) != endp) ||boot_id > 0xffff)return CMD_RET_USAGE;} else {return CMD_RET_USAGE;}- }
- if (efi_init_obj_list())
return CMD_RET_FAILURE;- if (efi_handle_fdt(NULL))
return CMD_RET_FAILURE;- return do_bootefi_bootmgr_exec(boot_id);
This function does not invoke:
allow_unaligned(); switch_to_non_secure_mode();
I've already fixed it in my pre-v3 patch.
Probably these calls should be moved to efi_init_obj_list() anyway.
Can be, but the name no longer represents its function.
-Takahiro Akashi
Best regards
Heinrich
+}
/* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { diff --git a/cmd/nvedit.c b/cmd/nvedit.c index de16c72c23f2..ce746bbf1b3e 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE( run, CONFIG_SYS_MAXARGS, 1, do_run, "run commands in an environment variable", "var [...]\n"
- " - run the commands in the environment variable(s) 'var'",
- " - run the commands in the environment variable(s) 'var'"
+#if defined(CONFIG_CMD_BOOTEFI)
- "\n"
- "run -e [BootXXXX]\n"
- " - load and run UEFI app based on 'BootXXXX' UEFI variable",
+#else
- ,
+#endif var_complete ); #endif diff --git a/common/cli.c b/common/cli.c index 51b8d5f85cbb..fbb09d5049a4 100644 --- a/common/cli.c +++ b/common/cli.c @@ -12,6 +12,7 @@ #include <cli.h> #include <cli_hush.h> #include <console.h> +#include <efi_loader.h> #include <fdtdec.h> #include <malloc.h>
@@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
+#ifdef CONFIG_CMD_BOOTEFI
- if (!strcmp(argv[1], "-e")) {
argc--;argv++;return do_bootefi_run(cmdtp, flag, argc, argv);- }
+#endif
- for (i = 1; i < argc; ++i) { char *arg;
diff --git a/include/command.h b/include/command.h index 200c7a5e9f4e..feddef300ccc 100644 --- a/include/command.h +++ b/include/command.h @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t; #if defined(CONFIG_CMD_RUN) extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); #endif +#if defined(CONFIG_CMD_BOOTEFI) +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); +#endif
/* common/command.c */ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
participants (2)
-
AKASHI Takahiro -
Heinrich Schuchardt