Re: [U-Boot] [PATCH v13 1/4] sandbox: smbios: Update to support sandbox

On 11/14/2018 07:50 AM, 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 v13:

• Update code to deal with the struct_table_address member

Changes in v12: None Changes in v11:

• Fix the EFI code that has since been added and relies on broken behaviour

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

• Drop incorrect map_sysmem() in write_smbios_table()

  lib/efi_loader/efi_smbios.c | 20 +++++++++----- lib/smbios.c | 52 ++++++++++++++++++++++++++++++------- 2 files changed, 56 insertions(+), 16 deletions(-)

diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index 38e42fa2432..a81488495e2 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -7,6 +7,7 @@

#include <common.h> #include <efi_loader.h> +#include <mapmem.h> #include <smbios.h>

static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID; @@ -19,17 +20,19 @@ static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID; efi_status_t efi_smbios_register(void) { /* Map within the low 32 bits, to allow for 32bit SMBIOS tables */

• u64 dmi = U32_MAX;

• u64 dmi_addr = U32_MAX; efi_status_t ret;

• void *dmi;

  /* Reserve 4kiB page for SMBIOS */ ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,

• 		 EFI_RUNTIME_SERVICES_DATA, 1, &dmi);
  

• 		 EFI_RUNTIME_SERVICES_DATA, 1, &dmi_addr);
  
  

  if (ret != EFI_SUCCESS) { /* Could not find space in lowmem, use highmem instead */ ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,

• 			 EFI_RUNTIME_SERVICES_DATA, 1, &dmi);
  

• 			 EFI_RUNTIME_SERVICES_DATA, 1,
  

• 			 &dmi_addr);
  
  

  if (ret != EFI_SUCCESS) return ret;

@@ -39,11 +42,14 @@ efi_status_t efi_smbios_register(void) * Generate SMBIOS tables - we know that efi_allocate_pages() returns * a 4k-aligned address, so it is safe to assume that * write_smbios_table() will write the table at that address.

• *
  

• * Note that on sandbox, efi_allocate_pages() unfortunately returns a
  

• * pointer even though it uses a uint64_t type. Convert it.
  

  */

• assert(!(dmi & 0xf));
• write_smbios_table(dmi);

• assert(!(dmi_addr & 0xf));

• dmi = (void *)(uintptr_t)dmi_addr;

• write_smbios_table(map_to_sysmem(dmi));

  /* And expose them to our EFI payload */

• return efi_install_configuration_table(&smbios_guid,

• 			       (void *)(uintptr_t)dmi);
  

• return efi_install_configuration_table(&smbios_guid, dmi); }

diff --git a/lib/smbios.c b/lib/smbios.c index 326eb00230d..2cd08b5e16f 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -6,6 +6,7 @@ */

#include <common.h> +#include <mapmem.h> #include <smbios.h> #include <tables_csum.h> #include <version.h> @@ -72,9 +73,10 @@ static int smbios_string_table_len(char *start)

static int smbios_write_type0(ulong *current, int handle) {

• struct smbios_type0 *t = (struct smbios_type0 *)*current;

• struct smbios_type0 *t; int len = sizeof(struct smbios_type0);

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type0)); fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle); t->vendor = smbios_add_string(t->eos, "U-Boot");

@@ -101,16 +103,18 @@ static int smbios_write_type0(ulong *current, int handle)

len = t->length + smbios_string_table_len(t->eos); *current += len;

• unmap_sysmem(t);

  return len; }

  static int smbios_write_type1(ulong *current, int handle) {

• struct smbios_type1 *t = (struct smbios_type1 *)*current;

• struct smbios_type1 *t; int len = sizeof(struct smbios_type1); char *serial_str = env_get("serial#");

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type1)); fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle); t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);

@@ -122,15 +126,17 @@ static int smbios_write_type1(ulong *current, int handle)

len = t->length + smbios_string_table_len(t->eos); *current += len;

• unmap_sysmem(t);

  return len; }

  static int smbios_write_type2(ulong *current, int handle) {

• struct smbios_type2 *t = (struct smbios_type2 *)*current;

• struct smbios_type2 *t; int len = sizeof(struct smbios_type2);

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type2)); fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle); t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);

@@ -140,15 +146,17 @@ static int smbios_write_type2(ulong *current, int handle)

len = t->length + smbios_string_table_len(t->eos); *current += len;

• unmap_sysmem(t);

  return len; }

  static int smbios_write_type3(ulong *current, int handle) {

• struct smbios_type3 *t = (struct smbios_type3 *)*current;

• struct smbios_type3 *t; int len = sizeof(struct smbios_type3);

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type3)); fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle); t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);

@@ -160,6 +168,7 @@ static int smbios_write_type3(ulong *current, int handle)

len = t->length + smbios_string_table_len(t->eos); *current += len;

• unmap_sysmem(t);

  return len; }

@@ -198,9 +207,10 @@ static void smbios_write_type4_dm(struct smbios_type4 *t)

static int smbios_write_type4(ulong *current, int handle) {

• struct smbios_type4 *t = (struct smbios_type4 *)*current;

• struct smbios_type4 *t; int len = sizeof(struct smbios_type4);

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type4)); fill_smbios_header(t, SMBIOS_PROCESSOR_INFORMATION, len, handle); t->processor_type = SMBIOS_PROCESSOR_TYPE_CENTRAL;

@@ -214,32 +224,37 @@ static int smbios_write_type4(ulong *current, int handle)

len = t->length + smbios_string_table_len(t->eos); *current += len;

• unmap_sysmem(t);

  return len; }

  static int smbios_write_type32(ulong *current, int handle) {

• struct smbios_type32 *t = (struct smbios_type32 *)*current;

• struct smbios_type32 *t; int len = sizeof(struct smbios_type32);

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type32)); fill_smbios_header(t, SMBIOS_SYSTEM_BOOT_INFORMATION, len, handle);

  *current += len;

• unmap_sysmem(t);

  return len; }

  static int smbios_write_type127(ulong *current, int handle) {

• struct smbios_type127 *t = (struct smbios_type127 *)*current;

• struct smbios_type127 *t; int len = sizeof(struct smbios_type127);

• t = map_sysmem(*current, len); memset(t, 0, sizeof(struct smbios_type127)); fill_smbios_header(t, SMBIOS_END_OF_TABLE, len, handle);

  *current += len;

• unmap_sysmem(t);

  return len; }

@@ -257,6 +272,7 @@ static smbios_write_type smbios_write_funcs[] = { ulong write_smbios_table(ulong addr) { struct smbios_entry *se;

• ulong table_addr; ulong tables; int len = 0; int max_struct_size = 0;

@@ -268,7 +284,7 @@ ulong write_smbios_table(ulong addr) /* 16 byte align the table address */ addr = ALIGN(addr, 16);

• se = (struct smbios_entry *)(uintptr_t)addr;

• se = map_sysmem(addr, sizeof(struct smbios_entry)); memset(se, 0, sizeof(struct smbios_entry));

  addr += sizeof(struct smbios_entry);

@@ -290,7 +306,24 @@ ulong write_smbios_table(ulong addr) se->max_struct_size = max_struct_size; memcpy(se->intermediate_anchor, "_DMI_", 5); se->struct_table_length = len;

• se->struct_table_address = tables;

• /*

• * We must use a pointer here so things work correctly on sandbox. The
  

• * user of this table is not aware of the mapping of addresses to
  

• * sandbox's DRAM buffer.
  

• */
  

• table_addr = (ulong)map_sysmem(tables, 0);
• if (sizeof(table_addr) >= sizeof(u32) && table_addr >= (1ULL << 32)) {

sizeof(long) >= sizeof(u32) will always be true on any platform U-Boot supports, no?

How about

if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX)

• /*
  

•  * We need to put this >32-bit pointer into the table but the
  

•  * field is only 32 bits wide.
  

•  */
  

• printf("WARNING: SMBIOS table_address overflow %llx\n",
  

•        (unsigned long long)table_addr);
  

• table_addr = 0;
  

I'm also not sure what to do about the error case. IMHO ideally we should propagate the error to the upper layers, but consider it non-fatal.

• }

• se->struct_table_address = table_addr;

• se->struct_count = handle;

  /* calculate checksums */

@@ -298,6 +331,7 @@ ulong write_smbios_table(ulong addr) isize = sizeof(struct smbios_entry) - SMBIOS_INTERMEDIATE_OFFSET; se->intermediate_checksum = table_compute_checksum(istart, isize); se->checksum = table_compute_checksum(se, sizeof(struct smbios_entry));

• unmap_sysmem(se);

  return addr; }