On 10/8/23 23:36, Simon Glass wrote:

When the SMBIOS table is written to an address above 4GB a 32-bit table address is not large enough.

Use an SMBIOS3 table in that case.

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

include/smbios.h | 22 +++++++++++++++++++++- lib/smbios.c | 22 ++++++++++++++++++---- 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/include/smbios.h b/include/smbios.h index c9df2706f5a6..ddabb558299e 100644 --- a/include/smbios.h +++ b/include/smbios.h @@ -12,7 +12,8 @@

/* SMBIOS spec version implemented */ #define SMBIOS_MAJOR_VER 3 -#define SMBIOS_MINOR_VER 0 +#define SMBIOS_MINOR_VER 7

• enum { SMBIOS_STR_MAX = 64, /* Maximum length allowed for a string */

@@ -54,6 +55,25 @@ struct __packed smbios_entry { u8 bcd_rev; };

+struct __packed smbios3_entry {

• u8 anchor[5];
• u8 checksum;
• u8 length;
• u8 major_ver;
• u8 minor_ver;
• u8 docrev;
• u8 entry_point_rev;
• u8 reserved;
• u32 max_struct_size;
• u64 struct_table_address;

+};

+/* These two structures should use the same amount of 16-byte-aligned space */ +static_assert(ALIGN(16, sizeof(struct smbios_entry)) ==

•      ALIGN(16, sizeof(struct smbios3_entry)));
  

• /* BIOS characteristics */ #define BIOS_CHARACTERISTICS_PCI_SUPPORTED (1 << 7) #define BIOS_CHARACTERISTICS_UPGRADEABLE (1 << 11)

diff --git a/lib/smbios.c b/lib/smbios.c index c7a557bc9b7b..82e259f31791 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -487,7 +487,11 @@ ulong write_smbios_table(ulong addr) addr = ALIGN(addr, 16); start_addr = addr;

• addr += sizeof(struct smbios_entry);

• /*

• * So far we don't know which struct will be used, but they both end
  

• * up using the same amount of 16-bit-aligned space
  

• */
  

• addr += max(sizeof(struct smbios_entry), sizeof(struct smbios3_entry)); addr = ALIGN(addr, 16); tables = addr;

@@ -513,13 +517,23 @@ ulong write_smbios_table(ulong addr) */ table_addr = (ulong)map_sysmem(tables, 0); if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {

You have to check the end address of the table not the start address here.

The SMBIOS specification says that you should always supply a 32bit SMBIOS entry point. Of course this is not possible on boards that don't have low memory.

In install_smbios_table() this can be achieved by calling efi_allocate_pages() with EFI_MAX_ALLOCATE_TYPE and a maximum address of 4 GiB - 1. Only if this fails use high memory.

Best regards

Heinrich

• struct smbios3_entry *se;
  

  /*
  • 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);
  

• addr = 0;
  

• printf("WARNING: Using SMBIOS3.0 due to table-address overflow %lx\n",
  

•        table_addr);
  

• se = map_sysmem(start_addr, sizeof(struct smbios_entry));
  

• memset(se, '\0', sizeof(struct smbios_entry));
  

• memcpy(se->anchor, "_SM3_", 5);
  

• se->length = sizeof(struct smbios3_entry);
  

• se->major_ver = SMBIOS_MAJOR_VER;
  

• se->minor_ver = SMBIOS_MINOR_VER;
  

• se->docrev = 0;
  

• se->entry_point_rev = 1;
  

• se->max_struct_size = len;
  

• se->struct_table_address = table_addr;
  

  } else { struct smbios_entry *se;

Hi Both,

[...]

...

...

@@ -513,13 +517,23 @@ ulong write_smbios_table(ulong addr) */ table_addr = (ulong)map_sysmem(tables, 0); if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {

You have to check the end address of the table not the start address here.

The SMBIOS specification says that you should always supply a 32bit SMBIOS entry point. Of course this is not possible on boards that don't have low memory.

In install_smbios_table() this can be achieved by calling efi_allocate_pages() with EFI_MAX_ALLOCATE_TYPE and a maximum address of 4 GiB - 1. Only if this fails use high memory.

Hmm perhaps we need a way to allocate memory below 4GB in U-Boot? How does this EFI function work?

Yes, prior to 53fab13a7b11 ("efi: Use the installed SMBIOS tables") efi_allocate_pages() was called directly from efi_smbios_register(). My boards all malloc() memory above the 4GB boundary and I just recently rebased and hit a regression caused by this.

In my testing, calling efi_allocate_pages() from install_smbios_table() did work, but I figure that's probably not intended behaviour...

I can confirm that this series does allow my boards to boot again, but dmidecode is unable to decode the SMBIOS 3.0 table (even with STRICT_DEVMEM disabled fwiw).

Below is EFI and board memory map info after applying this patch series.

=> efidebug tables Cannot read EFI system partition Cannot read EFI system partition Failed to persist EFI variables 000000017ea22040 32313633-3532-3634-2d66-3765662d3463 EFI Conformance Profiles Table 000000017ea21040 36366265-3139-6138-2d37-6565662d3430 Runtime properties 000000017fb13000 64663266-3531-3434-2d39-3739342d3461 (unknown) => efidebug memmap Type Start End Attributes ================ ================ ================ ========== CONVENTIONAL 0000000080000000-000000017ea1f000 WB BOOT DATA 000000017ea1f000-000000017ea21000 WB RUNTIME DATA 000000017ea21000-000000017ea22000 WB|RT BOOT DATA 000000017ea22000-000000017ea23000 WB RUNTIME DATA 000000017ea23000-000000017ea25000 WB|RT BOOT DATA 000000017ea25000-000000017ea26000 WB RUNTIME DATA 000000017ea26000-000000017ea36000 WB|RT BOOT DATA 000000017ea36000-000000017eaef000 WB BOOT CODE 000000017eaef000-000000017fb13000 WB RUNTIME DATA 000000017fb13000-000000017fb14000 WB|RT BOOT CODE 000000017fb14000-000000017ff20000 WB RUNTIME CODE 000000017ff20000-000000017ff30000 WB|RT BOOT CODE 000000017ff30000-0000000180000000 WB BOOT DATA 0000000180000000-000000027c7a0000 WB => bdinfo boot_params = 0x0000000000000000 DRAM bank = 0x0000000000000000 -> start = 0x0000000080000000 -> size = 0x0000000100000000 DRAM bank = 0x0000000000000001 -> start = 0x0000000180000000 -> size = 0x00000000fc7a0000 flashstart = 0x0000000000000000 flashsize = 0x0000000000000000 flashoffset = 0x0000000000000000 baudrate = 115200 bps relocaddr = 0x000000017ff26000 reloc off = 0x000000017ff26000 Build = 64-bit fdt_blob = 0x000000017faef6c0 new_fdt = 0x000000017faef6c0 fdt_size = 0x0000000000017680 Video = framebuffer@9D400000 active FB base = 0x000000009d400000 FB size = 1080x2160x32 lmb_dump_all: memory.cnt = 0x1 / max = 0x40 memory[0] [0x80000000-0x27c79ffff], 0x1fc7a0000 bytes flags: 0 reserved.cnt = 0x7 / max = 0x40 reserved[0] [0x85700000-0x85cfffff], 0x00600000 bytes flags: 4 reserved[1] [0x85e00000-0x85efffff], 0x00100000 bytes flags: 4 reserved[2] [0x85fc0000-0x890fffff], 0x03140000 bytes flags: 4 reserved[3] [0x8ab00000-0x8c416fff], 0x01917000 bytes flags: 4 reserved[4] [0x8c500000-0x97bfffff], 0x0b700000 bytes flags: 4 reserved[5] [0x9d400000-0x9f7fffff], 0x02400000 bytes flags: 4 reserved[6] [0x17ea1f000-0x27c79ffff], 0xfdd81000 bytes flags: 0 devicetree = board arch_number = 0x0000000000000000 TLB addr = 0x000000017fff0000 irq_sp = 0x000000017faef6b0 sp start = 0x000000017faef6b0 Early malloc usage: a78 / 2000

I don't think EFI has been set up by the time this function is called.

Regards, Simon

On 18/10/2023 04:33, Simon Glass wrote:

Hi Caleb,

On Tue, 17 Oct 2023 at 11:59, Caleb Connolly caleb.connolly@linaro.org wrote:

...

Hi Both,

[...]

...

...

...

@@ -513,13 +517,23 @@ ulong write_smbios_table(ulong addr) */ table_addr = (ulong)map_sysmem(tables, 0); if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {

You have to check the end address of the table not the start address here.

The SMBIOS specification says that you should always supply a 32bit SMBIOS entry point. Of course this is not possible on boards that don't have low memory.

In install_smbios_table() this can be achieved by calling efi_allocate_pages() with EFI_MAX_ALLOCATE_TYPE and a maximum address of 4 GiB - 1. Only if this fails use high memory.

Hmm perhaps we need a way to allocate memory below 4GB in U-Boot? How does this EFI function work?

Yes, prior to 53fab13a7b11 ("efi: Use the installed SMBIOS tables") efi_allocate_pages() was called directly from efi_smbios_register(). My boards all malloc() memory above the 4GB boundary and I just recently rebased and hit a regression caused by this.

In my testing, calling efi_allocate_pages() from install_smbios_table() did work, but I figure that's probably not intended behaviour...

I can confirm that this series does allow my boards to boot again, but dmidecode is unable to decode the SMBIOS 3.0 table (even with STRICT_DEVMEM disabled fwiw).

OK thank you for digging into this. So I suppose I can repeat that in qemu, with a suitable distro?

I would assume so, I'm running Alpine in my testing. I'd be more than happy to test any patches as well.

...

Below is EFI and board memory map info after applying this patch series.

=> efidebug tables Cannot read EFI system partition Cannot read EFI system partition Failed to persist EFI variables 000000017ea22040 32313633-3532-3634-2d66-3765662d3463 EFI Conformance Profiles Table 000000017ea21040 36366265-3139-6138-2d37-6565662d3430 Runtime properties 000000017fb13000 64663266-3531-3434-2d39-3739342d3461 (unknown) => efidebug memmap Type Start End Attributes ================ ================ ================ ========== CONVENTIONAL 0000000080000000-000000017ea1f000 WB BOOT DATA 000000017ea1f000-000000017ea21000 WB RUNTIME DATA 000000017ea21000-000000017ea22000 WB|RT BOOT DATA 000000017ea22000-000000017ea23000 WB RUNTIME DATA 000000017ea23000-000000017ea25000 WB|RT BOOT DATA 000000017ea25000-000000017ea26000 WB RUNTIME DATA 000000017ea26000-000000017ea36000 WB|RT BOOT DATA 000000017ea36000-000000017eaef000 WB BOOT CODE 000000017eaef000-000000017fb13000 WB RUNTIME DATA 000000017fb13000-000000017fb14000 WB|RT BOOT CODE 000000017fb14000-000000017ff20000 WB RUNTIME CODE 000000017ff20000-000000017ff30000 WB|RT BOOT CODE 000000017ff30000-0000000180000000 WB BOOT DATA 0000000180000000-000000027c7a0000 WB => bdinfo boot_params = 0x0000000000000000 DRAM bank = 0x0000000000000000 -> start = 0x0000000080000000 -> size = 0x0000000100000000 DRAM bank = 0x0000000000000001 -> start = 0x0000000180000000 -> size = 0x00000000fc7a0000 flashstart = 0x0000000000000000 flashsize = 0x0000000000000000 flashoffset = 0x0000000000000000 baudrate = 115200 bps relocaddr = 0x000000017ff26000 reloc off = 0x000000017ff26000 Build = 64-bit fdt_blob = 0x000000017faef6c0 new_fdt = 0x000000017faef6c0 fdt_size = 0x0000000000017680 Video = framebuffer@9D400000 active FB base = 0x000000009d400000 FB size = 1080x2160x32 lmb_dump_all: memory.cnt = 0x1 / max = 0x40 memory[0] [0x80000000-0x27c79ffff], 0x1fc7a0000 bytes flags: 0 reserved.cnt = 0x7 / max = 0x40 reserved[0] [0x85700000-0x85cfffff], 0x00600000 bytes flags: 4 reserved[1] [0x85e00000-0x85efffff], 0x00100000 bytes flags: 4 reserved[2] [0x85fc0000-0x890fffff], 0x03140000 bytes flags: 4 reserved[3] [0x8ab00000-0x8c416fff], 0x01917000 bytes flags: 4 reserved[4] [0x8c500000-0x97bfffff], 0x0b700000 bytes flags: 4 reserved[5] [0x9d400000-0x9f7fffff], 0x02400000 bytes flags: 4 reserved[6] [0x17ea1f000-0x27c79ffff], 0xfdd81000 bytes flags: 0 devicetree = board arch_number = 0x0000000000000000 TLB addr = 0x000000017fff0000 irq_sp = 0x000000017faef6b0 sp start = 0x000000017faef6b0 Early malloc usage: a78 / 2000

...

I don't think EFI has been set up by the time this function is called.

Regards, Simon

-- // Caleb (they/them)

Regards, Simon

On 10/8/23 23:36, Simon Glass wrote:

EFI does not use the 'anchor string' to determine the SMBIOS table version, instead preferring to have two separate GUIDs. Use the correct one, depending on the table version.

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

include/efi_api.h | 4 ++++ lib/efi_loader/efi_smbios.c | 12 ++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h index 8f5ef5f680f3..1abac2ed7563 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -430,6 +430,10 @@ struct efi_runtime_services { EFI_GUID(0xeb9d2d31, 0x2d88, 0x11d3, \ 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)

+#define SMBIOS3_TABLE_GUID \

• EFI_GUID(0xf2fd1544, 0x9794, 0x4a2c, \

•  0x99, 0x2e, 0xe5, 0xbb, 0xcf, 0x20, 0xe3, 0x94)
  

• #define EFI_LOAD_FILE_PROTOCOL_GUID \ EFI_GUID(0x56ec3091, 0x954c, 0x11d2, \ 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)

diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index 48446f654d9b..9ef736853d10 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -15,6 +15,8 @@ #include <smbios.h> #include <linux/sizes.h>

+const efi_guid_t smbios3_guid = SMBIOS3_TABLE_GUID;

• enum { TABLE_SIZE = SZ_4K, };

@@ -26,8 +28,10 @@ enum { */ efi_status_t efi_smbios_register(void) {

• const efi_guid_t *guid; ulong addr; efi_status_t ret;

• void *buf;

  addr = gd->arch.smbios_start;

Why did you define the macro gd_smbios_start() if it does not point to the correct field and you don't use it? See 50834884a815 ("Record the position of the SMBIOS tables")

if (!addr) { @@ -43,8 +47,12 @@ efi_status_t efi_smbios_register(void) log_debug("EFI using SMBIOS tables at %lx\n", addr);

/* Install SMBIOS information as configuration table */

• return efi_install_configuration_table(&smbios_guid,

• 			       map_sysmem(addr, 0));
  

• buf = map_sysmem(addr, 10);

Where does this arbitrary value of 10 come from?

The second parameter can safely be set to 0 as the SMBIOS table is not in PCIe mapped memory on the sandbox.

• guid = !memcmp(buf, "_SM_", 4) ? &smbios_guid : &smbios3_guid;
• ret = efi_install_configuration_table(guid, buf);
• unmap_sysmem(buf);

Even on the sandbox this is a NOP as this is not a PCIe address.

Best regards

Heinrich

• return ret; }

  static int install_smbios_table(void)