| Message ID | 20171204212832.130100-8-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi: Enable basic sandbox support for EFI loader | expand |
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; > } >
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
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; }
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(-)