Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 15 Feb 2022 19:45:21 +0000 | From | Mark Rutland <> | Subject | Re: [PATCH 2/2] perf: Expand perf_branch_entry.type |
| |
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.
| ![\](/images/icornerr.gif) |