2010/8/9 Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Thanks for updating the patch.<br>
Just a few short comments, this is not a full review.<br>
<div><div></div><div class="h5"><br>
On 09.08.2010 01:05, Sean Nelson wrote:<br>
> Here is a new version of the internal dmi decoding. To use the new<br>
> decoder, the user needs to use "-p internal:dmi=new", to cross-check<br>
> with the external dmidecode use "-p internal:dmi=check". Hopefully if<br>
> this is comitted, it will make it easier for people to test the new<br>
> internal decoder.<br>
><br>
> Signed-off-by: Sean Nelson <<a href="mailto:audiohacked@gmail.com">audiohacked@gmail.com</a>><br>
><br>
</div></div>> diff --git a/dmi.c b/dmi.c<br>
> index f18907f..b2a5daa 100644<br>
> --- a/dmi.c<br>
> +++ b/dmi.c<br>
> @@ -1,76 +1,227 @@<br>
> /*<br>
> * This file is part of the flashrom project.<br>
> *<br>
> * Copyright (C) 2009,2010 Michael Karcher<br>
> + * Copyright (C) 2010 Sean Nelson<br>
> *<br>
> * This program is free software; you can redistribute it and/or modify<br>
> * it under the terms of the GNU General Public License as published by<br>
> * the Free Software Foundation; either version 2 of the License, or<br>
> * (at your option) any later version.<br>
> *<br>
> * This program is distributed in the hope that it will be useful,<br>
> * but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
> * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the<br>
> * GNU General Public License for more details.<br>
> *<br>
> * You should have received a copy of the GNU General Public License<br>
> * along with this program; if not, write to the Free Software<br>
> * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA<br>
> */<br>
><br>
> #include <string.h><br>
> #include <stdio.h><br>
> #include <stdlib.h><br>
><br>
> +#include <unistd.h><br>
> +#include <fcntl.h><br>
> +#include <sys/mman.h><br>
><br>
<br>
Will this work on DOS and Windows?<br></blockquote><div><br>DOS:<br><br>flashrom v0.9.2-r1136 on FreeDOS 7 (i786), built with libpci 3.1.5, GCC 4.3.2, little endian<br>flashrom is free software, get the source code at <a href="http://www.flashrom.org">http://www.flashrom.org</a><br>
<br>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.<br>Initializing internal programmer<br>
No coreboot table found.<br>DMI string system-manufacturer: ""<br>DMI string system-product-name: ""<br>DMI string system-version: ""<br>DMI string baseboard-manufacturer: ""<br>DMI string baseboard-product-name: ""<br>
DMI string baseboard-version: ""<br>DMI string chassis-type: ""<br>DMI string chassis-type: ""<br>No DMI table found.<br><br>Linux (Arch Linux):<br><br>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<br>
flashrom is free software, get the source code at <a href="http://www.flashrom.org">http://www.flashrom.org</a><br><br>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.<br>
Initializing internal programmer<br>No coreboot table found.<br>sh: dmidecode: command not found<br>dmidecode execution unsucessfull - continuing without DMI info<br><br>FreeBSD:<br><br>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<br>
flashrom is free software, get the source code at <a href="http://www.flashrom.org">http://www.flashrom.org</a><br><br>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.<br>
Initializing internal programmer<br>No coreboot table found.<br>DMI string system-manufacturer: " "<br>DMI string system-product-name: "P4i65GV"<br>DMI string system-version: "1.00"<br>
DMI string baseboard-manufacturer: " "<br>DMI string baseboard-product-name: "P4i65GV"<br>DMI string baseboard-version: "1.0"<br>DMI string chassis-type: ""<br>DMI string chassis-type: ""<br>
No DMI table found.<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
<br>
> +<br>
> #include "flash.h"<br>
> #include "programmer.h"<br>
><br>
> int has_dmi_support = 0;<br>
><br>
> #if STANDALONE<br>
><br>
<br>
Hm. STANDALONE should activate the builtin decoder. We might elect to<br>
have a separate CONFIG_DMI which would replace !STANDALONE in this file.<br>
<br>
<br>
><br>
> /* Stub to indicate missing DMI functionality.<br>
> * has_dmi_support is 0 by default, so nothing to do here.<br>
> * Because dmidecode is not available on all systems, the goal is to implement<br>
> * the DMI subset we need directly in this file.<br>
> */<br>
> void dmi_init(void)<br>
> {<br>
> }<br>
><br>
> int dmi_match(const char *pattern)<br>
> {<br>
> return 0;<br>
> }<br>
><br>
> #else /* STANDALONE */<br>
><br>
> -static const char *dmidecode_names[] = {<br>
> - "system-manufacturer",<br>
> - "system-product-name",<br>
> - "system-version",<br>
> - "baseboard-manufacturer",<br>
> - "baseboard-product-name",<br>
> - "baseboard-version",<br>
> +/* the actual code, based on the dmidecode parsing logic */<br>
> +static const struct string_keyword {<br>
><br>
<br>
Kill string_keyword.<br>
<br>
<br>
> + const char *keyword;<br>
> + unsigned char type;<br>
> + unsigned char offset;<br>
> +} flashrom_dmi_strings[] = {<br>
> + { "system-manufacturer", 1, 0x04 },<br>
> + { "system-product-name", 1, 0x05 },<br>
> + { "system-version", 1, 0x06 },<br>
> + { "baseboard-manufacturer", 2, 0x04 },<br>
> + { "baseboard-product-name", 2, 0x05 },<br>
> + { "baseboard-version", 2, 0x06 },<br>
> + { "chassis-type", 3, 0x05 },<br>
> };<br>
><br>
> #define DMI_COMMAND_LEN_MAX 260<br>
> static const char *dmidecode_command = "dmidecode";<br>
> +static char *external_dmi[ARRAY_SIZE(flashrom_dmi_strings)];<br>
> +#define DMI_MAX_ANSWER_LEN 4096<br>
><br>
> -static char *dmistrings[ARRAY_SIZE(dmidecode_names)];<br>
> +static char *dmistrings[ARRAY_SIZE(flashrom_dmi_strings)];<br>
><br>
> -/* Strings longer than 4096 in DMI are just insane. */<br>
><br>
<br>
Why did the comment disappear?<br>
<br>
<br>
> -#define DMI_MAX_ANSWER_LEN 4096<br>
> +#define WORD(x) (unsigned short)(*(const unsigned short *)(x))<br>
> +#define DWORD(x) (unsigned int)(*(const unsigned int *)(x))<br>
><br>
<br>
Ugh. Those #defines are really ugly. Can't you use mmio_read[bwl] or<br>
something similar?<br>
<br>
<br>
> +<br>
> +static int checksum(const unsigned char *buf, size_t len)<br>
><br>
<br>
dmi_checksum?<br>
<br>
<br>
> +{<br>
> + unsigned char sum = 0;<br>
> + size_t a;<br>
> +<br>
> + for (a = 0; a < len; a++)<br>
> + sum += buf[a];<br>
> + return (sum == 0);<br>
> +}<br>
> +<br>
> +static char *dmi_string(char *bp, unsigned char length, unsigned char s)<br>
><br>
<br>
Better names for bp and s would be appreciated unless this is a straight<br>
copy from the original dmidecode code (in which case we'd have to add<br>
the author to the copyright header).<br>
<br>
<br>
> +{<br>
> + size_t i, len;<br>
> +<br>
> + if (s == 0)<br>
> + return "Not Specified";<br>
> +<br>
> + bp += length;<br>
> + while (s > 1 && *bp) {<br>
> + bp += strlen(bp);<br>
> + bp++;<br>
><br>
<br>
We happily walk off the cliff here (mapped area) if the DMI strings<br>
contain garbage.<br>
<br>
<br>
> + s--;<br>
> + }<br>
> +<br>
> + if (!*bp)<br>
> + return "<BAD INDEX>";<br>
> +<br>
> + len = strlen(bp);<br>
> + for (i = 0; i < len; i++)<br>
> + if (bp[i] < 32 || bp[i] == 127)<br>
> + bp[i] = '.';<br>
><br>
<br>
We write to bp?<br>
<br>
<br>
> +<br>
> + return bp;<br>
> +}<br>
> +<br>
> +static int dmi_chassis_type(unsigned char code)<br>
> +{<br>
> + switch(code) {<br>
> + case 0x08: /* Portable */<br>
> + case 0x09: /* Laptop */<br>
> + case 0x0A: /* Notebook */<br>
> + case 0x0E: /* Sub Notebook */<br>
> + return 1;<br>
> + default:<br>
> + return 0;<br>
> + }<br>
> +}<br>
> +<br>
> +static void dmi_table(unsigned int base, unsigned short len, unsigned short num)<br>
> +{<br>
> + unsigned char *data;<br>
> + unsigned char *dmi_table_mem;<br>
> + int key;<br>
> + int i=0, j=0;<br>
><br>
<br>
spaces<br>
<br>
<br>
> +<br>
> + dmi_table_mem = physmap_try_ro("DMI Tables", base, len);<br>
><br>
<br>
If physmap_try_ro fails this will explode spectacularly.<br>
<br>
<br>
> + data = dmi_table_mem;<br>
> +<br>
> + /* 4 is the length of an SMBIOS structure header */<br>
> + while (i < num && data+4 <= dmi_table_mem + len) {<br>
> + unsigned char *next;<br>
> + /*<br>
> + * If a short entry is found (less than 4 bytes), not only it<br>
> + * is invalid, but we cannot reliably locate the next entry.<br>
> + * Better stop at this point, and let the user know his/her<br>
> + * table is broken.<br>
> + */<br>
> + if (data[1] < 4) {<br>
> + msg_perr("Invalid entry length (%u). DMI table is "<br>
> + "broken! Stop.\n\n", (unsigned int)data[1]);<br>
> + break;<br>
> + }<br>
> +<br>
> + /* Stop decoding after chassis segment */<br>
> + if (data[0] == 4)<br>
> + break;<br>
> +<br>
> + /* look for the next handle */<br>
> + next = data + data[1];<br>
> + while (next - dmi_table_mem + 1 < len && (next[0] != 0 || next[1] != 0))<br>
> + next++;<br>
> + next += 2;<br>
> +<br>
> + for (j = 0; j < ARRAY_SIZE(flashrom_dmi_strings); j++)<br>
> + {<br>
> + unsigned char offset = flashrom_dmi_strings[j].offset;<br>
> + unsigned char type = flashrom_dmi_strings[j].type;<br>
> +<br>
> + if (offset >= data[1])<br>
> + return;<br>
> +<br>
> + key = (type << 8) | offset;<br>
> +<br>
> + switch (key)<br>
> + {<br>
> + case 0x305: /* detect if laptop */<br>
> + if (dmi_chassis_type(data[offset])) {<br>
> + msg_pdbg("Laptop detected via DMI\n");<br>
><br>
<br>
Can you print the chassis type in hex as well, even if it is not a<br>
laptop? This will be helpful if a vendor screws up the DMI tables.<br>
<br>
<br>
> + is_laptop = 1;<br>
> + }<br>
> + break;<br>
> + default:<br>
> + if (type == data[0]) {<br>
> + dmistrings[j] = dmi_string((char*)data, data[1], data[offset]);<br>
> + msg_pinfo("DMI string %s: \"%s\"\n",<br>
> + flashrom_dmi_strings[j].keyword, dmistrings[j]);<br>
> + }<br>
> + }<br>
> + }<br>
> + data = next;<br>
> + i++;<br>
> + }<br>
> +<br>
> + physunmap(dmi_table, len);<br>
> +}<br>
> +<br>
> +static int smbios_decode(unsigned char *buf)<br>
> +{<br>
> + if (!checksum(buf, buf[0x05])<br>
> + || (memcmp(buf + 0x10, "_DMI_", 5) != 0)<br>
> + || !checksum(buf + 0x10, 0x0F))<br>
> + return 0;<br>
> +<br>
> + dmi_table(DWORD(buf + 0x18), WORD(buf + 0x16), WORD(buf + 0x1C));<br>
> +<br>
> + return 1;<br>
> +}<br>
> +<br>
> +static int legacy_decode(unsigned char *buf)<br>
> +{<br>
> + if (!checksum(buf, 0x0F))<br>
> + return 0;<br>
> +<br>
> + dmi_table(DWORD(buf + 0x08), WORD(buf + 0x06), WORD(buf + 0x0C));<br>
> +<br>
> + return 1;<br>
> +}<br>
><br>
> static char *get_dmi_string(const char *string_name)<br>
> {<br>
> FILE *dmidecode_pipe;<br>
> char *result;<br>
> char answerbuf[DMI_MAX_ANSWER_LEN];<br>
> char commandline[DMI_COMMAND_LEN_MAX + 40];<br>
><br>
> snprintf(commandline, sizeof(commandline),<br>
> "%s -s %s", dmidecode_command, string_name);<br>
> dmidecode_pipe = popen(commandline, "r");<br>
> if (!dmidecode_pipe) {<br>
> msg_perr("DMI pipe open error\n");<br>
> @@ -109,45 +260,111 @@ static char *get_dmi_string(const char *string_name)<br>
> answerbuf[strlen(answerbuf) - 1] == '\n')<br>
> answerbuf[strlen(answerbuf) - 1] = 0;<br>
> msg_pdbg("DMI string %s: \"%s\"\n", string_name, answerbuf);<br>
><br>
> result = strdup(answerbuf);<br>
> if (!result)<br>
> puts("WARNING: Out of memory - DMI support fails");<br>
><br>
> return result;<br>
> }<br>
><br>
> void dmi_init(void)<br>
> {<br>
> - int i;<br>
> - char *chassis_type;<br>
> + int found = 0;<br>
> + size_t fp;<br>
> + unsigned char *dmi_mem = NULL;<br>
> + char *arg = NULL;<br>
> + int use_new_dmi = 0;<br>
> + int i = 0;<br>
> +<br>
> + arg = extract_programmer_param("dmi");<br>
> + if (arg && !strcmp(arg,"new")) {<br>
> + use_new_dmi = 1;<br>
> + } else if (arg && !strcmp(arg, "check")) {<br>
> + use_new_dmi = 2;<br>
> + } else if (arg && !strlen(arg)) {<br>
> + msg_perr("Missing argument for dmi. Falling back to external dmi\n");<br>
> + } else if (arg) {<br>
> + msg_perr("Unknown argument for dmi: %s. Falling back to external dmi\n", arg);<br>
><br>
<br>
You could also return here. If the user doesn't know what he wants, we<br>
shouldn't continue.<br>
<br>
<br>
> + }<br>
> + free(arg);<br>
><br>
> has_dmi_support = 1;<br>
> - for (i = 0; i < ARRAY_SIZE(dmidecode_names); i++) {<br>
> - dmistrings[i] = get_dmi_string(dmidecode_names[i]);<br>
> - if (!dmistrings[i]) {<br>
> - has_dmi_support = 0;<br>
> - return;<br>
> +<br>
> + if (use_new_dmi > 0) {<br>
> + dmi_mem = physmap_try_ro("DMI", 0xF0000, 0x10000);<br>
> +<br>
> + if (!dmi_mem)<br>
> + goto func_exit;<br>
> +<br>
> + for (fp = 0; fp <= 0xFFF0; fp += 16) {<br>
> + if (memcmp(dmi_mem + fp, "_SM_", 4) == 0 && fp <= 0xFFE0) {<br>
> + if (smbios_decode(dmi_mem+fp)) {<br>
> + found++;<br>
> + fp += 16;<br>
> + }<br>
> + }<br>
> + else if (memcmp(dmi_mem + fp, "_DMI_", 5) == 0)<br>
> + if (legacy_decode(dmi_mem + fp))<br>
> + found++;<br>
> }<br>
> }<br>
><br>
> - chassis_type = get_dmi_string("chassis-type");<br>
> - if (chassis_type && (!strcmp(chassis_type, "Notebook") ||<br>
> - !strcmp(chassis_type, "Portable"))) {<br>
> - msg_pdbg("Laptop detected via DMI\n");<br>
> - is_laptop = 1;<br>
> + if (use_new_dmi == 2) {<br>
> + for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings); i++)<br>
> + {<br>
> + external_dmi[i] = get_dmi_string(flashrom_dmi_strings[i].keyword);<br>
> + if (!external_dmi[i])<br>
> + goto func_exit;<br>
> + }<br>
> +<br>
> + for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings)-1; i++)<br>
><br>
<br>
spaces<br>
<br>
<br>
> + {<br>
> + if (strcmp(dmistrings[i], external_dmi[i]))<br>
> + msg_perr("dmidecode vs internal-dmi differs: %s\n", flashrom_dmi_strings[i].keyword);<br>
> + else<br>
> + msg_pspew("Matching of dmidecode and internal-dmi succeeded!\n");<br>
><br>
<br>
Hm. Do we cross-check the laptop info?<br>
<br>
<br>
> + }<br>
> }<br>
> - free(chassis_type);<br>
> +<br>
> + if (use_new_dmi == 0) {<br>
> + char *chassis_type;<br>
> + has_dmi_support = 1;<br>
> + for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings); i++) {<br>
> + dmistrings[i] = get_dmi_string(flashrom_dmi_strings[i].keyword);<br>
> + if (!dmistrings[i]) {<br>
> + has_dmi_support = 0;<br>
> + return;<br>
> + }<br>
> + }<br>
> +<br>
> + chassis_type = get_dmi_string("chassis-type");<br>
> + if (chassis_type && (!strcmp(chassis_type, "Notebook") ||<br>
> + !strcmp(chassis_type, "Portable"))) {<br>
> + msg_pdbg("Laptop detected via DMI\n");<br>
> + is_laptop = 1;<br>
> + }<br>
> + free(chassis_type);<br>
> + }<br>
> +<br>
> +func_exit:<br>
> + if (!found)<br>
> + {<br>
> + msg_pinfo("No DMI table found.\n");<br>
> + has_dmi_support = 0;<br>
> + }<br>
> +<br>
> + physunmap(dmi_mem, 0x10000);<br>
> }<br>
><br>
> /**<br>
> * Does an substring/prefix/postfix/whole-string match.<br>
> *<br>
> * The pattern is matched as-is. The only metacharacters supported are '^'<br>
> * at the beginning and '$' at the end. So you can look for "^prefix",<br>
> * "suffix$", "substring" or "^complete string$".<br>
> *<br>
> * @param value The string to check.<br>
> * @param pattern The pattern.<br>
> * @return Nonzero if pattern matches.<br>
> */<br>
> @@ -185,21 +402,21 @@ static int dmi_compare(const char *value, const char *pattern)<br>
> if (anchored)<br>
> return strncmp(value, pattern, patternlen) == 0;<br>
> else<br>
> return strstr(value, pattern) != NULL;<br>
> }<br>
><br>
> int dmi_match(const char *pattern)<br>
> {<br>
> int i;<br>
><br>
> if (!has_dmi_support)<br>
> return 0;<br>
><br>
> - for (i = 0; i < ARRAY_SIZE(dmidecode_names); i++)<br>
> + for (i = 0; i < ARRAY_SIZE(flashrom_dmi_strings); i++)<br>
> if (dmi_compare(dmistrings[i], pattern))<br>
> return 1;<br>
><br>
> return 0;<br>
> }<br>
><br>
> #endif /* STANDALONE */<br>
><br>
><br>
<br>
Regards,<br>
Carl-Daniel<br>
<font color="#888888"><br>
--<br>
<a href="http://www.hailfinger.org/" target="_blank">http://www.hailfinger.org/</a><br>
<br>
<br>
_______________________________________________<br>
flashrom mailing list<br>
<a href="mailto:flashrom@flashrom.org">flashrom@flashrom.org</a><br>
<a href="http://www.flashrom.org/mailman/listinfo/flashrom" target="_blank">http://www.flashrom.org/mailman/listinfo/flashrom</a><br>
</font></blockquote></div><br>