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
