[U-Boot,v2,07/16] sandbox: smbios: Update to support sandbox

Message ID 20171204212832.130100-8-sjg@chromium.org
State New
Delegated to: Alexander Graf
Headers show
Series
  • efi: Enable basic sandbox support for EFI loader
Related show

Commit Message

Simon Glass Dec. 4, 2017, 9:28 p.m.
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 v2: None

 lib/smbios.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

Comments

Heinrich Schuchardt Feb. 18, 2018, 12:14 p.m. | #1
On 12/04/2017 10:28 PM, 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>
> ---
> 
> Changes in v2: None
> 
>  lib/smbios.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/smbios.c b/lib/smbios.c
> index 8f19ad89c1..56481d448d 100644
> --- a/lib/smbios.c
> +++ b/lib/smbios.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <common.h>
> +#include <mapmem.h>
>  #include <smbios.h>
>  #include <tables_csum.h>
>  #include <version.h>
> @@ -75,9 +76,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");
> @@ -104,16 +106,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);
> @@ -125,15 +129,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);
> @@ -143,15 +149,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);
> @@ -163,6 +171,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;
>  }
> @@ -201,9 +210,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;
> @@ -217,32 +227,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;
>  }
> @@ -271,7 +286,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);
> @@ -280,9 +295,13 @@ ulong write_smbios_table(ulong addr)
>  
>  	/* populate minimum required tables */
>  	for (i = 0; i < ARRAY_SIZE(smbios_write_funcs); i++) {
> -		int tmp = smbios_write_funcs[i]((ulong *)&addr, handle++);
> +		ulong *ptr = map_sysmem(addr, 0);

This replaces &addr by addr and breaks booting on ARM64.

Did you mean:
		ulong *ptr = map_sysmem((phys_addr_t)&addr, 0);

The coding would get much easier to maintain if the map_sysmem()
function would be eliminated from U-Boot.

Why don't you simply use the address range that mmap() has provided?
This would avoid double book keeping.

Regards

Heinrich

> +		int tmp;
> +
> +		tmp = smbios_write_funcs[i](ptr, handle++);
>  		max_struct_size = max(max_struct_size, tmp);
>  		len += tmp;
> +		unmap_sysmem(ptr);
>  	}
>  
>  	memcpy(se->anchor, "_SM_", 4);
> @@ -300,6 +319,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;
>  }
>
Simon Glass Feb. 19, 2018, 3:48 p.m. | #2
Hi Heinrich,

On 18 February 2018 at 05:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 12/04/2017 10:28 PM, 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>
>> ---
>>
>> Changes in v2: None
>>
>>  lib/smbios.c | 38 +++++++++++++++++++++++++++++---------
>>  1 file changed, 29 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/smbios.c b/lib/smbios.c
>> index 8f19ad89c1..56481d448d 100644
>> --- a/lib/smbios.c
>> +++ b/lib/smbios.c
>> @@ -7,6 +7,7 @@
>>   */
>>
>>  #include <common.h>
>> +#include <mapmem.h>
>>  #include <smbios.h>
>>  #include <tables_csum.h>
>>  #include <version.h>
>> @@ -75,9 +76,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");
>> @@ -104,16 +106,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);
>> @@ -125,15 +129,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);
>> @@ -143,15 +149,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);
>> @@ -163,6 +171,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;
>>  }
>> @@ -201,9 +210,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;
>> @@ -217,32 +227,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;
>>  }
>> @@ -271,7 +286,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);
>> @@ -280,9 +295,13 @@ ulong write_smbios_table(ulong addr)
>>
>>       /* populate minimum required tables */
>>       for (i = 0; i < ARRAY_SIZE(smbios_write_funcs); i++) {
>> -             int tmp = smbios_write_funcs[i]((ulong *)&addr, handle++);
>> +             ulong *ptr = map_sysmem(addr, 0);
>
> This replaces &addr by addr and breaks booting on ARM64.
>
> Did you mean:
>                 ulong *ptr = map_sysmem((phys_addr_t)&addr, 0);

No, actually I don't think this is really needed at all since the
version is done by the functions it calls.

>
> The coding would get much easier to maintain if the map_sysmem()
> function would be eliminated from U-Boot.
>
> Why don't you simply use the address range that mmap() has provided?
> This would avoid double book keeping.

It is on my mind. But the address is used in U-Boot (e.g. in scripts).
Every other board has a defined and known SDRAM start address, and I
think it is best that sandbox has this too. It is also a good marker
of the conversion between an address and a pointer. IMO using a cast
is a bit ad-hoc.

Regards,
Simon

Patch

diff --git a/lib/smbios.c b/lib/smbios.c
index 8f19ad89c1..56481d448d 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -7,6 +7,7 @@ 
  */
 
 #include <common.h>
+#include <mapmem.h>
 #include <smbios.h>
 #include <tables_csum.h>
 #include <version.h>
@@ -75,9 +76,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");
@@ -104,16 +106,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);
@@ -125,15 +129,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);
@@ -143,15 +149,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);
@@ -163,6 +171,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;
 }
@@ -201,9 +210,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;
@@ -217,32 +227,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;
 }
@@ -271,7 +286,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);
@@ -280,9 +295,13 @@  ulong write_smbios_table(ulong addr)
 
 	/* populate minimum required tables */
 	for (i = 0; i < ARRAY_SIZE(smbios_write_funcs); i++) {
-		int tmp = smbios_write_funcs[i]((ulong *)&addr, handle++);
+		ulong *ptr = map_sysmem(addr, 0);
+		int tmp;
+
+		tmp = smbios_write_funcs[i](ptr, handle++);
 		max_struct_size = max(max_struct_size, tmp);
 		len += tmp;
+		unmap_sysmem(ptr);
 	}
 
 	memcpy(se->anchor, "_SM_", 4);
@@ -300,6 +319,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;
 }