Maxim Polyakov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: [WIP] util/inteltool: add opt to override cores number ......................................................................
[WIP] util/inteltool: add opt to override cores number
In some cases, it is necessary to increase (if the server contains several processors) or decrease (dump should be more compact) the number of CPU cores for dump MSRs.
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 15 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/1
diff --git a/util/inteltool/cpu.c b/util/inteltool/cpu.c index f100851..9ae46c9 100644 --- a/util/inteltool/cpu.c +++ b/util/inteltool/cpu.c @@ -219,7 +219,7 @@ return error; }
-int print_intel_core_msrs(void) +int print_intel_core_msrs(unsigned int cores_num) { unsigned int i, core, id; msr_t msr; @@ -2256,8 +2256,10 @@
close(fd_msr);
- unsigned int max_core_limit = cpu->max_core_limit ? 8 : cpu->max_core_limit; - for (core = 0; core < max_core_limit; core++) { + if (!cores_num) + cores_num = cpu->max_core_limit; + + for (core = 0; core < cores_num; core++) { #ifndef __DARWIN__ char msrfilename[64]; memset(msrfilename, 0, 64); diff --git a/util/inteltool/inteltool.c b/util/inteltool/inteltool.c index db80bd2..bc9af2b 100644 --- a/util/inteltool/inteltool.c +++ b/util/inteltool/inteltool.c @@ -476,7 +476,7 @@
static void print_usage(const char *name) { - printf("usage: %s [-vh?gGrpmedPMaAsfSRx]\n", name); + printf("usage: %s [-vh?gGrpmedPMcaAsfSRx]\n", name); printf("\n" " -v | --version: print the version\n" " -h | --help: print this help\n\n" @@ -493,6 +493,7 @@ " -d | --dmibar: dump northbridge DMIBAR registers\n" " -P | --pciexpress: dump northbridge PCIEXBAR registers\n\n" " -M | --msrs: dump CPU MSRs\n" + " -c <num> | --cores <num>: override CPU cores number for MSR dump\n\n" " -A | --ambs: dump AMB registers\n" " -x | --sgx: dump SGX status\n" " -a | --all: dump all known (safe) registers\n" @@ -508,6 +509,7 @@ struct pci_dev *sb = NULL, *nb, *gfx = NULL, *ahci = NULL, *dev; const char *dump_spd_file = NULL; int opt, option_index = 0; + unsigned int cores = 0; unsigned int id, i;
char *sbname = "unknown", *nbname = "unknown", *gfxname = "unknown"; @@ -532,6 +534,7 @@ {"dmibar", 0, 0, 'd'}, {"pciexpress", 0, 0, 'P'}, {"msrs", 0, 0, 'M'}, + {"cores", required_argument, 0, 'c'}, {"ambs", 0, 0, 'A'}, {"spi", 0, 0, 's'}, {"spd", 0, 0, 'S'}, @@ -543,7 +546,7 @@ {0, 0, 0, 0} };
- while ((opt = getopt_long(argc, argv, "vh?gGrpmedPMaAsfRS:x", + while ((opt = getopt_long(argc, argv, "vh?gGrpmedPMc:aAsfRS:x", long_options, &option_index)) != EOF) { switch (opt) { case 'v': @@ -587,6 +590,9 @@ case 'M': dump_coremsrs = 1; break; + case 'c': + cores = strtoul(optarg, NULL, 0); + break; case 'a': dump_gpios = 1; show_gpio_diffs = 1; @@ -813,7 +819,7 @@ }
if (dump_coremsrs) { - print_intel_core_msrs(); + print_intel_core_msrs(cores); printf("\n\n"); }
diff --git a/util/inteltool/inteltool.h b/util/inteltool/inteltool.h index fc6dc4b..7708d70 100644 --- a/util/inteltool/inteltool.h +++ b/util/inteltool/inteltool.h @@ -385,7 +385,7 @@ void unmap_physical(void *virt_addr, size_t len);
unsigned int cpuid(unsigned int op); -int print_intel_core_msrs(void); +int print_intel_core_msrs(unsigned int cores_num); int print_mchbar(struct pci_dev *nb, struct pci_access *pacc, const char *dump_spd_file); int print_pmbase(struct pci_dev *sb, struct pci_access *pacc); int print_rcba(struct pci_dev *sb);
Hello Stefan Reinauer,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35919
to look at the new patch set (#2).
Change subject: [WIP] util/inteltool: add opt to override cores number ......................................................................
[WIP] util/inteltool: add opt to override cores number
WIP: core number range for dump? In some cases, it is necessary to increase (if the server contains several processors) or decrease (dump should be more compact) the number of CPU cores for dump MSRs.
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 15 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/2
Hello Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35919
to look at the new patch set (#3).
Change subject: [WIP] inteltool: add opt to set cores range to dump MSRs ......................................................................
[WIP] inteltool: add opt to set cores range to dump MSRs
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 22 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/3
Hello Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35919
to look at the new patch set (#4).
Change subject: [WIP] util/inteltool: set cores range to dump MSRs ......................................................................
[WIP] util/inteltool: set cores range to dump MSRs
Adds the ability to dump only for the specified range of CPU cores. The range is set using command line switches:
$ sudo ./inteltool --msr --range 0-7 $ sudo ./inteltool --msr --range 7-15 $ sudo ./inteltool --msr --range 32
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 21 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/4
Hello Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35919
to look at the new patch set (#5).
Change subject: util/inteltool: set cores range to dump MSRs ......................................................................
util/inteltool: set cores range to dump MSRs
Adds the ability to dump only for the specified range of CPU cores. The range is set using command line switches:
$ sudo ./inteltool --msr --range 0-7 $ sudo ./inteltool --msr --range 7-15 $ sudo ./inteltool --msr --range 32
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 21 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/5
Hello Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35919
to look at the new patch set (#6).
Change subject: [WIP]util/inteltool: set cores range to dump MSRs ......................................................................
[WIP]util/inteltool: set cores range to dump MSRs
Adds the ability to dump only for the specified range of CPU cores. The range is set using command line switches:
$ sudo ./inteltool --msr --range 0-7 $ sudo ./inteltool --msr --range 7-15 $ sudo ./inteltool --msr --range 32
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 21 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/6
Hello build bot (Jenkins), Stefan Reinauer,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35919
to look at the new patch set (#7).
Change subject: util/inteltool: set cores range to dump MSRs ......................................................................
util/inteltool: set cores range to dump MSRs
Adds the ability to dump only for the specified range of CPU cores. This is very useful if we are analyzing a server processor with a large number of cores.
The range is set using command line option --range <start-end>:
$ sudo ./inteltool --msr --range 0-7 $ sudo ./inteltool --msr --range 7-15 $ sudo ./inteltool --msr --range 32
Printing information with -M or --msrs option remains unchanged.
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 40 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/7
Hello build bot (Jenkins), Stefan Reinauer,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35919
to look at the new patch set (#8).
Change subject: inteltool: Allow to set core range for MSRs dump ......................................................................
inteltool: Allow to set core range for MSRs dump
Adds the ability to dump only for the specified range of CPU cores. This is very useful if we are analyzing a server processor with a large number of cores.
The range is set using command line option --range <start-end>:
$ sudo ./inteltool --msrs --range 0-7 $ sudo ./inteltool --msrs --range 7-15 $ sudo ./inteltool --msrs --range 32
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 47 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/8
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set core range for MSRs dump ......................................................................
Patch Set 11:
This change is ready for review.
Hello build bot (Jenkins), Stefan Reinauer,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35919
to look at the new patch set (#12).
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
inteltool: Allow to set cores range for MSRs dump
Adds the ability to output MSRs dump for the specified range of CPU cores. This makes it easier to reverse engineer server multicore processors using the intertool utility.
The range is set using command line option --range <start-end>:
$ sudo ./inteltool --msrs --range 0-7 $ sudo ./inteltool --msrs --range 7-15 $ sudo ./inteltool --msrs --range 32
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 39 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/12
Hello build bot (Jenkins), Stefan Reinauer,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35919
to look at the new patch set (#13).
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
inteltool: Allow to set cores range for MSRs dump
Adds the ability to output MSRs dump for the specified range of CPU cores. This makes it easier to reverse engineer server multicore processors using the intertool utility.
The range is set using --range <start-end> command line option:
$ sudo ./inteltool --msrs --range 0-7 $ sudo ./inteltool --msrs --range 7-15 $ sudo ./inteltool --msrs --range 32
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 39 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/13
Attention is currently required from: Lance Zhao, Nico Huber, Maxim Polyakov, Johnny Lin, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 13:
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/35919/comment/6f17d076_828f3e51 PS13, Line 11: intertool typo: inte*l*tool
File util/inteltool/cpu.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/a49a1b1b_f5902d0d PS13, Line 2300: range_end != (unsigned int)(-1) uh... Use UINT_MAX?
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/a78d5b48_9b61eb22 PS13, Line 520: range How about naming this option `cpu-range`? This should avoid name clashes if anyone wants to add another range option in the future.
https://review.coreboot.org/c/coreboot/+/35919/comment/6d02f9a2_49478d7b PS13, Line 590: (unsigned int)(-1) UINT_MAX
https://review.coreboot.org/c/coreboot/+/35919/comment/62a5d416_2bcce9a8 PS13, Line 666: if (!dump_coremsrs) { AFAIUI, this will bail out if --range is specified before -M. I'd move this check out of the argument parsing loop.
https://review.coreboot.org/c/coreboot/+/35919/comment/f2635d1d_c9a686f3 PS13, Line 671: errno = 0; : if (strlen(optarg) == 0 || errno) { What do you use `errno` for here?
https://review.coreboot.org/c/coreboot/+/35919/comment/6858872a_6df53dee PS13, Line 676: sscanf(optarg, "%u-%u", &cores_range_start, &cores_range_end);
The sscanf function returns the value of the macro EOF if an input failure occurs before any conversion. Otherwise, the sscanf function returns the number of input items assigned, which can be fewer than provided for, or even zero, in the event of an early matching failure.
Please handle this function's return value.
https://review.coreboot.org/c/coreboot/+/35919/comment/a9f0a2ca_2c2cf4ac PS13, Line 681: } else if (cores_range_end == (unsigned int)(-1)) { Please use the return value of `sscanf()` to handle this instead of repeating a magic number.
Attention is currently required from: Lance Zhao, Nico Huber, Maxim Polyakov, Johnny Lin, Patrick Rudolph. Hello Lance Zhao, build bot (Jenkins), Nico Huber, Johnny Lin, Stefan Reinauer, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35919
to look at the new patch set (#14).
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
inteltool: Allow to set cores range for MSRs dump
Adds the ability to output MSRs dump for the specified range of CPU cores. This makes it easier to reverse engineer server multicore processors using the inteltool utility.
The range is set using --cpu-range <start-end> command line option:
$ sudo ./inteltool -M --cpu-range 0-7 $ sudo ./inteltool -M --cpu-range 7-15 $ sudo ./inteltool -M --cpu-range 32
$ sudo ./inteltool -M" will print a register dump for all cores, just as before.
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 32 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/14
Attention is currently required from: Lance Zhao, Nico Huber, Johnny Lin, Angel Pons, Patrick Rudolph. Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 14:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/35919/comment/2eef6899_a678e5ac PS13, Line 11: intertool
typo: inte*l*tool
Done
Patchset:
PS14: Thanks for the review!
File util/inteltool/cpu.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/602c23e9_ee9a729d PS13, Line 2300: range_end != (unsigned int)(-1)
uh... […]
Yes, it looks better, thanks!
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/6215553c_84eeb5ef PS13, Line 520: range
How about naming this option `cpu-range`? This should avoid name clashes if anyone wants to add anot […]
Ok, I agree, let's use --cpu-range.
https://review.coreboot.org/c/coreboot/+/35919/comment/0d1b19a1_ef2cf376 PS13, Line 590: (unsigned int)(-1)
UINT_MAX
Done
https://review.coreboot.org/c/coreboot/+/35919/comment/065070dd_1377c214 PS13, Line 666: if (!dump_coremsrs) {
AFAIUI, this will bail out if --range is specified before -M. […]
It seems to me that this check is unnecessary and there is no point in it. It just won't work if the -M option isn't set. This code block was removed in the last patch.
https://review.coreboot.org/c/coreboot/+/35919/comment/d12d21fc_008c1804 PS13, Line 671: errno = 0; : if (strlen(optarg) == 0 || errno) {
What do you use `errno` for here?
Sorry for the inattention. This is not necessary here.
https://review.coreboot.org/c/coreboot/+/35919/comment/f9f33a33_7eebbdaa PS13, Line 676: sscanf(optarg, "%u-%u", &cores_range_start, &cores_range_end);
The sscanf function returns the value of the macro EOF if an input failure occurs before any conve […]
Ok, this way is much better, thanks!
https://review.coreboot.org/c/coreboot/+/35919/comment/dbd1b948_e07eb3bc PS13, Line 681: } else if (cores_range_end == (unsigned int)(-1)) {
Please use the return value of `sscanf()` to handle this instead of repeating a magic number.
Done
Attention is currently required from: Lance Zhao, Nico Huber, Maxim Polyakov, Johnny Lin, Angel Pons, Patrick Rudolph. Hello Lance Zhao, build bot (Jenkins), Nico Huber, Johnny Lin, Stefan Reinauer, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35919
to look at the new patch set (#15).
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
inteltool: Allow to set cores range for MSRs dump
Adds the ability to output MSRs dump for the specified range of CPU cores. This makes it easier to reverse engineer server multicore processors using the inteltool utility.
The range is set using --cpu-range <start-end> command line option:
$ sudo ./inteltool -M --cpu-range 0-7 $ sudo ./inteltool -M --cpu-range 7-15 $ sudo ./inteltool -M --cpu-range 32
$ sudo ./inteltool -M will print a register dump for all cores, just as before.
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 34 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/15
Attention is currently required from: Lance Zhao, Nico Huber, Johnny Lin, Angel Pons, Patrick Rudolph. Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 16:
(1 comment)
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/551974bb_b701480b PS13, Line 666: if (!dump_coremsrs) {
It seems to me that this check is unnecessary and there is no point in it. […]
Do you agree with this? Resolved?
Attention is currently required from: Felix Singer, Lance Zhao, Nico Huber, Maxim Polyakov, Johnny Lin, Tim Wawrzynczak, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 16: Code-Review+1
(2 comments)
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/603f9994_2a14a542 PS13, Line 666: if (!dump_coremsrs) {
Do you agree with this? Resolved?
Sometimes, the best way to solve a problem is to remove what causes the problem to exist. 😄
(Yes, I agree)
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/93cd719b_7d22a525 PS16, Line 671: if (sscanf(optarg, "%u-%u", &cores_range_start, &cores_range_end) == 1) { : /* the end of the range is not specified - only for one core */ : cores_range_end = cores_range_start; : } else if (cores_range_end < cores_range_start) { : printf("Error: invalid cores range <%u-%u>!\n", : cores_range_start, cores_range_end); : exit(1); : } I was thinking of something like this:
const int sscanf_ret = sscanf(optarg, "%u-%u", &cores_range_start, &cores_range_end); if (sscanf_ret == 1) { /* the end of the range is not specified - only for one core */ cores_range_end = cores_range_start; } else if (sscanf_ret != 2) { print_usage(argv[0]); exit(1); } else if (cores_range_end < cores_range_start) { printf("Error: invalid cores range <%u-%u>!\n", cores_range_start, cores_range_end); exit(1); }
The main difference is the addition of the second check, `sscanf_ret != 2`. If true, it means the specified argument format isn't correct, and we shouldn't rely on the numeric values. I'm not sure when a negative number would be returned (if possible), but the return value can very well be zero: when the argument doesn't match the expected format and no numbers were scanned.
Attention is currently required from: Felix Singer, Lance Zhao, Nico Huber, Maxim Polyakov, Johnny Lin, Tim Wawrzynczak, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Hello Felix Singer, Lance Zhao, build bot (Jenkins), Nico Huber, Johnny Lin, Tim Wawrzynczak, Stefan Reinauer, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35919
to look at the new patch set (#17).
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
inteltool: Allow to set cores range for MSRs dump
Adds the ability to output MSRs dump for the specified range of CPU cores. This makes it easier to reverse engineer server multicore processors using the inteltool utility.
The range is set using --cpu-range <start-end> command line option:
$ sudo ./inteltool -M --cpu-range 0-7 $ sudo ./inteltool -M --cpu-range 7-15 $ sudo ./inteltool -M --cpu-range 32
$ sudo ./inteltool -M will print a register dump for all cores, just as before.
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 38 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/17
Attention is currently required from: Felix Singer, Lance Zhao, Nico Huber, Johnny Lin, Tim Wawrzynczak, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 17:
(2 comments)
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/c140c4c1_808edbb5 PS13, Line 666: if (!dump_coremsrs) {
Sometimes, the best way to solve a problem is to remove what causes the problem to exist. 😄 […]
I would say this seriously, without humor, it's just like you said ) Also... The less code, the faster the review. This relates to all large projects.
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/c398eab7_d41f7383 PS16, Line 671: if (sscanf(optarg, "%u-%u", &cores_range_start, &cores_range_end) == 1) { : /* the end of the range is not specified - only for one core */ : cores_range_end = cores_range_start; : } else if (cores_range_end < cores_range_start) { : printf("Error: invalid cores range <%u-%u>!\n", : cores_range_start, cores_range_end); : exit(1); : }
I was thinking of something like this: […]
Yes, I agree with you, this check is necessary. Thx
Attention is currently required from: Felix Singer, Lance Zhao, Nico Huber, Maxim Polyakov, Johnny Lin, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 17: Code-Review+1
Attention is currently required from: Felix Singer, Lance Zhao, Maxim Polyakov, Johnny Lin, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 18: Code-Review+1
(1 comment)
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/74b104e4_996a623e PS18, Line 521: <start-end> As the dash is meant literally, should this be
--cpu-range <start>-<end>:
?
Attention is currently required from: Felix Singer, Lance Zhao, Maxim Polyakov, Johnny Lin, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 18:
(1 comment)
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/b085f9fd_692f0089 PS18, Line 521: <start-end>
As the dash is meant literally, should this be […]
Actually, it's
--cpu-range <start>[-<end>]:
isn't it?
Attention is currently required from: Felix Singer, Lance Zhao, Nico Huber, Johnny Lin, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 18:
(1 comment)
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/8faae7a7_787ddc97 PS18, Line 521: <start-end>
Actually, it's […]
I'm not sure, but maybe it should look like this: --cpu-range [<core> | <start-end>] ?
The fact is that when we use one parameter only: sudo ./intertool -M --cpu-range 10, inteltool prints a dump for one core only.
I didn't add a separate option --cpu-core <core>. But maybe this option is needed?
Attention is currently required from: Felix Singer, Lance Zhao, Maxim Polyakov, Johnny Lin, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 18:
(1 comment)
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/f3d3b640_93b9192e PS18, Line 521: <start-end>
I'm not sure, but maybe it should look like this: […]
Technically `<start-end>` already covers the single-core case. Because `start-end` is just a symbol here. So we could keep just that. I just noticed that we could also make the dash explicit.
I guess any of these two would be more accurate:
--cpu-range [<core> | <start>-<end>]
--cpu-range <start>[-<end>]
(Only almost accurate, because I assume sscanf() also accepts `<start>-` ;) maybe even any trailing garbage. But that's an implementation detail and not the expected syntax.)
Attention is currently required from: Felix Singer, Lance Zhao, Nico Huber, Maxim Polyakov, Johnny Lin, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 18:
(1 comment)
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/65d24f3b_93319bd0 PS18, Line 521: <start-end>
Technically `<start-end>` already covers the single-core case. Because `start-end` is just a symbol here. So we could keep just that. I just noticed that we could also make the dash explicit.
I guess any of these two would be more accurate:
--cpu-range [<core> | <start>-<end>] --cpu-range <start>[-<end>](Only almost accurate, because I assume sscanf() also accepts `<start>-` ;) maybe even any trailing garbage. But that's an implementation detail and not the expected syntax.)
+1
Attention is currently required from: Felix Singer, Lance Zhao, Nico Huber, Maxim Polyakov, Johnny Lin, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Hello Felix Singer, Lance Zhao, build bot (Jenkins), Nico Huber, Johnny Lin, Tim Wawrzynczak, Stefan Reinauer, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35919
to look at the new patch set (#19).
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
inteltool: Allow to set cores range for MSRs dump
Adds the ability to output MSRs dump for the specified range of CPU cores. This makes it easier to reverse engineer server multicore processors using the inteltool utility.
The range is set using --cpu-range <start>[-<end>] command line option:
$ sudo ./inteltool -M --cpu-range 0-7 $ sudo ./inteltool -M --cpu-range 7-15 $ sudo ./inteltool -M --cpu-range 32
$ sudo ./inteltool -M will print a register dump for all cores, just as before.
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 38 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/35919/19
Attention is currently required from: Felix Singer, Lance Zhao, Nico Huber, Johnny Lin, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph. Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 19:
(1 comment)
File util/inteltool/inteltool.c:
https://review.coreboot.org/c/coreboot/+/35919/comment/f0d489cc_781ce618 PS18, Line 521: <start-end>
Technically `<start-end>` already covers the single-core case. Because […]
Ok, I see. Thanks for your comments.
In this case, I would prefer the shorter option: --cpu-range <start>[-<end>] (this fits perfectly into the current boundaries in the usage help).
And yes, you are right, we are not immune from the case when the second parameter is incorrect: 1-qwerty, or even the first - 1qwerty (both cases will return 1 for us)... If we want to avoid this, for example, we can calculate the number of characters for cores_range_start and compare this with the length of the optarg or use something better than sscanf. But I don't think it needs to be complicated.
Attention is currently required from: Felix Singer, Lance Zhao, Nico Huber, Maxim Polyakov, Johnny Lin, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph, Felix Held. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 20: Code-Review+1
Attention is currently required from: Felix Singer, Lance Zhao, Maxim Polyakov, Johnny Lin, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph, Felix Held. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
Patch Set 20: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35919 )
Change subject: inteltool: Allow to set cores range for MSRs dump ......................................................................
inteltool: Allow to set cores range for MSRs dump
Adds the ability to output MSRs dump for the specified range of CPU cores. This makes it easier to reverse engineer server multicore processors using the inteltool utility.
The range is set using --cpu-range <start>[-<end>] command line option:
$ sudo ./inteltool -M --cpu-range 0-7 $ sudo ./inteltool -M --cpu-range 7-15 $ sudo ./inteltool -M --cpu-range 32
$ sudo ./inteltool -M will print a register dump for all cores, just as before.
Change-Id: I3a037cf7ac270d2b51d6e453334c358ff47b4105 Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35919 Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/inteltool/cpu.c M util/inteltool/inteltool.c M util/inteltool/inteltool.h 3 files changed, 38 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Tim Wawrzynczak: Looks good to me, but someone else must approve
diff --git a/util/inteltool/cpu.c b/util/inteltool/cpu.c index db63d36..7425229 100644 --- a/util/inteltool/cpu.c +++ b/util/inteltool/cpu.c @@ -7,6 +7,7 @@ #include <stdlib.h> #include <string.h> #include <errno.h> +#include <limits.h>
#include "inteltool.h"
@@ -265,9 +266,9 @@ #endif }
-int print_intel_core_msrs(void) +int print_intel_msrs(unsigned int range_start, unsigned int range_end) { - unsigned int i, core, id, core_num = get_number_of_cores(); + unsigned int i, core, id; msr_t msr;
#define IA32_PLATFORM_ID 0x0017 @@ -2295,7 +2296,15 @@
close(fd_msr);
- for (core = 0; core < core_num; core++) { + const unsigned int cores_range_max_limit = get_number_of_cores() - 1; + if (range_end > cores_range_max_limit) { + if (range_end != UINT_MAX) + printf("Warning: the range exceeds the maximum core number %d!\n", + cores_range_max_limit); + range_end = cores_range_max_limit; + } + + for (core = range_start; core <= range_end; core++) { #ifndef __DARWIN__ char msrfilename[64]; memset(msrfilename, 0, 64); diff --git a/util/inteltool/inteltool.c b/util/inteltool/inteltool.c index 7cff80f..f8c5e91 100644 --- a/util/inteltool/inteltool.c +++ b/util/inteltool/inteltool.c @@ -10,6 +10,7 @@ #include <sys/mman.h> #include <unistd.h> #include <errno.h> +#include <limits.h> #include "inteltool.h" #include "pcr.h"
@@ -21,6 +22,7 @@
enum long_only_opts { LONG_OPT_PCR = 0x100, + LONG_OPT_RANGE = 0x101, };
/* @@ -516,6 +518,7 @@ " -d | --dmibar: dump northbridge DMIBAR registers\n" " -P | --pciexpress: dump northbridge PCIEXBAR registers\n\n" " -M | --msrs: dump CPU MSRs\n" + " --cpu-range <start>[-<end>]: (optional) set CPU cores range for -M (--msrs) option\n" " -A | --ambs: dump AMB registers\n" " -x | --sgx: dump SGX status\n" " -t | --tme: dump TME status\n" @@ -585,6 +588,8 @@ size_t pcr_count = 0; uint8_t dump_pcr[MAX_PCR_PORTS];
+ unsigned int cores_range_start = 0, cores_range_end = UINT_MAX; + static struct option long_options[] = { {"version", 0, 0, 'v'}, {"help", 0, 0, 'h'}, @@ -598,6 +603,7 @@ {"dmibar", 0, 0, 'd'}, {"pciexpress", 0, 0, 'P'}, {"msrs", 0, 0, 'M'}, + {"cpu-range", required_argument, 0, LONG_OPT_RANGE}, {"ambs", 0, 0, 'A'}, {"spi", 0, 0, 's'}, {"spd", 0, 0, 'S'}, @@ -657,6 +663,24 @@ case 'M': dump_coremsrs = 1; break; + case LONG_OPT_RANGE: + if (strlen(optarg) == 0) { + print_usage(argv[0]); + exit(1); + } + const int sscanf_ret = sscanf(optarg, "%u-%u", &cores_range_start, &cores_range_end); + if (sscanf_ret == 1) { + /* the end of the range is not specified - only for one core */ + cores_range_end = cores_range_start; + } else if (sscanf_ret != 2) { + print_usage(argv[0]); + exit(1); + } else if (cores_range_end < cores_range_start) { + printf("Error: invalid cores range <%u-%u>!\n", + cores_range_start, cores_range_end); + exit(1); + } + break; case 'a': dump_gpios = 1; show_gpio_diffs = 1; @@ -859,7 +883,7 @@ }
if (dump_coremsrs) { - print_intel_core_msrs(); + print_intel_msrs(cores_range_start, cores_range_end); printf("\n\n"); }
diff --git a/util/inteltool/inteltool.h b/util/inteltool/inteltool.h index 678aa47..4a6eba4 100644 --- a/util/inteltool/inteltool.h +++ b/util/inteltool/inteltool.h @@ -396,7 +396,7 @@ void unmap_physical(void *virt_addr, size_t len);
unsigned int cpuid(unsigned int op); -int print_intel_core_msrs(void); +int print_intel_msrs(unsigned int range_start, unsigned int range_end); int print_mchbar(struct pci_dev *nb, struct pci_access *pacc, const char *dump_spd_file); int print_pmbase(struct pci_dev *sb, struct pci_access *pacc); int print_lpc(struct pci_dev *sb, struct pci_access *pacc);
-
Angel Pons (Code Review) -
Maxim Polyakov (Code Review)
-
Nico Huber (Code Review)
-
Tim Wawrzynczak (Code Review)