Stefan Reinauer wrote: > > (It would be nice to have msrtool diff mode available in coreinfo, > > using a file stored in cbfs for comparison!) > > I think a diff mode would be better implemented in msrtool itself
The diff mode has been implemented in msrtool already since first committed rc1. :) But currently it assumes that diff should always be between a file and the machine that msrtool is running on. > -- If the machine is able to boot coreinfo, it's quite likely > already that it can also boot a kernel. This is true! > If not, you're stuck with printk debugging anyways. Yep. Patches attached for reading both values from the file. //Peter
>From e69272b38a3beeadcf1052ab50631de4750fffaf Mon Sep 17 00:00:00 2001 From: Peter Stuge <[email protected]> Date: Sat, 16 Jan 2010 20:03:16 +0100 Subject: [PATCH 1/2] msrtool: Add endptr to str2msr() showing how many characters were parsed This also introduces a small change in the user interface for immediate mode (-i). Previously, whitespace could separate high and low words in an MSR as such: msrtool -i 4c00000f='f2f100ff 56960004' That is no longer allowed, a space character now ends the MSR value. Any other character can still be used as separator however, so the following syntax still works as expected: msrtool -i 4c00000f=f2f100ff:56960004 Signed-off-by: Peter Stuge <[email protected]> --- msrtool.c | 4 ++-- msrtool.h | 2 +- msrutils.c | 9 +++++---- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/msrtool.c b/msrtool.c index 570f1fb..41e7e2c 100644 --- a/msrtool.c +++ b/msrtool.c @@ -208,7 +208,7 @@ int do_diff(const char *difffn) { m1start = line + tmp + m1pos; for (len = strlen(m1start) - 1; NULL != strchr("\r\n", m1start[len]); --len) m1start[len] = 0; - if (!str2msr(m1start, &m1)) { + if (!str2msr(m1start, &m1, NULL)) { fprintf(stderr, "%s:%d: invalid MSR value '%s'\n", difffn, linenum, m1start); continue; } @@ -288,7 +288,7 @@ int main(int argc, char *argv[]) { fprintf(stderr, "missing value in -i argument!\n"); break; } - if (!str2msr(++optarg, &msrval)) + if (!str2msr(++optarg, &msrval, NULL)) fprintf(stderr, "invalid value in -i argument!\n"); break; case 's': diff --git a/msrtool.h b/msrtool.h index d8fc00f..cf77b49 100644 --- a/msrtool.h +++ b/msrtool.h @@ -170,7 +170,7 @@ const struct msrdef *findmsrdef(const uint32_t addr); uint32_t msraddrbyname(const char *name); void dumpmsrdefs(const struct targetdef *t); int dumpmsrdefsvals(FILE *f, const struct targetdef *t, const uint8_t cpu); -uint8_t str2msr(char *str, struct msr *msr); +uint8_t str2msr(char *str, struct msr *msr, char **endptr); void decodemsr(const uint8_t cpu, const uint32_t addr, const struct msr val); uint8_t diff_msr(FILE *fout, const uint32_t addr, const struct msr a, const struct msr b); diff --git a/msrutils.c b/msrutils.c index dfb6617..5a5fbd8 100644 --- a/msrutils.c +++ b/msrutils.c @@ -193,7 +193,7 @@ int dumpmsrdefsvals(FILE *f, const struct targetdef *t, const uint8_t cpu) { * Parse a hexadecimal string into an MSR value. * * Leading 0x or 0X is optional, the string is always parsed as hexadecimal. - * Any non-hexadecimal character can be used to separate the high 32 bits and + * Any non-hexadecimal character except ' ' can separate the high 32 bits and * the low 32 bits. If there is such a separator, high and low values do not * need to be zero padded. If there is no separator, the last <=8 digits are * the low 32 bits and any characters before them are the high 32 bits. @@ -205,15 +205,16 @@ int dumpmsrdefsvals(FILE *f, const struct targetdef *t, const uint8_t cpu) { * @param str The string to parse. The string must be writable but will be * restored before return. * @param msr Pointer to the struct msr where the value will be stored. + * @param endptr If endpotr is not NULL, *endptr will point to after the MSR. * @return 1 on success, 0 on parse failure. msr is unchanged on failure. */ -uint8_t str2msr(char *str, struct msr *msr) { +uint8_t str2msr(char *str, struct msr *msr, char **endptr) { char c; size_t len, lo; if (0 == strncmp(str, "0x", 2) || 0 == strncmp(str, "0X", 2)) str += 2; len = strspn(str, HEXCHARS); - if (len <= 8 && 0 == str[len]) { + if (len <= 8 && (0 == str[len] || ' ' == str[len])) { msr->hi = 0; lo = 0; } else if (len <= 8) { @@ -231,7 +232,7 @@ uint8_t str2msr(char *str, struct msr *msr) { msr->hi = strtoul(str, NULL, 16); str[lo] = c; } - msr->lo = strtoul(str + lo, NULL, 16); + msr->lo = strtoul(str + lo, endptr, 16); return 1; } -- 1.6.3.3
>From f81bb5673d84bac44339f7443f3275d5bceb2796 Mon Sep 17 00:00:00 2001 From: Peter Stuge <[email protected]> Date: Sat, 16 Jan 2010 20:19:24 +0100 Subject: [PATCH 2/2] msrtool: Read both MSR values in diff mode from file, if available Previously, msrtool would assume that MSR values should be compared between stored value in file and current value in hardware which msrtool was running on. This does not always fit the use case and with this change msrtool can now compare two sets of MSR values stored in a file. If only one MSR value is stored in the file, msrtool will behave as previously and read the second MSR value from hardware. This change means that msrtool does not always need access to the system MSR functions so it can now be run as a regular user when using diff mode with both MSR values stored in the file. Signed-off-by: Peter Stuge <[email protected]> --- msrtool.c | 59 +++++++++++++++++++++++++++++++++++++---------------------- 1 files changed, 37 insertions(+), 22 deletions(-) diff --git a/msrtool.c b/msrtool.c index 41e7e2c..47b4938 100644 --- a/msrtool.c +++ b/msrtool.c @@ -93,6 +93,16 @@ static void *add_target(const struct targetdef *t) { return targets; } +static int found_system() { + if (!sys || (sys && !sys->name)) { + fprintf(stderr, "Unable to detect the current operating system!\n"); + fprintf(stderr, "On Linux, please run 'modprobe msr' and try again.\n"); + fprintf(stderr, "Please send a report or patch to [email protected]. Thanks for your help!\n"); + fprintf(stderr, "\n"); + } + return (sys && sys->name); +} + int do_stream(const char *streamfn, uint8_t ignoreinput) { char tmpfn[20], line[256]; uint8_t tn; @@ -132,6 +142,8 @@ int do_stream(const char *streamfn, uint8_t ignoreinput) { } } + if (!found_system()) + goto done; if (!sys->open(cpu, SYS_RDONLY)) goto done; if (ignoreinput) { @@ -180,9 +192,9 @@ done: } int do_diff(const char *difffn) { - char tmpfn[20], line[512], *m1start; + char tmpfn[20], line[512], *m1start, *m2start; size_t len; - int ret = 1, tmp, m1pos; + int ret = 1, tmp, m1pos, sys_opened = 0; FILE *fin = NULL, *fout = stdout; uint8_t rev = 0; uint32_t addr, linenum; @@ -199,8 +211,6 @@ int do_diff(const char *difffn) { return 1; } - if (!sys->open(cpu, SYS_RDONLY)) - goto done; for (linenum = 1; NULL != fgets(line, sizeof(line), fin); ++linenum) { tmp = strncmp("0x", line, 2) ? 0 : 2; if (sscanf(line + tmp, "%8x %n%*x", &addr, &m1pos) < 1) @@ -208,12 +218,24 @@ int do_diff(const char *difffn) { m1start = line + tmp + m1pos; for (len = strlen(m1start) - 1; NULL != strchr("\r\n", m1start[len]); --len) m1start[len] = 0; - if (!str2msr(m1start, &m1, NULL)) { - fprintf(stderr, "%s:%d: invalid MSR value '%s'\n", difffn, linenum, m1start); + if (!str2msr(m1start, &m1, &m2start)) { + fprintf(stderr, "%s:%d: invalid MSR1 value '%s'\n", difffn, linenum, m1start); continue; } - if (!sys->rdmsr(cpu, addr, &m2)) - goto done; + while (' ' == *m2start) + ++m2start; + if (!str2msr(m2start, &m2, NULL)) { + fprintf(stderr, "%s:%d: invalid MSR2 value '%s' - reading from hardware!\n", difffn, linenum, m2start); + if (!sys_opened) { + if (!found_system()) + goto done; + sys_opened = sys->open(cpu, SYS_RDONLY); + if (!sys_opened) + goto done; + } + if (!sys->rdmsr(cpu, addr, &m2)) + goto done; + } if (diff_msr(fout, addr, rev ? m2 : m1, rev ? m1 : m2)) fprintf(fout, "\n"); } @@ -222,7 +244,8 @@ int do_diff(const char *difffn) { else ret = 0; done: - sys->close(cpu); + if (sys_opened) + sys->close(cpu); if (strcmp(difffn, "-")) { if (ret) unlink(tmpfn); @@ -351,13 +374,6 @@ int main(int argc, char *argv[]) { return 0; } - if (sys && !sys->name) { - fprintf(stderr, "Unable to detect the current operating system!\n"); - fprintf(stderr, "On Linux, please do 'modprobe msr' and retry.\n"); - fprintf(stderr, "Please send a report or patch to [email protected]. Thanks for your help!\n"); - fprintf(stderr, "\n"); - } - if (!targets_found || !targets) { fprintf(stderr, "Unable to detect a known target; can not decode any MSRs! (Use -t to force)\n"); fprintf(stderr, "Please send a report or patch to [email protected]. Thanks for your help!\n"); @@ -370,9 +386,6 @@ int main(int argc, char *argv[]) { return 0; } - if (sys && !sys->name) - return 1; - if (listmsrs) { if (streamfn) return do_stream(streamfn, 1); @@ -388,9 +401,6 @@ int main(int argc, char *argv[]) { if (streamfn) return do_stream(streamfn, 0); - if (!sys->open(cpu, SYS_RDONLY)) - return 1; - if (difffn) { ret = do_diff(difffn); goto done; @@ -402,6 +412,11 @@ int main(int argc, char *argv[]) { goto done; } + if (!found_system()) + return 1; + if (!sys->open(cpu, SYS_RDONLY)) + return 1; + for (; optind < argc; optind++) { addr = msraddrbyname(argv[optind]); if (!sys->rdmsr(cpu, addr, &msrval)) -- 1.6.3.3
-- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
