[PATCH] internal dmi decoder

[Sean Nelson]

Here is a new version of the internal dmi decoding. To use the new 
decoder, the user needs to use "-p internal:dmi=new", to cross-check 
with the external dmidecode use "-p internal:dmi=check". Hopefully if 
this is comitted, it will make it easier for people to test the new 
internal decoder.

Signed-off-by: Sean Nelson

[Carl-Daniel Hailfinger]

Thanks for updating the patch.
Just a few short comments, this is not a full review.

On 09.08.2010 01:05, Sean Nelson wrote:
> Here is a new version of the internal dmi decoding. To use the new
> decoder, the user needs to use "-p internal:dmi=new", to cross-check
> with the external dmidecode use "-p internal:dmi=check". Hopefully if
> this is comitted, it will make it easier for people to test the new
> internal decoder.
>
> Signed-off-by: Sean Nelson
>
> diff --git a/dmi.c b/dmi.c
> index f18907f..b2a5daa 100644
> --- a/dmi.c
> +++ b/dmi.c
> @@ -1,76 +1,227 @@
>  /*
> This file is part of the flashrom project.
> Copyright (C) 2009,2010 Michael Karcher
> Copyright (C) 2010 Sean Nelson
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> #include <string.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <sys/mman.h>

Will this work on DOS and Windows?
> #include "flash.h"
>  #include "programmer.h"
>
> int has_dmi_support = 0;
>
> #if STANDALONE

Hm. STANDALONE should activate the builtin decoder. We might elect to
have a separate CONFIG_DMI which would replace !STANDALONE in this file.
> /* Stub to indicate missing DMI functionality.
> has_dmi_support is 0 by default, so nothing to do here.
> Because dmidecode is not available on all systems, the goal is to implement
> the DMI subset we need directly in this file.
>
> */
>  void dmi_init(void)
>  {
>  }
>
> int dmi_match(const char *pattern)
>  {
>    return 0;
>  }
>
> #else /* STANDALONE */
>
> -static const char *dmidecode_names[] = {
> "system-manufacturer",
> "system-product-name",
> "system-version",
> "baseboard-manufacturer",
> "baseboard-product-name",
> "baseboard-version",
>
> +/* the actual code, based on the dmidecode parsing logic */
> +static const struct string_keyword {

Kill string_keyword.
> const char *keyword;
> unsigned char type;
> unsigned char offset;
>
> +} flashrom_dmi_strings[] = {
> { "system-manufacturer", 1, 0x04 },
> { "system-product-name", 1, 0x05 },
> { "system-version", 1, 0x06 },
> { "baseboard-manufacturer", 2, 0x04 },
> { "baseboard-product-name", 2, 0x05 },
> { "baseboard-version", 2, 0x06 },
> { "chassis-type", 3, 0x05 },
>
> };
>
> #define DMI_COMMAND_LEN_MAX 260
>  static const char *dmidecode_command = "dmidecode";
> +static char *external_dmi[ARRAY_SIZE(flashrom_dmi_strings)];
> +#define DMI_MAX_ANSWER_LEN 4096
>
> -static char *dmistrings[ARRAY_SIZE(dmidecode_names)];
> +static char *dmistrings[ARRAY_SIZE(flashrom_dmi_strings)];
>
> -/* Strings longer than 4096 in DMI are just insane. */

Why did the comment disappear?
> -#define DMI_MAX_ANSWER_LEN 4096
> +#define WORD(x) (unsigned short)(*(const unsigned short *)(x))
> +#define DWORD(x) (unsigned int)(*(const unsigned int *)(x))

Ugh. Those #defines are really ugly. Can't you use mmio_read[bwl] or
something similar?
> +static int checksum(const unsigned char *buf, size_t len)

dmi_checksum?
> +{
> unsigned char sum = 0;
> size_t a;
> for (a = 0; a < len; a++)
> sum += buf[a];
> return (sum == 0);
>
> +}
>
> +static char *dmi_string(char *bp, unsigned char length, unsigned char s)

Better names for bp and s would be appreciated unless this is a straight
copy from the original dmidecode code (in which case we'd have to add
the author to the copyright header).
> +{
> size_t i, len;
> if (s == 0)
> return "Not Specified";
> bp += length;
> while (s > 1 && *bp) {
> bp += strlen(bp);
> bp++;

We happily walk off the cliff here (mapped area) if the DMI strings
contain garbage.
> s--;
> }
> if (!*bp)
> return "<BAD INDEX>";
> len = strlen(bp);
> for (i = 0; i < len; i++)
> if (bp[i] < 32 || bp[i] == 127)
> 	bp[i] = '.';

We write to bp?
> return bp;
>
> +}
>
> +static int dmi_chassis_type(unsigned char code)
> +{
> switch(code) {
> case 0x08: /* Portable */
> case 0x09: /* Laptop */
> case 0x0A: /* Notebook */
> case 0x0E: /* Sub Notebook */
> 	return 1;
> default:
> 	return 0;
> }
>
> +}
>
> +static void dmi_table(unsigned int base, unsigned short len, unsigned short num)
> +{
> unsigned char *data;
> unsigned char *dmi_table_mem;
> int key;
> int i=0, j=0;

spaces
> dmi_table_mem = physmap_try_ro("DMI Tables", base, len);

If physmap_try_ro fails this will explode spectacularly.
> data = dmi_table_mem;
> /* 4 is the length of an SMBIOS structure header */
> while (i < num && data+4 <= dmi_table_mem + len) {
> unsigned char *next;
> /*
>  * If a short entry is found (less than 4 bytes), not only it
>  * is invalid, but we cannot reliably locate the next entry.
>  * Better stop at this point, and let the user know his/her
>  * table is broken.
>  */
> if (data[1] < 4) {
> 	msg_perr("Invalid entry length (%u). DMI table is "
> 		"broken! Stop.\n\n", (unsigned int)data[1]);
> 	break;
> }
> /* Stop decoding after chassis segment */
> if (data[0] == 4)
> 	break;
> /* look for the next handle */
> next = data + data[1];
> while (next - dmi_table_mem + 1 < len && (next[0] != 0 || next[1] != 0))
> 	next++;
> next += 2;
> for (j = 0; j < ARRAY_SIZE(flashrom_dmi_strings); j++)
> {
> 	unsigned char offset = flashrom_dmi_strings[j].offset;
> 	unsigned char type = flashrom_dmi_strings[j].type;
> 	if (offset >= data[1])
> 		return;
> 	key = (type << 8) | offset;
> 	switch (key)
> 	{
> 	case 0x305: /* detect if laptop */
> 		if (dmi_chassis_type(data[offset])) {
> 			msg_pdbg("Laptop detected via DMI\n");

Can you print the chassis type in hex as well, even if it is not a
laptop? This will be helpful if a vendor screws up the DMI tables.
> 			is_laptop = 1;
> 		}
> 		break;
> 	default:
> 		if (type == data[0]) {
> 			dmistrings[j] = dmi_string((char*)data,	data[1], data[offset]);
> 			msg_pinfo("DMI string %s: \"%s\"\n",
> 				flashrom_dmi_strings[j].keyword, dmistrings[j]);
> 		}
> 	}
> }
> data = next;
> i++;
> }
> physunmap(dmi_table, len);
>
> +}
>
> +static int smbios_decode(unsigned char *buf)
> +{
> if (!checksum(buf, buf[0x05])
> || (memcmp(buf + 0x10, "_DMI_", 5) != 0) 
> || !checksum(buf + 0x10, 0x0F))
> 	return 0;
> dmi_table(DWORD(buf + 0x18), WORD(buf + 0x16), WORD(buf + 0x1C));
> return 1;
>
> +}
>
> +static int legacy_decode(unsigned char *buf)
> +{
> if (!checksum(buf, 0x0F))
> return 0;
> dmi_table(DWORD(buf + 0x08), WORD(buf + 0x06), WORD(buf + 0x0C));
> return 1;
>
> +}
>
> static char *get_dmi_string(const char *string_name)
>  {
>    FILE *dmidecode_pipe;
>    char *result;
>    char answerbuf[DMI_MAX_ANSWER_LEN];
>    char commandline[DMI_COMMAND_LEN_MAX + 40];
>
> snprintf(commandline, sizeof(commandline),
>    	 "%s -s %s", dmidecode_command, string_name);
>    dmidecode_pipe = popen(commandline, "r");
>    if (!dmidecode_pipe) {
>    	msg_perr("DMI pipe open error\n");
> @@ -109,45 +260,111 @@ static char *get_dmi_string(const char *string_name)
>        answerbuf[strlen(answerbuf) - 1] == '\n')
>    	answerbuf[strlen(answerbuf) - 1] = 0;
>    msg_pdbg("DMI string %s: "%s"\n", string_name, answerbuf);
>
> result = strdup(answerbuf);
>    if (!result)
>    	puts("WARNING: Out of memory - DMI support fails");
>
> return result;
>  }
>
> void dmi_init(void)
>  {
> int i;
> char *chassis_type;
> int found = 0;
> size_t fp;
> unsigned char *dmi_mem = NULL;
> char *arg = NULL;
> int use_new_dmi = 0;
> int i = 0;
> arg = extract_programmer_param("dmi");
> if (arg && !strcmp(arg,"new")) {
> use_new_dmi = 1;
> } else if (arg && !strcmp(arg, "check")) {
> use_new_dmi = 2;
> } else if (arg && !strlen(arg)) {
> msg_perr("Missing argument for dmi. Falling back to external dmi\n");
> } else if (arg) {
> msg_perr("Unknown argument for dmi: %s. Falling back to external dmi\n", arg);

You could also return here. If the user doesn't know what he wants, we
shouldn't continue.
> }
>
> free(arg);
>
> has_dmi_support = 1;
> for (i = 0; i < ARRAY_SIZE(dmidecode_names); i++) {
> dmistrings[i] = get_dmi_string(dmidecode_names[i]);
> if (!dmistrings[i]) {
> 	has_dmi_support = 0;
> 	return;
> if (use_new_dmi > 0) {
> dmi_mem = physmap_try_ro("DMI", 0xF0000, 0x10000);
> if (!dmi_mem)
> 	goto func_exit;
> for (fp = 0; fp <= 0xFFF0; fp += 16) {
> 	if (memcmp(dmi_mem + fp, "_SM_", 4) == 0 && fp <= 0xFFE0) {
> 		if (smbios_decode(dmi_mem+fp)) {
> 			found++;
> 			fp += 16;
> 		}
> 	}
> 	else if (memcmp(dmi_mem + fp, "_DMI_", 5) == 0)
> 		if (legacy_decode(dmi_mem + fp))
> 			found++;
> }
> }
> chassis_type = get_dmi_string("chassis-type");
> if (chassis_type && (!strcmp(chassis_type, "Notebook") ||
> 	     !strcmp(chassis_type, "Portable"))) {
> msg_pdbg("Laptop detected via DMI\n");
> is_laptop = 1;
> if (use_new_dmi == 2) {
> for (i = 0;  i < ARRAY_SIZE(flashrom_dmi_strings); i++)
> {
> 	external_dmi[i] = get_dmi_string(flashrom_dmi_strings[i].keyword);
> 	if (!external_dmi[i])
> 		goto func_exit;
> }
> for (i = 0;  i < ARRAY_SIZE(flashrom_dmi_strings)-1; i++)

spaces
> {
> 	if (strcmp(dmistrings[i], external_dmi[i])) 
> 		msg_perr("dmidecode vs internal-dmi differs: %s\n", flashrom_dmi_strings[i].keyword);
> 	else
> 		msg_pspew("Matching of dmidecode and internal-dmi succeeded!\n");

Hm. Do we cross-check the laptop info?
> }
> }
> free(chassis_type);
> if (use_new_dmi == 0) {
> char *chassis_type;
> has_dmi_support = 1;
> for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings); i++) {
> 	dmistrings[i] = get_dmi_string(flashrom_dmi_strings[i].keyword);
> 	if (!dmistrings[i]) {
> 		has_dmi_support = 0;
> 		return;
> 	}
> }
> chassis_type = get_dmi_string("chassis-type");
> if (chassis_type && (!strcmp(chassis_type, "Notebook") ||
> 		     !strcmp(chassis_type, "Portable"))) {
> 	msg_pdbg("Laptop detected via DMI\n");
> 	is_laptop = 1;
> }
> free(chassis_type);
> }
>
> +func_exit:
> if (!found)
> {
> msg_pinfo("No DMI table found.\n");
> has_dmi_support = 0;
> }
> physunmap(dmi_mem, 0x10000);
>
> }
>
> /**
> Does an substring/prefix/postfix/whole-string match.
> The pattern is matched as-is. The only metacharacters supported are '^'
> at the beginning and '$' at the end. So you can look for "^prefix",
> "suffix$", "substring" or "^complete string$".
> @param value The string to check.
> @param pattern The pattern.
> @return Nonzero if pattern matches.
>
> */
> @@ -185,21 +402,21 @@ static int dmi_compare(const char *value, const char *pattern)
>    if (anchored)
>    	return strncmp(value, pattern, patternlen) == 0;
>    else
>    	return strstr(value, pattern) != NULL;
>  }
>
> int dmi_match(const char *pattern)
>  {
>    int i;
>
> if (!has_dmi_support)
>    	return 0;
> for (i = 0; i < ARRAY_SIZE(dmidecode_names); i++)
>
> for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings); i++)
> if (dmi_compare(dmistrings[i], pattern))
> 	return 1;
>
> return 0;
>
> }
>
> #endif /* STANDALONE */

Regards,
Carl-Daniel

[Idwer Vollering]

2010/8/9 Carl-Daniel Hailfinger
> Thanks for updating the patch.
> Just a few short comments, this is not a full review.
>
> On 09.08.2010 01:05, Sean Nelson wrote:
> > Here is a new version of the internal dmi decoding. To use the new
> > decoder, the user needs to use "-p internal:dmi=new", to cross-check
> > with the external dmidecode use "-p internal:dmi=check". Hopefully if
> > this is comitted, it will make it easier for people to test the new
> > internal decoder.
> >
> > Signed-off-by: Sean Nelson
> >
> > diff --git a/dmi.c b/dmi.c
> > index f18907f..b2a5daa 100644
> > --- a/dmi.c
> > +++ b/dmi.c
> > @@ -1,76 +1,227 @@
> >  /*
> > This file is part of the flashrom project.
> > Copyright (C) 2009,2010 Michael Karcher
> > Copyright (C) 2010 Sean Nelson
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License as published by
> > the Free Software Foundation; either version 2 of the License, or
> > (at your option) any later version.
> > This program is distributed in the hope that it will be useful,
> > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > GNU General Public License for more details.
> > You should have received a copy of the GNU General Public License
> > along with this program; if not, write to the Free Software
> > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301
>
> USA
> > */
> >
> > #include <string.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >
> > +#include <unistd.h>
> > +#include <fcntl.h>
> > +#include <sys/mman.h>
>
> Will this work on DOS and Windows?

DOS:

flashrom v0.9.2-r1136 on FreeDOS 7 (i786), built with libpci 3.1.5, GCC
4.3.2, little endian
flashrom is free software, get the source code at http://www.flashrom.org

Calibrating delay loop... OS timer resolution is 170000 usecs, 1873M loops
per second, 10 myus = 0 us, 100 myus = 0 us, 1000 myus = 0 us, 10000 myus =
0 us, 680000 myus = 600000 us, OK.
Initializing internal programmer
No coreboot table found.
DMI string system-manufacturer: ""
DMI string system-product-name: ""
DMI string system-version: ""
DMI string baseboard-manufacturer: ""
DMI string baseboard-product-name: ""
DMI string baseboard-version: ""
DMI string chassis-type: ""
DMI string chassis-type: ""
No DMI table found.

Linux (Arch Linux):

flashrom v0.9.2-r1136 on Linux 2.6.34-ARCH (i686), built with libpci 3.1.7,
GCC 4.5.0 20100610 (prerelease), little
endi                                      an
flashrom is free software, get the source code at http://www.flashrom.org

Calibrating delay loop... OS timer resolution is 1 usecs, 2002M loops per
second, 10 myus = 10 us, 100 myus = 100 us,
10                                      00 myus = 1136 us, 10000 myus =
10275 us, 4 myus = 5 us, OK.
Initializing internal programmer
No coreboot table found.
sh: dmidecode: command not found
dmidecode execution unsucessfull - continuing without DMI info

FreeBSD:

flashrom v0.9.2-r1136 on FreeBSD 8.1-RELEASE (i386), built with libpci
3.1.7, GCC 4.2.1 20070719  [FreeBSD], little endian
flashrom is free software, get the source code at http://www.flashrom.org

Calibrating delay loop... OS timer resolution is 6 usecs, 1973M loops per
second, 10 myus = 11 us, 100 myus = 112 us, 1000 myus = 1005 us, 10000 myus
= 9742 us, 24 myus = 48 us, OK.
Initializing internal programmer
No coreboot table found.
DMI string system-manufacturer: "        "
DMI string system-product-name: "P4i65GV"
DMI string system-version: "1.00"
DMI string baseboard-manufacturer: "        "
DMI string baseboard-product-name: "P4i65GV"
DMI string baseboard-version: "1.0"
DMI string chassis-type: ""
DMI string chassis-type: ""
No DMI table found.
> > #include "flash.h"
> >  #include "programmer.h"
> >
> > int has_dmi_support = 0;
> >
> > #if STANDALONE
>
> Hm. STANDALONE should activate the builtin decoder. We might elect to
> have a separate CONFIG_DMI which would replace !STANDALONE in this file.
> > /* Stub to indicate missing DMI functionality.
> > has_dmi_support is 0 by default, so nothing to do here.
> > Because dmidecode is not available on all systems, the goal is to
>
> implement
> > the DMI subset we need directly in this file.
> >
> > */
> >  void dmi_init(void)
> >  {
> >  }
> >
> > int dmi_match(const char *pattern)
> >  {
> >       return 0;
> >  }
> >
> > #else /* STANDALONE */
> >
> > -static const char *dmidecode_names[] = {
> > "system-manufacturer",
> > "system-product-name",
> > "system-version",
> > "baseboard-manufacturer",
> > "baseboard-product-name",
> > "baseboard-version",
> >
> > +/* the actual code, based on the dmidecode parsing logic */
> > +static const struct string_keyword {
>
> Kill string_keyword.
> > const char *keyword;
> > unsigned char type;
> > unsigned char offset;
> >
> > +} flashrom_dmi_strings[] = {
> > { "system-manufacturer", 1, 0x04 },
> > { "system-product-name", 1, 0x05 },
> > { "system-version", 1, 0x06 },
> > { "baseboard-manufacturer", 2, 0x04 },
> > { "baseboard-product-name", 2, 0x05 },
> > { "baseboard-version", 2, 0x06 },
> > { "chassis-type", 3, 0x05 },
> >
> > };
> >
> > #define DMI_COMMAND_LEN_MAX 260
> >  static const char *dmidecode_command = "dmidecode";
> > +static char *external_dmi[ARRAY_SIZE(flashrom_dmi_strings)];
> > +#define DMI_MAX_ANSWER_LEN 4096
> >
> > -static char *dmistrings[ARRAY_SIZE(dmidecode_names)];
> > +static char *dmistrings[ARRAY_SIZE(flashrom_dmi_strings)];
> >
> > -/* Strings longer than 4096 in DMI are just insane. */
>
> Why did the comment disappear?
> > -#define DMI_MAX_ANSWER_LEN 4096
> > +#define WORD(x) (unsigned short)(*(const unsigned short *)(x))
> > +#define DWORD(x) (unsigned int)(*(const unsigned int *)(x))
>
> Ugh. Those #defines are really ugly. Can't you use mmio_read[bwl] or
> something similar?
> > +static int checksum(const unsigned char *buf, size_t len)
>
> dmi_checksum?
> > +{
> > unsigned char sum = 0;
> > size_t a;
> > for (a = 0; a < len; a++)
> >         sum += buf[a];
> > return (sum == 0);
> >
> > +}
> >
> > +static char *dmi_string(char *bp, unsigned char length, unsigned char s)
>
> Better names for bp and s would be appreciated unless this is a straight
> copy from the original dmidecode code (in which case we'd have to add
> the author to the copyright header).
> > +{
> > size_t i, len;
> > if (s == 0)
> >         return "Not Specified";
> > bp += length;
> > while (s > 1 && *bp) {
> >         bp += strlen(bp);
> >         bp++;
>
> We happily walk off the cliff here (mapped area) if the DMI strings
> contain garbage.
> >         s--;
> > }
> > if (!*bp)
> >         return "<BAD INDEX>";
> > len = strlen(bp);
> > for (i = 0; i < len; i++)
> >         if (bp[i] < 32 || bp[i] == 127)
> >                 bp[i] = '.';
>
> We write to bp?
> > return bp;
> >
> > +}
> >
> > +static int dmi_chassis_type(unsigned char code)
> > +{
> > switch(code) {
> >         case 0x08: /* Portable */
> >         case 0x09: /* Laptop */
> >         case 0x0A: /* Notebook */
> >         case 0x0E: /* Sub Notebook */
> >                 return 1;
> >         default:
> >                 return 0;
> > }
> >
> > +}
> >
> > +static void dmi_table(unsigned int base, unsigned short len, unsigned
>
> short num)
> > +{
> > unsigned char *data;
> > unsigned char *dmi_table_mem;
> > int key;
> > int i=0, j=0;
>
> spaces
> > dmi_table_mem = physmap_try_ro("DMI Tables", base, len);
>
> If physmap_try_ro fails this will explode spectacularly.
> > data = dmi_table_mem;
> > /* 4 is the length of an SMBIOS structure header */
> > while (i < num && data+4 <= dmi_table_mem + len) {
> >         unsigned char *next;
> >         /*
> >          * If a short entry is found (less than 4 bytes), not only
>
> it
> >          * is invalid, but we cannot reliably locate the next
>
> entry.
> >          * Better stop at this point, and let the user know his/her
> >          * table is broken.
> >          */
> >         if (data[1] < 4) {
> >                 msg_perr("Invalid entry length (%u). DMI table is "
> >                         "broken! Stop.\n\n", (unsigned
>
> int)data[1]);
> >                 break;
> >         }
> >         /* Stop decoding after chassis segment */
> >         if (data[0] == 4)
> >                 break;
> >         /* look for the next handle */
> >         next = data + data[1];
> >         while (next - dmi_table_mem + 1 < len && (next[0] != 0 ||
>
> next[1] != 0))
> >                 next++;
> >         next += 2;
> >         for (j = 0; j < ARRAY_SIZE(flashrom_dmi_strings); j++)
> >         {
> >                 unsigned char offset =
>
> flashrom_dmi_strings[j].offset;
> >                 unsigned char type = flashrom_dmi_strings[j].type;
> >                 if (offset >= data[1])
> >                         return;
> >                 key = (type << 8) | offset;
> >                 switch (key)
> >                 {
> >                 case 0x305: /* detect if laptop */
> >                         if (dmi_chassis_type(data[offset])) {
> >                                 msg_pdbg("Laptop detected via
>
> DMI\n");
> >
>
> Can you print the chassis type in hex as well, even if it is not a
> laptop? This will be helpful if a vendor screws up the DMI tables.
> >                                 is_laptop = 1;
> >                         }
> >                         break;
> >                 default:
> >                         if (type == data[0]) {
> >                                 dmistrings[j] =
>
> dmi_string((char*)data, data[1], data[offset]);
> >                                 msg_pinfo("DMI string %s:
>
> "%s"\n",
> >
>
> flashrom_dmi_strings[j].keyword, dmistrings[j]);
> >                         }
> >                 }
> >         }
> >         data = next;
> >         i++;
> > }
> > physunmap(dmi_table, len);
> >
> > +}
> >
> > +static int smbios_decode(unsigned char *buf)
> > +{
> > if (!checksum(buf, buf[0x05])
> >         || (memcmp(buf + 0x10, "_DMI_", 5) != 0)
> >         || !checksum(buf + 0x10, 0x0F))
> >                 return 0;
> > dmi_table(DWORD(buf + 0x18), WORD(buf + 0x16), WORD(buf + 0x1C));
> > return 1;
> >
> > +}
> >
> > +static int legacy_decode(unsigned char *buf)
> > +{
> > if (!checksum(buf, 0x0F))
> >         return 0;
> > dmi_table(DWORD(buf + 0x08), WORD(buf + 0x06), WORD(buf + 0x0C));
> > return 1;
> >
> > +}
> >
> > static char *get_dmi_string(const char *string_name)
> >  {
> >       FILE *dmidecode_pipe;
> >       char *result;
> >       char answerbuf[DMI_MAX_ANSWER_LEN];
> >       char commandline[DMI_COMMAND_LEN_MAX + 40];
> >   snprintf(commandline, sizeof(commandline),
> >            "%s -s %s", dmidecode_command, string_name);
> >   dmidecode_pipe = popen(commandline, "r");
> >   if (!dmidecode_pipe) {
> >           msg_perr("DMI pipe open error\n");
> >
> > @@ -109,45 +260,111 @@ static char *get_dmi_string(const char
>
> *string_name)
> >       answerbuf[strlen(answerbuf) - 1] == '\n')
> >           answerbuf[strlen(answerbuf) - 1] = 0;
> >   msg_pdbg("DMI string %s: \"%s\"\n", string_name, answerbuf);
> >
> >   result = strdup(answerbuf);
> >   if (!result)
> >           puts("WARNING: Out of memory - DMI support fails");
> >
> >   return result;
> >
> > }
> >
> > void dmi_init(void)
> >  {
> > int i;
> > char *chassis_type;
> > int found = 0;
> > size_t fp;
> > unsigned char *dmi_mem = NULL;
> > char *arg = NULL;
> > int use_new_dmi = 0;
> > int i = 0;
> > arg = extract_programmer_param("dmi");
> > if (arg && !strcmp(arg,"new")) {
> >         use_new_dmi = 1;
> > } else if (arg && !strcmp(arg, "check")) {
> >         use_new_dmi = 2;
> > } else if (arg && !strlen(arg)) {
> >         msg_perr("Missing argument for dmi. Falling back to
>
> external dmi\n");
> > } else if (arg) {
> >         msg_perr("Unknown argument for dmi: %s. Falling back to
>
> external dmi\n", arg);
> >
>
> You could also return here. If the user doesn't know what he wants, we
> shouldn't continue.
> > }
> > free(arg);
> >
> > has_dmi_support = 1;
> > for (i = 0; i < ARRAY_SIZE(dmidecode_names); i++) {
> >         dmistrings[i] = get_dmi_string(dmidecode_names[i]);
> >         if (!dmistrings[i]) {
> >                 has_dmi_support = 0;
> >                 return;
> > if (use_new_dmi > 0) {
> >         dmi_mem = physmap_try_ro("DMI", 0xF0000, 0x10000);
> >         if (!dmi_mem)
> >                 goto func_exit;
> >         for (fp = 0; fp <= 0xFFF0; fp += 16) {
> >                 if (memcmp(dmi_mem + fp, "_SM_", 4) == 0 && fp <=
>
> 0xFFE0) {
> >                         if (smbios_decode(dmi_mem+fp)) {
> >                                 found++;
> >                                 fp += 16;
> >                         }
> >                 }
> >                 else if (memcmp(dmi_mem + fp, "_DMI_", 5) == 0)
> >                         if (legacy_decode(dmi_mem + fp))
> >                                 found++;
> >         }
> > }
> >
> > chassis_type = get_dmi_string("chassis-type");
> > if (chassis_type && (!strcmp(chassis_type, "Notebook") ||
> >                      !strcmp(chassis_type, "Portable"))) {
> >         msg_pdbg("Laptop detected via DMI\n");
> >         is_laptop = 1;
> > if (use_new_dmi == 2) {
> >         for (i = 0;  i < ARRAY_SIZE(flashrom_dmi_strings); i++)
> >         {
> >                 external_dmi[i] =
>
> get_dmi_string(flashrom_dmi_strings[i].keyword);
> >                 if (!external_dmi[i])
> >                         goto func_exit;
> >         }
> >         for (i = 0;  i < ARRAY_SIZE(flashrom_dmi_strings)-1; i++)
>
> spaces
> >         {
> >                 if (strcmp(dmistrings[i], external_dmi[i]))
> >                         msg_perr("dmidecode vs internal-dmi
>
> differs: %s\n", flashrom_dmi_strings[i].keyword);
> >                 else
> >                         msg_pspew("Matching of dmidecode and
>
> internal-dmi succeeded!\n");
> >
>
> Hm. Do we cross-check the laptop info?
> >         }
> > }
> > free(chassis_type);
> > if (use_new_dmi == 0) {
> >         char *chassis_type;
> >         has_dmi_support = 1;
> >         for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings); i++) {
> >                 dmistrings[i] =
>
> get_dmi_string(flashrom_dmi_strings[i].keyword);
> >                 if (!dmistrings[i]) {
> >                         has_dmi_support = 0;
> >                         return;
> >                 }
> >         }
> >         chassis_type = get_dmi_string("chassis-type");
> >         if (chassis_type && (!strcmp(chassis_type, "Notebook") ||
> >                              !strcmp(chassis_type, "Portable"))) {
> >                 msg_pdbg("Laptop detected via DMI\n");
> >                 is_laptop = 1;
> >         }
> >         free(chassis_type);
> > }
> >
> > +func_exit:
> > if (!found)
> > {
> >         msg_pinfo("No DMI table found.\n");
> >         has_dmi_support = 0;
> > }
> > physunmap(dmi_mem, 0x10000);
> >
> > }
> >
> > /**
> > Does an substring/prefix/postfix/whole-string match.
> > The pattern is matched as-is. The only metacharacters supported are
>
> '^'
> > at the beginning and '$' at the end. So you can look for "^prefix",
> > "suffix$", "substring" or "^complete string$".
> > @param value The string to check.
> > @param pattern The pattern.
> > @return Nonzero if pattern matches.
> >
> > */
> > @@ -185,21 +402,21 @@ static int dmi_compare(const char *value, const
>
> char *pattern)
> >   if (anchored)
> >           return strncmp(value, pattern, patternlen) == 0;
> >   else
> >           return strstr(value, pattern) != NULL;
> >
> > }
> >
> > int dmi_match(const char *pattern)
> >  {
> >       int i;
> >   if (!has_dmi_support)
> >           return 0;
> >
> > for (i = 0; i < ARRAY_SIZE(dmidecode_names); i++)
> > for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings); i++)
> >         if (dmi_compare(dmistrings[i], pattern))
> >                 return 1;
> >
> > return 0;
> >
> > }
> >
> > #endif /* STANDALONE */
>
> Regards,
> Carl-Daniel
>
> flashrom mailing list
> [e-mail]
> http://www.flashrom.org/mailman/listinfo/flashrom

[Sean Nelson]

Updated patch 2010-09-19. Uses its own copy of the dmi_chassis_types 
list. Each dmi_init prints if its either internal DMI or external 
Dmidecode. Selectable using -D'STANDALONE=1'

Signed-off-by: Sean Nelson

[Carl-Daniel Hailfinger]

On 20.09.2010 02:28, Sean Nelson wrote:
> Updated patch 2010-09-19. Uses its own copy of the dmi_chassis_types
> list. Each dmi_init prints if its either internal DMI or external
> Dmidecode. Selectable using -D'STANDALONE=1'
>
> Signed-off-by: Sean Nelson

Looks like you forgot to attach the patch.

Regards,
Carl-Daniel

[Sean Nelson]

On 9/26/10 2:45 PM, Carl-Daniel Hailfinger wrote:
> On 20.09.2010 02:28, Sean Nelson wrote:
> > Updated patch 2010-09-19. Uses its own copy of the dmi_chassis_types
> > list. Each dmi_init prints if its either internal DMI or external
> > Dmidecode. Selectable using -D'STANDALONE=1'
> >
> > Signed-off-by: Sean Nelson
>
> Looks like you forgot to attach the patch.
>
> Regards,
> Carl-Daniel

Opps!

[Carl-Daniel Hailfinger]

On 27.09.2010 04:28, Sean Nelson wrote:
> On 9/26/10 2:45 PM, Carl-Daniel Hailfinger wrote:
> > On 20.09.2010 02:28, Sean Nelson wrote:
> > > Updated patch 2010-09-19. Uses its own copy of the dmi_chassis_types
> > > list. Each dmi_init prints if its either internal DMI or external
> > > Dmidecode. Selectable using -D'STANDALONE=1'
> > >
> > > Signed-off-by: Sean Nelson

I killed some duplicated code, but I'm sure there is more potential for
cleanup.

Sean, I remember you originally had some code which compared internal
and external DMI decoder results. Do you still have that code somewhere?

Add internal DMI decoder, default it to on.

Signed-off-by: Sean Nelson

Index: flashrom-snelson_internal_dmi/dmi.c
===================================================================
--- flashrom-snelson_internal_dmi/dmi.c	(Revision 1237)
+++ flashrom-snelson_internal_dmi/dmi.c	(Arbeitskopie)
@@ -25,33 +25,22 @@
 #include "flash.h"
 #include "programmer.h"

+#define STANDALONE 1
+
 int has_dmi_support = 0;

-#if STANDALONE
-
-/* Stub to indicate missing DMI functionality.
- * has_dmi_support is 0 by default, so nothing to do here.
- * Because dmidecode is not available on all systems, the goal is to implement
- * the DMI subset we need directly in this file.
- */
-void dmi_init(void)
-{
-}
-
-int dmi_match(const char *pattern)
-{
-	return 0;
-}
-
-#else /* STANDALONE */
-
-static const char *dmidecode_names[] = {
-	"system-manufacturer",
-	"system-product-name",
-	"system-version",
-	"baseboard-manufacturer",
-	"baseboard-product-name",
-	"baseboard-version",
+static const struct {
+	const char *keyword;
+	unsigned char type;
+	unsigned char offset;
+} flashrom_dmi_strings[] = {
+	{ "system-manufacturer", 1, 0x04 },
+	{ "system-product-name", 1, 0x05 },
+	{ "system-version", 1, 0x06 },
+	{ "baseboard-manufacturer", 2, 0x04 },
+	{ "baseboard-product-name", 2, 0x05 },
+	{ "baseboard-version", 2, 0x06 },
+	{ "chassis-type", 3, 0x05 },
 };

/* A full list of chassis types can be found in the System Management BIOS
@@ -74,11 +63,197 @@
    {0x0e, 1, "Sub Notebook"},
 };

+static char *dmistrings[ARRAY_SIZE(flashrom_dmi_strings)];
+
+#if STANDALONE
+static int dmi_checksum(const unsigned char *buf, size_t len)
+{
+	unsigned char sum = 0;
+	size_t a;
+
+	for (a = 0; a < len; a++)
+		sum += buf[a];
+	return (sum == 0);
+}
+
+static char *dmi_string(char *buffer, unsigned char length, unsigned char string_id)
+{
+	size_t i, len;
+
+	if (string_id == 0)
+		return "Not Specified";
+
+	buffer += length; /* skip to after the handle's data length byte */
+	/* Continue till we hit a null which denotes end of string in dmi
+	   or as long as we're not grabing the first string. The string 
+	   should be no longer than 64 bytes. We continue looping because
+	   we "jump" to the data string. */
+	for (; string_id > 1; string_id--) {
+		buffer += strlen(buffer); /* skip previous data strings */
+		buffer++; /* skip the data string length byte */
+	}
+
+	if (!*buffer) /* as long as the current byte we're on isn't null */
+		return "<BAD INDEX>";
+
+	len = strlen(buffer);
+	if (len > 64)
+		len = 64;
+
+	for (i = 0; i < len; i++) /* sometimes we need to fix junk bytes in the string */
+		if (buffer[i] < 32 || buffer[i] == 127)
+			buffer[i] = '.';
+
+	return buffer;
+}
+
+static int dmi_chassis_type(unsigned char code)
+{
+	int i;
+	for (i = 0; i < ARRAY_SIZE(dmi_chassis_types); i++) {
+		if (code == dmi_chassis_types[i].type) {
+			break;
+		}
+	}
+	msg_pdbg("DMI string chassis-type: "%s"\n", dmi_chassis_types[i].name );
+	if (dmi_chassis_types[i].is_laptop) {
+		msg_pdbg("Laptop detected via DMI\n");
+		is_laptop = 1;
+	}
+	return 0;
+}
+
+static void dmi_table(unsigned int base, unsigned short len, unsigned short num)
+{
+	unsigned char *data;
+	unsigned char *dmi_table_mem;
+	int i = 0, j = 0;
+
+	dmi_table_mem = physmap_try_ro("DMI Tables", base, len);
+	if (!dmi_table_mem) {
+		msg_perr("Unable to access DMI Tables\n");
+		return;
+	}
+
+	data = dmi_table_mem;
+
+	/* 4 is the length of an SMBIOS structure header */
+	while (i < num && data+4 <= dmi_table_mem + len) {
+		unsigned char *next;
+		/*
+		 * If a short entry is found (less than 4 bytes), not only it
+		 * is invalid, but we cannot reliably locate the next entry.
+		 * Better stop at this point, and let the user know his/her
+		 * table is broken.
+		 */
+		if (data[1] < 4) {
+			msg_perr("Invalid entry length (%u). DMI table is "
+				"broken! Stop.\n\n", (unsigned int)data[1]);
+			break;
+		}
+
+		/* Stop decoding after chassis segment */
+		if (data[0] == 4)
+			break;
+
+		/* look for the next handle */
+		next = data + data[1];
+		while (next - dmi_table_mem + 1 < len && (next[0] != 0 || next[1] != 0))
+			next++;
+		next += 2;
+
+		for (j = 0; j < ARRAY_SIZE(flashrom_dmi_strings); j++)
+		{
+			unsigned char offset = flashrom_dmi_strings[j].offset;
+			unsigned char type = flashrom_dmi_strings[j].type;
+
+			if (offset >= data[1])
+				return;
+
+
+			switch ((type << 8)|offset)
+			{
+			case 0x0305: /* detect if laptop */
+				if (type == data[0]) {
+					dmi_chassis_type(data[offset]);
+				}
+				break;
+			default:
+				if (type == data[0]) {
+					dmistrings[j] = dmi_string((char*)data, data[1], data[offset]);
+					msg_pdbg("DMI string %s: "%s"\n",
+						flashrom_dmi_strings[j].keyword, dmistrings[j]);
+				}
+			}
+		}
+		data = next;
+		i++;
+	}
+
+	physunmap(dmi_table, len);
+}
+
+static int smbios_decode(unsigned char *buf)
+{
+	if (!dmi_checksum(buf, buf[0x05]) 
+		|| (memcmp(buf + 0x10, "_DMI_", 5) != 0) 
+		|| !dmi_checksum(buf + 0x10, 0x0F))
+			return 0;
+
+	dmi_table(mmio_readl(buf + 0x18), mmio_readw(buf + 0x16), mmio_readw(buf + 0x1C));
+
+	return 1;
+}
+
+static int legacy_decode(unsigned char *buf)
+{
+	if (!dmi_checksum(buf, 0x0F))
+		return 0;
+
+	dmi_table(mmio_readl(buf + 0x08), mmio_readw(buf + 0x06), mmio_readw(buf + 0x0C));
+
+	return 1;
+}
+
+void dmi_init(void)
+{
+	int found = 0;
+	size_t fp;
+	unsigned char *dmi_mem = NULL;
+	has_dmi_support = 1;
+
+	msg_pdbg("Trying Internal DMI decoder.\n");
+	dmi_mem = physmap_try_ro("DMI", 0xF0000, 0x10000);
+	if (!dmi_mem)
+		goto func_exit;
+
+	for (fp = 0; fp <= 0xFFF0; fp += 16) {
+		if (memcmp(dmi_mem + fp, "_SM_", 4) == 0 && fp <= 0xFFE0) {
+			if (smbios_decode(dmi_mem+fp)) {
+				found++;
+				fp += 16;
+			}
+		}
+		else if (memcmp(dmi_mem + fp, "_DMI_", 5) == 0)
+			if (legacy_decode(dmi_mem + fp))
+				found++;
+	}
+
+func_exit:
+	if (!found)
+	{
+		msg_pinfo("No DMI table found.\n");
+		has_dmi_support = 0;
+	}
+
+	physunmap(dmi_mem, 0x10000);
+}
+
+#else /* STANDALONE */
+
 #define DMI_COMMAND_LEN_MAX 260
 static const char *dmidecode_command = "dmidecode";

-static char *dmistrings[ARRAY_SIZE(dmidecode_names)];
-
 /* Strings longer than 4096 in DMI are just insane. */
 #define DMI_MAX_ANSWER_LEN 4096

@@ -142,9 +317,10 @@
    int i;
    char *chassis_type;

+	msg_pdbg("Trying External DMI decoder.\n");
    has_dmi_support = 1;
-	for (i = 0; i < ARRAY_SIZE(dmidecode_names); i++) {
-		dmistrings[i] = get_dmi_string(dmidecode_names[i]);
+	for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings); i++) {
+		dmistrings[i] = get_dmi_string(flashrom_dmi_strings[i].keyword);
    	if (!dmistrings[i]) {
    		has_dmi_support = 0;
    		return;
@@ -165,6 +341,8 @@
    free(chassis_type);
 }

+#endif /* STANDALONE */
+
 /**
  * Does an substring/prefix/postfix/whole-string match.
  *
@@ -220,11 +398,9 @@
    if (!has_dmi_support)
    	return 0;

-	for (i = 0; i < ARRAY_SIZE(dmidecode_names); i++)
+	for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings); i++)
    	if (dmi_compare(dmistrings[i], pattern))
    		return 1;

return 0;
 }
-
-#endif /* STANDALONE */

[Michael Karcher]

Am Dienstag, den 30.11.2010, 15:50 +0100 schrieb Carl-Daniel Hailfinger:
> I killed some duplicated code, but I'm sure there is more potential
> for cleanup.
>
> Sean, I remember you originally had some code which compared internal
> and external DMI decoder results. Do you still have that code
> somewhere?
>
> Add internal DMI decoder, default it to on.
>
> Signed-off-by: Sean Nelson
> -static const char *dmidecode_names[] = {
>   "system-manufacturer",
>   "system-product-name",
>   "system-version",
>   "baseboard-manufacturer",
>   "baseboard-product-name",
>   "baseboard-version",
>
> +static const struct {
>   const char *keyword;
>   unsigned char type;
>   unsigned char offset;
>
> +} flashrom_dmi_strings[] = {
>   { "system-manufacturer", 1, 0x04 },
>   { "system-product-name", 1, 0x05 },
>   { "system-version", 1, 0x06 },
>   { "baseboard-manufacturer", 2, 0x04 },
>   { "baseboard-product-name", 2, 0x05 },
>   { "baseboard-version", 2, 0x06 },
>   { "chassis-type", 3, 0x05 },

This hunk adds an entry for "chassis-type" to the DMI strings. This
seems to have unintended consequences:
1) The chassis type string is (in dmistrings[6]) also a match candidate
for DMI board enable entries, and
2) The internal DMI decoder never initializes dmistrings[6], while the
external one does.
3) If dmistrings[6] is NULL (by using the internal decoder), dmi_compare
crashes on it.
> +static char *dmi_string(char *buffer, unsigned char length, unsigned char string_id)
> +{
> size_t i, len;
> if (string_id == 0)
> return "Not Specified";

OK.
> buffer += length; /* skip to after the handle's data length byte */

Comment misleading/wrong: You don't skip until after the length byte,
but you skip "length" bytes to get past the (formatted) data.
> /* Continue till we hit a null which denotes end of string in dmi
>   or as long as we're not grabing the first string. The string 
>   should be no longer than 64 bytes. We continue looping because
>   we "jump" to the data string. */

It would be nice if you really checked that you don't hit the end of the
string table, which is indicated by a zero-length string. Currently, if
there is 1 entry in the table and the third entry is requested, this
loops skips the first valid entry in the first iteration, skips the end
marker in the second iteration and then continues past-the-structure to
get the third entry.
> for (; string_id > 1; string_id--) {
> buffer += strlen(buffer); /* skip previous data strings */

Comment misleading: This line skips only one string, of course. The
plural is obtained by looping that line, so that comment ("data
strings") should be before the loop, or changed to "skip current data
string" or "skip next data string".
> buffer++; /* skip the data string length byte */

Comment misleading/wrong: You are not skippin a length byte, you are
skipping the terminating zero. I also don't see the purpose of not just
adding "strlen(buffer)+1" in the previous line, as I consider this
expression to be a standard idiom for "size including NUL byte".
> }
> if (!*buffer) /* as long as the current byte we're on isn't null */
> return "<BAD INDEX>";

If you move this into the loop (as first instruction) you fix the
overrun-the-end-marker problem.
> len = strlen(buffer);
> if (len > 64)
> len = 64;

I don't find the limit of 64 bytes in the specs. Why don't you use
DMI_MAX_ANSWER_LEN here? Of course you may reduce it to 64, but using
the symbolic constant seems more clean than some arbitrary limit (even
if mentioned in the function header comment).
> +static int dmi_chassis_type(unsigned char code)
> +{
> int i;
> for (i = 0; i < ARRAY_SIZE(dmi_chassis_types); i++) {
> if (code == dmi_chassis_types[i].type) {
> 	break;
> }
> }

Hmm, if you don't match in this loop, you end up with i ==
ARRAY_SIZE(dmi_chassis_types), and access out-of-bound elements in the
following lines. I don't think you want that.
> msg_pdbg("DMI string chassis-type: "%s"\n", dmi_chassis_types[i].name );
> if (dmi_chassis_types[i].is_laptop) {
> msg_pdbg("Laptop detected via DMI\n");
> is_laptop = 1;
> }
> return 0;
>
> +}
> +static void dmi_table(unsigned int base, unsigned short len, unsigned short num)
> +{
> unsigned char *data;
> unsigned char *dmi_table_mem;
> int i = 0, j = 0;
> dmi_table_mem = physmap_try_ro("DMI Tables", base, len);
> if (!dmi_table_mem) {
> msg_perr("Unable to access DMI Tables\n");
> return;
> }
> data = dmi_table_mem;
> /* 4 is the length of an SMBIOS structure header */
> while (i < num && data+4 <= dmi_table_mem + len) {
> unsigned char *next;
> /*
>  * If a short entry is found (less than 4 bytes), not only it
>  * is invalid, but we cannot reliably locate the next entry.
>  * Better stop at this point, and let the user know his/her
>  * table is broken.
>  */

indeed.
> if (data[1] < 4) {
> 	msg_perr("Invalid entry length (%u). DMI table is "
> 		"broken! Stop.\n\n", (unsigned int)data[1]);
> 	break;
> }
> /* Stop decoding after chassis segment */
> if (data[0] == 4)
> 	break;

I can find no statement that DMI entries *have* to be sorted in the DMI
spec. If it's not specified, it's dangerous to rely on that (even though
the basic BIOS entries usually are sorted). If I missed the
specification about order, please give a reference.
> /* look for the next handle */
> next = data + data[1];

better comment: Skip the string section / unformatted section or
something like that
> while (next - dmi_table_mem + 1 < len && (next[0] != 0 || next[1] != 0))
> 	next++;
> next += 2;

Abort if the limit dmi_table_mem has been hit!
> for (j = 0; j < ARRAY_SIZE(flashrom_dmi_strings); j++)
> {
> 	unsigned char offset = flashrom_dmi_strings[j].offset;
> 	unsigned char type = flashrom_dmi_strings[j].type;

check data[0] == type at this point, and continue if not.
> 	if (offset >= data[1])
> 		return;

Add error message about bad DMI structure. It also is important to check
the type before, because this aborts if an encountered entry doesn't
include the offset we look for, even if that entry is for something
completely different, which is legitimately shorter.
> 	switch ((type << 8)|offset)
> 	{
> 	case 0x0305: /* detect if laptop */
> 		if (type == data[0]) {
> 			dmi_chassis_type(data[offset]);
> 		}
> 		break;
> 	default:
> 		if (type == data[0]) {
> 			dmistrings[j] = dmi_string((char*)data, data[1], data[offset]);
> 			msg_pdbg("DMI string %s: \"%s\"\n",
> 				flashrom_dmi_strings[j].keyword, dmistrings[j]);
> 		}
> 	}

This is a single case switch merging two values into one switch value.
As I explained in the beginning, adding the chassis type to the
flashrom_dmi_strings array seems like a bad idea (the chassis type is no
string, this is one root of the problem), so I would suggest to move
handling of that entry out of this loop. You can just use
  if(data[0] == 3) dmi_chassis_type(data[5]);
As the type check has already been moved to the beginning of the loop
body, just the assignment and the the msg_pdbg of this loop stay.
> }
> data = next;
> i++;
> }
> physunmap(dmi_table, len);
>
> +}
> +static int smbios_decode(unsigned char *buf)
> +{
> if (!dmi_checksum(buf, buf[0x05])
> || (memcmp(buf + 0x10, "_DMI_", 5) != 0) 
> || !dmi_checksum(buf + 0x10, 0x0F))
> 	return 0;
> dmi_table(mmio_readl(buf + 0x18), mmio_readw(buf + 0x16), mmio_readw(buf + 0x1C));
> return 1;
>
> +}
>
> +static int legacy_decode(unsigned char *buf)
> +{
> if (!dmi_checksum(buf, 0x0F))
> return 0;
> dmi_table(mmio_readl(buf + 0x08), mmio_readw(buf + 0x06), mmio_readw(buf + 0x0C));
> return 1;
>
> +}

To me, it looks like we don't care about the extra data of the more
modern "_SM_" structure and the second 16 bytes are exactly the legacy
structure, so just searching for the legacy table should be enough.
> +void dmi_init(void)
> +{
> int found = 0;
> size_t fp;
> unsigned char *dmi_mem = NULL;
> has_dmi_support = 1;
> msg_pdbg("Trying Internal DMI decoder.\n");
> dmi_mem = physmap_try_ro("DMI", 0xF0000, 0x10000);
> if (!dmi_mem)
> goto func_exit;
> for (fp = 0; fp <= 0xFFF0; fp += 16) {
> if (memcmp(dmi_mem + fp, "_SM_", 4) == 0 && fp <= 0xFFE0) {
> 	if (smbios_decode(dmi_mem+fp)) {
> 		found++;
> 		fp += 16;
> 	}
> }
> else if (memcmp(dmi_mem + fp, "_DMI_", 5) == 0)
> 	if (legacy_decode(dmi_mem + fp))
> 		found++;
> }

Do we want to handle multiple tables? Otherwise, we can just skip to
unmapping as soon as the first table was found.
> msg_pdbg("Trying External DMI decoder.\n");

s/Trying/Using/

Regards,
  Michael Karcher

[PATCH] Add an internal DMI decoder

[Stefan Tauner]

Previously we had to rely on dmidecode to decode the DMI/SMBIOS table.
This patch integrates a DMI decoder into flashrom. The old behavior of calling
dmidecode can be brought back by setting DMI_INTERNAL to 0.

Significant portions of this patch were taken from dmidecode, hence add its
authors to the copyright notice (dmidecode is also GPL2+). We do a few things
differently though. First of all we do more bounds checking to prevent accessing
unmapped memory. We do not support disovery via EFI (yet), but memory
scanning only. We handle the chassis-type lock bit correctly.

The API to the rest of flashrom remains stable, but the output changes slightly.
To share as much code as possible (which actually is not much), i have added
dmi_fill methods that get called in dmi_init. They are reponsible to fill the
dmi_strings array and setting the is_laptop variable. After it is called, dmi_init
prints all dmi_strings. Previously the strings were printed in the order they were
discovered, followed by the chassis-type, which is now output earlier (in dmi_fill).

Signed-off-by: Sean Nelson
Signed-off-by: Stefan Tauner

---
relies on my physmap-automagic-alignment patch to work reliably.
the diff is a bit unreadable imo and i touch almost everything...
---
 dmi.c |  307 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 261 insertions(+), 46 deletions(-)

diff --git a/dmi.c b/dmi.c
index 4f36245..c09a292 100644
--- a/dmi.c
+++ b/dmi.c
@@ -1,7 +1,10 @@
 /*
  * This file is part of the flashrom project.
  *
+ * Copyright (C) 2000-2002 Alan Cox 
+ * Copyright (C) 2002-2010 Jean Delvare 
  * Copyright (C) 2009,2010 Michael Karcher
+ * Copyright (C) 2011 Stefan Tauner
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -25,33 +28,28 @@
 #include "flash.h"
 #include "programmer.h"

-int has_dmi_support = 0;
-
-#if STANDALONE
-
-/* Stub to indicate missing DMI functionality.
- * has_dmi_support is 0 by default, so nothing to do here.
- * Because dmidecode is not available on all systems, the goal is to implement
- * the DMI subset we need directly in this file.
- */
-void dmi_init(void)
-{
-}
+/* Select internal DMI decoder instead of relying on dmidecode */
+#define DMI_INTERNAL 1
+/* Enable SMBIOS decoding. Currently legacy DMI decoding is enough. */
+#define SM_SUPPORT 0

-int dmi_match(const char *pattern)
-{
-	return 0;
-}
+/* Strings longer than 4096 in DMI are just insane. */
+#define DMI_MAX_ANSWER_LEN 4096

-#else /* STANDALONE */
+int has_dmi_support = 0;

-static const char *dmidecode_names[] = {
-	"system-manufacturer",
-	"system-product-name",
-	"system-version",
-	"baseboard-manufacturer",
-	"baseboard-product-name",
-	"baseboard-version",
+static struct {
+	const char *const keyword;
+	const uint8_t type;
+	const uint8_t offset;
+	char *value;
+} dmi_strings[] = {
+	{ "system-manufacturer", 1, 0x04 },
+	{ "system-product-name", 1, 0x05 },
+	{ "system-version", 1, 0x06 },
+	{ "baseboard-manufacturer", 2, 0x04 },
+	{ "baseboard-product-name", 2, 0x05 },
+	{ "baseboard-version", 2, 0x06 },
 };

/* This list is used to identify supposed laptops. The is_laptop field has the
@@ -65,9 +63,9 @@ static const char *dmidecode_names[] = {
  * The types below are the most common ones.
  */
 static const struct {
-	unsigned char type;
-	unsigned char is_laptop;
-	const char *name;
+	uint8_t type;
+	uint8_t is_laptop;
+	char *name;
 } dmi_chassis_types[] = {
    {0x01, 2, "Other"},
    {0x02, 2, "Unknown"},
@@ -83,20 +81,219 @@ static const struct {
    {0x17, 0, "Rack Mount Chassis"},
 };

-#define DMI_COMMAND_LEN_MAX 260
-static const char *dmidecode_command = "dmidecode";
+#if DMI_INTERNAL
+static int dmi_checksum(const uint8_t const *buf, size_t len)
+{
+	uint8_t sum = 0;
+	size_t a;

-static char *dmistrings[ARRAY_SIZE(dmidecode_names)];
+	for (a = 0; a < len; a++)
+		sum += buf[a];
+	return (sum == 0);
+}

-/* Strings longer than 4096 in DMI are just insane. */
-#define DMI_MAX_ANSWER_LEN 4096
+static char *dmi_string(uint8_t *buf, uint8_t string_id, uint8_t *limit)
+{
+	size_t i, len;
+
+	if (string_id == 0)
+		return "Not Specified";
+
+	while (string_id > 1 && string_id--) {
+		if (buf > limit) {
+			msg_perr("DMI table is broken (string portion out of "
+				 "bounds)!\n");
+			return "<OUT OF BOUNDS>";
+		}
+		buf += strnlen((char *)buf, limit - buf) + 1;
+	}
+
+	if (!*buf) /* as long as the current byte we're on isn't null */
+		return "<BAD INDEX>";
+
+	len = strnlen((char *)buf, limit - buf);
+	if (len > DMI_MAX_ANSWER_LEN)
+		len = DMI_MAX_ANSWER_LEN;
+
+	/* fix junk bytes in the string */
+	for (i = 0; i < len; i++)
+		if (buf[i] < 32 || buf[i] >= 127)
+			buf[i] = ' ';
+
+	return (char *)buf;
+}
+
+static void dmi_chassis_type(uint8_t code)
+{
+	int i;
+	code &= 0x7f; /* bits 6:0 are chassis type, 7th bit is the lock bit */
+	is_laptop = 2;
+	for (i = 0; i < ARRAY_SIZE(dmi_chassis_types); i++) {
+		if (code == dmi_chassis_types[i].type) {
+			msg_pdbg("DMI string chassis-type: "%s"\n",
+				 dmi_chassis_types[i].name);
+			is_laptop = dmi_chassis_types[i].is_laptop;
+			break;
+		}
+	}
+
+	switch (is_laptop) {
+	case 1:
+		msg_pdbg("Laptop detected via DMI.\n");
+		break;
+	case 2:
+		msg_pdbg("DMI chassis-type is not specific enough.\n");
+		break;
+	}
+}
+
+static void dmi_table(uint32_t base, uint16_t len, uint16_t num)
+{
+	uint8_t *data;
+	uint8_t *dmi_table_mem;
+	uint8_t *limit;
+	int i = 0, j = 0;
+
+	dmi_table_mem = physmap_try_ro("DMI Table", base, len);
+	if (!dmi_table_mem) {
+		msg_perr("Unable to access DMI Table\n");
+		return;
+	}
+
+	limit = dmi_table_mem + len;
+	data = dmi_table_mem;
+
+	/* SMBIOS structure header is always 4 B long and contains:
+	 *  - uint8_t type;	// see dmi_chassis_types's type
+	 *  - uint8_t length;	// data section w/ header w/o strings
+	 *  - uint16_t handle;
+	 */
+	while (i < num && data + 4 <= limit) {
+		/* - If a short entry is found (less than 4 bytes), not only it
+		 *   is invalid, but we cannot reliably locate the next entry.
+		 * - If the length value indicates that this structure spreads
+		 *   accross the table border, something is fishy too.
+		 * Better stop at this point, and let the user know his/her
+		 * table is broken.
+		 */
+		if (data[1] < 4 || data + data[1] > limit) {
+			msg_perr("DMI table is broken (bogus header)!\n");
+			break;
+		}
+
+		if(data[0] == 3) {
+			if (data + 5 <= limit)
+				dmi_chassis_type(data[5]);
+			/* else the table is broken, but laptop detection is
+			 * optional, hence continue. */
+		} else
+			for (j = 0; j < ARRAY_SIZE(dmi_strings); j++) {
+				uint8_t offset = dmi_strings[j].offset;
+				uint8_t type = dmi_strings[j].type;
+
+				if (data[0] != type)
+					continue;
+
+				if (data[1] <= offset || data+offset > limit) {
+					msg_perr("DMI table is broken (offset "
+						 "out of bounds)!\n");
+					goto out;
+				}
+
+				/* the dmi table will be unmapped later, hence
+				 * fill the struct with duplicated strings. */
+				dmi_strings[j].value =
+					strdup(dmi_string(data + data[1],
+							  data[offset],
+							  limit));
+			}
+		/* Find next structure by skipping data and string sections */
+		data += data[1];
+		while (data + 1 <= limit) {
+			if (data[0] == 0 && data[1] == 0)
+				break;
+			data++;
+		}
+		data += 2;
+		i++;
+	}
+out:
+	physunmap(dmi_table, len);
+}
+
+#if SM_SUPPORT
+static int smbios_decode(uint8_t *buf)
+{
+	/* TODO: other checks mentioned in the conformance guidelines? */
+	if (!dmi_checksum(buf, buf[0x05]) ||
+	    (memcmp(buf + 0x10, "_DMI_", 5) != 0) ||
+	    !dmi_checksum(buf + 0x10, 0x0F))
+			return 0;
+
+	dmi_table(mmio_readl(buf + 0x18),
+		  mmio_readw(buf + 0x16),
+		  mmio_readw(buf + 0x1C));
+
+	return 1;
+}
+#endif
+
+static int legacy_decode(uint8_t *buf)
+{
+	if (!dmi_checksum(buf, 0x0F))
+		return 0;
+
+	dmi_table(mmio_readl(buf + 0x08),
+		  mmio_readw(buf + 0x06),
+		  mmio_readw(buf + 0x0C));
+
+	return 1;
+}
+
+int dmi_fill(void)
+{
+	size_t fp;
+	uint8_t *dmi_mem;
+
+	msg_pdbg("Using Internal DMI decoder.\n");
+	/* There are two ways specified to gain access to the SMBIOS table:
+	 * - EFI's configuration table contains a pointer to the SMBIOS table.
+	 *   On linux it can be obtained from sysfs. EFI's SMBIOS GUID is:
+	 *   {0xeb9d2d31,0x2d88,0x11d3,0x9a,0x16,0x0,0x90,0x27,0x3f,0xc1,0x4d}
+	 * - Scanning physical memory address range 0x000F0000h to 0x000FFFFF
+	 *   for the anchor-string(s). */
+	dmi_mem = physmap_try_ro("DMI", 0xF0000, 0x10000);
+	if (!dmi_mem)
+		return -1;
+
+	for (fp = 0; fp <= 0xFFF0; fp += 16) {
+#if SM_SUPPORT
+		if (memcmp(dmi_mem + fp, "_SM_", 4) == 0 && fp <= 0xFFE0) {
+			if (smbios_decode(dmi_mem + fp))
+				goto out;
+		} else
+#endif
+		if (memcmp(dmi_mem + fp, "_DMI_", 5) == 0)
+			if (legacy_decode(dmi_mem + fp))
+				goto out;
+	}
+	msg_pinfo("No DMI table found.\n");
+out:
+	physunmap(dmi_mem, 0x10000);
+	return 0;
+}
+
+#else /* DMI_INTERNAL */
+
+#define DMI_COMMAND_LEN_MAX 300
+static const char *dmidecode_command = "dmidecode";

static char *get_dmi_string(const char *string_name)
 {
    FILE *dmidecode_pipe;
    char *result;
    char answerbuf[DMI_MAX_ANSWER_LEN];
-	char commandline[DMI_COMMAND_LEN_MAX + 40];
+	char commandline[DMI_COMMAND_LEN_MAX];

snprintf(commandline, sizeof(commandline),
    	 "%s -s %s", dmidecode_command, string_name);
@@ -137,7 +334,6 @@ static char *get_dmi_string(const char *string_name)
    /* Chomp trailing newline. */
    if (answerbuf[0] != 0 && answerbuf[strlen(answerbuf) - 1] == '\n')
    	answerbuf[strlen(answerbuf) - 1] = 0;
-	msg_pdbg("DMI string %s: "%s"\n", string_name, answerbuf);

result = strdup(answerbuf);
    if (!result)
@@ -146,24 +342,25 @@ static char *get_dmi_string(const char *string_name)
    return result;
 }

-void dmi_init(void)
+int dmi_fill(void)
 {
    int i;
    char *chassis_type;

-	has_dmi_support = 1;
-	for (i = 0; i < ARRAY_SIZE(dmidecode_names); i++) {
-		dmistrings[i] = get_dmi_string(dmidecode_names[i]);
-		if (!dmistrings[i]) {
-			has_dmi_support = 0;
-			return;
+	msg_pdbg("Using External DMI decoder.\n");
+	for (i = 0; i < ARRAY_SIZE(dmi_strings); i++) {
+		dmi_strings[i].value = get_dmi_string(dmi_strings[i].keyword);
+		if (!dmi_strings[i].value) {
+			/* FIXME: free previous values */
+			return 1;
    	}
    }

chassis_type = get_dmi_string("chassis-type");
    if (chassis_type == NULL)
-		return;
+		return 0; /* chassis-type handling is optional anyway */

+	msg_pdbg("DMI string chassis-type: "%s"\n", chassis_type);
    is_laptop = 2;
    for (i = 0; i < ARRAY_SIZE(dmi_chassis_types); i++) {
    	if (strcasecmp(chassis_type, dmi_chassis_types[i].name) == 0) {
@@ -181,6 +378,26 @@ void dmi_init(void)
    	break;
    }
    free(chassis_type);
+	return 0;
+}
+
+#endif /* DMI_INTERNAL */
+
+void dmi_init(void)
+{
+	int i;
+
+	/* dmi_fill fills the dmi_strings array, and if possible sets
+	 * the global is_laptop variable. */
+	if (dmi_fill())
+		return;
+
+	has_dmi_support = 1;
+	for (i = 0; i < ARRAY_SIZE(dmi_strings); i++) {
+		msg_pdbg("DMI string %s: "%s"\n",
+			 dmi_strings[i].keyword,
+			 dmi_strings[i].value);
+	}
 }

/**
@@ -237,11 +454,9 @@ int dmi_match(const char *pattern)
    if (!has_dmi_support)
    	return 0;

-	for (i = 0; i < ARRAY_SIZE(dmidecode_names); i++)
-		if (dmi_compare(dmistrings[i], pattern))
+	for (i = 0; i < ARRAY_SIZE(dmi_strings); i++)
+		if (dmi_compare(dmi_strings[i].value, pattern))
    		return 1;

return 0;
 }
-
-#endif /* STANDALONE */