[U-Boot] [PATCH v11 0/6] efi_loader: Code refactoring and improvement

[U-Boot] [PATCH v11 0/6] efi_loader: Code refactoring and improvement

[Simon Glass]

This collects the patches previously sent to break up the very large
functions in efi_loader into smaller pieces. Now that the other sandbox
stuff is applied, perhaps it is time to apply these patches.

This also adds a few new patches to fix more recent breakages.
Unfortunately we still cannot enable the efi loader tests since one of
the tests fails. Thus we should expect additional failures to appear
until that is resolved.

Changes in v11:
- Add a new patch to drop setup_ok
- Add a new patch to put CPUs under a cpu-bus node
- Drop patches previously applied
- Fix the EFI code that has since been added and relies on broken behaviour

Changes in v9:
- Add comments to bootefi_test_prepare() about the memset()s

Changes in v7:
- Drop patch "efi: Init the 'rows' and 'cols' variables"
- Drop patches previous applied

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
- Rebase to master

Changes in v4:
- Rebase to master

Changes in v3:
- Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
- Add new patch to split out test init/uninit into functions
- Add patch to create a function to set up for running EFI code
- Drop incorrect map_sysmem() in write_smbios_table()

Simon Glass (6):
  sandbox: Put CPUs under a cpu-bus node
  efi_loader: Drop setup_ok
  sandbox: smbios: Update to support sandbox
  efi: Split out test init/uninit into functions
  efi: Create a function to set up for running EFI code
  efi: Rename bootefi_test_finish() to bootefi_run_finish()

 arch/sandbox/dts/test.dts       |  18 ++--
 cmd/bootefi.c                   | 146 +++++++++++++++++++++-----------
 include/efi_selftest.h          |   2 -
 lib/efi_loader/efi_smbios.c     |  20 +++--
 lib/efi_selftest/efi_selftest.c |  14 +--
 lib/smbios.c                    |  32 +++++--
 6 files changed, 149 insertions(+), 83 deletions(-)

-- 
2.19.1.331.ge82ca0e54c-goog

[U-Boot] [PATCH v11 1/6] sandbox: Put CPUs under a cpu-bus node

[Simon Glass]

The CPU uclass expects that all CPUs have a parent device which is a
cpu-bus. Fix up the sandbox test DT to follow this convention. This allow
the code in smbios_write_type4_dm() to work, since it calls
dev_get_parent_platdata() on each CPU.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v11:
- Add a new patch to put CPUs under a cpu-bus node

Changes in v9: None
Changes in v7: None
Changes in v5: None
Changes in v4: None
Changes in v3: None

 arch/sandbox/dts/test.dts | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 420b72f4dbc..dc24fef3b21 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -344,16 +344,18 @@
 		mbox-names = "other", "test";
 	};
 
-	cpu-test1 {
-		compatible = "sandbox,cpu_sandbox";
-	};
+	cpus {
+		cpu-test1 {
+			compatible = "sandbox,cpu_sandbox";
+		};
 
-	cpu-test2 {
-		compatible = "sandbox,cpu_sandbox";
-	};
+		cpu-test2 {
+			compatible = "sandbox,cpu_sandbox";
+		};
 
-	cpu-test3 {
-		compatible = "sandbox,cpu_sandbox";
+		cpu-test3 {
+			compatible = "sandbox,cpu_sandbox";
+		};
 	};
 
 	misc-test {
-- 
2.19.1.331.ge82ca0e54c-goog

[U-Boot] [PATCH v11 1/6] sandbox: Put CPUs under a cpu-bus node

[Heinrich Schuchardt]

On 10/15/2018 04:17 PM, Simon Glass wrote:
> The CPU uclass expects that all CPUs have a parent device which is a
> cpu-bus. Fix up the sandbox test DT to follow this convention. This allow
> the code in smbios_write_type4_dm() to work, since it calls
> dev_get_parent_platdata() on each CPU.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v11:
> - Add a new patch to put CPUs under a cpu-bus node
> 
> Changes in v9: None
> Changes in v7: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> 
>  arch/sandbox/dts/test.dts | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> index 420b72f4dbc..dc24fef3b21 100644
> --- a/arch/sandbox/dts/test.dts
> +++ b/arch/sandbox/dts/test.dts
> @@ -344,16 +344,18 @@
>  		mbox-names = "other", "test";
>  	};
>  
> -	cpu-test1 {
> -		compatible = "sandbox,cpu_sandbox";
> -	};
> +	cpus {

Our device trees should adhere to the
"Devicetree Specification, Release v0.2"
available on https://www.devicetree.org/specifications/

Please, add all required properties:

- #address-cells
- #size-cells

> +		cpu-test1 {

The device tree specification has: "The node name for every CPU node
should be cpu". Please, call the CPUs cpu1 - cpu3.

Please, add all required properties:

- device-type
- reg
- clock-frequency
- timebase-frequency

Best regards

Heinrich

> +			compatible = "sandbox,cpu_sandbox";
> +		};
>  
> -	cpu-test2 {
> -		compatible = "sandbox,cpu_sandbox";
> -	};
> +		cpu-test2 {
> +			compatible = "sandbox,cpu_sandbox";
> +		};
>  
> -	cpu-test3 {
> -		compatible = "sandbox,cpu_sandbox";
> +		cpu-test3 {
> +			compatible = "sandbox,cpu_sandbox";
> +		};
>  	};
>  
>  	misc-test {
> 

[U-Boot] [PATCH v11 2/6] efi_loader: Drop setup_ok

[Simon Glass]

This value is stored in data which appears to be read-only with sandbox on
my Ubuntu 18.04 machine. In any case it is not good practice to store
run-time data in a build-time linker list.

The value does not seem to be that useful, since tests that fail to setup
are likely to fail to run also. Let's drop it for now.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v11:
- Add a new patch to drop setup_ok

Changes in v9: None
Changes in v7: None
Changes in v5: None
Changes in v4: None
Changes in v3: None

 include/efi_selftest.h          |  2 --
 lib/efi_selftest/efi_selftest.c | 14 +++++++-------
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/include/efi_selftest.h b/include/efi_selftest.h
index 56beac305ec..49d3d6d0b47 100644
--- a/include/efi_selftest.h
+++ b/include/efi_selftest.h
@@ -129,7 +129,6 @@ u16 efi_st_get_key(void);
  * @setup:	set up the unit test
  * @teardown:	tear down the unit test
  * @execute:	execute the unit test
- * @setup_ok:	setup was successful (set at runtime)
  * @on_request:	test is only executed on request
  */
 struct efi_unit_test {
@@ -139,7 +138,6 @@ struct efi_unit_test {
 		     const struct efi_system_table *systable);
 	int (*execute)(void);
 	int (*teardown)(void);
-	int setup_ok;
 	bool on_request;
 };
 
diff --git a/lib/efi_selftest/efi_selftest.c b/lib/efi_selftest/efi_selftest.c
index dd338db687e..dfd11be2302 100644
--- a/lib/efi_selftest/efi_selftest.c
+++ b/lib/efi_selftest/efi_selftest.c
@@ -74,20 +74,20 @@ void efi_st_exit_boot_services(void)
  */
 static int setup(struct efi_unit_test *test, unsigned int *failures)
 {
-	if (!test->setup) {
-		test->setup_ok = EFI_ST_SUCCESS;
+	int ret;
+
+	if (!test->setup)
 		return EFI_ST_SUCCESS;
-	}
 	efi_st_printc(EFI_LIGHTBLUE, "\nSetting up '%s'\n", test->name);
-	test->setup_ok = test->setup(handle, systable);
-	if (test->setup_ok != EFI_ST_SUCCESS) {
+	ret = test->setup(handle, systable);
+	if (ret) {
 		efi_st_error("Setting up '%s' failed\n", test->name);
 		++*failures;
 	} else {
 		efi_st_printc(EFI_LIGHTGREEN,
 			      "Setting up '%s' succeeded\n", test->name);
 	}
-	return test->setup_ok;
+	return ret;
 }
 
 /*
@@ -197,7 +197,7 @@ void efi_st_do_tests(const u16 *testname, unsigned int phase,
 			continue;
 		if (steps & EFI_ST_SETUP)
 			setup(test, failures);
-		if (steps & EFI_ST_EXECUTE && test->setup_ok == EFI_ST_SUCCESS)
+		if (steps & EFI_ST_EXECUTE)
 			execute(test, failures);
 		if (steps & EFI_ST_TEARDOWN)
 			teardown(test, failures);
-- 
2.19.1.331.ge82ca0e54c-goog

[U-Boot] [PATCH v11 2/6] efi_loader: Drop setup_ok

[Heinrich Schuchardt]

On 10/15/2018 04:17 PM, Simon Glass wrote:
> This value is stored in data which appears to be read-only with sandbox on
> my Ubuntu 18.04 machine. In any case it is not good practice to store
> run-time data in a build-time linker list.
Yes this should be changed. Otherwise a reset of the board will not put
us back into the initial status.

> 
> The value does not seem to be that useful, since tests that fail to setup
> are likely to fail to run also. Let's drop it for now.

With your change we will run execute() even if setup() fails. This
contradicts the commit message. Please, find a solution that avoids
calling execute() after a failed setup().

Best regards

Heinrich


> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v11:
> - Add a new patch to drop setup_ok
> 
> Changes in v9: None
> Changes in v7: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> 
>  include/efi_selftest.h          |  2 --
>  lib/efi_selftest/efi_selftest.c | 14 +++++++-------
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/include/efi_selftest.h b/include/efi_selftest.h
> index 56beac305ec..49d3d6d0b47 100644
> --- a/include/efi_selftest.h
> +++ b/include/efi_selftest.h
> @@ -129,7 +129,6 @@ u16 efi_st_get_key(void);
>   * @setup:	set up the unit test
>   * @teardown:	tear down the unit test
>   * @execute:	execute the unit test
> - * @setup_ok:	setup was successful (set at runtime)
>   * @on_request:	test is only executed on request
>   */
>  struct efi_unit_test {
> @@ -139,7 +138,6 @@ struct efi_unit_test {
>  		     const struct efi_system_table *systable);
>  	int (*execute)(void);
>  	int (*teardown)(void);
> -	int setup_ok;
>  	bool on_request;
>  };
>  
> diff --git a/lib/efi_selftest/efi_selftest.c b/lib/efi_selftest/efi_selftest.c
> index dd338db687e..dfd11be2302 100644
> --- a/lib/efi_selftest/efi_selftest.c
> +++ b/lib/efi_selftest/efi_selftest.c
> @@ -74,20 +74,20 @@ void efi_st_exit_boot_services(void)
>   */
>  static int setup(struct efi_unit_test *test, unsigned int *failures)
>  {
> -	if (!test->setup) {
> -		test->setup_ok = EFI_ST_SUCCESS;
> +	int ret;
> +
> +	if (!test->setup)
>  		return EFI_ST_SUCCESS;
> -	}
>  	efi_st_printc(EFI_LIGHTBLUE, "\nSetting up '%s'\n", test->name);
> -	test->setup_ok = test->setup(handle, systable);
> -	if (test->setup_ok != EFI_ST_SUCCESS) {
> +	ret = test->setup(handle, systable);
> +	if (ret) {
>  		efi_st_error("Setting up '%s' failed\n", test->name);
>  		++*failures;
>  	} else {
>  		efi_st_printc(EFI_LIGHTGREEN,
>  			      "Setting up '%s' succeeded\n", test->name);
>  	}
> -	return test->setup_ok;
> +	return ret;
>  }
>  
>  /*
> @@ -197,7 +197,7 @@ void efi_st_do_tests(const u16 *testname, unsigned int phase,
>  			continue;
>  		if (steps & EFI_ST_SETUP)
>  			setup(test, failures);
> -		if (steps & EFI_ST_EXECUTE && test->setup_ok == EFI_ST_SUCCESS)
> +		if (steps & EFI_ST_EXECUTE)
>  			execute(test, failures);
>  		if (steps & EFI_ST_TEARDOWN)
>  			teardown(test, failures);
> 

[U-Boot] [PATCH v11 2/6] efi_loader: Drop setup_ok

[Simon Glass]

Hi,

On 15 October 2018 at 11:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 10/15/2018 04:17 PM, Simon Glass wrote:
>> This value is stored in data which appears to be read-only with sandbox on
>> my Ubuntu 18.04 machine. In any case it is not good practice to store
>> run-time data in a build-time linker list.
> Yes this should be changed. Otherwise a reset of the board will not put
> us back into the initial status.
>
>>
>> The value does not seem to be that useful, since tests that fail to setup
>> are likely to fail to run also. Let's drop it for now.
>
> With your change we will run execute() even if setup() fails. This
> contradicts the commit message. Please, find a solution that avoids
> calling execute() after a failed setup().

How about we just exit if setup() fails? It should not fail. The test
can fail, but not the setup.

Regards,
Simon

[U-Boot] [PATCH v11 2/6] efi_loader: Drop setup_ok

[Heinrich Schuchardt]

On 10/19/2018 05:25 AM, Simon Glass wrote:
> Hi,
> 
> On 15 October 2018 at 11:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> On 10/15/2018 04:17 PM, Simon Glass wrote:
>>> This value is stored in data which appears to be read-only with sandbox on
>>> my Ubuntu 18.04 machine. In any case it is not good practice to store
>>> run-time data in a build-time linker list.
>> Yes this should be changed. Otherwise a reset of the board will not put
>> us back into the initial status.
>>
>>>
>>> The value does not seem to be that useful, since tests that fail to setup
>>> are likely to fail to run also. Let's drop it for now.
>>
>> With your change we will run execute() even if setup() fails. This
>> contradicts the commit message. Please, find a solution that avoids
>> calling execute() after a failed setup().
> 
> How about we just exit if setup() fails? It should not fail. The test
> can fail, but not the setup.
> 
> Regards,
> Simon
> 
I just have sent an alternative patch.

Thanks for reporting the issue.

Best regards

Heinrich

[U-Boot] [PATCH v11 3/6] sandbox: smbios: Update to support sandbox

[Simon Glass]

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 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                | 32 ++++++++++++++++++++++++--------
 2 files changed, 37 insertions(+), 15 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..87109d431a2 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;
 }
@@ -268,7 +283,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);
@@ -298,6 +313,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.331.ge82ca0e54c-goog

[U-Boot] [PATCH v11 3/6] sandbox: smbios: Update to support sandbox

[Alexander Graf]

On 15.10.18 16:17, Simon Glass wrote:
> 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>

Unfortunately this won't work. The SMBIOS2 structure itself contains a
physical pointer to the target address (which in EFI lands really has to
be linear physical pointer). This pointer gets set based on "addr" in
write_smbios_table():

        tables = addr;
        [...]
        se->struct_table_address = tables;

So I think the only thing we can do for now is to just graciously fail
SMBIOS generation (maybe only on sandbox?) when we can not find a
pointer that is < U32_MAX.

The shortcoming above was fixed with SMBIOS3, so the "good" path forward
would be to add SMBIOS3 support and just not rely on 32bit pointers at
all. I don't remember OTOH if SMBIOS3 stores offsets or 64bit pointers
to the tables. Depending on that we can either use your maps or we can't.


Alex

[U-Boot] [PATCH v11 3/6] sandbox: smbios: Update to support sandbox

[Simon Glass]

Hi Alex,

On 16 October 2018 at 06:55, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 15.10.18 16:17, Simon Glass wrote:
>> 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>
>
> Unfortunately this won't work. The SMBIOS2 structure itself contains a
> physical pointer to the target address (which in EFI lands really has to
> be linear physical pointer). This pointer gets set based on "addr" in
> write_smbios_table():
>
>         tables = addr;
>         [...]
>         se->struct_table_address = tables;

Does that actually matter? We will never actually boot anything on
sandbox that will use that address.

Also sandbox addresses are always <4GB (they start@0).

>
> So I think the only thing we can do for now is to just graciously fail
> SMBIOS generation (maybe only on sandbox?) when we can not find a
> pointer that is < U32_MAX.
>
> The shortcoming above was fixed with SMBIOS3, so the "good" path forward
> would be to add SMBIOS3 support and just not rely on 32bit pointers at
> all. I don't remember OTOH if SMBIOS3 stores offsets or 64bit pointers
> to the tables. Depending on that we can either use your maps or we can't.

Maybe I prefer device tree as it avoid this sort of thing :-)

Regards,
Simon

[U-Boot] [PATCH v11 3/6] sandbox: smbios: Update to support sandbox

[Alexander Graf]

On 19.10.18 05:25, Simon Glass wrote:
> Hi Alex,
> 
> On 16 October 2018 at 06:55, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 15.10.18 16:17, Simon Glass wrote:
>>> 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>
>>
>> Unfortunately this won't work. The SMBIOS2 structure itself contains a
>> physical pointer to the target address (which in EFI lands really has to
>> be linear physical pointer). This pointer gets set based on "addr" in
>> write_smbios_table():
>>
>>         tables = addr;
>>         [...]
>>         se->struct_table_address = tables;
> 
> Does that actually matter? We will never actually boot anything on
> sandbox that will use that address.

Why not? We can boot the UEFI Shell today - and that can use the "address".

> Also sandbox addresses are always <4GB (they start@0).

U-Boot addresses are <4GB but pointers are >4GB.

U-Boot addresses are also a U-Boot internal concept. We don't have any
means to expose their semantics to UEFI applications. So anything inside
a data structure that is shared with UEFI that says "address" really
means "pointer".

So since any UEFI application that we execute is only aware of pointers,
we can not represent the pointer in the field. And that breaks every
consumer of it.

> 
>>
>> So I think the only thing we can do for now is to just graciously fail
>> SMBIOS generation (maybe only on sandbox?) when we can not find a
>> pointer that is < U32_MAX.
>>
>> The shortcoming above was fixed with SMBIOS3, so the "good" path forward
>> would be to add SMBIOS3 support and just not rely on 32bit pointers at
>> all. I don't remember OTOH if SMBIOS3 stores offsets or 64bit pointers
>> to the tables. Depending on that we can either use your maps or we can't.
> 
> Maybe I prefer device tree as it avoid this sort of thing :-)

DT is orthogonal to SMBIOS. SMBIOS describes OEM information, such as
"chassy name", "how DIMM slots are populated", etc.

Sure, you could start and map SMBIOS information into a Linux specific
DT node, but what's the point if we have SMBIOS already ;).


Alex

[U-Boot] [PATCH v11 3/6] sandbox: smbios: Update to support sandbox

[Simon Glass]

Hi Alex,

On 19 October 2018 at 01:27, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 19.10.18 05:25, Simon Glass wrote:
>> Hi Alex,
>>
>> On 16 October 2018 at 06:55, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 15.10.18 16:17, Simon Glass wrote:
>>>> 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>
>>>
>>> Unfortunately this won't work. The SMBIOS2 structure itself contains a
>>> physical pointer to the target address (which in EFI lands really has to
>>> be linear physical pointer). This pointer gets set based on "addr" in
>>> write_smbios_table():
>>>
>>>         tables = addr;
>>>         [...]
>>>         se->struct_table_address = tables;
>>
>> Does that actually matter? We will never actually boot anything on
>> sandbox that will use that address.
>
> Why not? We can boot the UEFI Shell today - and that can use the "address".

OK, so UEFI shell uses the SMBIOS tables? I didn't know that.

>
>> Also sandbox addresses are always <4GB (they start@0).
>
> U-Boot addresses are <4GB but pointers are >4GB.

Actually I have applied a patch to fix that.

>
> U-Boot addresses are also a U-Boot internal concept. We don't have any
> means to expose their semantics to UEFI applications. So anything inside
> a data structure that is shared with UEFI that says "address" really
> means "pointer".
>
> So since any UEFI application that we execute is only aware of pointers,
> we can not represent the pointer in the field. And that breaks every
> consumer of it.

Yes we must use pointers in this case, I agree. This needs a
map_sysmem() call and a check that it does not overflow.

>
>>
>>>
>>> So I think the only thing we can do for now is to just graciously fail
>>> SMBIOS generation (maybe only on sandbox?) when we can not find a
>>> pointer that is < U32_MAX.
>>>
>>> The shortcoming above was fixed with SMBIOS3, so the "good" path forward
>>> would be to add SMBIOS3 support and just not rely on 32bit pointers at
>>> all. I don't remember OTOH if SMBIOS3 stores offsets or 64bit pointers
>>> to the tables. Depending on that we can either use your maps or we can't.
>>
>> Maybe I prefer device tree as it avoid this sort of thing :-)
>
> DT is orthogonal to SMBIOS. SMBIOS describes OEM information, such as
> "chassy name", "how DIMM slots are populated", etc.
>
> Sure, you could start and map SMBIOS information into a Linux specific
> DT node, but what's the point if we have SMBIOS already ;).

I'm not suggesting we do it, just whining about these legacy data structures :-)

Regards,
Simon

[U-Boot] [PATCH v11 3/6] sandbox: smbios: Update to support sandbox

[Alexander Graf]

> Am 22.10.2018 um 18:49 schrieb Simon Glass <sjg@chromium.org>:
> 
> Hi Alex,
> 
>> On 19 October 2018 at 01:27, Alexander Graf <agraf@suse.de> wrote:
>> 
>> 
>>> On 19.10.18 05:25, Simon Glass wrote:
>>> Hi Alex,
>>> 
>>>> On 16 October 2018 at 06:55, Alexander Graf <agraf@suse.de> wrote:
>>>> 
>>>> 
>>>>> On 15.10.18 16:17, Simon Glass wrote:
>>>>> 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>
>>>> 
>>>> Unfortunately this won't work. The SMBIOS2 structure itself contains a
>>>> physical pointer to the target address (which in EFI lands really has to
>>>> be linear physical pointer). This pointer gets set based on "addr" in
>>>> write_smbios_table():
>>>> 
>>>>        tables = addr;
>>>>        [...]
>>>>        se->struct_table_address = tables;
>>> 
>>> Does that actually matter? We will never actually boot anything on
>>> sandbox that will use that address.
>> 
>> Why not? We can boot the UEFI Shell today - and that can use the "address".
> 
> OK, so UEFI shell uses the SMBIOS tables? I didn't know that.

There is a command to print the contents of smbios tables - and that one does use them :).

> 
>> 
>>> Also sandbox addresses are always <4GB (they start@0).
>> 
>> U-Boot addresses are <4GB but pointers are >4GB.
> 
> Actually I have applied a patch to fix that.
> 
>> 
>> U-Boot addresses are also a U-Boot internal concept. We don't have any
>> means to expose their semantics to UEFI applications. So anything inside
>> a data structure that is shared with UEFI that says "address" really
>> means "pointer".
>> 
>> So since any UEFI application that we execute is only aware of pointers,
>> we can not represent the pointer in the field. And that breaks every
>> consumer of it.
> 
> Yes we must use pointers in this case, I agree. This needs a
> map_sysmem() call and a check that it does not overflow.
> 
>> 
>>> 
>>>> 
>>>> So I think the only thing we can do for now is to just graciously fail
>>>> SMBIOS generation (maybe only on sandbox?) when we can not find a
>>>> pointer that is < U32_MAX.
>>>> 
>>>> The shortcoming above was fixed with SMBIOS3, so the "good" path forward
>>>> would be to add SMBIOS3 support and just not rely on 32bit pointers at
>>>> all. I don't remember OTOH if SMBIOS3 stores offsets or 64bit pointers
>>>> to the tables. Depending on that we can either use your maps or we can't.
>>> 
>>> Maybe I prefer device tree as it avoid this sort of thing :-)
>> 
>> DT is orthogonal to SMBIOS. SMBIOS describes OEM information, such as
>> "chassy name", "how DIMM slots are populated", etc.
>> 
>> Sure, you could start and map SMBIOS information into a Linux specific
>> DT node, but what's the point if we have SMBIOS already ;).
> 
> I'm not suggesting we do it, just whining about these legacy data structures :-)

:)

Alex

> 
> Regards,
> Simon

[U-Boot] [PATCH v11 3/6] sandbox: smbios: Update to support sandbox

[Simon Glass]

Hi Alex,

On 22 October 2018 at 12:32, Alexander Graf <agraf@suse.de> wrote:
>
>
>
> > Am 22.10.2018 um 18:49 schrieb Simon Glass <sjg@chromium.org>:
> >
> > Hi Alex,
> >
> >> On 19 October 2018 at 01:27, Alexander Graf <agraf@suse.de> wrote:
> >>
> >>
> >>> On 19.10.18 05:25, Simon Glass wrote:
> >>> Hi Alex,
> >>>
> >>>> On 16 October 2018 at 06:55, Alexander Graf <agraf@suse.de> wrote:
> >>>>
> >>>>
> >>>>> On 15.10.18 16:17, Simon Glass wrote:
> >>>>> 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>
> >>>>
> >>>> Unfortunately this won't work. The SMBIOS2 structure itself contains a
> >>>> physical pointer to the target address (which in EFI lands really has to
> >>>> be linear physical pointer). This pointer gets set based on "addr" in
> >>>> write_smbios_table():
> >>>>
> >>>>        tables = addr;
> >>>>        [...]
> >>>>        se->struct_table_address = tables;
> >>>
> >>> Does that actually matter? We will never actually boot anything on
> >>> sandbox that will use that address.
> >>
> >> Why not? We can boot the UEFI Shell today - and that can use the "address".
> >
> > OK, so UEFI shell uses the SMBIOS tables? I didn't know that.
>
> There is a command to print the contents of smbios tables - and that one does use them :).

OK, but it should work without any trouble due to my patch in dm/next.

This seems like another reason to make sandbox use 32-bit pointers
where possible.

Regards,
Simon

[U-Boot] [PATCH v11 4/6] efi: Split out test init/uninit into functions

[Simon Glass]

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 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 | 90 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 65 insertions(+), 25 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 82d755ceb31..88b8b0172db 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -454,6 +454,64 @@ 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: Pointer to a struct which will hold the loaded image info.
+ *    This struct will be inited by this function before use.
+ * @obj: Pointer to a struct which will hold the loaded image object
+ *    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 **imagep,
+					 struct efi_loaded_image_obj **objp,
+					 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);
+	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
+	r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
+				   objp, imagep);
+	if (r)
+		return r;
+	/*
+	 * gd lives in a fixed register which may get clobbered while we execute
+	 * the payload. So save it here and restore it on every callback entry
+	 */
+	efi_save_gd();
+
+	/* Transfer environment variable efi_selftest as load options */
+	set_load_options(*imagep, "efi_selftest");
+
+	return 0;
+}
+
+/**
+ * bootefi_test_finish() - finish up after running an EFI test
+ *
+ * @image: Pointer to a struct which holds the loaded image info
+ * @obj: Pointer to a struct which holds the loaded image object
+ */
+static void bootefi_test_finish(struct efi_loaded_image *image,
+				struct efi_loaded_image_obj *obj)
+{
+	efi_restore_gd();
+	free(image->load_options);
+	efi_delete_handle(&obj->parent);
+}
+#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
+
 static int do_bootefi_bootmgr_exec(void)
 {
 	struct efi_device_path *device_path, *file_path;
@@ -532,34 +590,16 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 #endif
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
 	if (!strcmp(argv[1], "selftest")) {
-		struct efi_loaded_image_obj *image_handle;
-		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_handle,
-					   &loaded_image_info);
-		if (r != EFI_SUCCESS)
+		struct efi_loaded_image_obj *obj;
+		struct efi_loaded_image *image;
+
+		if (bootefi_test_prepare(&image, &obj, "\\selftest",
+					 (uintptr_t)&efi_selftest))
 			return CMD_RET_FAILURE;
 
-		/*
-		 * gd lives in a fixed register which may get clobbered while we
-		 * execute the payload. So save it here and restore it on every
-		 * callback entry
-		 */
-		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_handle, &systab);
-		efi_restore_gd();
-		free(loaded_image_info->load_options);
-		efi_delete_handle(&image_handle->parent);
+		r = efi_selftest(obj, &systab);
+		bootefi_test_finish(image, obj);
 		return r != EFI_SUCCESS;
 	} else
 #endif
-- 
2.19.1.331.ge82ca0e54c-goog

[U-Boot] [PATCH v11 4/6] efi: Split out test init/uninit into functions

[Heinrich Schuchardt]

On 10/15/2018 04:17 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 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 | 90 +++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 65 insertions(+), 25 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 82d755ceb31..88b8b0172db 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -454,6 +454,64 @@ 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: Pointer to a struct which will hold the loaded image info.
> + *    This struct will be inited by this function before use.
> + * @obj: Pointer to a struct which will hold the loaded image object
> + *    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 **imagep,
> +					 struct efi_loaded_image_obj **objp,
> +					 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);
> +	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
> +	r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
> +				   objp, imagep);
> +	if (r)
> +		return r;
> +	/*
> +	 * gd lives in a fixed register which may get clobbered while we execute
> +	 * the payload. So save it here and restore it on every callback entry
> +	 */
> +	efi_save_gd();
> +
> +	/* Transfer environment variable efi_selftest as load options */
> +	set_load_options(*imagep, "efi_selftest");
> +
> +	return 0;
> +}
> +
> +/**
> + * bootefi_test_finish() - finish up after running an EFI test
> + *
> + * @image: Pointer to a struct which holds the loaded image info
> + * @obj: Pointer to a struct which holds the loaded image object
> + */
> +static void bootefi_test_finish(struct efi_loaded_image *image,
> +				struct efi_loaded_image_obj *obj)
> +{
> +	efi_restore_gd();
> +	free(image->load_options);
> +	efi_delete_handle(&obj->parent);
> +}
> +#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
> +
>  static int do_bootefi_bootmgr_exec(void)
>  {
>  	struct efi_device_path *device_path, *file_path;
> @@ -532,34 +590,16 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  #endif
>  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>  	if (!strcmp(argv[1], "selftest")) {

Why not move the whole body of the if clause to a single separate
function and do the same for 'hello'.

> -		struct efi_loaded_image_obj *image_handle;
> -		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_handle,
> -					   &loaded_image_info);
> -		if (r != EFI_SUCCESS)
> +		struct efi_loaded_image_obj *obj;

obj what? Use a speaking name: img_handle.

> +		struct efi_loaded_image *image;

This name obscures the variable content. The variable holds the
EFI_LOADED_IMAGE_PROTOCOL. So you could call it img_prot.

Regards

Heinrich

> +
> +		if (bootefi_test_prepare(&image, &obj, "\\selftest",
> +					 (uintptr_t)&efi_selftest))
>  			return CMD_RET_FAILURE;
>  
> -		/*
> -		 * gd lives in a fixed register which may get clobbered while we
> -		 * execute the payload. So save it here and restore it on every
> -		 * callback entry
> -		 */
> -		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_handle, &systab);
> -		efi_restore_gd();
> -		free(loaded_image_info->load_options);
> -		efi_delete_handle(&image_handle->parent);
> +		r = efi_selftest(obj, &systab);
> +		bootefi_test_finish(image, obj);
>  		return r != EFI_SUCCESS;
>  	} else
>  #endif
> 

[U-Boot] [PATCH v11 4/6] efi: Split out test init/uninit into functions

[Simon Glass]

HI Heinrich,

On 15 October 2018 at 11:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 10/15/2018 04:17 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 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 | 90 +++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 65 insertions(+), 25 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 82d755ceb31..88b8b0172db 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -454,6 +454,64 @@ 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: Pointer to a struct which will hold the loaded image info.
>> + *    This struct will be inited by this function before use.
>> + * @obj: Pointer to a struct which will hold the loaded image object
>> + *    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 **imagep,
>> +                                      struct efi_loaded_image_obj **objp,
>> +                                      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);
>> +     bootefi_image_path = efi_dp_from_file(NULL, 0, path);
>> +     r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
>> +                                objp, imagep);
>> +     if (r)
>> +             return r;
>> +     /*
>> +      * gd lives in a fixed register which may get clobbered while we execute
>> +      * the payload. So save it here and restore it on every callback entry
>> +      */
>> +     efi_save_gd();
>> +
>> +     /* Transfer environment variable efi_selftest as load options */
>> +     set_load_options(*imagep, "efi_selftest");
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * bootefi_test_finish() - finish up after running an EFI test
>> + *
>> + * @image: Pointer to a struct which holds the loaded image info
>> + * @obj: Pointer to a struct which holds the loaded image object
>> + */
>> +static void bootefi_test_finish(struct efi_loaded_image *image,
>> +                             struct efi_loaded_image_obj *obj)
>> +{
>> +     efi_restore_gd();
>> +     free(image->load_options);
>> +     efi_delete_handle(&obj->parent);
>> +}
>> +#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
>> +
>>  static int do_bootefi_bootmgr_exec(void)
>>  {
>>       struct efi_device_path *device_path, *file_path;
>> @@ -532,34 +590,16 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>  #endif
>>  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>>       if (!strcmp(argv[1], "selftest")) {
>
> Why not move the whole body of the if clause to a single separate
> function and do the same for 'hello'.

Because the goal here is to have a 'prepare' function and a 'finish'
function, to make it easier to see what is going on (which is that we
are running a test).

>
>> -             struct efi_loaded_image_obj *image_handle;
>> -             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_handle,
>> -                                        &loaded_image_info);
>> -             if (r != EFI_SUCCESS)
>> +             struct efi_loaded_image_obj *obj;
>
> obj what? Use a speaking name: img_handle.

This is exactly what I find confusing about EFI. What does this have
to do with a handle? It is an image object, isn't it? But then in the
next line we have an image.So I don't want 'img' in my variable since
the next variable is an image:

>
>> +             struct efi_loaded_image *image;
>
> This name obscures the variable content. The variable holds the
> EFI_LOADED_IMAGE_PROTOCOL. So you could call it img_prot.

OK I'll change this one, but it's pretty horrible. A loaded image
protocol? What does that even mean in the real world? Just
gobbledygook I think.

Regards,
Simon

[U-Boot] [PATCH v11 5/6] efi: Create a function to set up for running EFI code

[Simon Glass]

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 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 | 91 +++++++++++++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 43 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 88b8b0172db..6a9a610a65c 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -320,6 +320,30 @@ 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 **imagep,
+					struct efi_loaded_image_obj **objp)
+{
+	efi_status_t ret;
+
+	ret = efi_setup_loaded_image(device_path, image_path, objp, imagep);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	/*
+	 * gd lives in a fixed register which may get clobbered while we execute
+	 * the payload. So save it here and restore it on every callback entry
+	 */
+	efi_save_gd();
+
+	/* Transfer environment variable as load options */
+	set_load_options(*imagep, load_options_path);
+
+	return 0;
+}
+
 /**
  * do_bootefi_exec() - execute EFI binary
  *
@@ -338,8 +362,8 @@ static efi_status_t do_bootefi_exec(void *efi,
 	efi_handle_t mem_handle = NULL;
 	struct efi_device_path *memdp = NULL;
 	efi_status_t ret;
-	struct efi_loaded_image_obj *image_handle = NULL;
-	struct efi_loaded_image *loaded_image_info = NULL;
+	struct efi_loaded_image_obj *obj = NULL;
+	struct efi_loaded_image *image = NULL;
 
 	EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
 				     struct efi_system_table *st);
@@ -369,21 +393,13 @@ static efi_status_t do_bootefi_exec(void *efi,
 		assert(device_path && image_path);
 	}
 
-	ret = efi_setup_loaded_image(device_path, image_path, &image_handle,
-				     &loaded_image_info);
-	if (ret != EFI_SUCCESS)
-		goto exit;
-
-	/*
-	 * gd lives in a fixed register which may get clobbered while we execute
-	 * the payload. So save it here and restore it on every callback entry
-	 */
-	efi_save_gd();
+	ret = bootefi_run_prepare("bootargs", device_path, image_path,
+				  &image, &obj);
+	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_handle, efi, loaded_image_info);
+	entry = efi_load_pe(obj, efi, image);
 	if (!entry) {
 		ret = EFI_LOAD_ERROR;
 		goto exit;
@@ -391,10 +407,9 @@ static efi_status_t do_bootefi_exec(void *efi,
 
 	if (memdp) {
 		struct efi_device_path_memory *mdp = (void *)memdp;
-		mdp->memory_type = loaded_image_info->image_code_type;
-		mdp->start_address = (uintptr_t)loaded_image_info->image_base;
-		mdp->end_address = mdp->start_address +
-				loaded_image_info->image_size;
+		mdp->memory_type = image->image_code_type;
+		mdp->start_address = (uintptr_t)image->image_base;
+		mdp->end_address = mdp->start_address + image->image_size;
 	}
 
 	/* we don't support much: */
@@ -404,8 +419,8 @@ static efi_status_t do_bootefi_exec(void *efi,
 	/* Call our payload! */
 	debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
 
-	if (setjmp(&image_handle->exit_jmp)) {
-		ret = image_handle->exit_status;
+	if (setjmp(&obj->exit_jmp)) {
+		ret = obj->exit_status;
 		goto exit;
 	}
 
@@ -417,7 +432,7 @@ static efi_status_t do_bootefi_exec(void *efi,
 
 		/* Move into EL2 and keep running there */
 		armv8_switch_to_el2((ulong)entry,
-				    (ulong)image_handle,
+				    (ulong)obj,
 				    (ulong)&systab, 0, (ulong)efi_run_in_el2,
 				    ES_TO_AARCH64);
 
@@ -434,7 +449,7 @@ static efi_status_t do_bootefi_exec(void *efi,
 		secure_ram_addr(_do_nonsec_entry)(
 					efi_run_in_hyp,
 					(uintptr_t)entry,
-					(uintptr_t)image_handle,
+					(uintptr_t)obj,
 					(uintptr_t)&systab);
 
 		/* Should never reach here, efi exits with longjmp */
@@ -442,12 +457,12 @@ static efi_status_t do_bootefi_exec(void *efi,
 	}
 #endif
 
-	ret = efi_do_enter(image_handle, &systab, entry);
+	ret = efi_do_enter(obj, &systab, entry);
 
 exit:
 	/* image has returned, loaded-image obj goes *poof*: */
-	if (image_handle)
-		efi_delete_handle(&image_handle->parent);
+	if (obj)
+		efi_delete_handle(&obj->parent);
 	if (mem_handle)
 		efi_delete_handle(mem_handle);
 
@@ -468,33 +483,22 @@ 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 **imagep,
 					 struct efi_loaded_image_obj **objp,
-					 const char *path, ulong test_func)
+					 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,
 					      (uintptr_t)test_func);
 	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
-	r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
-				   objp, imagep);
-	if (r)
-		return r;
-	/*
-	 * gd lives in a fixed register which may get clobbered while we execute
-	 * the payload. So save it here and restore it on every callback entry
-	 */
-	efi_save_gd();
 
-	/* Transfer environment variable efi_selftest as load options */
-	set_load_options(*imagep, "efi_selftest");
-
-	return 0;
+	return bootefi_run_prepare(load_options_path, bootefi_device_path,
+				   bootefi_image_path, imagep, objp);
 }
 
 /**
@@ -594,7 +598,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		struct efi_loaded_image *image;
 
 		if (bootefi_test_prepare(&image, &obj, "\\selftest",
-					 (uintptr_t)&efi_selftest))
+					 (uintptr_t)&efi_selftest,
+					 "efi_selftest"))
 			return CMD_RET_FAILURE;
 
 		/* Execute the test */
-- 
2.19.1.331.ge82ca0e54c-goog

[U-Boot] [PATCH v11 6/6] efi: Rename bootefi_test_finish() to bootefi_run_finish()

[Simon Glass]

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 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 | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 6a9a610a65c..d5a03ee7f6d 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -344,6 +344,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
+ *
+ * @image: Pointer to a struct which holds the loaded image info
+ * @obj: Pointer to a struct which holds the loaded image object
+ */
+static void bootefi_run_finish(struct efi_loaded_image *image,
+			       struct efi_loaded_image_obj *obj)
+{
+	efi_restore_gd();
+	free(image->load_options);
+	efi_delete_handle(&obj->parent);
+}
+
 /**
  * do_bootefi_exec() - execute EFI binary
  *
@@ -461,8 +475,7 @@ static efi_status_t do_bootefi_exec(void *efi,
 
 exit:
 	/* image has returned, loaded-image obj goes *poof*: */
-	if (obj)
-		efi_delete_handle(&obj->parent);
+	bootefi_run_finish(image, obj);
 	if (mem_handle)
 		efi_delete_handle(mem_handle);
 
@@ -500,20 +513,6 @@ static efi_status_t bootefi_test_prepare(struct efi_loaded_image **imagep,
 	return bootefi_run_prepare(load_options_path, bootefi_device_path,
 				   bootefi_image_path, imagep, objp);
 }
-
-/**
- * bootefi_test_finish() - finish up after running an EFI test
- *
- * @image: Pointer to a struct which holds the loaded image info
- * @obj: Pointer to a struct which holds the loaded image object
- */
-static void bootefi_test_finish(struct efi_loaded_image *image,
-				struct efi_loaded_image_obj *obj)
-{
-	efi_restore_gd();
-	free(image->load_options);
-	efi_delete_handle(&obj->parent);
-}
 #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
 
 static int do_bootefi_bootmgr_exec(void)
@@ -604,7 +603,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 		/* Execute the test */
 		r = efi_selftest(obj, &systab);
-		bootefi_test_finish(image, obj);
+		bootefi_run_finish(image, obj);
 		return r != EFI_SUCCESS;
 	} else
 #endif
-- 
2.19.1.331.ge82ca0e54c-goog