[U-Boot] [PATCH v2 1/1] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior

[Heinrich Schuchardt]

On 3/6/19 1:17 AM, AKASHI Takahiro wrote:
> On Tue, Mar 05, 2019 at 08:38:40PM +0100, Heinrich Schuchardt wrote:
>> On 3/5/19 2:58 AM, AKASHI Takahiro wrote:
>>> See UEFI v2.7, section 3.1.2 for details of the specification.
>>>
>>> With efidebug command, you can run any EFI boot option as follows:
>>>   => 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 ...)
>>>   => env print -e
>>>   <snip ...>
>>>   BootCurrent: {boot,run}(blob)
>>>   00000000:  02 00                    ..
>>>   BootOrder: {boot,run}(blob)
>>>   00000000:  01 00 02 00              ....
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
>>
>> Please, use scripts/get_maintainer.pl to determine the correct
>> recipients. You missed Alex's new email address.
> 
> Okay.
> 
>>> ---
>>>  lib/efi_loader/efi_bootmgr.c | 40 ++++++++++++++++++++++++++++++++----
>>>  1 file changed, 36 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index 417016102b48..1575c5c09e46 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -141,6 +141,7 @@ 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",
>>> @@ -151,6 +152,16 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>>>  		if (ret != EFI_SUCCESS)
>>>  			goto error;
>>>  
>>> +		attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> +			     EFI_VARIABLE_RUNTIME_ACCESS;
>>> +		size = sizeof(n);
>>> +		ret = EFI_CALL(efi_set_variable(
>>> +				L"BootCurrent",
>>> +				(efi_guid_t *)&efi_global_variable_guid,
>>> +				attributes, size, &n));
>>> +		if (ret != EFI_SUCCESS)
>>> +			goto error;
>>> +
>>>  		printf("Booting: %ls\n", lo.label);
>>>  		efi_dp_split_file_path(lo.file_path, device_path, file_path);
>>>  	}
>>> @@ -162,21 +173,42 @@ error:
>>>  }
>>>  
>>>  /*
>>> - * Attempt to load, in the order specified by BootOrder EFI variable, the
>>> - * available load-options, finding and returning the first one that can
>>> - * be loaded successfully.
>>> + * Attempt to load from BootNext or in the order specified by BootOrder
>>> + * EFI variable, the available load-options, finding and returning
>>> + * the first one that can be loaded successfully.
>>>   */
>>>  void *efi_bootmgr_load(struct efi_device_path **device_path,
>>>  		       struct efi_device_path **file_path)
>>>  {
>>> -	uint16_t *bootorder;
>>> +	u16 bootnext, *bootorder;
>>
>> bootnext has enough space for the terminating \n. That is way too small.
>>
>> You want to call efi_get_variable() twice. Get the buffer size needed in
>> the first round. malloc() a buffer. Then actually read the variable.
>> Finally free() the buffer.
> 
> No.
> "BootNext" is always a 16-bit integer, and "bootnext" is a u16 variable,
> not a pointer.
> So we don't need to call get_variable() twice. That is why I didn't use
> get_var() here. I believe that you seem to like "code efficiency."

I see.

> 
>>>  	efi_uintn_t size;
>>>  	void *image = NULL;
>>>  	int i, num;
>>> +	efi_status_t ret;
>>>  
>>>  	bs = systab.boottime;
>>>  	rs = systab.runtime;
>>>  
>>> +	/* BootNext */
>>> +	size = sizeof(bootnext);
>>> +	ret = EFI_CALL(efi_get_variable(L"BootNext",
>>> +					(efi_guid_t *)&efi_global_variable_guid,
>>> +					NULL, &size, &bootnext));
>>> +	if (ret == EFI_SUCCESS) {

As we expect the variable to have size 2, we should check the size field
too.

>>
>> The expected value of ret for an existing variable of size > 0 is
>> EFI_BUFFER_TOO_SMALL.

Now let's assume that the variable has been created with the wrong size
(e.g. using `env set -e`). In that case we should either try to delete
it or write an error message or both.

>>
>>> +		/* delete BootNext */
>>> +		ret = EFI_CALL(efi_set_variable(
>>> +					L"BootNext",
>>> +					(efi_guid_t *)&efi_global_variable_guid,
>>> +					0, 0, &bootnext));
>>> +		if (ret == EFI_SUCCESS) {
>>
>> Why would loading the boot entry depend on the return value here?
> 
> Deleting "BootNex" is required by UEFI spec.
> In addition, if we fail to delete it, we will see the same application
> start again when executing boot manager next time.
> 

BootNext is a non-volatile variable. Once we have a non-volatile backend
for variables the call may fail because the the NV storage is not
writable. Another case leading to an error would be the variable having
been created with EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS
(which we do not yet support).

Should this stop the boot process? If yes, we need at least an error
message. But I would propose that if the NV storage is not writable we
continue booting after writing a debug message.

Best regards

Heinrich

> Thanks,
> -Takahiro Akashi
> 
>>> +			image = try_load_entry(bootnext,
>>
>> Best regards
>>
>> Heinrich
>>
>>> +					       device_path, file_path);
>>> +			if (image)
>>> +				goto error;
>>> +		}
>>> +	}
>>> +
>>> +	/* BootOrder */
>>>  	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
>>>  	if (!bootorder) {
>>>  		printf("BootOrder not defined\n");
>>>
>>
>