Re: [PATCH v3 4/5] perf tool ibs: Sync amd ibs header file

[Ravi Bangoria]

Hi Ian,

On 20-May-22 5:18 AM, Ian Rogers wrote:
> On Wed, May 18, 2022 at 10:45 PM Ravi Bangoria <ravi.bangoria@xxxxxxx> wrote:
>>
>> IBS support has been enhanced with two new features in upcoming uarch:
>> 1. DataSrc extension and 2. L3 miss filtering. Additional set of bits
>> has been introduced in IBS registers to exploit these features.
>> New bits are already defining in arch/x86/ header. Sync it with tools
>> header file.
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@xxxxxxx>
>> ---
>>  tools/arch/x86/include/asm/amd-ibs.h | 16 ++++++++++------
>>  tools/perf/util/amd-sample-raw.c     |  4 ++--
>>  2 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/arch/x86/include/asm/amd-ibs.h b/tools/arch/x86/include/asm/amd-ibs.h
>> index 765e9e752d03..9a3312e12e2e 100644
>> --- a/tools/arch/x86/include/asm/amd-ibs.h
>> +++ b/tools/arch/x86/include/asm/amd-ibs.h
>> @@ -29,7 +29,10 @@ union ibs_fetch_ctl {
>>                         rand_en:1,      /* 57: random tagging enable */
>>                         fetch_l2_miss:1,/* 58: L2 miss for sampled fetch
>>                                          *      (needs IbsFetchComp) */
>> -                       reserved:5;     /* 59-63: reserved */
>> +                       l3_miss_only:1, /* 59: Collect L3 miss samples only */
>> +                       fetch_oc_miss:1,/* 60: Op cache miss for the sampled fetch */
>> +                       fetch_l3_miss:1,/* 61: L3 cache miss for the sampled fetch */
>> +                       reserved:2;     /* 62-63: reserved */
>>         };
>>  };
>>
>> @@ -38,14 +41,14 @@ union ibs_op_ctl {
>>         __u64 val;
>>         struct {
>>                 __u64   opmaxcnt:16,    /* 0-15: periodic op max. count */
>> -                       reserved0:1,    /* 16: reserved */
>> +                       l3_miss_only:1, /* 16: Collect L3 miss samples only */
>>                         op_en:1,        /* 17: op sampling enable */
>>                         op_val:1,       /* 18: op sample valid */
>>                         cnt_ctl:1,      /* 19: periodic op counter control */
>>                         opmaxcnt_ext:7, /* 20-26: upper 7 bits of periodic op maximum count */
>> -                       reserved1:5,    /* 27-31: reserved */
>> +                       reserved0:5,    /* 27-31: reserved */
>>                         opcurcnt:27,    /* 32-58: periodic op counter current count */
>> -                       reserved2:5;    /* 59-63: reserved */
>> +                       reserved1:5;    /* 59-63: reserved */
>>         };
>>  };
>>
>> @@ -71,11 +74,12 @@ union ibs_op_data {
>>  union ibs_op_data2 {
>>         __u64 val;
>>         struct {
>> -               __u64   data_src:3,     /* 0-2: data source */
>> +               __u64   data_src_lo:3,  /* 0-2: data source low */
>>                         reserved0:1,    /* 3: reserved */
>>                         rmt_node:1,     /* 4: destination node */
>>                         cache_hit_st:1, /* 5: cache hit state */
>> -                       reserved1:57;   /* 5-63: reserved */
>> +                       data_src_hi:2,  /* 6-7: data source high */
>> +                       reserved1:56;   /* 8-63: reserved */
>>         };
>>  };
>>
>> diff --git a/tools/perf/util/amd-sample-raw.c b/tools/perf/util/amd-sample-raw.c
>> index d19d765195c5..3b623ea6ee7e 100644
>> --- a/tools/perf/util/amd-sample-raw.c
>> +++ b/tools/perf/util/amd-sample-raw.c
> 
> nit: given the commit message this should probably be a separate patch.

Renaming of an existing field in only header file creates a build issue.
So I had to include it in this patch. Would documenting it in the patch
description help?

  Also rename existing ibs_op_data field "data_src" to "data_src_lo".

Thanks for the review,
Ravi

> 
> Thanks,
> Ian
> 
>> @@ -98,9 +98,9 @@ static void pr_ibs_op_data2(union ibs_op_data2 reg)
>>         };
>>
>>         printf("ibs_op_data2:\t%016llx %sRmtNode %d%s\n", reg.val,
>> -              reg.data_src == 2 ? (reg.cache_hit_st ? "CacheHitSt 1=O-State "
>> +              reg.data_src_lo == 2 ? (reg.cache_hit_st ? "CacheHitSt 1=O-State "
>>                                                      : "CacheHitSt 0=M-state ") : "",
>> -              reg.rmt_node, data_src_str[reg.data_src]);
>> +              reg.rmt_node, data_src_str[reg.data_src_lo]);
>>  }
>>
>>  static void pr_ibs_op_data3(union ibs_op_data3 reg)
>> --
>> 2.27.0
>>