* [U-Boot] [PATCH v15 1/4] sandbox: smbios: Update to support sandbox
2018-11-22 20:46 [U-Boot] [PATCH v15 0/4] efi_loader: Code refactoring and improvement Simon Glass
@ 2018-11-22 20:46 ` Simon Glass
2018-11-24 21:27 ` Alexander Graf
2018-11-25 13:28 ` [U-Boot] [U-Boot, v15, " Alexander Graf
2018-11-22 20:46 ` [U-Boot] [PATCH v15 2/4] efi: Split out test init/uninit into functions Simon Glass
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ messages in thread
From: Simon Glass @ 2018-11-22 20:46 UTC (permalink / raw)
To: u-boot
At present this code casts addresses to pointers so cannot be used with
sandbox. Update it to use mapmem instead.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v15: None
Changes in v14:
- Fix condition for invalid pointer
Changes in v13:
- Update code to deal with the struct_table_address member
Changes in v12: None
Changes in v11:
- Fix the EFI code that has since been added and relies on broken behaviour
Changes in v9: None
Changes in v7: None
Changes in v5: None
Changes in v4: None
Changes in v3:
- Drop incorrect map_sysmem() in write_smbios_table()
lib/efi_loader/efi_smbios.c | 20 +++++++++-----
lib/smbios.c | 52 ++++++++++++++++++++++++++++++-------
2 files changed, 56 insertions(+), 16 deletions(-)
diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
index 38e42fa2432..a81488495e2 100644
--- a/lib/efi_loader/efi_smbios.c
+++ b/lib/efi_loader/efi_smbios.c
@@ -7,6 +7,7 @@
#include <common.h>
#include <efi_loader.h>
+#include <mapmem.h>
#include <smbios.h>
static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
@@ -19,17 +20,19 @@ static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
efi_status_t efi_smbios_register(void)
{
/* Map within the low 32 bits, to allow for 32bit SMBIOS tables */
- u64 dmi = U32_MAX;
+ u64 dmi_addr = U32_MAX;
efi_status_t ret;
+ void *dmi;
/* Reserve 4kiB page for SMBIOS */
ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
- EFI_RUNTIME_SERVICES_DATA, 1, &dmi);
+ EFI_RUNTIME_SERVICES_DATA, 1, &dmi_addr);
if (ret != EFI_SUCCESS) {
/* Could not find space in lowmem, use highmem instead */
ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
- EFI_RUNTIME_SERVICES_DATA, 1, &dmi);
+ EFI_RUNTIME_SERVICES_DATA, 1,
+ &dmi_addr);
if (ret != EFI_SUCCESS)
return ret;
@@ -39,11 +42,14 @@ efi_status_t efi_smbios_register(void)
* Generate SMBIOS tables - we know that efi_allocate_pages() returns
* a 4k-aligned address, so it is safe to assume that
* write_smbios_table() will write the table at that address.
+ *
+ * Note that on sandbox, efi_allocate_pages() unfortunately returns a
+ * pointer even though it uses a uint64_t type. Convert it.
*/
- assert(!(dmi & 0xf));
- write_smbios_table(dmi);
+ assert(!(dmi_addr & 0xf));
+ dmi = (void *)(uintptr_t)dmi_addr;
+ write_smbios_table(map_to_sysmem(dmi));
/* And expose them to our EFI payload */
- return efi_install_configuration_table(&smbios_guid,
- (void *)(uintptr_t)dmi);
+ return efi_install_configuration_table(&smbios_guid, dmi);
}
diff --git a/lib/smbios.c b/lib/smbios.c
index 326eb00230d..e8ee55c4aeb 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -6,6 +6,7 @@
*/
#include <common.h>
+#include <mapmem.h>
#include <smbios.h>
#include <tables_csum.h>
#include <version.h>
@@ -72,9 +73,10 @@ static int smbios_string_table_len(char *start)
static int smbios_write_type0(ulong *current, int handle)
{
- struct smbios_type0 *t = (struct smbios_type0 *)*current;
+ struct smbios_type0 *t;
int len = sizeof(struct smbios_type0);
+ t = map_sysmem(*current, len);
memset(t, 0, sizeof(struct smbios_type0));
fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle);
t->vendor = smbios_add_string(t->eos, "U-Boot");
@@ -101,16 +103,18 @@ static int smbios_write_type0(ulong *current, int handle)
len = t->length + smbios_string_table_len(t->eos);
*current += len;
+ unmap_sysmem(t);
return len;
}
static int smbios_write_type1(ulong *current, int handle)
{
- struct smbios_type1 *t = (struct smbios_type1 *)*current;
+ struct smbios_type1 *t;
int len = sizeof(struct smbios_type1);
char *serial_str = env_get("serial#");
+ t = map_sysmem(*current, len);
memset(t, 0, sizeof(struct smbios_type1));
fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle);
t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);
@@ -122,15 +126,17 @@ static int smbios_write_type1(ulong *current, int handle)
len = t->length + smbios_string_table_len(t->eos);
*current += len;
+ unmap_sysmem(t);
return len;
}
static int smbios_write_type2(ulong *current, int handle)
{
- struct smbios_type2 *t = (struct smbios_type2 *)*current;
+ struct smbios_type2 *t;
int len = sizeof(struct smbios_type2);
+ t = map_sysmem(*current, len);
memset(t, 0, sizeof(struct smbios_type2));
fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle);
t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);
@@ -140,15 +146,17 @@ static int smbios_write_type2(ulong *current, int handle)
len = t->length + smbios_string_table_len(t->eos);
*current += len;
+ unmap_sysmem(t);
return len;
}
static int smbios_write_type3(ulong *current, int handle)
{
- struct smbios_type3 *t = (struct smbios_type3 *)*current;
+ struct smbios_type3 *t;
int len = sizeof(struct smbios_type3);
+ t = map_sysmem(*current, len);
memset(t, 0, sizeof(struct smbios_type3));
fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle);
t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);
@@ -160,6 +168,7 @@ static int smbios_write_type3(ulong *current, int handle)
len = t->length + smbios_string_table_len(t->eos);
*current += len;
+ unmap_sysmem(t);
return len;
}
@@ -198,9 +207,10 @@ static void smbios_write_type4_dm(struct smbios_type4 *t)
static int smbios_write_type4(ulong *current, int handle)
{
- struct smbios_type4 *t = (struct smbios_type4 *)*current;
+ struct smbios_type4 *t;
int len = sizeof(struct smbios_type4);
+ t = map_sysmem(*current, len);
memset(t, 0, sizeof(struct smbios_type4));
fill_smbios_header(t, SMBIOS_PROCESSOR_INFORMATION, len, handle);
t->processor_type = SMBIOS_PROCESSOR_TYPE_CENTRAL;
@@ -214,32 +224,37 @@ static int smbios_write_type4(ulong *current, int handle)
len = t->length + smbios_string_table_len(t->eos);
*current += len;
+ unmap_sysmem(t);
return len;
}
static int smbios_write_type32(ulong *current, int handle)
{
- struct smbios_type32 *t = (struct smbios_type32 *)*current;
+ struct smbios_type32 *t;
int len = sizeof(struct smbios_type32);
+ t = map_sysmem(*current, len);
memset(t, 0, sizeof(struct smbios_type32));
fill_smbios_header(t, SMBIOS_SYSTEM_BOOT_INFORMATION, len, handle);
*current += len;
+ unmap_sysmem(t);
return len;
}
static int smbios_write_type127(ulong *current, int handle)
{
- struct smbios_type127 *t = (struct smbios_type127 *)*current;
+ struct smbios_type127 *t;
int len = sizeof(struct smbios_type127);
+ t = map_sysmem(*current, len);
memset(t, 0, sizeof(struct smbios_type127));
fill_smbios_header(t, SMBIOS_END_OF_TABLE, len, handle);
*current += len;
+ unmap_sysmem(t);
return len;
}
@@ -257,6 +272,7 @@ static smbios_write_type smbios_write_funcs[] = {
ulong write_smbios_table(ulong addr)
{
struct smbios_entry *se;
+ ulong table_addr;
ulong tables;
int len = 0;
int max_struct_size = 0;
@@ -268,7 +284,7 @@ ulong write_smbios_table(ulong addr)
/* 16 byte align the table address */
addr = ALIGN(addr, 16);
- se = (struct smbios_entry *)(uintptr_t)addr;
+ se = map_sysmem(addr, sizeof(struct smbios_entry));
memset(se, 0, sizeof(struct smbios_entry));
addr += sizeof(struct smbios_entry);
@@ -290,7 +306,24 @@ ulong write_smbios_table(ulong addr)
se->max_struct_size = max_struct_size;
memcpy(se->intermediate_anchor, "_DMI_", 5);
se->struct_table_length = len;
- se->struct_table_address = tables;
+
+ /*
+ * We must use a pointer here so things work correctly on sandbox. The
+ * user of this table is not aware of the mapping of addresses to
+ * sandbox's DRAM buffer.
+ */
+ table_addr = (ulong)map_sysmem(tables, 0);
+ if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {
+ /*
+ * We need to put this >32-bit pointer into the table but the
+ * field is only 32 bits wide.
+ */
+ printf("WARNING: SMBIOS table_address overflow %llx\n",
+ (unsigned long long)table_addr);
+ table_addr = 0;
+ }
+ se->struct_table_address = table_addr;
+
se->struct_count = handle;
/* calculate checksums */
@@ -298,6 +331,7 @@ ulong write_smbios_table(ulong addr)
isize = sizeof(struct smbios_entry) - SMBIOS_INTERMEDIATE_OFFSET;
se->intermediate_checksum = table_compute_checksum(istart, isize);
se->checksum = table_compute_checksum(se, sizeof(struct smbios_entry));
+ unmap_sysmem(se);
return addr;
}
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v15 2/4] efi: Split out test init/uninit into functions
2018-11-22 20:46 [U-Boot] [PATCH v15 0/4] efi_loader: Code refactoring and improvement Simon Glass
2018-11-22 20:46 ` [U-Boot] [PATCH v15 1/4] sandbox: smbios: Update to support sandbox Simon Glass
@ 2018-11-22 20:46 ` Simon Glass
2018-11-23 23:23 ` Heinrich Schuchardt
2018-11-22 20:46 ` [U-Boot] [PATCH v15 3/4] efi: Create a function to set up for running EFI code Simon Glass
2018-11-22 20:46 ` [U-Boot] [PATCH v15 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish() Simon Glass
3 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2018-11-22 20:46 UTC (permalink / raw)
To: u-boot
The functions in bootefi are very long because they mix high-level code
and control with the low-level implementation. To help with this, create
functions which handle preparing for running the test and cleaning up
afterwards.
Also shorten the awfully long variable names here.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v15:
- Add check for return values to bootefi_test_prepare()
- Drop call to efi_save_gd() in bootefi_test_prepare()
- Fix minor checkpatch nit with bracket
Changes in v14:
- Go back to the horrible long variable names
Changes in v13:
- Rebase to efi/efi-next
Changes in v12:
- Rename image to image_prot
Changes in v11: None
Changes in v9:
- Add comments to bootefi_test_prepare() about the memset()s
Changes in v7: None
Changes in v5:
- Drop call to efi_init_obj_list() which is now done in do_bootefi()
Changes in v4: None
Changes in v3:
- Add new patch to split out test init/uninit into functions
cmd/bootefi.c | 81 +++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 65 insertions(+), 16 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 3e37805ea13..3d98bffa542 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -453,6 +453,67 @@ exit:
return ret;
}
+#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
+/**
+ * bootefi_test_prepare() - prepare to run an EFI test
+ *
+ * This sets things up so we can call EFI functions. This involves preparing
+ * the 'gd' pointer and setting up the load ed image data structures.
+ *
+ * @image_objp: loaded_image_infop: Pointer to a struct which will hold the
+ * loaded image object. This struct will be inited by this function before
+ * use.
+ * @loaded_image_infop: Pointer to a struct which will hold the loaded image
+ * info. This struct will be inited by this function before use.
+ * @path: File path to the test being run (often just the test name with a
+ * backslash before it
+ * @test_func: Address of the test function that is being run
+ * @return 0 if OK, -ve on error
+ */
+static efi_status_t bootefi_test_prepare
+ (struct efi_loaded_image_obj **image_objp,
+ struct efi_loaded_image **loaded_image_infop,
+ const char *path,
+ ulong test_func)
+{
+ efi_status_t r;
+
+ /* Construct a dummy device path */
+ bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
+ (uintptr_t)test_func,
+ (uintptr_t)test_func);
+ if (!bootefi_device_path)
+ return EFI_OUT_OF_RESOURCES;
+ bootefi_image_path = efi_dp_from_file(NULL, 0, path);
+ if (!bootefi_image_path)
+ return EFI_OUT_OF_RESOURCES;
+ r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
+ image_objp, loaded_image_infop);
+ if (r)
+ return r;
+ /* efi_save_gd() has been called in efi_init_obj_list() */
+
+ /* Transfer environment variable efi_selftest as load options */
+ set_load_options(*loaded_image_infop, "efi_selftest");
+
+ return 0;
+}
+
+/**
+ * bootefi_test_finish() - finish up after running an EFI test
+ *
+ * @image_obj: Pointer to a struct which holds the loaded image object
+ * @loaded_image_info: Pointer to a struct which holds the loaded image info
+ */
+static void bootefi_test_finish(struct efi_loaded_image_obj *image_obj,
+ struct efi_loaded_image *loaded_image_info)
+{
+ efi_restore_gd();
+ free(loaded_image_info->load_options);
+ efi_delete_handle(&image_obj->header);
+}
+#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
+
static int do_bootefi_bootmgr_exec(void)
{
struct efi_device_path *device_path, *file_path;
@@ -528,26 +589,14 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
struct efi_loaded_image_obj *image_obj;
struct efi_loaded_image *loaded_image_info;
- /* Construct a dummy device path. */
- bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
- (uintptr_t)&efi_selftest,
- (uintptr_t)&efi_selftest);
- bootefi_image_path = efi_dp_from_file(NULL, 0, "\\selftest");
-
- r = efi_setup_loaded_image(bootefi_device_path,
- bootefi_image_path, &image_obj,
- &loaded_image_info);
- if (r != EFI_SUCCESS)
+ if (bootefi_test_prepare(&image_obj, &loaded_image_info,
+ "\\selftest",
+ (uintptr_t)&efi_selftest))
return CMD_RET_FAILURE;
- efi_save_gd();
- /* Transfer environment variable efi_selftest as load options */
- set_load_options(loaded_image_info, "efi_selftest");
/* Execute the test */
r = efi_selftest(&image_obj->header, &systab);
- efi_restore_gd();
- free(loaded_image_info->load_options);
- efi_delete_handle(&image_obj->header);
+ bootefi_test_finish(image_obj, loaded_image_info);
return r != EFI_SUCCESS;
} else
#endif
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v15 2/4] efi: Split out test init/uninit into functions
2018-11-22 20:46 ` [U-Boot] [PATCH v15 2/4] efi: Split out test init/uninit into functions Simon Glass
@ 2018-11-23 23:23 ` Heinrich Schuchardt
2018-11-24 18:49 ` Simon Glass
0 siblings, 1 reply; 13+ messages in thread
From: Heinrich Schuchardt @ 2018-11-23 23:23 UTC (permalink / raw)
To: u-boot
On 11/22/18 9:46 PM, Simon Glass wrote:
> The functions in bootefi are very long because they mix high-level code
> and control with the low-level implementation. To help with this, create
> functions which handle preparing for running the test and cleaning up
> afterwards.
>
> Also shorten the awfully long variable names here.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v15:
> - Add check for return values to bootefi_test_prepare()
> - Drop call to efi_save_gd() in bootefi_test_prepare()
> - Fix minor checkpatch nit with bracket
>
> Changes in v14:
> - Go back to the horrible long variable names
>
> Changes in v13:
> - Rebase to efi/efi-next
>
> Changes in v12:
> - Rename image to image_prot
>
> Changes in v11: None
> Changes in v9:
> - Add comments to bootefi_test_prepare() about the memset()s
>
> Changes in v7: None
> Changes in v5:
> - Drop call to efi_init_obj_list() which is now done in do_bootefi()
>
> Changes in v4: None
> Changes in v3:
> - Add new patch to split out test init/uninit into functions
>
> cmd/bootefi.c | 81 +++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 65 insertions(+), 16 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 3e37805ea13..3d98bffa542 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -453,6 +453,67 @@ exit:
> return ret;
> }
>
> +#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> +/**
> + * bootefi_test_prepare() - prepare to run an EFI test
> + *
> + * This sets things up so we can call EFI functions. This involves preparing
> + * the 'gd' pointer and setting up the load ed image data structures.
> + *
> + * @image_objp: loaded_image_infop: Pointer to a struct which will hold the
> + * loaded image object. This struct will be inited by this function before
> + * use.
> + * @loaded_image_infop: Pointer to a struct which will hold the loaded image
> + * info. This struct will be inited by this function before use.
> + * @path: File path to the test being run (often just the test name with a
> + * backslash before it
> + * @test_func: Address of the test function that is being run
> + * @return 0 if OK, -ve on error
> + */
> +static efi_status_t bootefi_test_prepare
> + (struct efi_loaded_image_obj **image_objp,
> + struct efi_loaded_image **loaded_image_infop,
> + const char *path,
> + ulong test_func)
> +{
> + efi_status_t r;
> +
> + /* Construct a dummy device path */
> + bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> + (uintptr_t)test_func,
> + (uintptr_t)test_func);
> + if (!bootefi_device_path)
> + return EFI_OUT_OF_RESOURCES;
> + bootefi_image_path = efi_dp_from_file(NULL, 0, path);
> + if (!bootefi_image_path)
Please, do not leak memory.
efi_free_pool(bootefi_device_path);
> + return EFI_OUT_OF_RESOURCES;
> + r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
> + image_objp, loaded_image_infop);
> + if (r)
> + return r;
> + /* efi_save_gd() has been called in efi_init_obj_list() */
This comment is only of historic interest.
> +
> + /* Transfer environment variable efi_selftest as load options */
> + set_load_options(*loaded_image_infop, "efi_selftest");
> +
> + return 0;
> +}
> +
> +/**
> + * bootefi_test_finish() - finish up after running an EFI test
> + *
> + * @image_obj: Pointer to a struct which holds the loaded image object
> + * @loaded_image_info: Pointer to a struct which holds the loaded image info
> + */
> +static void bootefi_test_finish(struct efi_loaded_image_obj *image_obj,
> + struct efi_loaded_image *loaded_image_info)
> +{
> + efi_restore_gd();
> + free(loaded_image_info->load_options);
> + efi_delete_handle(&image_obj->header);
> +}
> +#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
> +
> static int do_bootefi_bootmgr_exec(void)
> {
> struct efi_device_path *device_path, *file_path;
> @@ -528,26 +589,14 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> struct efi_loaded_image_obj *image_obj;
In later patches we should get rid of any reference to struct
efi_loaed_image_obj in bootefi.c. A handle is enough.
Best regards
Heinrich
> struct efi_loaded_image *loaded_image_info;
>
> - /* Construct a dummy device path. */
> - bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> - (uintptr_t)&efi_selftest,
> - (uintptr_t)&efi_selftest);
> - bootefi_image_path = efi_dp_from_file(NULL, 0, "\\selftest");
> -
> - r = efi_setup_loaded_image(bootefi_device_path,
> - bootefi_image_path, &image_obj,
> - &loaded_image_info);
> - if (r != EFI_SUCCESS)
> + if (bootefi_test_prepare(&image_obj, &loaded_image_info,
> + "\\selftest",
> + (uintptr_t)&efi_selftest))
> return CMD_RET_FAILURE;
>
> - efi_save_gd();
> - /* Transfer environment variable efi_selftest as load options */
> - set_load_options(loaded_image_info, "efi_selftest");
> /* Execute the test */
> r = efi_selftest(&image_obj->header, &systab);
> - efi_restore_gd();
> - free(loaded_image_info->load_options);
> - efi_delete_handle(&image_obj->header);
> + bootefi_test_finish(image_obj, loaded_image_info);
> return r != EFI_SUCCESS;
> } else
> #endif
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v15 2/4] efi: Split out test init/uninit into functions
2018-11-23 23:23 ` Heinrich Schuchardt
@ 2018-11-24 18:49 ` Simon Glass
0 siblings, 0 replies; 13+ messages in thread
From: Simon Glass @ 2018-11-24 18:49 UTC (permalink / raw)
To: u-boot
Hi Heinrich,
On Fri, 23 Nov 2018 at 16:23, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/22/18 9:46 PM, Simon Glass wrote:
> > The functions in bootefi are very long because they mix high-level code
> > and control with the low-level implementation. To help with this, create
> > functions which handle preparing for running the test and cleaning up
> > afterwards.
> >
> > Also shorten the awfully long variable names here.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v15:
> > - Add check for return values to bootefi_test_prepare()
> > - Drop call to efi_save_gd() in bootefi_test_prepare()
> > - Fix minor checkpatch nit with bracket
> >
> > Changes in v14:
> > - Go back to the horrible long variable names
> >
> > Changes in v13:
> > - Rebase to efi/efi-next
> >
> > Changes in v12:
> > - Rename image to image_prot
> >
> > Changes in v11: None
> > Changes in v9:
> > - Add comments to bootefi_test_prepare() about the memset()s
> >
> > Changes in v7: None
> > Changes in v5:
> > - Drop call to efi_init_obj_list() which is now done in do_bootefi()
> >
> > Changes in v4: None
> > Changes in v3:
> > - Add new patch to split out test init/uninit into functions
> >
> > cmd/bootefi.c | 81 +++++++++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 65 insertions(+), 16 deletions(-)
> >
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 3e37805ea13..3d98bffa542 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -453,6 +453,67 @@ exit:
> > return ret;
> > }
> >
> > +#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> > +/**
> > + * bootefi_test_prepare() - prepare to run an EFI test
> > + *
> > + * This sets things up so we can call EFI functions. This involves preparing
> > + * the 'gd' pointer and setting up the load ed image data structures.
> > + *
> > + * @image_objp: loaded_image_infop: Pointer to a struct which will hold the
> > + * loaded image object. This struct will be inited by this function before
> > + * use.
> > + * @loaded_image_infop: Pointer to a struct which will hold the loaded image
> > + * info. This struct will be inited by this function before use.
> > + * @path: File path to the test being run (often just the test name with a
> > + * backslash before it
> > + * @test_func: Address of the test function that is being run
> > + * @return 0 if OK, -ve on error
> > + */
> > +static efi_status_t bootefi_test_prepare
> > + (struct efi_loaded_image_obj **image_objp,
> > + struct efi_loaded_image **loaded_image_infop,
> > + const char *path,
> > + ulong test_func)
> > +{
> > + efi_status_t r;
> > +
> > + /* Construct a dummy device path */
> > + bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> > + (uintptr_t)test_func,
> > + (uintptr_t)test_func);
> > + if (!bootefi_device_path)
> > + return EFI_OUT_OF_RESOURCES;
> > + bootefi_image_path = efi_dp_from_file(NULL, 0, path);
> > + if (!bootefi_image_path)
>
> Please, do not leak memory.
If you mean bootefi_device_path() I am not sure how to free it as per
my previous message. Also, this leak is pre-existing. I suggest you
take a look when these patches land as I think you understand the
naming in EFI better than I do. I cannot find any function that frees
a struct efi_device_path *.
>
> efi_free_pool(bootefi_device_path);
>
>
> > + return EFI_OUT_OF_RESOURCES;
> > + r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
> > + image_objp, loaded_image_infop);
> > + if (r)
> > + return r;
> > + /* efi_save_gd() has been called in efi_init_obj_list() */
>
> This comment is only of historic interest.
OK I can remove it and send v16, but I'll hold off to see what else is
happening.
>
> > +
> > + /* Transfer environment variable efi_selftest as load options */
> > + set_load_options(*loaded_image_infop, "efi_selftest");
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * bootefi_test_finish() - finish up after running an EFI test
> > + *
> > + * @image_obj: Pointer to a struct which holds the loaded image object
> > + * @loaded_image_info: Pointer to a struct which holds the loaded image info
> > + */
> > +static void bootefi_test_finish(struct efi_loaded_image_obj *image_obj,
> > + struct efi_loaded_image *loaded_image_info)
> > +{
> > + efi_restore_gd();
> > + free(loaded_image_info->load_options);
> > + efi_delete_handle(&image_obj->header);
> > +}
> > +#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
> > +
> > static int do_bootefi_bootmgr_exec(void)
> > {
> > struct efi_device_path *device_path, *file_path;
> > @@ -528,26 +589,14 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > struct efi_loaded_image_obj *image_obj;
>
> In later patches we should get rid of any reference to struct
> efi_loaed_image_obj in bootefi.c. A handle is enough.
I don't understand this naming at all. Is a handle like a void *?
Regards,
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v15 3/4] efi: Create a function to set up for running EFI code
2018-11-22 20:46 [U-Boot] [PATCH v15 0/4] efi_loader: Code refactoring and improvement Simon Glass
2018-11-22 20:46 ` [U-Boot] [PATCH v15 1/4] sandbox: smbios: Update to support sandbox Simon Glass
2018-11-22 20:46 ` [U-Boot] [PATCH v15 2/4] efi: Split out test init/uninit into functions Simon Glass
@ 2018-11-22 20:46 ` Simon Glass
2018-11-22 20:46 ` [U-Boot] [PATCH v15 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish() Simon Glass
3 siblings, 0 replies; 13+ messages in thread
From: Simon Glass @ 2018-11-22 20:46 UTC (permalink / raw)
To: u-boot
There is still duplicated code in efi_loader for tests and normal
operation.
Add a new bootefi_run_prepare() function which holds common code used to
set up U-Boot to run EFI code. Make use of this from the existing
bootefi_test_prepare() function, as well as do_bootefi_exec().
Also shorten a few variable names.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v15:
- Fix minor checkpatch nit with bracket
Changes in v14:
- Go back to the horrible long variable names
Changes in v13: None
Changes in v12: None
Changes in v11: None
Changes in v9: None
Changes in v7: None
Changes in v5:
- Drop call to efi_init_obj_list() which is now done in do_bootefi()
- Introduce load_options_path to specifyc U-Boot env var for load_options_path
Changes in v4:
- Rebase to master
Changes in v3:
- Add patch to create a function to set up for running EFI code
cmd/bootefi.c | 55 ++++++++++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 22 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 3d98bffa542..0ca84ff7168 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -325,6 +325,27 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)
return ret;
}
+static efi_status_t bootefi_run_prepare(const char *load_options_path,
+ struct efi_device_path *device_path,
+ struct efi_device_path *image_path,
+ struct efi_loaded_image_obj **image_objp,
+ struct efi_loaded_image **loaded_image_infop)
+{
+ efi_status_t ret;
+
+ ret = efi_setup_loaded_image(device_path, image_path, image_objp,
+ loaded_image_infop);
+ if (ret != EFI_SUCCESS)
+ return ret;
+
+ /* efi_save_gd() has been called in efi_init_obj_list() */
+
+ /* Transfer environment variable as load options */
+ set_load_options(*loaded_image_infop, load_options_path);
+
+ return 0;
+}
+
/**
* do_bootefi_exec() - execute EFI binary
*
@@ -374,13 +395,11 @@ static efi_status_t do_bootefi_exec(void *efi,
assert(device_path && image_path);
}
- ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
- &loaded_image_info);
- if (ret != EFI_SUCCESS)
- goto exit;
+ ret = bootefi_run_prepare("bootargs", device_path, image_path,
+ &image_obj, &loaded_image_info);
+ if (ret)
+ return ret;
- /* Transfer environment variable bootargs as load options */
- set_load_options(loaded_image_info, "bootargs");
/* Load the EFI payload */
entry = efi_load_pe(image_obj, efi, loaded_image_info);
if (!entry) {
@@ -468,16 +487,14 @@ exit:
* @path: File path to the test being run (often just the test name with a
* backslash before it
* @test_func: Address of the test function that is being run
+ * @load_options_path: U-Boot environment variable to use as load options
* @return 0 if OK, -ve on error
*/
static efi_status_t bootefi_test_prepare
(struct efi_loaded_image_obj **image_objp,
- struct efi_loaded_image **loaded_image_infop,
- const char *path,
- ulong test_func)
+ struct efi_loaded_image **loaded_image_infop, const char *path,
+ ulong test_func, const char *load_options_path)
{
- efi_status_t r;
-
/* Construct a dummy device path */
bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
(uintptr_t)test_func,
@@ -487,16 +504,10 @@ static efi_status_t bootefi_test_prepare
bootefi_image_path = efi_dp_from_file(NULL, 0, path);
if (!bootefi_image_path)
return EFI_OUT_OF_RESOURCES;
- r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
- image_objp, loaded_image_infop);
- if (r)
- return r;
- /* efi_save_gd() has been called in efi_init_obj_list() */
- /* Transfer environment variable efi_selftest as load options */
- set_load_options(*loaded_image_infop, "efi_selftest");
-
- return 0;
+ return bootefi_run_prepare(load_options_path, bootefi_device_path,
+ bootefi_image_path, image_objp,
+ loaded_image_infop);
}
/**
@@ -590,8 +601,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
struct efi_loaded_image *loaded_image_info;
if (bootefi_test_prepare(&image_obj, &loaded_image_info,
- "\\selftest",
- (uintptr_t)&efi_selftest))
+ "\\selftest", (uintptr_t)&efi_selftest,
+ "efi_selftest"))
return CMD_RET_FAILURE;
/* Execute the test */
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v15 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish()
2018-11-22 20:46 [U-Boot] [PATCH v15 0/4] efi_loader: Code refactoring and improvement Simon Glass
` (2 preceding siblings ...)
2018-11-22 20:46 ` [U-Boot] [PATCH v15 3/4] efi: Create a function to set up for running EFI code Simon Glass
@ 2018-11-22 20:46 ` Simon Glass
2018-11-24 21:26 ` Alexander Graf
3 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2018-11-22 20:46 UTC (permalink / raw)
To: u-boot
This function can be used from do_bootefi_exec() so that we use mostly the
same code for a normal EFI application and an EFI test.
Rename the function and use it in both places.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v15:
- Add a comment about a leaked device path
Changes in v14:
- Go back to the horrible long variable names
- Hopefully correct error paths in do_bootefi_exec()
Changes in v13:
- Drop 'efi_loader: Drop setup_ok' as we have an existing patch for that
- Drop patches previously applied
Changes in v12: None
Changes in v11:
- Drop patches previously applied
Changes in v9: None
Changes in v7:
- Drop patch "efi: Init the 'rows' and 'cols' variables"
- Drop patches previous applied
Changes in v5:
- Rebase to master
Changes in v4:
- Rebase to master
Changes in v3:
- Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
cmd/bootefi.c | 46 ++++++++++++++++++++++++----------------------
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 0ca84ff7168..5831c991a8e 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -346,6 +346,20 @@ static efi_status_t bootefi_run_prepare(const char *load_options_path,
return 0;
}
+/**
+ * bootefi_run_finish() - finish up after running an EFI test
+ *
+ * @loaded_image_info: Pointer to a struct which holds the loaded image info
+ * @image_objj: Pointer to a struct which holds the loaded image object
+ */
+static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
+ struct efi_loaded_image *loaded_image_info)
+{
+ efi_restore_gd();
+ free(loaded_image_info->load_options);
+ efi_delete_handle(&image_obj->header);
+}
+
/**
* do_bootefi_exec() - execute EFI binary
*
@@ -386,11 +400,11 @@ static efi_status_t do_bootefi_exec(void *efi,
*/
ret = efi_create_handle(&mem_handle);
if (ret != EFI_SUCCESS)
- goto exit;
+ return ret; /* TODO: leaks device_path */
ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
device_path);
if (ret != EFI_SUCCESS)
- goto exit;
+ goto err_add_protocol;
} else {
assert(device_path && image_path);
}
@@ -398,13 +412,13 @@ static efi_status_t do_bootefi_exec(void *efi,
ret = bootefi_run_prepare("bootargs", device_path, image_path,
&image_obj, &loaded_image_info);
if (ret)
- return ret;
+ goto err_prepare;
/* Load the EFI payload */
entry = efi_load_pe(image_obj, efi, loaded_image_info);
if (!entry) {
ret = EFI_LOAD_ERROR;
- goto exit;
+ goto err_prepare;
}
if (memdp) {
@@ -424,7 +438,7 @@ static efi_status_t do_bootefi_exec(void *efi,
if (setjmp(&image_obj->exit_jmp)) {
ret = image_obj->exit_status;
- goto exit;
+ goto err_prepare;
}
#ifdef CONFIG_ARM64
@@ -462,10 +476,11 @@ static efi_status_t do_bootefi_exec(void *efi,
ret = efi_do_enter(&image_obj->header, &systab, entry);
-exit:
+err_prepare:
/* image has returned, loaded-image obj goes *poof*: */
- if (image_obj)
- efi_delete_handle(&image_obj->header);
+ bootefi_run_finish(image_obj, loaded_image_info);
+
+err_add_protocol:
if (mem_handle)
efi_delete_handle(mem_handle);
@@ -510,19 +525,6 @@ static efi_status_t bootefi_test_prepare
loaded_image_infop);
}
-/**
- * bootefi_test_finish() - finish up after running an EFI test
- *
- * @image_obj: Pointer to a struct which holds the loaded image object
- * @loaded_image_info: Pointer to a struct which holds the loaded image info
- */
-static void bootefi_test_finish(struct efi_loaded_image_obj *image_obj,
- struct efi_loaded_image *loaded_image_info)
-{
- efi_restore_gd();
- free(loaded_image_info->load_options);
- efi_delete_handle(&image_obj->header);
-}
#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
static int do_bootefi_bootmgr_exec(void)
@@ -607,7 +609,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
/* Execute the test */
r = efi_selftest(&image_obj->header, &systab);
- bootefi_test_finish(image_obj, loaded_image_info);
+ bootefi_run_finish(image_obj, loaded_image_info);
return r != EFI_SUCCESS;
} else
#endif
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v15 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish()
2018-11-22 20:46 ` [U-Boot] [PATCH v15 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish() Simon Glass
@ 2018-11-24 21:26 ` Alexander Graf
2018-11-25 12:53 ` Simon Glass
0 siblings, 1 reply; 13+ messages in thread
From: Alexander Graf @ 2018-11-24 21:26 UTC (permalink / raw)
To: u-boot
On 22.11.18 21:46, Simon Glass wrote:
> This function can be used from do_bootefi_exec() so that we use mostly the
> same code for a normal EFI application and an EFI test.
>
> Rename the function and use it in both places.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v15:
> - Add a comment about a leaked device path
>
> Changes in v14:
> - Go back to the horrible long variable names
> - Hopefully correct error paths in do_bootefi_exec()
>
> Changes in v13:
> - Drop 'efi_loader: Drop setup_ok' as we have an existing patch for that
> - Drop patches previously applied
>
> Changes in v12: None
> Changes in v11:
> - Drop patches previously applied
>
> Changes in v9: None
> Changes in v7:
> - Drop patch "efi: Init the 'rows' and 'cols' variables"
> - Drop patches previous applied
>
> Changes in v5:
> - Rebase to master
>
> Changes in v4:
> - Rebase to master
>
> Changes in v3:
> - Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
>
> cmd/bootefi.c | 46 ++++++++++++++++++++++++----------------------
> 1 file changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 0ca84ff7168..5831c991a8e 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -346,6 +346,20 @@ static efi_status_t bootefi_run_prepare(const char *load_options_path,
> return 0;
> }
>
> +/**
> + * bootefi_run_finish() - finish up after running an EFI test
> + *
> + * @loaded_image_info: Pointer to a struct which holds the loaded image info
> + * @image_objj: Pointer to a struct which holds the loaded image object
> + */
> +static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
> + struct efi_loaded_image *loaded_image_info)
> +{
> + efi_restore_gd();
> + free(loaded_image_info->load_options);
> + efi_delete_handle(&image_obj->header);
> +}
> +
> /**
> * do_bootefi_exec() - execute EFI binary
> *
> @@ -386,11 +400,11 @@ static efi_status_t do_bootefi_exec(void *efi,
> */
> ret = efi_create_handle(&mem_handle);
> if (ret != EFI_SUCCESS)
> - goto exit;
> + return ret; /* TODO: leaks device_path */
> ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> device_path);
> if (ret != EFI_SUCCESS)
> - goto exit;
> + goto err_add_protocol;
> } else {
> assert(device_path && image_path);
> }
> @@ -398,13 +412,13 @@ static efi_status_t do_bootefi_exec(void *efi,
> ret = bootefi_run_prepare("bootargs", device_path, image_path,
> &image_obj, &loaded_image_info);
> if (ret)
> - return ret;
> + goto err_prepare;
>
> /* Load the EFI payload */
> entry = efi_load_pe(image_obj, efi, loaded_image_info);
> if (!entry) {
> ret = EFI_LOAD_ERROR;
> - goto exit;
> + goto err_prepare;
> }
>
> if (memdp) {
> @@ -424,7 +438,7 @@ static efi_status_t do_bootefi_exec(void *efi,
>
> if (setjmp(&image_obj->exit_jmp)) {
> ret = image_obj->exit_status;
> - goto exit;
> + goto err_prepare;
> }
>
> #ifdef CONFIG_ARM64
> @@ -462,10 +476,11 @@ static efi_status_t do_bootefi_exec(void *efi,
>
> ret = efi_do_enter(&image_obj->header, &systab, entry);
>
> -exit:
> +err_prepare:
> /* image has returned, loaded-image obj goes *poof*: */
> - if (image_obj)
> - efi_delete_handle(&image_obj->header);
> + bootefi_run_finish(image_obj, loaded_image_info);
So here we now free loaded_image_info->load_options which we didn't do
before. That means the patch does change behavior.
I think the change is correct though. For the sake of bisectability, I'd
prefer if you could add a tiny patch before this patch that just adds
the free(loaded_image_info->load_options) in this spot. We can then have
this refactoring really be neutral as to behavior.
Thanks,
Alex
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v15 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish()
2018-11-24 21:26 ` Alexander Graf
@ 2018-11-25 12:53 ` Simon Glass
2018-11-25 13:27 ` Alexander Graf
0 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2018-11-25 12:53 UTC (permalink / raw)
To: u-boot
Hi Alex,
On Sat, 24 Nov 2018 at 14:26, Alexander Graf <agraf@suse.de> wrote:
>
>
>
> On 22.11.18 21:46, Simon Glass wrote:
> > This function can be used from do_bootefi_exec() so that we use mostly the
> > same code for a normal EFI application and an EFI test.
> >
> > Rename the function and use it in both places.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v15:
> > - Add a comment about a leaked device path
> >
> > Changes in v14:
> > - Go back to the horrible long variable names
> > - Hopefully correct error paths in do_bootefi_exec()
> >
> > Changes in v13:
> > - Drop 'efi_loader: Drop setup_ok' as we have an existing patch for that
> > - Drop patches previously applied
> >
> > Changes in v12: None
> > Changes in v11:
> > - Drop patches previously applied
> >
> > Changes in v9: None
> > Changes in v7:
> > - Drop patch "efi: Init the 'rows' and 'cols' variables"
> > - Drop patches previous applied
> >
> > Changes in v5:
> > - Rebase to master
> >
> > Changes in v4:
> > - Rebase to master
> >
> > Changes in v3:
> > - Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
> >
> > cmd/bootefi.c | 46 ++++++++++++++++++++++++----------------------
> > 1 file changed, 24 insertions(+), 22 deletions(-)
> >
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 0ca84ff7168..5831c991a8e 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -346,6 +346,20 @@ static efi_status_t bootefi_run_prepare(const char *load_options_path,
> > return 0;
> > }
> >
> > +/**
> > + * bootefi_run_finish() - finish up after running an EFI test
> > + *
> > + * @loaded_image_info: Pointer to a struct which holds the loaded image info
> > + * @image_objj: Pointer to a struct which holds the loaded image object
> > + */
> > +static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
> > + struct efi_loaded_image *loaded_image_info)
> > +{
> > + efi_restore_gd();
> > + free(loaded_image_info->load_options);
> > + efi_delete_handle(&image_obj->header);
> > +}
> > +
> > /**
> > * do_bootefi_exec() - execute EFI binary
> > *
> > @@ -386,11 +400,11 @@ static efi_status_t do_bootefi_exec(void *efi,
> > */
> > ret = efi_create_handle(&mem_handle);
> > if (ret != EFI_SUCCESS)
> > - goto exit;
> > + return ret; /* TODO: leaks device_path */
> > ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> > device_path);
> > if (ret != EFI_SUCCESS)
> > - goto exit;
> > + goto err_add_protocol;
> > } else {
> > assert(device_path && image_path);
> > }
> > @@ -398,13 +412,13 @@ static efi_status_t do_bootefi_exec(void *efi,
> > ret = bootefi_run_prepare("bootargs", device_path, image_path,
> > &image_obj, &loaded_image_info);
> > if (ret)
> > - return ret;
> > + goto err_prepare;
> >
> > /* Load the EFI payload */
> > entry = efi_load_pe(image_obj, efi, loaded_image_info);
> > if (!entry) {
> > ret = EFI_LOAD_ERROR;
> > - goto exit;
> > + goto err_prepare;
> > }
> >
> > if (memdp) {
> > @@ -424,7 +438,7 @@ static efi_status_t do_bootefi_exec(void *efi,
> >
> > if (setjmp(&image_obj->exit_jmp)) {
> > ret = image_obj->exit_status;
> > - goto exit;
> > + goto err_prepare;
> > }
> >
> > #ifdef CONFIG_ARM64
> > @@ -462,10 +476,11 @@ static efi_status_t do_bootefi_exec(void *efi,
> >
> > ret = efi_do_enter(&image_obj->header, &systab, entry);
> >
> > -exit:
> > +err_prepare:
> > /* image has returned, loaded-image obj goes *poof*: */
> > - if (image_obj)
> > - efi_delete_handle(&image_obj->header);
> > + bootefi_run_finish(image_obj, loaded_image_info);
>
> So here we now free loaded_image_info->load_options which we didn't do
> before. That means the patch does change behavior.
>
> I think the change is correct though. For the sake of bisectability, I'd
> prefer if you could add a tiny patch before this patch that just adds
> the free(loaded_image_info->load_options) in this spot. We can then have
> this refactoring really be neutral as to behavior.
I agree, it worries me that we are fixing up complex code in a refactor.
Can you just use v14?
Regards,
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v15 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish()
2018-11-25 12:53 ` Simon Glass
@ 2018-11-25 13:27 ` Alexander Graf
2018-11-25 13:30 ` Simon Glass
0 siblings, 1 reply; 13+ messages in thread
From: Alexander Graf @ 2018-11-25 13:27 UTC (permalink / raw)
To: u-boot
On 25.11.18 13:53, Simon Glass wrote:
> Hi Alex,
>
> On Sat, 24 Nov 2018 at 14:26, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>>
>> On 22.11.18 21:46, Simon Glass wrote:
>>> This function can be used from do_bootefi_exec() so that we use mostly the
>>> same code for a normal EFI application and an EFI test.
>>>
>>> Rename the function and use it in both places.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v15:
>>> - Add a comment about a leaked device path
>>>
>>> Changes in v14:
>>> - Go back to the horrible long variable names
>>> - Hopefully correct error paths in do_bootefi_exec()
>>>
>>> Changes in v13:
>>> - Drop 'efi_loader: Drop setup_ok' as we have an existing patch for that
>>> - Drop patches previously applied
>>>
>>> Changes in v12: None
>>> Changes in v11:
>>> - Drop patches previously applied
>>>
>>> Changes in v9: None
>>> Changes in v7:
>>> - Drop patch "efi: Init the 'rows' and 'cols' variables"
>>> - Drop patches previous applied
>>>
>>> Changes in v5:
>>> - Rebase to master
>>>
>>> Changes in v4:
>>> - Rebase to master
>>>
>>> Changes in v3:
>>> - Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
>>>
>>> cmd/bootefi.c | 46 ++++++++++++++++++++++++----------------------
>>> 1 file changed, 24 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 0ca84ff7168..5831c991a8e 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -346,6 +346,20 @@ static efi_status_t bootefi_run_prepare(const char *load_options_path,
>>> return 0;
>>> }
>>>
>>> +/**
>>> + * bootefi_run_finish() - finish up after running an EFI test
>>> + *
>>> + * @loaded_image_info: Pointer to a struct which holds the loaded image info
>>> + * @image_objj: Pointer to a struct which holds the loaded image object
>>> + */
>>> +static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
>>> + struct efi_loaded_image *loaded_image_info)
>>> +{
>>> + efi_restore_gd();
>>> + free(loaded_image_info->load_options);
>>> + efi_delete_handle(&image_obj->header);
>>> +}
>>> +
>>> /**
>>> * do_bootefi_exec() - execute EFI binary
>>> *
>>> @@ -386,11 +400,11 @@ static efi_status_t do_bootefi_exec(void *efi,
>>> */
>>> ret = efi_create_handle(&mem_handle);
>>> if (ret != EFI_SUCCESS)
>>> - goto exit;
>>> + return ret; /* TODO: leaks device_path */
>>> ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
>>> device_path);
>>> if (ret != EFI_SUCCESS)
>>> - goto exit;
>>> + goto err_add_protocol;
>>> } else {
>>> assert(device_path && image_path);
>>> }
>>> @@ -398,13 +412,13 @@ static efi_status_t do_bootefi_exec(void *efi,
>>> ret = bootefi_run_prepare("bootargs", device_path, image_path,
>>> &image_obj, &loaded_image_info);
>>> if (ret)
>>> - return ret;
>>> + goto err_prepare;
>>>
>>> /* Load the EFI payload */
>>> entry = efi_load_pe(image_obj, efi, loaded_image_info);
>>> if (!entry) {
>>> ret = EFI_LOAD_ERROR;
>>> - goto exit;
>>> + goto err_prepare;
>>> }
>>>
>>> if (memdp) {
>>> @@ -424,7 +438,7 @@ static efi_status_t do_bootefi_exec(void *efi,
>>>
>>> if (setjmp(&image_obj->exit_jmp)) {
>>> ret = image_obj->exit_status;
>>> - goto exit;
>>> + goto err_prepare;
>>> }
>>>
>>> #ifdef CONFIG_ARM64
>>> @@ -462,10 +476,11 @@ static efi_status_t do_bootefi_exec(void *efi,
>>>
>>> ret = efi_do_enter(&image_obj->header, &systab, entry);
>>>
>>> -exit:
>>> +err_prepare:
>>> /* image has returned, loaded-image obj goes *poof*: */
>>> - if (image_obj)
>>> - efi_delete_handle(&image_obj->header);
>>> + bootefi_run_finish(image_obj, loaded_image_info);
>>
>> So here we now free loaded_image_info->load_options which we didn't do
>> before. That means the patch does change behavior.
>>
>> I think the change is correct though. For the sake of bisectability, I'd
>> prefer if you could add a tiny patch before this patch that just adds
>> the free(loaded_image_info->load_options) in this spot. We can then have
>> this refactoring really be neutral as to behavior.
>
> I agree, it worries me that we are fixing up complex code in a refactor.
>
> Can you just use v14?
v14 has the same problem, so unfortunately not :).
Alex
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v15 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish()
2018-11-25 13:27 ` Alexander Graf
@ 2018-11-25 13:30 ` Simon Glass
0 siblings, 0 replies; 13+ messages in thread
From: Simon Glass @ 2018-11-25 13:30 UTC (permalink / raw)
To: u-boot
Hi Alex,
On Sun, 25 Nov 2018 at 06:27, Alexander Graf <agraf@suse.de> wrote:
>
>
>
> On 25.11.18 13:53, Simon Glass wrote:
> > Hi Alex,
> >
> > On Sat, 24 Nov 2018 at 14:26, Alexander Graf <agraf@suse.de> wrote:
> >>
> >>
> >>
> >> On 22.11.18 21:46, Simon Glass wrote:
> >>> This function can be used from do_bootefi_exec() so that we use mostly the
> >>> same code for a normal EFI application and an EFI test.
> >>>
> >>> Rename the function and use it in both places.
> >>>
> >>> Signed-off-by: Simon Glass <sjg@chromium.org>
> >>> ---
> >>>
> >>> Changes in v15:
> >>> - Add a comment about a leaked device path
> >>>
> >>> Changes in v14:
> >>> - Go back to the horrible long variable names
> >>> - Hopefully correct error paths in do_bootefi_exec()
> >>>
> >>> Changes in v13:
> >>> - Drop 'efi_loader: Drop setup_ok' as we have an existing patch for that
> >>> - Drop patches previously applied
> >>>
> >>> Changes in v12: None
> >>> Changes in v11:
> >>> - Drop patches previously applied
> >>>
> >>> Changes in v9: None
> >>> Changes in v7:
> >>> - Drop patch "efi: Init the 'rows' and 'cols' variables"
> >>> - Drop patches previous applied
> >>>
> >>> Changes in v5:
> >>> - Rebase to master
> >>>
> >>> Changes in v4:
> >>> - Rebase to master
> >>>
> >>> Changes in v3:
> >>> - Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
> >>>
> >>> cmd/bootefi.c | 46 ++++++++++++++++++++++++----------------------
> >>> 1 file changed, 24 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index 0ca84ff7168..5831c991a8e 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -346,6 +346,20 @@ static efi_status_t bootefi_run_prepare(const char *load_options_path,
> >>> return 0;
> >>> }
> >>>
> >>> +/**
> >>> + * bootefi_run_finish() - finish up after running an EFI test
> >>> + *
> >>> + * @loaded_image_info: Pointer to a struct which holds the loaded image info
> >>> + * @image_objj: Pointer to a struct which holds the loaded image object
> >>> + */
> >>> +static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
> >>> + struct efi_loaded_image *loaded_image_info)
> >>> +{
> >>> + efi_restore_gd();
> >>> + free(loaded_image_info->load_options);
> >>> + efi_delete_handle(&image_obj->header);
> >>> +}
> >>> +
> >>> /**
> >>> * do_bootefi_exec() - execute EFI binary
> >>> *
> >>> @@ -386,11 +400,11 @@ static efi_status_t do_bootefi_exec(void *efi,
> >>> */
> >>> ret = efi_create_handle(&mem_handle);
> >>> if (ret != EFI_SUCCESS)
> >>> - goto exit;
> >>> + return ret; /* TODO: leaks device_path */
> >>> ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> >>> device_path);
> >>> if (ret != EFI_SUCCESS)
> >>> - goto exit;
> >>> + goto err_add_protocol;
> >>> } else {
> >>> assert(device_path && image_path);
> >>> }
> >>> @@ -398,13 +412,13 @@ static efi_status_t do_bootefi_exec(void *efi,
> >>> ret = bootefi_run_prepare("bootargs", device_path, image_path,
> >>> &image_obj, &loaded_image_info);
> >>> if (ret)
> >>> - return ret;
> >>> + goto err_prepare;
> >>>
> >>> /* Load the EFI payload */
> >>> entry = efi_load_pe(image_obj, efi, loaded_image_info);
> >>> if (!entry) {
> >>> ret = EFI_LOAD_ERROR;
> >>> - goto exit;
> >>> + goto err_prepare;
> >>> }
> >>>
> >>> if (memdp) {
> >>> @@ -424,7 +438,7 @@ static efi_status_t do_bootefi_exec(void *efi,
> >>>
> >>> if (setjmp(&image_obj->exit_jmp)) {
> >>> ret = image_obj->exit_status;
> >>> - goto exit;
> >>> + goto err_prepare;
> >>> }
> >>>
> >>> #ifdef CONFIG_ARM64
> >>> @@ -462,10 +476,11 @@ static efi_status_t do_bootefi_exec(void *efi,
> >>>
> >>> ret = efi_do_enter(&image_obj->header, &systab, entry);
> >>>
> >>> -exit:
> >>> +err_prepare:
> >>> /* image has returned, loaded-image obj goes *poof*: */
> >>> - if (image_obj)
> >>> - efi_delete_handle(&image_obj->header);
> >>> + bootefi_run_finish(image_obj, loaded_image_info);
> >>
> >> So here we now free loaded_image_info->load_options which we didn't do
> >> before. That means the patch does change behavior.
> >>
> >> I think the change is correct though. For the sake of bisectability, I'd
> >> prefer if you could add a tiny patch before this patch that just adds
> >> the free(loaded_image_info->load_options) in this spot. We can then have
> >> this refactoring really be neutral as to behavior.
> >
> > I agree, it worries me that we are fixing up complex code in a refactor.
> >
> > Can you just use v14?
>
> v14 has the same problem, so unfortunately not :).
Ah yes. Oh dear. I will have another go.
Regards,
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread