[PATCH v2 3/6] smbios: Use SMBIOS 3.0 to support an address above 4GB
Tom Rini
trini at konsulko.com
Sat Dec 2 20:02:53 CET 2023
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 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.
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.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231202/9121bc89/attachment.sig>
More information about the U-Boot
mailing list