[prev in list] [next in list] [prev in thread] [next in thread] 

List:       u-boot
Subject:    Re: [PATCH v5 04/12] smbios: Use SMBIOS 3.0 to support an address above 4GB
From:       Heinrich Schuchardt <xypron.glpk () gmx ! de>
Date:       2023-12-31 16:14:26
Message-ID: 2d61b84d-9e1c-450b-8ccc-b8c166d4cda6 () gmx ! de
[Download RAW message or body]

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;
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]