[v2,03/14] efi_loader: bootmgr: allow for running a given load option

Message ID 20181105090653.7409-4-takahiro.akashi@linaro.org
State New
Headers show
Series
  • efi: make efi and bootmgr more usable
Related show

Commit Message

AKASHI Takahiro Nov. 5, 2018, 9:06 a.m.
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>
---
 include/efi_loader.h         | 3 ++-
 lib/efi_loader/efi_bootmgr.c | 8 +++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

Comments

Alexander Graf Dec. 2, 2018, 11:22 p.m. | #1
On 05.11.18 10:06, AKASHI Takahiro wrote:
> 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>

I don't see you changing the caller, so this hunk won't compile on its own?

Please make sure that every single step in your patch set compiles.

> ---
>  include/efi_loader.h         | 3 ++-
>  lib/efi_loader/efi_bootmgr.c | 8 +++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index d83de906fbce..ce0f420b5004 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -540,7 +540,8 @@ struct efi_load_option {
>  
>  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 a095df3f540b..74eb832a8c92 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -170,7 +170,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)
>  {
>  	uint16_t *bootorder;
> @@ -183,6 +184,11 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
>  	bs = systab.boottime;
>  	rs = systab.runtime;
>  
> +	if (boot_id != -1) {
> +		image = try_load_entry(boot_id, device_path, file_path);
> +		goto error;

I think at this point it might be better to split the function, no?
You're not actually reusing any code from efi_bootmgr_load with the 2
instantiation types (explicit boot id, loop through boot order).


Alex

> +	}
> +
>  	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
>  	if (!bootorder)
>  		goto error;
>
AKASHI Takahiro Dec. 3, 2018, 3:20 a.m. | #2
On Mon, Dec 03, 2018 at 12:22:37AM +0100, Alexander Graf wrote:
> 
> 
> On 05.11.18 10:06, AKASHI Takahiro wrote:
> > 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>
> 
> I don't see you changing the caller, so this hunk won't compile on its own?

You're correct.

> Please make sure that every single step in your patch set compiles.

I will try to double-check in the future.

> > ---
> >  include/efi_loader.h         | 3 ++-
> >  lib/efi_loader/efi_bootmgr.c | 8 +++++++-
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index d83de906fbce..ce0f420b5004 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -540,7 +540,8 @@ struct efi_load_option {
> >  
> >  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 a095df3f540b..74eb832a8c92 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -170,7 +170,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)
> >  {
> >  	uint16_t *bootorder;
> > @@ -183,6 +184,11 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
> >  	bs = systab.boottime;
> >  	rs = systab.runtime;
> >  
> > +	if (boot_id != -1) {
> > +		image = try_load_entry(boot_id, device_path, file_path);
> > +		goto error;
> 
> I think at this point it might be better to split the function, no?

It's one way to fix, but I want simply to fix the issue by
modifying the caller, do_bootefi_bootmgr_exec() because

> You're not actually reusing any code from efi_bootmgr_load with the 2
> instantiation types (explicit boot id, loop through boot order).

I will add "BootNext" support to efi_bootmgr_load() so that it will be
a single point of loading. See my relevant patch:
https://lists.denx.de/pipermail/u-boot/2018-November/349281.html

Thanks,
-Takahiro Akashi


> 
> Alex
> 
> > +	}
> > +
> >  	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
> >  	if (!bootorder)
> >  		goto error;
> >
Alexander Graf Dec. 3, 2018, 1:54 p.m. | #3
On 03.12.18 04:20, AKASHI Takahiro wrote:
> On Mon, Dec 03, 2018 at 12:22:37AM +0100, Alexander Graf wrote:
>>
>>
>> On 05.11.18 10:06, AKASHI Takahiro wrote:
>>> 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>
>>
>> I don't see you changing the caller, so this hunk won't compile on its own?
> 
> You're correct.
> 
>> Please make sure that every single step in your patch set compiles.
> 
> I will try to double-check in the future.
> 
>>> ---
>>>  include/efi_loader.h         | 3 ++-
>>>  lib/efi_loader/efi_bootmgr.c | 8 +++++++-
>>>  2 files changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index d83de906fbce..ce0f420b5004 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -540,7 +540,8 @@ struct efi_load_option {
>>>  
>>>  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 a095df3f540b..74eb832a8c92 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -170,7 +170,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)
>>>  {
>>>  	uint16_t *bootorder;
>>> @@ -183,6 +184,11 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
>>>  	bs = systab.boottime;
>>>  	rs = systab.runtime;
>>>  
>>> +	if (boot_id != -1) {
>>> +		image = try_load_entry(boot_id, device_path, file_path);
>>> +		goto error;
>>
>> I think at this point it might be better to split the function, no?
> 
> It's one way to fix, but I want simply to fix the issue by
> modifying the caller, do_bootefi_bootmgr_exec() because
> 
>> You're not actually reusing any code from efi_bootmgr_load with the 2
>> instantiation types (explicit boot id, loop through boot order).
> 
> I will add "BootNext" support to efi_bootmgr_load() so that it will be
> a single point of loading. See my relevant patch:
> https://lists.denx.de/pipermail/u-boot/2018-November/349281.html

I don't fully understand. I would expect BootNext to only take effect
when *not* booting an explicit ID? So that would fit quite nicely into a
function based split.


Alex

Patch

diff --git a/include/efi_loader.h b/include/efi_loader.h
index d83de906fbce..ce0f420b5004 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -540,7 +540,8 @@  struct efi_load_option {
 
 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 a095df3f540b..74eb832a8c92 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -170,7 +170,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)
 {
 	uint16_t *bootorder;
@@ -183,6 +184,11 @@  void *efi_bootmgr_load(struct efi_device_path **device_path,
 	bs = systab.boottime;
 	rs = systab.runtime;
 
+	if (boot_id != -1) {
+		image = try_load_entry(boot_id, device_path, file_path);
+		goto error;
+	}
+
 	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
 	if (!bootorder)
 		goto error;