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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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@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."

> >  	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) {
> 
> The expected value of ret for an existing variable of size > 0 is
> EFI_BUFFER_TOO_SMALL.
> 
> > +		/* 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.

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");
> > 
>