[PATCH 3/3] trace: Fix alignment logic in flyrecord header

[Simon Glass]

Hi Michal,

On Mon, 11 Sept 2023 at 06:32, Michal Simek <michal.simek at 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.simek at amd.com>
> ---
>
>  tools/proftool.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
>

Reviewed-by: Simon Glass <sjg at 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