| Message ID | 20231231152555.464874-5-sjg@chromium.org |
|---|---|
| State | Accepted |
| Commit | 70924294f375c351339f029b9615b52608e04cf4 |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | smbios: Deal with tables beyond 4GB | expand |
On 12/31/23 16:25, 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. > > - Use log_debug() for warning > - Rebase on Heinrich's smbios.h patch > - Set the checksum for SMBIOS3 > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > (no changes since v4) > > Changes in v4: > - Check the start of the table rather than the end > > Changes in v2: > - Check the end of the table rather than the start. > > include/smbios.h | 6 +++++- > lib/smbios.c | 30 +++++++++++++++++++++++++----- > 2 files changed, 30 insertions(+), 6 deletions(-) > > diff --git a/include/smbios.h b/include/smbios.h > index e601283d293..77be58887a2 100644 > --- a/include/smbios.h > +++ b/include/smbios.h > @@ -12,7 +12,7 @@ > > /* 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 */ > @@ -80,6 +80,10 @@ struct __packed smbios3_entry { > 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 eea72670bd9..7f79d969c92 100644 > --- a/lib/smbios.c > +++ b/lib/smbios.c > @@ -567,7 +567,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; > > @@ -590,16 +594,32 @@ ulong write_smbios_table(ulong addr) > * 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. > + * > + * Check the address of the end of the tables. If it is above 4GB then > + * it is sensible to use SMBIOS3 even if the start of the table is below > + * 4GB (this case is very unlikely to happen in practice) > */ > 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) { All SMBIOS specifications since version 3.0.0, published 2015, allow using a SMBIOS3 anchor in all cases. SMBIOS_MAJOR_VER == 3. Our target is to keep the code size small. Why do you increase the code size here? 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; > + log_debug("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->doc_rev = 0; > + se->entry_point_rev = 1; > + se->max_struct_size = len; > + se->struct_table_address = table_addr; > + se->checksum = table_compute_checksum(se, > + sizeof(struct smbios3_entry)); > } else { > struct smbios_entry *se; >
On 12/31/23 17:11, Heinrich Schuchardt wrote: > On 12/31/23 16:25, 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. >> >> - Use log_debug() for warning >> - Rebase on Heinrich's smbios.h patch >> - Set the checksum for SMBIOS3 >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> (no changes since v4) >> >> Changes in v4: >> - Check the start of the table rather than the end >> >> Changes in v2: >> - Check the end of the table rather than the start. >> >> include/smbios.h | 6 +++++- >> lib/smbios.c | 30 +++++++++++++++++++++++++----- >> 2 files changed, 30 insertions(+), 6 deletions(-) >> >> diff --git a/include/smbios.h b/include/smbios.h >> index e601283d293..77be58887a2 100644 >> --- a/include/smbios.h >> +++ b/include/smbios.h >> @@ -12,7 +12,7 @@ >> >> /* 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 */ >> @@ -80,6 +80,10 @@ struct __packed smbios3_entry { >> 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 eea72670bd9..7f79d969c92 100644 >> --- a/lib/smbios.c >> +++ b/lib/smbios.c >> @@ -567,7 +567,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; >> >> @@ -590,16 +594,32 @@ ulong write_smbios_table(ulong addr) >> * 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. >> + * >> + * Check the address of the end of the tables. If it is above 4GB >> then >> + * it is sensible to use SMBIOS3 even if the start of the table >> is below >> + * 4GB (this case is very unlikely to happen in practice) >> */ >> 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) { > > > All SMBIOS specifications since version 3.0.0, published 2015, allow > using a SMBIOS3 anchor in all cases. SMBIOS_MAJOR_VER == 3. > > Our target is to keep the code size small. Why do you increase the code > size here? You drop SMBIOS2 in patch 8. Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > 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; >> + log_debug("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->doc_rev = 0; >> + se->entry_point_rev = 1; >> + se->max_struct_size = len; >> + se->struct_table_address = table_addr; >> + se->checksum = table_compute_checksum(se, >> + sizeof(struct smbios3_entry)); >> } else { >> struct smbios_entry *se; >> >
On 12/31/23 16:25, 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. > > - Use log_debug() for warning > - Rebase on Heinrich's smbios.h patch > - Set the checksum for SMBIOS3 > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > (no changes since v4) > > Changes in v4: > - Check the start of the table rather than the end > > Changes in v2: > - Check the end of the table rather than the start. > > include/smbios.h | 6 +++++- > lib/smbios.c | 30 +++++++++++++++++++++++++----- > 2 files changed, 30 insertions(+), 6 deletions(-) > > diff --git a/include/smbios.h b/include/smbios.h > index e601283d293..77be58887a2 100644 > --- a/include/smbios.h > +++ b/include/smbios.h > @@ -12,7 +12,7 @@ > > /* 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 */ > @@ -80,6 +80,10 @@ struct __packed smbios3_entry { > u64 struct_table_address; > }; > > +/* These two structures should use the same amount of 16-byte-aligned space */ I cannot see from where you take such a requirement. By chance it is fulfilled by the current definitions. If this a leftover from debugging we should remove it. Best regards Heinrich > +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 eea72670bd9..7f79d969c92 100644 > --- a/lib/smbios.c > +++ b/lib/smbios.c > @@ -567,7 +567,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; > > @@ -590,16 +594,32 @@ ulong write_smbios_table(ulong addr) > * 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. > + * > + * Check the address of the end of the tables. If it is above 4GB then > + * it is sensible to use SMBIOS3 even if the start of the table is below > + * 4GB (this case is very unlikely to happen in practice) > */ > 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; > + log_debug("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->doc_rev = 0; > + se->entry_point_rev = 1; > + se->max_struct_size = len; > + se->struct_table_address = table_addr; > + se->checksum = table_compute_checksum(se, > + sizeof(struct smbios3_entry)); > } else { > struct smbios_entry *se; >
Hi Heinrich, On Mon, Jan 1, 2024 at 10:34 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 12/31/23 16:25, 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. > > > > - Use log_debug() for warning > > - Rebase on Heinrich's smbios.h patch > > - Set the checksum for SMBIOS3 > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > (no changes since v4) > > > > Changes in v4: > > - Check the start of the table rather than the end > > > > Changes in v2: > > - Check the end of the table rather than the start. > > > > include/smbios.h | 6 +++++- > > lib/smbios.c | 30 +++++++++++++++++++++++++----- > > 2 files changed, 30 insertions(+), 6 deletions(-) > > > > diff --git a/include/smbios.h b/include/smbios.h > > index e601283d293..77be58887a2 100644 > > --- a/include/smbios.h > > +++ b/include/smbios.h > > @@ -12,7 +12,7 @@ > > > > /* 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 */ > > @@ -80,6 +80,10 @@ struct __packed smbios3_entry { > > u64 struct_table_address; > > }; > > > > +/* These two structures should use the same amount of 16-byte-aligned space */ > > I cannot see from where you take such a requirement. > By chance it is fulfilled by the current definitions. > If this a leftover from debugging we should remove it. Oh, I just assumed it was a requirement, perhaps. Yes we can remove this, if you like. In fact perhaps we should remove all SMBIOS2 stuff as a follow-up? Regards, Simon
Hi Heinrich, On Mon, Jan 1, 2024 at 10:34 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 12/31/23 16:25, 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. > > > > - Use log_debug() for warning > > - Rebase on Heinrich's smbios.h patch > > - Set the checksum for SMBIOS3 > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > (no changes since v4) > > > > Changes in v4: > > - Check the start of the table rather than the end > > > > Changes in v2: > > - Check the end of the table rather than the start. > > > > include/smbios.h | 6 +++++- > > lib/smbios.c | 30 +++++++++++++++++++++++++----- > > 2 files changed, 30 insertions(+), 6 deletions(-) > > Applied to u-boot-dm/next, thanks!
diff --git a/include/smbios.h b/include/smbios.h index e601283d293..77be58887a2 100644 --- a/include/smbios.h +++ b/include/smbios.h @@ -12,7 +12,7 @@ /* 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 */ @@ -80,6 +80,10 @@ struct __packed smbios3_entry { 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 eea72670bd9..7f79d969c92 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -567,7 +567,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; @@ -590,16 +594,32 @@ ulong write_smbios_table(ulong addr) * 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. + * + * Check the address of the end of the tables. If it is above 4GB then + * it is sensible to use SMBIOS3 even if the start of the table is below + * 4GB (this case is very unlikely to happen in practice) */ 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; + log_debug("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->doc_rev = 0; + se->entry_point_rev = 1; + se->max_struct_size = len; + se->struct_table_address = table_addr; + se->checksum = table_compute_checksum(se, + sizeof(struct smbios3_entry)); } else { struct smbios_entry *se;
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. - Use log_debug() for warning - Rebase on Heinrich's smbios.h patch - Set the checksum for SMBIOS3 Signed-off-by: Simon Glass <sjg@chromium.org> --- (no changes since v4) Changes in v4: - Check the start of the table rather than the end Changes in v2: - Check the end of the table rather than the start. include/smbios.h | 6 +++++- lib/smbios.c | 30 +++++++++++++++++++++++++----- 2 files changed, 30 insertions(+), 6 deletions(-)