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

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Feb 15, 2022 at 01:01:06PM -0600, Rob Herring wrote:
> 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?

I agree for arm64 specifically this probably doesn't matter; I just wanted to
have a clear explanation of why this *could* be a problem, since this could
affect other architectures.

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

Yup; something of that shape sounds good to me -- that was roughly what I had
suggested elsewhere.

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

I agree; this wasn't clear to me, and regardless I think many of the types
added in the prior patch should not be generic since they're very specific to
the Arm architecture.

> > (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'. 

I agree if you moved the entire field that's broken everywhere, but in this
case it *is* directly related to the size changing. In my example the meaning
of specific bits changed *because* the size of the field changed and in BE that
meant the low bits of the field moved, even though the field started at the
same position.

> 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?

IIRC there were recent problems in this area, and I think historically we've
broken ABI and people only noticed much later.

Thanks,
Mark.