[PATCH v2 0/6] smbios: Deal with tables beyond 4GB
When the malloc() region extends beyond 4GB on ARM we may end up with an SMBIOS table in that region.
Add support for writing an SMBIOS3 table, which supports a 64-bit address.
Note that this problem does not happen on x86 since it requires the tables to be placed just below 1MB in memory, unless CONFIG_BLOBLIST_TABLES is enabled.
Changes in v2: - Check the end of the table rather than the start. - Add a new patch to correct gd_smbios_start() - Add a note about why unmap_system() is called
Simon Glass (6): smbios: Refactor 32-bit code into an else statement smbios: Move the rest of the SMBIOS2 code smbios: Use SMBIOS 3.0 to support an address above 4GB smbios: Correct gd_smbios_start() efi: Use the correct GUID for the SMBIOS table smbios: Require the caller to align the SMBIOS table
include/asm-generic/global_data.h | 2 +- include/efi_api.h | 4 ++ include/smbios.h | 27 +++++++++-- lib/efi_loader/efi_smbios.c | 14 ++++-- lib/smbios.c | 79 ++++++++++++++++++------------- 5 files changed, 85 insertions(+), 41 deletions(-)
In preparation for adding support for SMBIOS3 move this code into an else statement. There is no functional change.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/smbios.c | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/lib/smbios.c b/lib/smbios.c index d7f4999e8b2a..96f67cd6ad7b 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -509,14 +509,6 @@ ulong write_smbios_table(ulong addr) len += tmp; }
- memcpy(se->anchor, "_SM_", 4); - se->length = sizeof(struct smbios_entry); - se->major_ver = SMBIOS_MAJOR_VER; - se->minor_ver = SMBIOS_MINOR_VER; - se->max_struct_size = max_struct_size; - memcpy(se->intermediate_anchor, "_DMI_", 5); - se->struct_table_length = len; - /* * 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 @@ -532,16 +524,28 @@ ulong write_smbios_table(ulong addr) (unsigned long long)table_addr); addr = 0; goto out; + } else { + memcpy(se->anchor, "_SM_", 4); + se->length = sizeof(struct smbios_entry); + se->major_ver = SMBIOS_MAJOR_VER; + se->minor_ver = SMBIOS_MINOR_VER; + se->max_struct_size = max_struct_size; + memcpy(se->intermediate_anchor, "_DMI_", 5); + se->struct_table_length = len; + + se->struct_table_address = table_addr; + + se->struct_count = handle; + + /* calculate checksums */ + istart = (char *)se + SMBIOS_INTERMEDIATE_OFFSET; + 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)); } - se->struct_table_address = table_addr; - - se->struct_count = handle; - - /* calculate checksums */ - istart = (char *)se + SMBIOS_INTERMEDIATE_OFFSET; - 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)); out: unmap_sysmem(se);
On 10/15/23 04:45, Simon Glass wrote:
In preparation for adding support for SMBIOS3 move this code into an else statement. There is no functional change.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
On Tue, 26 Dec 2023 at 12:46, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/15/23 04:45, Simon Glass wrote:
In preparation for adding support for SMBIOS3 move this code into an else statement. There is no functional change.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Move all of this logic into the else clause, since it will not be used for SMBIOS3
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/smbios.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/lib/smbios.c b/lib/smbios.c index 96f67cd6ad7b..c7a557bc9b7b 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -464,9 +464,8 @@ static struct smbios_write_method smbios_write_funcs[] = { ulong write_smbios_table(ulong addr) { ofnode parent_node = ofnode_null(); - struct smbios_entry *se; + ulong table_addr, start_addr; struct smbios_ctx ctx; - ulong table_addr; ulong tables; int len = 0; int max_struct_size = 0; @@ -486,9 +485,7 @@ ulong write_smbios_table(ulong addr)
/* 16 byte align the table address */ addr = ALIGN(addr, 16); - - se = map_sysmem(addr, sizeof(struct smbios_entry)); - memset(se, 0, sizeof(struct smbios_entry)); + start_addr = addr;
addr += sizeof(struct smbios_entry); addr = ALIGN(addr, 16); @@ -523,8 +520,11 @@ ulong write_smbios_table(ulong addr) printf("WARNING: SMBIOS table_address overflow %llx\n", (unsigned long long)table_addr); addr = 0; - goto out; } else { + struct smbios_entry *se; + + se = map_sysmem(start_addr, sizeof(struct smbios_entry)); + memset(se, '\0', sizeof(struct smbios_entry)); memcpy(se->anchor, "_SM_", 4); se->length = sizeof(struct smbios_entry); se->major_ver = SMBIOS_MAJOR_VER; @@ -545,9 +545,8 @@ ulong write_smbios_table(ulong addr) isize); se->checksum = table_compute_checksum(se, sizeof(struct smbios_entry)); + unmap_sysmem(se); } -out: - unmap_sysmem(se);
return addr; }
On 10/15/23 04:45, Simon Glass wrote:
Move all of this logic into the else clause, since it will not be used for SMBIOS3
Signed-off-by: Simon Glasssjg@chromium.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
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.
Note that we cannot use efi_allocate_pages() since this function has nothing to do with EFI. There is no equivalent function to allocate memory below 4GB in U-Boot. One solution would be to create a separate malloc() pool, or just always put the malloc() pool below 4GB.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Check the end of the table rather than the start.
include/smbios.h | 22 +++++++++++++++++++++- lib/smbios.c | 24 +++++++++++++++++++----- 2 files changed, 40 insertions(+), 6 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..92e98388084f 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;
@@ -512,14 +516,24 @@ ulong write_smbios_table(ulong addr) * sandbox's DRAM buffer. */ table_addr = (ulong)map_sysmem(tables, 0); - if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) { + if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) { + 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;
On 10/15/23 04:45, 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.
Note that we cannot use efi_allocate_pages() since this function has nothing to do with EFI. There is no equivalent function to allocate memory below 4GB in U-Boot. One solution would be to create a separate malloc() pool, or just always put the malloc() pool below 4GB.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
Check the end of the table rather than the start.
include/smbios.h | 22 +++++++++++++++++++++- lib/smbios.c | 24 +++++++++++++++++++----- 2 files changed, 40 insertions(+), 6 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..92e98388084f 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;
@@ -512,14 +516,24 @@ ulong write_smbios_table(ulong addr) * sandbox's DRAM buffer. */ table_addr = (ulong)map_sysmem(tables, 0);
- if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {
- if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) {
You have to check the end of the the last SMBIOS structure not the start of the first SMBIOS structure against UINT_MAX.
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);
This should be log_debug().
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;
You must fill the checksum:
se->checksum = table_compute_checksum(se, sizeof(struct smbios3_entry));
With the checksum filled I can use dmidecode in Linux based on the SMBIOS3 table:
ubuntu@ubuntu:~$ sudo dmidecode # dmidecode 3.5 # SMBIOS3 entry point at 0x27ef53000 Found SMBIOS entry point in EFI, reading table from /dev/mem. SMBIOS 3.7.0 present. # SMBIOS implementations newer than version 3.5.0 are not # fully supported by this version of dmidecode. Table at 0x27EF53020.
Handle 0x0000, DMI type 0, 24 bytes BIOS Information Vendor: U-Boot ...
Best regards
Heinrich
} else { struct smbios_entry *se;
Hi Heinrich,
On Mon, 20 Nov 2023 at 19:11, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/15/23 04:45, 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.
Note that we cannot use efi_allocate_pages() since this function has nothing to do with EFI. There is no equivalent function to allocate memory below 4GB in U-Boot. One solution would be to create a separate malloc() pool, or just always put the malloc() pool below 4GB.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
Check the end of the table rather than the start.
include/smbios.h | 22 +++++++++++++++++++++- lib/smbios.c | 24 +++++++++++++++++++----- 2 files changed, 40 insertions(+), 6 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..92e98388084f 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;@@ -512,14 +516,24 @@ ulong write_smbios_table(ulong addr) * sandbox's DRAM buffer. */ table_addr = (ulong)map_sysmem(tables, 0);
if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {
if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) {You have to check the end of the the last SMBIOS structure not the start of the first SMBIOS structure against UINT_MAX.
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);This should be log_debug().
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;You must fill the checksum:
se->checksum = table_compute_checksum(se, sizeof(struct smbios3_entry));
With the checksum filled I can use dmidecode in Linux based on the SMBIOS3 table:
ubuntu@ubuntu:~$ sudo dmidecode # dmidecode 3.5 # SMBIOS3 entry point at 0x27ef53000 Found SMBIOS entry point in EFI, reading table from /dev/mem. SMBIOS 3.7.0 present. # SMBIOS implementations newer than version 3.5.0 are not # fully supported by this version of dmidecode. Table at 0x27EF53020.
Handle 0x0000, DMI type 0, 24 bytes BIOS Information Vendor: U-Boot
OK, great, thank you for figuring this out. Do you think this might be a reasonable solution for -next ?
Regards, Simon
Hi Simon
On Tue, 21 Nov 2023 at 04:17, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Mon, 20 Nov 2023 at 19:11, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/15/23 04:45, 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.
Note that we cannot use efi_allocate_pages() since this function has nothing to do with EFI. There is no equivalent function to allocate memory below 4GB in U-Boot. One solution would be to create a separate malloc() pool, or just always put the malloc() pool below 4GB.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
Check the end of the table rather than the start.
include/smbios.h | 22 +++++++++++++++++++++- lib/smbios.c | 24 +++++++++++++++++++----- 2 files changed, 40 insertions(+), 6 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..92e98388084f 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;@@ -512,14 +516,24 @@ ulong write_smbios_table(ulong addr) * sandbox's DRAM buffer. */ table_addr = (ulong)map_sysmem(tables, 0);
if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {
if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) {You have to check the end of the the last SMBIOS structure not the start of the first SMBIOS structure against UINT_MAX.
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);This should be log_debug().
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;You must fill the checksum:
se->checksum = table_compute_checksum(se, sizeof(struct smbios3_entry));
With the checksum filled I can use dmidecode in Linux based on the SMBIOS3 table:
ubuntu@ubuntu:~$ sudo dmidecode # dmidecode 3.5 # SMBIOS3 entry point at 0x27ef53000 Found SMBIOS entry point in EFI, reading table from /dev/mem. SMBIOS 3.7.0 present. # SMBIOS implementations newer than version 3.5.0 are not # fully supported by this version of dmidecode. Table at 0x27EF53020.
Handle 0x0000, DMI type 0, 24 bytes BIOS Information Vendor: U-Boot
OK, great, thank you for figuring this out. Do you think this might be a reasonable solution for -next ?
What about [0] ? Do you expect this to land for 2024.01? I think we should one or the other, unless this is a hotfix for the upcoming release
[0] https://lore.kernel.org/u-boot/20231121025818.741258-1-sjg@chromium.org/
Thanks /Ilias
Regards, Simon
On Tue, Nov 21, 2023 at 10:40:39PM +0200, Ilias Apalodimas wrote:
Hi Simon
On Tue, 21 Nov 2023 at 04:17, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Mon, 20 Nov 2023 at 19:11, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/15/23 04:45, 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.
Note that we cannot use efi_allocate_pages() since this function has nothing to do with EFI. There is no equivalent function to allocate memory below 4GB in U-Boot. One solution would be to create a separate malloc() pool, or just always put the malloc() pool below 4GB.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
Check the end of the table rather than the start.
include/smbios.h | 22 +++++++++++++++++++++- lib/smbios.c | 24 +++++++++++++++++++----- 2 files changed, 40 insertions(+), 6 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..92e98388084f 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;@@ -512,14 +516,24 @@ ulong write_smbios_table(ulong addr) * sandbox's DRAM buffer. */ table_addr = (ulong)map_sysmem(tables, 0);
if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {
if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) {You have to check the end of the the last SMBIOS structure not the start of the first SMBIOS structure against UINT_MAX.
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);This should be log_debug().
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;You must fill the checksum:
se->checksum = table_compute_checksum(se, sizeof(struct smbios3_entry));
With the checksum filled I can use dmidecode in Linux based on the SMBIOS3 table:
ubuntu@ubuntu:~$ sudo dmidecode # dmidecode 3.5 # SMBIOS3 entry point at 0x27ef53000 Found SMBIOS entry point in EFI, reading table from /dev/mem. SMBIOS 3.7.0 present. # SMBIOS implementations newer than version 3.5.0 are not # fully supported by this version of dmidecode. Table at 0x27EF53020.
Handle 0x0000, DMI type 0, 24 bytes BIOS Information Vendor: U-Boot
OK, great, thank you for figuring this out. Do you think this might be a reasonable solution for -next ?
What about [0] ? Do you expect this to land for 2024.01? I think we should one or the other, unless this is a hotfix for the upcoming release
[0] https://lore.kernel.org/u-boot/20231121025818.741258-1-sjg@chromium.org/
I would like one of the two smaller changes to be agreed on for v2024.01 and we look at the bigger problems for v2024.04 (and heck, that can/should start now) around SMBIOS and memory allocation / reservation.
Hi,
On Tue, 21 Nov 2023 at 13:49, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 21, 2023 at 10:40:39PM +0200, Ilias Apalodimas wrote:
Hi Simon
On Tue, 21 Nov 2023 at 04:17, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Mon, 20 Nov 2023 at 19:11, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/15/23 04:45, 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.
Note that we cannot use efi_allocate_pages() since this function has nothing to do with EFI. There is no equivalent function to allocate memory below 4GB in U-Boot. One solution would be to create a separate malloc() pool, or just always put the malloc() pool below 4GB.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
Check the end of the table rather than the start.
include/smbios.h | 22 +++++++++++++++++++++- lib/smbios.c | 24 +++++++++++++++++++----- 2 files changed, 40 insertions(+), 6 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..92e98388084f 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;@@ -512,14 +516,24 @@ ulong write_smbios_table(ulong addr) * sandbox's DRAM buffer. */ table_addr = (ulong)map_sysmem(tables, 0);
if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {
if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) {You have to check the end of the the last SMBIOS structure not the start of the first SMBIOS structure against UINT_MAX.
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);This should be log_debug().
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;You must fill the checksum:
se->checksum = table_compute_checksum(se, sizeof(struct smbios3_entry));
With the checksum filled I can use dmidecode in Linux based on the SMBIOS3 table:
ubuntu@ubuntu:~$ sudo dmidecode # dmidecode 3.5 # SMBIOS3 entry point at 0x27ef53000 Found SMBIOS entry point in EFI, reading table from /dev/mem. SMBIOS 3.7.0 present. # SMBIOS implementations newer than version 3.5.0 are not # fully supported by this version of dmidecode. Table at 0x27EF53020.
Handle 0x0000, DMI type 0, 24 bytes BIOS Information Vendor: U-Boot
OK, great, thank you for figuring this out. Do you think this might be a reasonable solution for -next ?
What about [0] ? Do you expect this to land for 2024.01? I think we should one or the other, unless this is a hotfix for the upcoming release
[0] https://lore.kernel.org/u-boot/20231121025818.741258-1-sjg@chromium.org/
I would like one of the two smaller changes to be agreed on for v2024.01 and we look at the bigger problems for v2024.04 (and heck, that can/should start now) around SMBIOS and memory allocation / reservation.
Is this still pending?
From my side:
- we should not use EFI memory allocation to allocate an SMBIOS table - we should create the table on startup, like other tables
I believe that [0] is a reasonable fix, given the circumstances.
For -next perhaps we should go back to the SMBIOS3 series? I believe someone found a bug in it which is why it didn't work properly. We can still use SMBIOS2 when <4GB, in case there is a compatibility problem.
Regards, Simon
On Sat, Dec 02, 2023 at 11:27:15AM -0700, Simon Glass wrote:
Hi,
On Tue, 21 Nov 2023 at 13:49, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 21, 2023 at 10:40:39PM +0200, Ilias Apalodimas wrote:
Hi Simon
On Tue, 21 Nov 2023 at 04:17, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Mon, 20 Nov 2023 at 19:11, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/15/23 04:45, 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.
Note that we cannot use efi_allocate_pages() since this function has nothing to do with EFI. There is no equivalent function to allocate memory below 4GB in U-Boot. One solution would be to create a separate malloc() pool, or just always put the malloc() pool below 4GB.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
Check the end of the table rather than the start.
include/smbios.h | 22 +++++++++++++++++++++- lib/smbios.c | 24 +++++++++++++++++++----- 2 files changed, 40 insertions(+), 6 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..92e98388084f 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;@@ -512,14 +516,24 @@ ulong write_smbios_table(ulong addr) * sandbox's DRAM buffer. */ table_addr = (ulong)map_sysmem(tables, 0);
if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {
if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) {You have to check the end of the the last SMBIOS structure not the start of the first SMBIOS structure against UINT_MAX.
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);This should be log_debug().
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;You must fill the checksum:
se->checksum = table_compute_checksum(se, sizeof(struct smbios3_entry));
With the checksum filled I can use dmidecode in Linux based on the SMBIOS3 table:
ubuntu@ubuntu:~$ sudo dmidecode # dmidecode 3.5 # SMBIOS3 entry point at 0x27ef53000 Found SMBIOS entry point in EFI, reading table from /dev/mem. SMBIOS 3.7.0 present. # SMBIOS implementations newer than version 3.5.0 are not # fully supported by this version of dmidecode. Table at 0x27EF53020.
Handle 0x0000, DMI type 0, 24 bytes BIOS Information Vendor: U-Boot
OK, great, thank you for figuring this out. Do you think this might be a reasonable solution for -next ?
What about [0] ? Do you expect this to land for 2024.01? I think we should one or the other, unless this is a hotfix for the upcoming release
[0] https://lore.kernel.org/u-boot/20231121025818.741258-1-sjg@chromium.org/
I would like one of the two smaller changes to be agreed on for v2024.01 and we look at the bigger problems for v2024.04 (and heck, that can/should start now) around SMBIOS and memory allocation / reservation.
Is this still pending?
From my side:
- we should not use EFI memory allocation to allocate an SMBIOS table
- we should create the table on startup, like other tables
I believe that [0] is a reasonable fix, given the circumstances.
For -next perhaps we should go back to the SMBIOS3 series? I believe someone found a bug in it which is why it didn't work properly. We can still use SMBIOS2 when <4GB, in case there is a compatibility problem.
Yes, this is still pending and no, you and Heinrich have not yet agreed on which of your patches is an OK compromise for this release and then revisit all of the bigger issues that this has uncovered for follow-up releases.
Hi Tom,
On Sat, 2 Dec 2023 at 12:03, Tom Rini trini@konsulko.com wrote:
On Sat, Dec 02, 2023 at 11:27:15AM -0700, Simon Glass wrote:
Hi,
On Tue, 21 Nov 2023 at 13:49, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 21, 2023 at 10:40:39PM +0200, Ilias Apalodimas wrote:
Hi Simon
On Tue, 21 Nov 2023 at 04:17, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Mon, 20 Nov 2023 at 19:11, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/15/23 04:45, 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. > > Note that we cannot use efi_allocate_pages() since this function has > nothing to do with EFI. There is no equivalent function to allocate > memory below 4GB in U-Boot. One solution would be to create a separate > malloc() pool, or just always put the malloc() pool below 4GB. > > Signed-off-by: Simon Glass sjg@chromium.org > --- > > Changes in v2: > - Check the end of the table rather than the start. > > include/smbios.h | 22 +++++++++++++++++++++- > lib/smbios.c | 24 +++++++++++++++++++----- > 2 files changed, 40 insertions(+), 6 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..92e98388084f 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; > > @@ -512,14 +516,24 @@ ulong write_smbios_table(ulong addr) > * sandbox's DRAM buffer. > */ > table_addr = (ulong)map_sysmem(tables, 0); > - if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) { > + if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) {
You have to check the end of the the last SMBIOS structure not the start of the first SMBIOS structure against UINT_MAX.
> + 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);
This should be log_debug().
> + 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;
You must fill the checksum:
se->checksum = table_compute_checksum(se, sizeof(struct smbios3_entry));
With the checksum filled I can use dmidecode in Linux based on the SMBIOS3 table:
ubuntu@ubuntu:~$ sudo dmidecode # dmidecode 3.5 # SMBIOS3 entry point at 0x27ef53000 Found SMBIOS entry point in EFI, reading table from /dev/mem. SMBIOS 3.7.0 present. # SMBIOS implementations newer than version 3.5.0 are not # fully supported by this version of dmidecode. Table at 0x27EF53020.
Handle 0x0000, DMI type 0, 24 bytes BIOS Information Vendor: U-Boot
OK, great, thank you for figuring this out. Do you think this might be a reasonable solution for -next ?
What about [0] ? Do you expect this to land for 2024.01? I think we should one or the other, unless this is a hotfix for the upcoming release
[0] https://lore.kernel.org/u-boot/20231121025818.741258-1-sjg@chromium.org/
I would like one of the two smaller changes to be agreed on for v2024.01 and we look at the bigger problems for v2024.04 (and heck, that can/should start now) around SMBIOS and memory allocation / reservation.
Is this still pending?
From my side:
- we should not use EFI memory allocation to allocate an SMBIOS table
- we should create the table on startup, like other tables
I believe that [0] is a reasonable fix, given the circumstances.
For -next perhaps we should go back to the SMBIOS3 series? I believe someone found a bug in it which is why it didn't work properly. We can still use SMBIOS2 when <4GB, in case there is a compatibility problem.
Yes, this is still pending and no, you and Heinrich have not yet agreed on which of your patches is an OK compromise for this release and then revisit all of the bigger issues that this has uncovered for follow-up releases.
OK...before I went away I said that I was happy with whatever for this release.
It is for after that that I am concerned about the solution.
Regards, Simon
On 10/15/23 04:45, 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.
Note that we cannot use efi_allocate_pages() since this function has nothing to do with EFI. There is no equivalent function to allocate memory below 4GB in U-Boot. One solution would be to create a separate malloc() pool, or just always put the malloc() pool below 4GB.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
Check the end of the table rather than the start.
include/smbios.h | 22 +++++++++++++++++++++- lib/smbios.c | 24 +++++++++++++++++++----- 2 files changed, 40 insertions(+), 6 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;
This empty line is superfluous.
- u8 minor_ver;
- u8 docrev;
'doc_rev' would be more consistent with the other fields.
- u8 entry_point_rev;
- u8 reserved;
- u32 max_struct_size;
This empty line is superfluous.
Please, consider copying the comments from my patch
[PATCH v2,1/2] smbios: SMBIOS 3.0 (64-bit) Entry Point structure https://patchwork.ozlabs.org/project/uboot/patch/20231223010334.248291-2-xyp...
Otherwise looks good to me.
Heinrich
- 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..92e98388084f 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;
@@ -512,14 +516,24 @@ ulong write_smbios_table(ulong addr) * sandbox's DRAM buffer. */ table_addr = (ulong)map_sysmem(tables, 0);
- if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {
- if (sizeof(table_addr) > sizeof(u32) && addr >= (ulong)UINT_MAX) {
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;
Hi Heinrich,
On Tue, Dec 26, 2023 at 11:01 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/15/23 04:45, 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.
Note that we cannot use efi_allocate_pages() since this function has nothing to do with EFI. There is no equivalent function to allocate memory below 4GB in U-Boot. One solution would be to create a separate malloc() pool, or just always put the malloc() pool below 4GB.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
Check the end of the table rather than the start.
include/smbios.h | 22 +++++++++++++++++++++- lib/smbios.c | 24 +++++++++++++++++++----- 2 files changed, 40 insertions(+), 6 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;This empty line is superfluous.
u8 minor_ver;u8 docrev;'doc_rev' would be more consistent with the other fields.
u8 entry_point_rev;u8 reserved;u32 max_struct_size;This empty line is superfluous.
Please, consider copying the comments from my patch
[PATCH v2,1/2] smbios: SMBIOS 3.0 (64-bit) Entry Point structure https://patchwork.ozlabs.org/project/uboot/patch/20231223010334.248291-2-xyp...
Otherwise looks good to me.
Thanks, I added in your patch and resent the series.
I tested it with dmicode and it seems to work OK.
Regards, Simon
This should access arch-specific properties. Fix it and update the existing usage.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Add a new patch to correct gd_smbios_start()
include/asm-generic/global_data.h | 2 +- lib/efi_loader/efi_smbios.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index e8c6412e3f8d..c1f7818817f2 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -553,7 +553,7 @@ static_assert(sizeof(struct global_data) == GD_SIZE); #endif
#ifdef CONFIG_SMBIOS -#define gd_smbios_start() gd->smbios_start +#define gd_smbios_start() gd->arch.smbios_start #define gd_set_smbios_start(addr) gd->arch.smbios_start = addr #else #define gd_smbios_start() 0UL diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index 48446f654d9b..4ae4e4a7c236 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -29,7 +29,7 @@ efi_status_t efi_smbios_register(void) ulong addr; efi_status_t ret;
- addr = gd->arch.smbios_start; + addr = gd_smbios_start(); if (!addr) { log_err("No SMBIOS tables to install\n"); return EFI_NOT_FOUND;
On 10/15/23 04:45, Simon Glass wrote:
This should access arch-specific properties. Fix it and update the existing usage.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Changes in v2:
Add a new patch to correct gd_smbios_start()
include/asm-generic/global_data.h | 2 +- lib/efi_loader/efi_smbios.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index e8c6412e3f8d..c1f7818817f2 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -553,7 +553,7 @@ static_assert(sizeof(struct global_data) == GD_SIZE); #endif
#ifdef CONFIG_SMBIOS -#define gd_smbios_start() gd->smbios_start +#define gd_smbios_start() gd->arch.smbios_start #define gd_set_smbios_start(addr) gd->arch.smbios_start = addr #else #define gd_smbios_start() 0UL diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index 48446f654d9b..4ae4e4a7c236 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -29,7 +29,7 @@ efi_status_t efi_smbios_register(void) ulong addr; efi_status_t ret;
- addr = gd->arch.smbios_start;
- addr = gd_smbios_start(); if (!addr) { log_err("No SMBIOS tables to install\n"); return EFI_NOT_FOUND;
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.
Call unmap_system() to balance to the use of map_sysmem()
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Add a note about why unmap_system() is called
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 4ae4e4a7c236..80f38d78d877 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_smbios_start(); 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, 0); + guid = !memcmp(buf, "_SM_", 4) ? &smbios_guid : &smbios3_guid; + ret = efi_install_configuration_table(guid, buf); + unmap_sysmem(buf); + + return ret; }
static int install_smbios_table(void)
On 10/15/23 04:45, 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.
Call unmap_system() to balance to the use of map_sysmem()
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Changes in v2:
Add a note about why unmap_system() is called
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 4ae4e4a7c236..80f38d78d877 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_smbios_start(); 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, 0);
guid = !memcmp(buf, "_SM_", 4) ? &smbios_guid : &smbios3_guid;
ret = efi_install_configuration_table(guid, buf);
unmap_sysmem(buf);
return ret; }
static int install_smbios_table(void)
All callers handle this alignment, so drop the unnecessary code. This simplifies things a little.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
include/smbios.h | 5 +---- lib/smbios.c | 2 -- 2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/include/smbios.h b/include/smbios.h index ddabb558299e..31d997287588 100644 --- a/include/smbios.h +++ b/include/smbios.h @@ -248,12 +248,9 @@ static inline void fill_smbios_header(void *table, int type, * * This writes SMBIOS table at a given address. * - * @addr: start address to write SMBIOS table. If this is not - * 16-byte-aligned then it will be aligned before the table is - * written. + * @addr: start address to write SMBIOS table (must be 16-byte-aligned) * Return: end address of SMBIOS table (and start address for next entry) * or NULL in case of an error - * */ ulong write_smbios_table(ulong addr);
diff --git a/lib/smbios.c b/lib/smbios.c index 92e98388084f..5c9f108496d6 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -483,8 +483,6 @@ ulong write_smbios_table(ulong addr) ctx.dev = NULL; }
- /* 16 byte align the table address */ - addr = ALIGN(addr, 16); start_addr = addr;
/*
On 10/15/23 04:45, Simon Glass wrote:
All callers handle this alignment, so drop the unnecessary code. This simplifies things a little.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed: Heinrich Schuchardt xypron.glpk@gmx.de
(no changes since v1)
include/smbios.h | 5 +---- lib/smbios.c | 2 -- 2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/include/smbios.h b/include/smbios.h index ddabb558299e..31d997287588 100644 --- a/include/smbios.h +++ b/include/smbios.h @@ -248,12 +248,9 @@ static inline void fill_smbios_header(void *table, int type,
- This writes SMBIOS table at a given address.
- @addr: start address to write SMBIOS table. If this is not
16-byte-aligned then it will be aligned before the table is
written.
- @addr: start address to write SMBIOS table (must be 16-byte-aligned)
- Return: end address of SMBIOS table (and start address for next entry)
or NULL in case of an error
*/ ulong write_smbios_table(ulong addr);
diff --git a/lib/smbios.c b/lib/smbios.c index 92e98388084f..5c9f108496d6 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -483,8 +483,6 @@ ulong write_smbios_table(ulong addr) ctx.dev = NULL; }
/* 16 byte align the table address */
addr = ALIGN(addr, 16); start_addr = addr;
/*
On Tue, 26 Dec 2023 at 14:46, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 10/15/23 04:45, Simon Glass wrote:
All callers handle this alignment, so drop the unnecessary code. This simplifies things a little.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed: Heinrich Schuchardt xypron.glpk@gmx.de
(no changes since v1)
include/smbios.h | 5 +---- lib/smbios.c | 2 -- 2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/include/smbios.h b/include/smbios.h index ddabb558299e..31d997287588 100644 --- a/include/smbios.h +++ b/include/smbios.h @@ -248,12 +248,9 @@ static inline void fill_smbios_header(void *table, int type,
- This writes SMBIOS table at a given address.
- @addr: start address to write SMBIOS table. If this is not
16-byte-aligned then it will be aligned before the table is
written.
- @addr: start address to write SMBIOS table (must be 16-byte-aligned)
- Return: end address of SMBIOS table (and start address for next entry)
or NULL in case of an error
*/ ulong write_smbios_table(ulong addr);
diff --git a/lib/smbios.c b/lib/smbios.c index 92e98388084f..5c9f108496d6 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -483,8 +483,6 @@ ulong write_smbios_table(ulong addr) ctx.dev = NULL; }
/* 16 byte align the table address */addr = ALIGN(addr, 16);
I don't think this is fine. Failing to do so would break the SMBIOS tables. It's fine to require this but in that case, we should have add a check for the 16b alignment
Thanks /Ilias
start_addr = addr; /*
participants (4)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Simon Glass -
Tom Rini