perf: Fix interpretation of branch records

  Commit 93315e46b000 ("perf/core: Add speculation info to branch
entries") added a new field in between type and new_type. Perf has
its own copy of this struct so update it to match the kernel side.

This doesn't currently cause any issues because new_type is only used
by the Arm BRBE driver which isn't merged yet.

Fixes: 93315e46b000 ("perf/core: Add speculation info to branch entries")
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/branch.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

On Wed, Nov 30, 2022 at 8:52 AM James Clark <james.clark@arm.com> wrote:
>
> Commit 93315e46b000 ("perf/core: Add speculation info to branch
> entries") added a new field in between type and new_type. Perf has
> its own copy of this struct so update it to match the kernel side.
>
> This doesn't currently cause any issues because new_type is only used
> by the Arm BRBE driver which isn't merged yet.
>
> Fixes: 93315e46b000 ("perf/core: Add speculation info to branch entries")
> Signed-off-by: James Clark <james.clark@arm.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> ---
>  tools/perf/util/branch.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/branch.h b/tools/perf/util/branch.h
> index d6017c9b1872..3ed792db1125 100644
> --- a/tools/perf/util/branch.h
> +++ b/tools/perf/util/branch.h
> @@ -22,9 +22,10 @@ struct branch_flags {
>                         u64 abort:1;
>                         u64 cycles:16;
>                         u64 type:4;
> +                       u64 spec:2;
>                         u64 new_type:4;
>                         u64 priv:3;
> -                       u64 reserved:33;
> +                       u64 reserved:31;
>                 };
>         };
>  };
> --
> 2.25.1
>
  

On 11/30/22 22:21, James Clark wrote:
> Commit 93315e46b000 ("perf/core: Add speculation info to branch
> entries") added a new field in between type and new_type. Perf has
> its own copy of this struct so update it to match the kernel side.
> 
> This doesn't currently cause any issues because new_type is only used
> by the Arm BRBE driver which isn't merged yet.
> 
> Fixes: 93315e46b000 ("perf/core: Add speculation info to branch entries")
> Signed-off-by: James Clark <james.clark@arm.com>

Again, problem from having the same structure in two different places

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

> ---
>  tools/perf/util/branch.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/branch.h b/tools/perf/util/branch.h
> index d6017c9b1872..3ed792db1125 100644
> --- a/tools/perf/util/branch.h
> +++ b/tools/perf/util/branch.h
> @@ -22,9 +22,10 @@ struct branch_flags {
>  			u64 abort:1;
>  			u64 cycles:16;
>  			u64 type:4;
> +			u64 spec:2;
>  			u64 new_type:4;
>  			u64 priv:3;
> -			u64 reserved:33;
> +			u64 reserved:31;
>  		};
>  	};
>  };
  

On 11/30/2022 10:21 PM, James Clark wrote:
> Commit 93315e46b000 ("perf/core: Add speculation info to branch
> entries") added a new field in between type and new_type. Perf has
> its own copy of this struct so update it to match the kernel side.
> 
> This doesn't currently cause any issues because new_type is only used
> by the Arm BRBE driver which isn't merged yet.
> 
> Fixes: 93315e46b000 ("perf/core: Add speculation info to branch entries")

Technically, in the kernel sources, commit 93315e46b000 ("perf/core: Add
speculation info to branch entries") landed before commit b190bc4ac9e6
("perf: Extend branch type classification").

So I think the Fixes tag should instead have commit 0ddea8e2a0c2
("perf branch: Extend branch type classification") which added the
UAPI changes to the perf tool headers.

Aside from that, the patch looks good to me.

> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/util/branch.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/branch.h b/tools/perf/util/branch.h
> index d6017c9b1872..3ed792db1125 100644
> --- a/tools/perf/util/branch.h
> +++ b/tools/perf/util/branch.h
> @@ -22,9 +22,10 @@ struct branch_flags {
>  			u64 abort:1;
>  			u64 cycles:16;
>  			u64 type:4;
> +			u64 spec:2;
>  			u64 new_type:4;
>  			u64 priv:3;
> -			u64 reserved:33;
> +			u64 reserved:31;
>  		};
>  	};
>  };
  

On 01/12/2022 09:02, Sandipan Das wrote:
> On 11/30/2022 10:21 PM, James Clark wrote:
>> Commit 93315e46b000 ("perf/core: Add speculation info to branch
>> entries") added a new field in between type and new_type. Perf has
>> its own copy of this struct so update it to match the kernel side.
>>
>> This doesn't currently cause any issues because new_type is only used
>> by the Arm BRBE driver which isn't merged yet.
>>
>> Fixes: 93315e46b000 ("perf/core: Add speculation info to branch entries")
> 
> Technically, in the kernel sources, commit 93315e46b000 ("perf/core: Add
> speculation info to branch entries") landed before commit b190bc4ac9e6
> ("perf: Extend branch type classification").
> 
> So I think the Fixes tag should instead have commit 0ddea8e2a0c2
> ("perf branch: Extend branch type classification") which added the
> UAPI changes to the perf tool headers.

Yes maybe, or 831c05a7621b ("tools headers UAPI: Sync linux/perf_event.h
with the kernel sources") where spec was added to the perf copy of the
headers.

It's hard to say for sure which one is best. I think that the earliest
possible one is best in case anyone is debugging that kernel version
with userspace Perf. And to me it seems any kernel that has the spec
record should have it matching in userspace so I would still prefer
93315e46b000. Applying it on top of any other change would still lead to
misleading interpretation of the records.

> 
> Aside from that, the patch looks good to me.

Thanks for the review.

> 
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>  tools/perf/util/branch.h | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/branch.h b/tools/perf/util/branch.h
>> index d6017c9b1872..3ed792db1125 100644
>> --- a/tools/perf/util/branch.h
>> +++ b/tools/perf/util/branch.h
>> @@ -22,9 +22,10 @@ struct branch_flags {
>>  			u64 abort:1;
>>  			u64 cycles:16;
>>  			u64 type:4;
>> +			u64 spec:2;
>>  			u64 new_type:4;
>>  			u64 priv:3;
>> -			u64 reserved:33;
>> +			u64 reserved:31;
>>  		};
>>  	};
>>  };
> 
>
  

Em Thu, Dec 01, 2022 at 09:46:53AM +0530, Anshuman Khandual escreveu:
> 
> 
> On 11/30/22 22:21, James Clark wrote:
> > Commit 93315e46b000 ("perf/core: Add speculation info to branch
> > entries") added a new field in between type and new_type. Perf has
> > its own copy of this struct so update it to match the kernel side.
> > 
> > This doesn't currently cause any issues because new_type is only used
> > by the Arm BRBE driver which isn't merged yet.
> > 
> > Fixes: 93315e46b000 ("perf/core: Add speculation info to branch entries")
> > Signed-off-by: James Clark <james.clark@arm.com>
> 
> Again, problem from having the same structure in two different places

Indeed, this was my first reaction, how about backward compatibility, is
this really an ABI?

- Arnaldo
 
> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
 
> > ---
> >  tools/perf/util/branch.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/branch.h b/tools/perf/util/branch.h
> > index d6017c9b1872..3ed792db1125 100644
> > --- a/tools/perf/util/branch.h
> > +++ b/tools/perf/util/branch.h
> > @@ -22,9 +22,10 @@ struct branch_flags {
> >  			u64 abort:1;
> >  			u64 cycles:16;
> >  			u64 type:4;
> > +			u64 spec:2;
> >  			u64 new_type:4;
> >  			u64 priv:3;
> > -			u64 reserved:33;
> > +			u64 reserved:31;
> >  		};
> >  	};
> >  };