[U-Boot] [PATCH 1/2] smbios: add parsing API

Wed May 1 06:36:18 UTC 2019

> Am 29.04.2019 um 17:52 schrieb Christian Gmeiner <christian.gmeiner at gmail.com>:
> 
>> Am Mo., 29. Apr. 2019 um 17:35 Uhr schrieb Alexander Graf <agraf at csgraf.de>:
>> 
>> 
>>> On 29.04.19 16:29, Christian Gmeiner wrote:
>>> Hi
>>> 
>>>> Am Do., 25. Apr. 2019 um 12:09 Uhr schrieb Alexander Graf <agraf at csgraf.de>:
>>>> 
>>>>> On 16.04.19 13:38, Christian Gmeiner wrote:
>>>>> Add an very simple API to be able to access SMBIOS strings
>>>>> like vendor, model and bios version.
>>>>> 
>>>>> Signed-off-by: Christian Gmeiner <christian.gmeiner at gmail.com>
>>>>> ---
>>>>> include/smbios.h    |  37 ++++++++++
>>>>> lib/Kconfig         |   6 ++
>>>>> lib/Makefile        |   1 +
>>>>> lib/smbios-parser.c | 162 ++++++++++++++++++++++++++++++++++++++++++++
>>>>> 4 files changed, 206 insertions(+)
>>>>> create mode 100644 lib/smbios-parser.c
>>>>> 
>>>>> diff --git a/include/smbios.h b/include/smbios.h
>>>>> index 97b9ddce23..ad44e54245 100644
>>>>> --- a/include/smbios.h
>>>>> +++ b/include/smbios.h
>>>>> @@ -237,4 +237,41 @@ typedef int (*smbios_write_type)(ulong *addr, int handle);
>>>>>  */
>>>>> ulong write_smbios_table(ulong addr);
>>>>> 
>>>>> +
>>>>> +struct smbios_parser;
>>>>> +
>>>>> +/**
>>>>> + * smbios_parser_create() - Create smbios parser
>>>>> + *
>>>>> + * @addr:    start address to search for SMBIOS structure
>>>>> + * @len:     size of search area
>>>>> + * @return:  NULL or valid pointer to a struct smbios_parser
>>>>> + */
>>>>> +struct smbios_parser *smbios_parser_create(unsigned int addr, unsigned int len);
>>>>> +
>>>>> +/**
>>>>> + * smbios_parser_destroy() - Destroy smbios parser
>>>>> + *
>>>>> + * @p:       pointer to a struct smbios_parser
>>>>> + */
>>>>> +void smbios_parser_destroy(struct smbios_parser *p);
>>>>> +
>>>>> +/**
>>>>> + * smbios_header() - Search for SMBIOS header type
>>>>> + *
>>>>> + * @p:       pointer to a struct smbios_parser
>>>>> + * @type:    SMBIOS type
>>>>> + * @return:  NULL or a valid pointer to a struct smbios_header
>>>>> + */
>>>>> +struct smbios_header *smbios_header(const struct smbios_parser *p, int type);
>>>>> +
>>>>> +/**
>>>>> + * smbios_string() - Return string from SMBIOS
>>>>> + *
>>>>> + * @header:  pointer to struct smbios_header
>>>>> + * @index:   string index
>>>>> + * @return:  NULL or a valid const char pointer
>>>>> + */
>>>>> +const char *smbios_string(const struct smbios_header *header, int index);
>>>>> +
>>>>> #endif /* _SMBIOS_H_ */
>>>>> diff --git a/lib/Kconfig b/lib/Kconfig
>>>>> index 2120216593..59aafc63a7 100644
>>>>> --- a/lib/Kconfig
>>>>> +++ b/lib/Kconfig
>>>>> @@ -432,6 +432,12 @@ config SMBIOS_PRODUCT_NAME
>>>>> 
>>>>> endmenu
>>>>> 
>>>>> +config SMBIOS_PARSER
>>>>> +     bool "SMBIOS parser"
>>>>> +     default n
>>>>> +     help
>>>>> +       A simple parser for SMBIOS data.
>>>>> +
>>>>> source lib/efi/Kconfig
>>>>> source lib/efi_loader/Kconfig
>>>>> source lib/optee/Kconfig
>>>>> diff --git a/lib/Makefile b/lib/Makefile
>>>>> index 47829bfed5..f095bd420a 100644
>>>>> --- a/lib/Makefile
>>>>> +++ b/lib/Makefile
>>>>> @@ -34,6 +34,7 @@ obj-$(CONFIG_FIT) += fdtdec_common.o
>>>>> obj-$(CONFIG_TEST_FDTDEC) += fdtdec_test.o
>>>>> obj-$(CONFIG_GZIP_COMPRESSED) += gzip.o
>>>>> obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += smbios.o
>>>>> +obj-$(CONFIG_SMBIOS_PARSER) += smbios-parser.o
>>>>> obj-$(CONFIG_IMAGE_SPARSE) += image-sparse.o
>>>>> obj-y += ldiv.o
>>>>> obj-$(CONFIG_MD5) += md5.o
>>>>> diff --git a/lib/smbios-parser.c b/lib/smbios-parser.c
>>>>> new file mode 100644
>>>>> index 0000000000..b24fd7210c
>>>>> --- /dev/null
>>>>> +++ b/lib/smbios-parser.c
>>>>> @@ -0,0 +1,162 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>> +/*
>>>>> + * Copyright (C) 2019, Bachmann electronic GmbH
>>>>> + */
>>>>> +
>>>>> +#include <common.h>
>>>>> +#include <malloc.h>
>>>>> +#include <mapmem.h>
>>>>> +#include <smbios.h>
>>>>> +
>>>>> +struct smbios_parser {
>>>>> +     void *search_area;
>>>>> +     unsigned int search_area_len;
>>>>> +     struct smbios_entry *entry;
>>>>> +     void *data;
>>>>> +};
>>>>> +
>>>>> +static inline int verify_checksum(const struct smbios_entry *e)
>>>>> +{
>>>>> +     /*
>>>>> +      * Checksums for SMBIOS tables are calculated to have a value, so that
>>>>> +      * the sum over all bytes yields zero (using unsigned 8 bit arithmetic).
>>>>> +      */
>>>>> +     u8 *byte = (u8 *)e;
>>>>> +     u8 sum = 0;
>>>>> +
>>>>> +     for (int i = 0; i < e->length; i++)
>>>>> +             sum += byte[i];
>>>>> +
>>>>> +     return sum;
>>>>> +}
>>>>> +
>>>>> +static struct smbios_entry *find_smbios_entry(const struct smbios_parser *p)
>>>>> +{
>>>>> +     static const char sm_sig[] = "_SM_";
>>>>> +     u8 *buf = (u8 *)p->search_area;
>>>>> +     const u8 *end = (u8 *)p->search_area + p->search_area_len;
>>>>> +     struct smbios_entry *entry = NULL;
>>>>> +
>>>>> +     /* look for smbios entry */
>>>>> +     while (buf < end) {
>>>> 
>>>> Don't you have a way to pass the DMI pointer directly, maybe using cbfs?
>>>> Searching through the SMBIOS search area is so 1990s (and pretty much
>>>> only works on x86).
>>>> 
>>> Coreboot adds a smbios cbmem entry which can be used to get the base address.
>>> Will make use of that mechanism in the next version of the patch set.
>> 
>> 
>> Thanks, that will make this code a lot more robust :).
>> 
>> 
>>> 
>>>> I also don't think we really need all the smbios_parser logic. No need
>>>> to create special pseudo objects. IMHO you should either create an
>>>> actual UCLASS for SMBIOS with genuine callbacks and expose the table as
>>>> DM object or just make all search functions in this patch stateless. I'd
>>>> opt for the latter - it's how the SMBIOS creation works too.
>>>> 
>>> I am not sure what you mean with 'special pseudo objects' - struct
>>> smbios_parser?
>> 
>> 
>> Yes. The parser is stateful. I would ideally like it best if it wasn't :).
>> 
> 
> One of the reasons I need the smbios_parser is the map_sysmem(..)
> thing. Maybe I do not
> need it at all - that would make the whole thing much simpler.

I think you need it for sandbox, so it's good to have around. But I don't understand why map_sysmem() brings any statefulness requirements?

Alex