[PATCH v2 3/6] smbios: Use SMBIOS 3.0 to support an address above 4GB
Simon Glass
sjg at chromium.org
Sat Dec 2 19:27:15 CET 2023
Hi,
On Tue, 21 Nov 2023 at 13:49, Tom Rini <trini at 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 at chromium.org> wrote:
> > >
> > > Hi Heinrich,
> > >
> > > On Mon, 20 Nov 2023 at 19:11, Heinrich Schuchardt <xypron.glpk at 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 at 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 at 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
More information about the U-Boot
mailing list