Re: [PATCH 2/2] perf: Expand perf_branch_entry.type

[Rob Herring <robh@kernel.org>]

On Fri, Feb 04, 2022 at 04:02:04PM +0000, Mark Rutland wrote:
> On Fri, Feb 04, 2022 at 10:25:24AM +0530, Anshuman Khandual wrote:
> > On 2/2/22 5:27 PM, Mark Rutland wrote:
> > > On Fri, Jan 28, 2022 at 11:14:13AM +0530, Anshuman Khandual wrote:
> > >> @@ -1370,8 +1376,8 @@ struct perf_branch_entry {
> > >>  		in_tx:1,    /* in transaction */
> > >>  		abort:1,    /* transaction abort */
> > >>  		cycles:16,  /* cycle count to last branch */
> > >> -		type:4,     /* branch type */
> > >> -		reserved:40;
> > >> +		type:6,     /* branch type */
> > > 
> > > As above, is this a safe-change ABI-wise?
> > 
> > If the bit fields here cannot be expanded without breaking ABI, then
> > there is a fundamental problem. Only remaining option will be to add
> > new fields (with new width value) which could accommodate these new
> > required branch types.
> 
> Unfortunately, I think expanding this does break ABI, and is a fundamental
> problem, as:
> 
> (a) Any new values in the expanded field will be truncated when read by old
>     userspace, and so those may be mis-reported. Maybe we're not too worried
>     about this case.

'type' or specfically branch stack is not currently supported on arm64. 
Do we expect an old userspace which this didn't work on to start working 
with a new kernel?

Given at least some of the new types are arch specific, perhaps 
the existing type field should get a new 'PERF_BR_ARCH_SPECIFIC' or 
'PERF_BR_EXTENDED' value (or use PERF_BR_UNKNOWN?) which means read a 
new 'arch_type' field.

Another option is maybe some of these additional types just shouldn't be 
exposed to userspace? For example, are branches to FIQ useful or leaking 
any info about secure world? Debug mode branches also seem minimally 
useful to me (though I'm no expert in how this is used).


> (b) Depending on how the field is placed, existing values might get stored
>     differently. This could break any mismatched combination of
>     {old,new}-kernel and {old,new}-userspace.
> 
>     In practice, I think this means that this is broken for BE, and happens to
>     work for LE, but I don't know how bitfields are defined for each
>     architecture, so there could be other brokenness.
> 
> Consider the test case below:

[...]

> ... where the low bits of the field have moved, and so this is broken even for
> existing values!

So that is a separate issue to be fixed and not directly related to the 
size of 'type'. Looks like it needs similar '#if 
defined(__LITTLE_ENDIAN_BITFIELD)' treatment as some of the other struct 
bitfields. Though somehow BE PPC hasn't had issues?

Rob