| Message ID | 20181105090653.7409-4-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi: make efi and bootmgr more usable | expand |
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; >
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; > >
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
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;
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(-)