Hi Michal, On Mon, 11 Sept 2023 at 06:32, Michal Simek <michal.si...@amd.com> wrote: > > Current alignment which is using 16 bytes is not correct in connection to > trace_clocks description and it's length. > That's why use start_addr variable and record proper size based on used > entries. > > Fixes: be16fc81b2ed ("trace: Update proftool to use new binary format"). > Signed-off-by: Michal Simek <michal.si...@amd.com> > --- > > tools/proftool.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) >
Reviewed-by: Simon Glass <s...@chromium.org> See my comment below > diff --git a/tools/proftool.c b/tools/proftool.c > index 7c95a94482fc..f79128169e68 100644 > --- a/tools/proftool.c > +++ b/tools/proftool.c > @@ -1493,19 +1493,43 @@ static int write_pages(struct twriter *tw, enum > out_format_t out_format, > static int write_flyrecord(struct twriter *tw, enum out_format_t out_format, > int *missing_countp, int *skip_countp) > { > - unsigned long long start, len; > + unsigned long long start, start_addr, len; > int ret; > FILE *fout = tw->fout; > char str[200]; > > + /* Record start pointer */ > + start_addr = tw->ptr; I'd prefer start_ofs since it is not an address, is it? > + debug("Start of flyrecord header at: 0x%llx\n", start_addr); > + > tw->ptr += fprintf(fout, "flyrecord%c", 0); > > + /* flyrecord\0 - allocated 10 bytes */ > + start_addr += 10; > + > + /* > + * 8 bytes that are a 64-bit word containing the offset into the file > + * that holds the data for the CPU. > + * > + * 8 bytes that are a 64-bit word containing the size of the CPU > + * data at that offset. > + */ > + start_addr += 16; > + > snprintf(str, sizeof(str), > "[local] global counter uptime perf mono mono_raw boot > x86-tsc\n"); > len = strlen(str); > > + /* trace clock length - 8 bytes */ > + start_addr += 8; > + /* trace clock data */ > + start_addr += len; > + > + debug("Calculated flyrecord header end at: 0x%llx, trace clock len: > 0x%llx\n", > + start_addr, len); > + > /* trace data */ > - start = ALIGN(tw->ptr + 16, TRACE_PAGE_SIZE); > + start = ALIGN(start_addr, TRACE_PAGE_SIZE); Would it be possible to store the old tw->ptr value at the top of this function (e.g. in tw_base) and calculate start using (tw->ptr - tw_base)? It seems that there are two parallel trackers here. > tw->ptr += tputq(fout, start); > > /* use a placeholder for the size */ > @@ -1517,6 +1541,9 @@ static int write_flyrecord(struct twriter *tw, enum > out_format_t out_format, > tw->ptr += tputq(fout, len); > tw->ptr += tputs(fout, str); > > + debug("End of flyrecord header at: 0x%x, offset: 0x%llx\n", > + tw->ptr, start); > + > debug("trace text base %lx, map file %lx\n", text_base, text_offset); > > ret = write_pages(tw, out_format, missing_countp, skip_countp); > -- > 2.36.1 > Regards, Simon