Stefan Reinauer has uploaded this change for review. ( https://review.coreboot.org/c/em100/+/48729 )
Change subject: Add SPI trace map support ......................................................................
Add SPI trace map support
with -M you can specify a map file that will be used to print which section of the flash your SPI accesses go to. The format of the map file is simple:
Start (hex) End (hex) Length (hex) Area Name ----------- --------- ------------ --------- 00000000 01FFFFFF 02000000 Full Flash Image 00000000 00000FFF 00001000 Descriptor Region 00000014 00000017 00000004 FLMAP0 - Flash Map 0 Register 00000018 0000001B 00000004 FLMAP1 - Flash Map 1 Register [..] 00500000 01FFFFFF 01B00000 BIOS Region 00500000 00cfffff 00800000 RW_SECTION_A 00500000 0050ffff 00010000 VBLOCK_A 00510000 009fffbf 004effc0 FW_MAIN_A 009fffc0 009fffff 00000040 RW_FWID_A [..] 01805000 01874fff 00070000 GBB 01875000 01ffffff 0078b000 COREBOOT
Right now the section is only printed in brief mode.
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Change-Id: Ide04eb620ad213f9b8457bec7cd57daef6b5c671 --- M Makefile M em100.c M em100.h A map.c M trace.c 5 files changed, 166 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/29/48729/1
diff --git a/Makefile b/Makefile index 1248780..075518d 100644 --- a/Makefile +++ b/Makefile @@ -38,7 +38,7 @@
XZ = xz/xz_crc32.c xz/xz_crc64.c xz/xz_dec_bcj.c xz/xz_dec_lzma2.c xz/xz_dec_stream.c SOURCES = em100.c firmware.c fpga.c hexdump.c sdram.c spi.c system.c trace.c usb.c -SOURCES += image.c curl.c chips.c tar.c $(XZ) +SOURCES += map.c image.c curl.c chips.c tar.c $(XZ) OBJECTS = $(SOURCES:.c=.o)
all: dep em100 diff --git a/em100.c b/em100.c index 7a6f910..d7afc34 100644 --- a/em100.c +++ b/em100.c @@ -791,6 +791,7 @@ {"traceconsole", 0, 0, 'R'}, {"length", 1, 0, 'L'}, {"brief", 0, 0, 'b'}, + {"map-file", 1, 0, 'M'}, {"compatible", 0, 0, 'C'}, {NULL, 0, 0, 0} }; @@ -814,6 +815,7 @@ " -R|--traceconsole: trace console mode\n" " -L|--length HEX_VAL: length of buffer for traceconsole mode\n" " -b|--brief: brief mode for traces\n" + " -M|--map-file: Map file for trace mode\n" " -F|--firmware-update FILE|auto: update EM100pro firmware (dangerous)\n" " -f|--firmware-dump FILE: export raw EM100pro firmware to file\n" " -g|--firmware-write FILE: export EM100pro firmware to DPFW file\n" @@ -836,6 +838,7 @@ const char *desiredchip = NULL; const char *serialno = NULL; const char *filename = NULL, *read_filename = NULL; + const char *trace_mapfile = NULL; const char *firmware_in = NULL, *firmware_out = NULL; const char *holdpin = NULL; int do_start = 0, do_stop = 0; @@ -850,7 +853,7 @@ struct sigaction signal_action; struct em100 *em100 = &em100_state;
- while ((opt = getopt_long(argc, argv, "c:d:a:m:u:rsvtO:F:f:g:S:V:p:DCx:lUhTRL:b", + while ((opt = getopt_long(argc, argv, "c:d:a:m:u:rsvtO:F:f:g:S:V:p:DCx:lUhTRL:M:b", longopts, &idx)) != -1) { switch (opt) { case 'c': @@ -935,6 +938,13 @@ case 'b': trace_brief = 1; break; + case 'M': + if(access(optarg, F_OK ) == 0) + trace_mapfile = optarg; + else + printf("Can't open '%s'.\n", optarg); + break; + case 'C': compatibility = 1; break; @@ -1234,6 +1244,10 @@ if (!do_start && !do_stop) set_state(em100, 1);
+ if(trace && trace_mapfile) { + load_trace_mapfile(trace_mapfile); + } + printf ("Starting ");
if (trace || traceconsole) { diff --git a/em100.h b/em100.h index 4ba78a9..21d4324 100644 --- a/em100.h +++ b/em100.h @@ -176,6 +176,10 @@ int read_spi_trace_console(struct em100 *em100, unsigned long addr_offset, unsigned long addr_len);
+/* map.c */ +int load_trace_mapfile(const char *trace_mapfile); +void print_mapping(uint64_t addr, int full); + /* Archive handling */ typedef struct { unsigned char *address; diff --git a/map.c b/map.c new file mode 100644 index 0000000..5e1651c --- /dev/null +++ b/map.c @@ -0,0 +1,139 @@ +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <ctype.h> +#include "em100.h" + +#define MAX_LINE 256 + +//#define NEED_PREV + +typedef struct map_entry { + struct map_entry *next; +#ifdef NEED_PREV + struct map_entry *prev; +#endif + uint64_t start; + uint64_t end; + uint64_t len; + char name[MAX_LINE]; +} map_entry_t; + +static map_entry_t *map_entries = NULL; +static map_entry_t *map_entries_top = NULL; + +int load_trace_mapfile(const char *trace_mapfile) +{ + FILE *file; + char current_line[MAX_LINE]; + + file = fopen(trace_mapfile, "r"); + if (!file) { + perror(trace_mapfile); + return -1; + } + + while (fgets(current_line, sizeof(current_line), file) != NULL) { + unsigned long start, end, length; + const char d[] = " \t"; + char *num; + + //printf("%03ld: %s", strlen(current_line), current_line); + num = strtok(current_line, d); + if (!num) continue; + start = strtol(num, NULL, 16); + num = strtok(NULL, d); + if (!num) continue; + end = strtol(num, NULL, 16); + num = strtok(NULL, d); + if (!num) continue; + length = strtol(num, NULL, 16); + num = strtok(NULL, "\r\n"); + if (!num) continue; + while (isspace(*num)) num++; + + if ((start + length - 1 ) != end) { + continue; + } + + map_entry_t *tmp = malloc(sizeof(map_entry_t)); + if (!tmp) + continue; + + tmp->start = start; + tmp->end=end; + tmp->len=length; + strncpy(tmp->name, num, MAX_LINE - 1); + + if(!map_entries) + map_entries = tmp; + +#ifdef NEED_PREV + tmp->prev = map_entries_top; +#endif + tmp->next = NULL; + + if (map_entries_top) + map_entries_top->next=tmp; + map_entries_top = tmp; + } + +#ifdef TEST + map_entry_t *m; +#ifdef NEED_PREV + printf("Backwards\n"); + for (m=map_entries_top; m; m=m->prev) + printf("[%08lx][%08lx][%08lx]:[%s]\n", m->start, m->end, m->len, m->name); +#endif + + printf("Forwards\n"); + for (m=map_entries; m; m=m->next) + printf("[%08lx][%08lx][%08lx]:[%s]\n", m->start, m->end, m->len, m->name); +#endif + + fclose(file); + return 0; +} + +void print_mapping(uint64_t addr, int full) +{ + /* The whole purpose of strncat is to not copy the + * whole string. Apparently GCC lost its mind warning + * about that + */ +#if (__GNUC__ > 8) +# pragma GCC diagnostic push +# pragma GCC diagnostic ignored "-Wstringop-truncation" +#endif + char path[MAX_LINE] = "\0"; + + map_entry_t *m, *match = NULL; + for (m=map_entries; m; m=m->next) { + if (addr >= m->start && addr <= m->end) { + /* First one is "Flash Image", ignore that */ + if(match && full) { + int max_len = MAX_LINE - strlen(path) - 5; + strncat(path, " ‣ ", 5); + strncat(path, m->name, max_len); + } + match=m; + } + } + + if (!full && match) + strncat(path, match->name, MAX_LINE - 2); + + if(strlen(path) == 0) + strncat(path, "No match", MAX_LINE - 1); +#if (__GNUC__ > 8) +# pragma GCC diagnostic pop +#endif + +#ifdef TEST + printf("[%lx]", addr); +#endif + printf("%s", path); +#ifdef TEST + printf("\n"); +#endif +} diff --git a/trace.c b/trace.c index c04afcc..e1f065f 100644 --- a/trace.c +++ b/trace.c @@ -288,10 +288,14 @@ if (start_timestamp) start_timestamp = 0;
- if (spi_cmd_vals->address_type != ADDR_NONE) - printf("0x%02x @ 0x%08lx (%s)\n", + if (spi_cmd_vals->address_type != ADDR_NONE) { + printf("0x%02x @ 0x%08lx (%s)", spi_command, address, spi_cmd_vals->cmd_name); - else + /* No section parsing for SFDP commands */ + if (spi_cmd_vals->address_type != ADDR_NO_OFF_3B) + print_mapping(address, 1); + printf("\n"); + } else printf("0x%02x (%s)\n", spi_command, spi_cmd_vals->cmd_name); } else {
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/em100/+/48729
to look at the new patch set (#2).
Change subject: Add SPI trace map support ......................................................................
Add SPI trace map support
with -M you can specify a map file that will be used to print which section of the flash your SPI accesses go to. The format of the map file is simple:
Start (hex) End (hex) Length (hex) Area Name ----------- --------- ------------ --------- 00000000 01FFFFFF 02000000 Full Flash Image 00000000 00000FFF 00001000 Descriptor Region 00000014 00000017 00000004 FLMAP0 - Flash Map 0 Register 00000018 0000001B 00000004 FLMAP1 - Flash Map 1 Register [..] 00500000 01FFFFFF 01B00000 BIOS Region 00500000 00cfffff 00800000 RW_SECTION_A 00500000 0050ffff 00010000 VBLOCK_A 00510000 009fffbf 004effc0 FW_MAIN_A 009fffc0 009fffff 00000040 RW_FWID_A [..] 01805000 01874fff 00070000 GBB 01875000 01ffffff 0078b000 COREBOOT
Right now the section is only printed in brief mode.
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Change-Id: Ide04eb620ad213f9b8457bec7cd57daef6b5c671 --- M Makefile M em100.c M em100.h A map.c M trace.c 5 files changed, 146 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/29/48729/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/em100/+/48729 )
Change subject: Add SPI trace map support ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/em100/+/48729/2/em100.c File em100.c:
https://review.coreboot.org/c/em100/+/48729/2/em100.c@1247 PS2, Line 1247: ( nit: space before (
https://review.coreboot.org/c/em100/+/48729/2/map.c File map.c:
https://review.coreboot.org/c/em100/+/48729/2/map.c@33 PS2, Line 33: load_trace_mapfile We can get a little fancy with this using a structure that stores pointers to child and parent and organize this like a tree. That will have a few benefits: * Order in the map file is not important. * Duplicate entries can be caught and handled. * No need to have special logic for handling first entry as being "FLASH" - Keep dropping levels from the top of the tree if the tree has a single node. This will eliminate any redundant parents in the beginning or anywhere in the map file. * Reduces search space for each transaction.
Parent pointer is not strictly required, but can be helpful if we have to traverse back up.
https://review.coreboot.org/c/em100/+/48729/2/map.c@89 PS2, Line 89: void Would it make sense to update this function to also combine multiple occurrences of an operation to the same region into a single print? Basically, doing the same thing that the go tool is doing. It can also be done based on a command line arg if required.
It probably might be easier to handle this at the caller rather than within this function i.e. requires more helper functions for the map file.
1. void *map_get_matching_entry(uint64_t addr); 2. void map_get_path(void *entry, const char *str, size_t len); 3. bool map_entry_has_addr(void *entry, uint64_t addr);
The caller can then save what operation is being performed and map entry. If the next operation is the same as last one and going to the same entry, then it can just club the address together and print out a single entry. Something like:
SI_BIOS > UNIFIED_MRC_CACHE > RW_MRC_CACHE: 0xbb (dual I/O read): 0x01868c00.........0x01868d00
We can keep printing '.' for each operation to the same region and finally if the next operation is to a different region or after a certain timeout (~2-3 seconds), print out the last address that was accessed in the region.
And with the command line option we can control if the addresses should be combined together or a new line should be printed for each transaction.
https://review.coreboot.org/c/em100/+/48729/2/map.c@92 PS2, Line 92: GCC decided that it is a good idea to warn : * if strncpy is used to potentially only copy a substring. Is it complaining for: strncat(path, " ‣ ", 5);
-
Furquan Shaikh (Code Review) -
Stefan Reinauer (Code Review)