Message ID | 20190305055337.3793-4-takahiro.akashi@linaro.org |
---|---|
State | Superseded, archived |
Delegated to: | Heinrich Schuchardt |
Headers | show |
Series | efi_loader: rework bootefi/bootmgr | expand |
On 3/5/19 6:53 AM, AKASHI Takahiro wrote: > We need to know the size of image loaded so as to be able to use > load_image() API at do_bootefi_exec() in a later patch. > > So change the interface of efi_bootmgr_load(). > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > include/efi_loader.h | 5 +++-- > lib/efi_loader/efi_bootmgr.c | 41 +++++++++++++++++++++--------------- > 2 files changed, 27 insertions(+), 19 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 47a51ddc9406..3f51116155ad 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -564,8 +564,9 @@ 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, > - struct efi_device_path **file_path); > +efi_status_t efi_bootmgr_load(struct efi_device_path **device_path, > + struct efi_device_path **file_path, > + void **image, efi_uintn_t *size); > > #else /* CONFIG_IS_ENABLED(EFI_LOADER) */ > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > index 1575c5c09e46..38f3fa15f6ef 100644 > --- a/lib/efi_loader/efi_bootmgr.c > +++ b/lib/efi_loader/efi_bootmgr.c > @@ -120,14 +120,17 @@ static void *get_var(u16 *name, const efi_guid_t *vendor, > * if successful. This checks that the EFI_LOAD_OPTION is active (enabled) > * and that the specified file to boot exists. > */ > -static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, > - struct efi_device_path **file_path) > +static efi_status_t try_load_entry(u16 n, > + struct efi_device_path **device_path, > + struct efi_device_path **file_path, > + void **image, efi_uintn_t *image_size) > { > struct efi_load_option lo; > u16 varname[] = L"Boot0000"; > u16 hexmap[] = L"0123456789ABCDEF"; > - void *load_option, *image = NULL; > + void *load_option; > efi_uintn_t size; > + efi_status_t ret; > > varname[4] = hexmap[(n & 0xf000) >> 12]; > varname[5] = hexmap[(n & 0x0f00) >> 8]; > @@ -136,18 +139,17 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, > > load_option = get_var(varname, &efi_global_variable_guid, &size); > if (!load_option) > - return NULL; > + return EFI_LOAD_ERROR; > > 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); > > - ret = efi_load_image_from_path(lo.file_path, &image, &size); > + ret = efi_load_image_from_path(lo.file_path, image, image_size); > This call to efi_load_image_from_path() leads to duplication of logic. Why don't we simply call EFI_CALL(efi_load_image())) here and if it succeeds return from efi_bootmgr_load()? Best regards Heinrich > if (ret != EFI_SUCCESS) > goto error; > @@ -164,12 +166,14 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, > > printf("Booting: %ls\n", lo.label); > efi_dp_split_file_path(lo.file_path, device_path, file_path); > + } else { > + ret = EFI_LOAD_ERROR; > } > > error: > free(load_option); > > - return image; > + return ret; > } > > /* > @@ -177,12 +181,12 @@ error: > * 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) > +efi_status_t efi_bootmgr_load(struct efi_device_path **device_path, > + struct efi_device_path **file_path, > + void **image, efi_uintn_t *image_size) > { > u16 bootnext, *bootorder; > efi_uintn_t size; > - void *image = NULL; > int i, num; > efi_status_t ret; > > @@ -201,9 +205,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path, > (efi_guid_t *)&efi_global_variable_guid, > 0, 0, &bootnext)); > if (ret == EFI_SUCCESS) { > - image = try_load_entry(bootnext, > - device_path, file_path); > - if (image) > + ret = try_load_entry(bootnext, device_path, file_path, > + image, image_size); > + if (ret == EFI_SUCCESS) > goto error; > } > } > @@ -212,19 +216,22 @@ void *efi_bootmgr_load(struct efi_device_path **device_path, > bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size); > if (!bootorder) { > printf("BootOrder not defined\n"); > + ret = EFI_NOT_FOUND; > goto error; > } > > num = size / sizeof(uint16_t); > for (i = 0; i < num; i++) { > - debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]); > - image = try_load_entry(bootorder[i], device_path, file_path); > - if (image) > + debug("%s: trying to load Boot%04X\n", __func__, > + bootorder[i]); > + ret = try_load_entry(bootorder[i], device_path, file_path, > + image, image_size); > + if (ret == EFI_SUCCESS) > break; > } > > free(bootorder); > > error: > - return image; > + return ret; > } >
On Thu, Mar 21, 2019 at 12:41:06PM +0100, Heinrich Schuchardt wrote: > On 3/5/19 6:53 AM, AKASHI Takahiro wrote: > > We need to know the size of image loaded so as to be able to use > > load_image() API at do_bootefi_exec() in a later patch. > > > > So change the interface of efi_bootmgr_load(). > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > include/efi_loader.h | 5 +++-- > > lib/efi_loader/efi_bootmgr.c | 41 +++++++++++++++++++++--------------- > > 2 files changed, 27 insertions(+), 19 deletions(-) > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index 47a51ddc9406..3f51116155ad 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -564,8 +564,9 @@ 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, > > - struct efi_device_path **file_path); > > +efi_status_t efi_bootmgr_load(struct efi_device_path **device_path, > > + struct efi_device_path **file_path, > > + void **image, efi_uintn_t *size); > > > > #else /* CONFIG_IS_ENABLED(EFI_LOADER) */ > > > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > > index 1575c5c09e46..38f3fa15f6ef 100644 > > --- a/lib/efi_loader/efi_bootmgr.c > > +++ b/lib/efi_loader/efi_bootmgr.c > > @@ -120,14 +120,17 @@ static void *get_var(u16 *name, const efi_guid_t *vendor, > > * if successful. This checks that the EFI_LOAD_OPTION is active (enabled) > > * and that the specified file to boot exists. > > */ > > -static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, > > - struct efi_device_path **file_path) > > +static efi_status_t try_load_entry(u16 n, > > + struct efi_device_path **device_path, > > + struct efi_device_path **file_path, > > + void **image, efi_uintn_t *image_size) > > { > > struct efi_load_option lo; > > u16 varname[] = L"Boot0000"; > > u16 hexmap[] = L"0123456789ABCDEF"; > > - void *load_option, *image = NULL; > > + void *load_option; > > efi_uintn_t size; > > + efi_status_t ret; > > > > varname[4] = hexmap[(n & 0xf000) >> 12]; > > varname[5] = hexmap[(n & 0x0f00) >> 8]; > > @@ -136,18 +139,17 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, > > > > load_option = get_var(varname, &efi_global_variable_guid, &size); > > if (!load_option) > > - return NULL; > > + return EFI_LOAD_ERROR; > > > > 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); > > > > - ret = efi_load_image_from_path(lo.file_path, &image, &size); > > + ret = efi_load_image_from_path(lo.file_path, image, image_size); > > > > This call to efi_load_image_from_path() leads to duplication of logic. > > Why don't we simply call EFI_CALL(efi_load_image())) here and if it > succeeds return from efi_bootmgr_load()? Make sense. It will make do_bootefi_exec() simpler. This will also give us a reason why we have do_efibootmgr() apart from do_boot_efi(). Thanks, -Takahiro Akashi > Best regards > > Heinrich > > > if (ret != EFI_SUCCESS) > > goto error; > > @@ -164,12 +166,14 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, > > > > printf("Booting: %ls\n", lo.label); > > efi_dp_split_file_path(lo.file_path, device_path, file_path); > > + } else { > > + ret = EFI_LOAD_ERROR; > > } > > > > error: > > free(load_option); > > > > - return image; > > + return ret; > > } > > > > /* > > @@ -177,12 +181,12 @@ error: > > * 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) > > +efi_status_t efi_bootmgr_load(struct efi_device_path **device_path, > > + struct efi_device_path **file_path, > > + void **image, efi_uintn_t *image_size) > > { > > u16 bootnext, *bootorder; > > efi_uintn_t size; > > - void *image = NULL; > > int i, num; > > efi_status_t ret; > > > > @@ -201,9 +205,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path, > > (efi_guid_t *)&efi_global_variable_guid, > > 0, 0, &bootnext)); > > if (ret == EFI_SUCCESS) { > > - image = try_load_entry(bootnext, > > - device_path, file_path); > > - if (image) > > + ret = try_load_entry(bootnext, device_path, file_path, > > + image, image_size); > > + if (ret == EFI_SUCCESS) > > goto error; > > } > > } > > @@ -212,19 +216,22 @@ void *efi_bootmgr_load(struct efi_device_path **device_path, > > bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size); > > if (!bootorder) { > > printf("BootOrder not defined\n"); > > + ret = EFI_NOT_FOUND; > > goto error; > > } > > > > num = size / sizeof(uint16_t); > > for (i = 0; i < num; i++) { > > - debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]); > > - image = try_load_entry(bootorder[i], device_path, file_path); > > - if (image) > > + debug("%s: trying to load Boot%04X\n", __func__, > > + bootorder[i]); > > + ret = try_load_entry(bootorder[i], device_path, file_path, > > + image, image_size); > > + if (ret == EFI_SUCCESS) > > break; > > } > > > > free(bootorder); > > > > error: > > - return image; > > + return ret; > > } > > >
diff --git a/include/efi_loader.h b/include/efi_loader.h index 47a51ddc9406..3f51116155ad 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -564,8 +564,9 @@ 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, - struct efi_device_path **file_path); +efi_status_t efi_bootmgr_load(struct efi_device_path **device_path, + struct efi_device_path **file_path, + void **image, efi_uintn_t *size); #else /* CONFIG_IS_ENABLED(EFI_LOADER) */ diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 1575c5c09e46..38f3fa15f6ef 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -120,14 +120,17 @@ static void *get_var(u16 *name, const efi_guid_t *vendor, * if successful. This checks that the EFI_LOAD_OPTION is active (enabled) * and that the specified file to boot exists. */ -static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, - struct efi_device_path **file_path) +static efi_status_t try_load_entry(u16 n, + struct efi_device_path **device_path, + struct efi_device_path **file_path, + void **image, efi_uintn_t *image_size) { struct efi_load_option lo; u16 varname[] = L"Boot0000"; u16 hexmap[] = L"0123456789ABCDEF"; - void *load_option, *image = NULL; + void *load_option; efi_uintn_t size; + efi_status_t ret; varname[4] = hexmap[(n & 0xf000) >> 12]; varname[5] = hexmap[(n & 0x0f00) >> 8]; @@ -136,18 +139,17 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, load_option = get_var(varname, &efi_global_variable_guid, &size); if (!load_option) - return NULL; + return EFI_LOAD_ERROR; 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); - ret = efi_load_image_from_path(lo.file_path, &image, &size); + ret = efi_load_image_from_path(lo.file_path, image, image_size); if (ret != EFI_SUCCESS) goto error; @@ -164,12 +166,14 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, printf("Booting: %ls\n", lo.label); efi_dp_split_file_path(lo.file_path, device_path, file_path); + } else { + ret = EFI_LOAD_ERROR; } error: free(load_option); - return image; + return ret; } /* @@ -177,12 +181,12 @@ error: * 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) +efi_status_t efi_bootmgr_load(struct efi_device_path **device_path, + struct efi_device_path **file_path, + void **image, efi_uintn_t *image_size) { u16 bootnext, *bootorder; efi_uintn_t size; - void *image = NULL; int i, num; efi_status_t ret; @@ -201,9 +205,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path, (efi_guid_t *)&efi_global_variable_guid, 0, 0, &bootnext)); if (ret == EFI_SUCCESS) { - image = try_load_entry(bootnext, - device_path, file_path); - if (image) + ret = try_load_entry(bootnext, device_path, file_path, + image, image_size); + if (ret == EFI_SUCCESS) goto error; } } @@ -212,19 +216,22 @@ void *efi_bootmgr_load(struct efi_device_path **device_path, bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size); if (!bootorder) { printf("BootOrder not defined\n"); + ret = EFI_NOT_FOUND; goto error; } num = size / sizeof(uint16_t); for (i = 0; i < num; i++) { - debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]); - image = try_load_entry(bootorder[i], device_path, file_path); - if (image) + debug("%s: trying to load Boot%04X\n", __func__, + bootorder[i]); + ret = try_load_entry(bootorder[i], device_path, file_path, + image, image_size); + if (ret == EFI_SUCCESS) break; } free(bootorder); error: - return image; + return ret; }
We need to know the size of image loaded so as to be able to use load_image() API at do_bootefi_exec() in a later patch. So change the interface of efi_bootmgr_load(). Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- include/efi_loader.h | 5 +++-- lib/efi_loader/efi_bootmgr.c | 41 +++++++++++++++++++++--------------- 2 files changed, 27 insertions(+), 19 deletions(-)