[1/5] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior

Message ID 20181218050257.20142-2-takahiro.akashi@linaro.org
State Superseded
Headers show
Series
  • efi_loader: run a specific efi application more easily
Related show

Commit Message

AKASHI Takahiro Dec. 18, 2018, 5:02 a.m.
See UEFI v2.7, section 3.1.2 for details of the specification.

With my efishell command[1], 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 setvar BootNext =H0200
  => efi bootmgr
     (starting HELLO ...)
  => efi dumpvar
  <snip ...>
  BootCurrent: {boot,run}(blob)
  00000000:  02 00                    ..
  BootOrder: {boot,run}(blob)
  00000000:  01 00 02 00              ....

Using "run -e" would be more human-friendly, though.

[1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

Comments

Alexander Graf Dec. 23, 2018, 2:03 a.m. | #1
On 18.12.18 06:02, AKASHI Takahiro wrote:
> See UEFI v2.7, section 3.1.2 for details of the specification.
> 
> With my efishell command[1], 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 setvar BootNext =H0200
>   => efi bootmgr
>      (starting HELLO ...)
>   => efi dumpvar
>   <snip ...>
>   BootCurrent: {boot,run}(blob)
>   00000000:  02 00                    ..
>   BootOrder: {boot,run}(blob)
>   00000000:  01 00 02 00              ....
> 
> Using "run -e" would be more human-friendly, though.
> 
> [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index a095df3f540b..a54ae28343ce 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);

Every call into UEFI land (rs->foo(), bs->foo(), etc) has to go via the
EFI_CALL() macro. Otherwise we may destroy the "gd" pointer on ARM.

> +		if (ret != EFI_SUCCESS)
> +			goto error;
> +
>  		ret = efi_load_image_from_path(lo.file_path, &image);
>  
>  		if (ret != EFI_SUCCESS)
> @@ -173,16 +183,41 @@ error:
>  void *efi_bootmgr_load(struct efi_device_path **device_path,
>  		       struct efi_device_path **file_path)
>  {
> -	uint16_t *bootorder;
> +	u16 bootnext, *bootorder;
> +	u32 attributes;
>  	efi_uintn_t size;
>  	void *image = NULL;
>  	int i, num;
> +	efi_status_t ret;
>  
>  	__efi_entry_check();
>  
>  	bs = systab.boottime;
>  	rs = systab.runtime;
>  
> +	/* BootNext */
> +	size = sizeof(bootnext);
> +	ret = rs->get_variable(L"BootNext",
> +			       (efi_guid_t *)&efi_global_variable_guid,
> +			       NULL, &size, &bootnext);

... same here.

> +	if (!bootnext)
> +		goto run_list;
> +
> +	attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +		     EFI_VARIABLE_RUNTIME_ACCESS;
> +	size = 0;
> +	ret = rs->set_variable(L"BootNext",
> +			       (efi_guid_t *)&efi_global_variable_guid,
> +			       attributes, size, &bootnext);

... same here.


Alex

> +	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;
>
AKASHI Takahiro Dec. 25, 2018, 9:36 a.m. | #2
On Sun, Dec 23, 2018 at 03:03:51AM +0100, Alexander Graf wrote:
> 
> 
> On 18.12.18 06:02, AKASHI Takahiro wrote:
> > See UEFI v2.7, section 3.1.2 for details of the specification.
> > 
> > With my efishell command[1], 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 setvar BootNext =H0200
> >   => efi bootmgr
> >      (starting HELLO ...)
> >   => efi dumpvar
> >   <snip ...>
> >   BootCurrent: {boot,run}(blob)
> >   00000000:  02 00                    ..
> >   BootOrder: {boot,run}(blob)
> >   00000000:  01 00 02 00              ....
> > 
> > Using "run -e" would be more human-friendly, though.
> > 
> > [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a095df3f540b..a54ae28343ce 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);
> 
> Every call into UEFI land (rs->foo(), bs->foo(), etc) has to go via the
> EFI_CALL() macro. Otherwise we may destroy the "gd" pointer on ARM.

OK, but let me make sure one thing:
My efishell calls efi_* functions directly in some places as
Not all the features can be implemented only with boot/runtime services.
In those cases, we don't have to use EFI_CALL(), right?

Thanks,
-Takahiro Akashi

> > +		if (ret != EFI_SUCCESS)
> > +			goto error;
> > +
> >  		ret = efi_load_image_from_path(lo.file_path, &image);
> >  
> >  		if (ret != EFI_SUCCESS)
> > @@ -173,16 +183,41 @@ error:
> >  void *efi_bootmgr_load(struct efi_device_path **device_path,
> >  		       struct efi_device_path **file_path)
> >  {
> > -	uint16_t *bootorder;
> > +	u16 bootnext, *bootorder;
> > +	u32 attributes;
> >  	efi_uintn_t size;
> >  	void *image = NULL;
> >  	int i, num;
> > +	efi_status_t ret;
> >  
> >  	__efi_entry_check();
> >  
> >  	bs = systab.boottime;
> >  	rs = systab.runtime;
> >  
> > +	/* BootNext */
> > +	size = sizeof(bootnext);
> > +	ret = rs->get_variable(L"BootNext",
> > +			       (efi_guid_t *)&efi_global_variable_guid,
> > +			       NULL, &size, &bootnext);
> 
> ... same here.
> 
> > +	if (!bootnext)
> > +		goto run_list;
> > +
> > +	attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +		     EFI_VARIABLE_RUNTIME_ACCESS;
> > +	size = 0;
> > +	ret = rs->set_variable(L"BootNext",
> > +			       (efi_guid_t *)&efi_global_variable_guid,
> > +			       attributes, size, &bootnext);
> 
> ... same here.
> 
> 
> Alex
> 
> > +	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;
> >
Alexander Graf Dec. 26, 2018, 9:23 p.m. | #3
On 25.12.18 10:36, AKASHI Takahiro wrote:
> On Sun, Dec 23, 2018 at 03:03:51AM +0100, Alexander Graf wrote:
>>
>>
>> On 18.12.18 06:02, AKASHI Takahiro wrote:
>>> See UEFI v2.7, section 3.1.2 for details of the specification.
>>>
>>> With my efishell command[1], 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 setvar BootNext =H0200
>>>   => efi bootmgr
>>>      (starting HELLO ...)
>>>   => efi dumpvar
>>>   <snip ...>
>>>   BootCurrent: {boot,run}(blob)
>>>   00000000:  02 00                    ..
>>>   BootOrder: {boot,run}(blob)
>>>   00000000:  01 00 02 00              ....
>>>
>>> Using "run -e" would be more human-friendly, though.
>>>
>>> [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index a095df3f540b..a54ae28343ce 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);
>>
>> Every call into UEFI land (rs->foo(), bs->foo(), etc) has to go via the
>> EFI_CALL() macro. Otherwise we may destroy the "gd" pointer on ARM.
> 
> OK, but let me make sure one thing:
> My efishell calls efi_* functions directly in some places as
> Not all the features can be implemented only with boot/runtime services.
> In those cases, we don't have to use EFI_CALL(), right?

If your "efishell" is a UEFI binary, you can directly call boot/runtime
services. If it runs as part of the U-Boot code base, every call to
boot/runtime service callbacks *must* go through EFI_CALL().


Alex
Heinrich Schuchardt Dec. 26, 2018, 9:33 p.m. | #4
On 12/26/18 10:23 PM, Alexander Graf wrote:
> 
> 
> On 25.12.18 10:36, AKASHI Takahiro wrote:
>> On Sun, Dec 23, 2018 at 03:03:51AM +0100, Alexander Graf wrote:
>>>
>>>
>>> On 18.12.18 06:02, AKASHI Takahiro wrote:
>>>> See UEFI v2.7, section 3.1.2 for details of the specification.
>>>>
>>>> With my efishell command[1], 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 setvar BootNext =H0200
>>>>   => efi bootmgr
>>>>      (starting HELLO ...)
>>>>   => efi dumpvar
>>>>   <snip ...>
>>>>   BootCurrent: {boot,run}(blob)
>>>>   00000000:  02 00                    ..
>>>>   BootOrder: {boot,run}(blob)
>>>>   00000000:  01 00 02 00              ....
>>>>
>>>> Using "run -e" would be more human-friendly, though.
>>>>
>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
>>>>
>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>> ---
>>>>  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>>> index a095df3f540b..a54ae28343ce 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);
>>>
>>> Every call into UEFI land (rs->foo(), bs->foo(), etc) has to go via the
>>> EFI_CALL() macro. Otherwise we may destroy the "gd" pointer on ARM.
>>
>> OK, but let me make sure one thing:
>> My efishell calls efi_* functions directly in some places as
>> Not all the features can be implemented only with boot/runtime services.
>> In those cases, we don't have to use EFI_CALL(), right?
> 
> If your "efishell" is a UEFI binary, you can directly call boot/runtime
> services. If it runs as part of the U-Boot code base, every call to
> boot/runtime service callbacks *must* go through EFI_CALL().

No, that is not how the call counting has been set up. You will get an
assert error if debugging is enabled. We use EFI_CALL when we want to
call an API function from inside an API function.

Just have a look at all the selftest code. It never uses EFI_CALL.

In this respect the bootmgr code is broken.

Best regards

Heinrich

> 
> 
> Alex
>
Alexander Graf Dec. 26, 2018, 9:41 p.m. | #5
On 26.12.18 22:33, Heinrich Schuchardt wrote:
> On 12/26/18 10:23 PM, Alexander Graf wrote:
>>
>>
>> On 25.12.18 10:36, AKASHI Takahiro wrote:
>>> On Sun, Dec 23, 2018 at 03:03:51AM +0100, Alexander Graf wrote:
>>>>
>>>>
>>>> On 18.12.18 06:02, AKASHI Takahiro wrote:
>>>>> See UEFI v2.7, section 3.1.2 for details of the specification.
>>>>>
>>>>> With my efishell command[1], 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 setvar BootNext =H0200
>>>>>   => efi bootmgr
>>>>>      (starting HELLO ...)
>>>>>   => efi dumpvar
>>>>>   <snip ...>
>>>>>   BootCurrent: {boot,run}(blob)
>>>>>   00000000:  02 00                    ..
>>>>>   BootOrder: {boot,run}(blob)
>>>>>   00000000:  01 00 02 00              ....
>>>>>
>>>>> Using "run -e" would be more human-friendly, though.
>>>>>
>>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
>>>>>
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>> ---
>>>>>  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
>>>>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>>>> index a095df3f540b..a54ae28343ce 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);
>>>>
>>>> Every call into UEFI land (rs->foo(), bs->foo(), etc) has to go via the
>>>> EFI_CALL() macro. Otherwise we may destroy the "gd" pointer on ARM.
>>>
>>> OK, but let me make sure one thing:
>>> My efishell calls efi_* functions directly in some places as
>>> Not all the features can be implemented only with boot/runtime services.
>>> In those cases, we don't have to use EFI_CALL(), right?
>>
>> If your "efishell" is a UEFI binary, you can directly call boot/runtime
>> services. If it runs as part of the U-Boot code base, every call to
>> boot/runtime service callbacks *must* go through EFI_CALL().
> 
> No, that is not how the call counting has been set up. You will get an
> assert error if debugging is enabled. We use EFI_CALL when we want to
> call an API function from inside an API function.

Ah, true. I stand corrected.

> Just have a look at all the selftest code. It never uses EFI_CALL.
> 
> In this respect the bootmgr code is broken.

Yes. Maybe this calls for a few travis checks with debugging enabled?


Alex
Heinrich Schuchardt Dec. 27, 2018, 4:58 a.m. | #6
On 12/18/18 6:02 AM, AKASHI Takahiro wrote:
> See UEFI v2.7, section 3.1.2 for details of the specification.
> 
> With my efishell command[1], 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 setvar BootNext =H0200

If you already have an "efi boot" sub-command wouldn't the following
syntax make more sense?

    efi boot next 2



Best regards

Heinrich

>   => efi bootmgr
>      (starting HELLO ...)
>   => efi dumpvar
>   <snip ...>
>   BootCurrent: {boot,run}(blob)
>   00000000:  02 00                    ..
>   BootOrder: {boot,run}(blob)
>   00000000:  01 00 02 00              ....
> 
> Using "run -e" would be more human-friendly, though.
> 
> [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index a095df3f540b..a54ae28343ce 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,41 @@ error:
>  void *efi_bootmgr_load(struct efi_device_path **device_path,
>  		       struct efi_device_path **file_path)
>  {
> -	uint16_t *bootorder;
> +	u16 bootnext, *bootorder;
> +	u32 attributes;
>  	efi_uintn_t size;
>  	void *image = NULL;
>  	int i, num;
> +	efi_status_t ret;
>  
>  	__efi_entry_check();
>  
>  	bs = systab.boottime;
>  	rs = systab.runtime;
>  
> +	/* 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;
> +
> +	attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +		     EFI_VARIABLE_RUNTIME_ACCESS;

According to the UEFI spec you do not need any attributes when deleting
a variable.

Please, add a comment describing what you are doing here:

/* Delete BootNext variable */

Best regards

Heinrich

> +	size = 0;
> +	ret = rs->set_variable(L"BootNext",
> +			       (efi_guid_t *)&efi_global_variable_guid,
> +			       attributes, size, &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;
>
AKASHI Takahiro Jan. 7, 2019, 6:58 a.m. | #7
On Thu, Dec 27, 2018 at 05:58:49AM +0100, Heinrich Schuchardt wrote:
> On 12/18/18 6:02 AM, AKASHI Takahiro wrote:
> > See UEFI v2.7, section 3.1.2 for details of the specification.
> > 
> > With my efishell command[1], 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 setvar BootNext =H0200
> 
> If you already have an "efi boot" sub-command wouldn't the following
> syntax make more sense?
> 
>     efi boot next 2

OK.

> 
> 
> Best regards
> 
> Heinrich
> 
> >   => efi bootmgr
> >      (starting HELLO ...)
> >   => efi dumpvar
> >   <snip ...>
> >   BootCurrent: {boot,run}(blob)
> >   00000000:  02 00                    ..
> >   BootOrder: {boot,run}(blob)
> >   00000000:  01 00 02 00              ....
> > 
> > Using "run -e" would be more human-friendly, though.
> > 
> > [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a095df3f540b..a54ae28343ce 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,41 @@ error:
> >  void *efi_bootmgr_load(struct efi_device_path **device_path,
> >  		       struct efi_device_path **file_path)
> >  {
> > -	uint16_t *bootorder;
> > +	u16 bootnext, *bootorder;
> > +	u32 attributes;
> >  	efi_uintn_t size;
> >  	void *image = NULL;
> >  	int i, num;
> > +	efi_status_t ret;
> >  
> >  	__efi_entry_check();
> >  
> >  	bs = systab.boottime;
> >  	rs = systab.runtime;
> >  
> > +	/* 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;
> > +
> > +	attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +		     EFI_VARIABLE_RUNTIME_ACCESS;
> 
> According to the UEFI spec you do not need any attributes when deleting
> a variable.

OK.

> Please, add a comment describing what you are doing here:
> 
> /* Delete BootNext variable */

OK.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> > +	size = 0;
> > +	ret = rs->set_variable(L"BootNext",
> > +			       (efi_guid_t *)&efi_global_variable_guid,
> > +			       attributes, size, &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;
> > 
>

Patch

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index a095df3f540b..a54ae28343ce 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,41 @@  error:
 void *efi_bootmgr_load(struct efi_device_path **device_path,
 		       struct efi_device_path **file_path)
 {
-	uint16_t *bootorder;
+	u16 bootnext, *bootorder;
+	u32 attributes;
 	efi_uintn_t size;
 	void *image = NULL;
 	int i, num;
+	efi_status_t ret;
 
 	__efi_entry_check();
 
 	bs = systab.boottime;
 	rs = systab.runtime;
 
+	/* 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;
+
+	attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
+		     EFI_VARIABLE_RUNTIME_ACCESS;
+	size = 0;
+	ret = rs->set_variable(L"BootNext",
+			       (efi_guid_t *)&efi_global_variable_guid,
+			       attributes, size, &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;