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

[Michal Simek <michal.simek@amd.com>]

On 9/12/23 21:26, Simon Glass wrote:
> Hi Michal,
> 
> On Mon, 11 Sept 2023 at 06:32, Michal Simek <michal.simek@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@amd.com>
>> ---
>>
>>   tools/proftool.c | 31 +++++++++++++++++++++++++++++--
>>   1 file changed, 29 insertions(+), 2 deletions(-)
>>
> 
> Reviewed-by: Simon Glass <sjg@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?

make sense.

> 
>> +       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.

I didn't actual dig into that reasons why tw->ptr are used with all that tputq 
logics around.

I was thinking to use minus but because there is trace_clock description on this 
line you are not able to use tw->ptr because it is also changed below. It means 
I decided to do calculation ahead with calculating all bytes which will be 
described here.

Thanks,
Michal