Re: [PATCH v5 3/4] perf/arm64: Add BRBE support for bpf_get_branch_snapshot()
Puranjay Mohan <[email protected]> Thu, 6 Aug 2026 13:46:10 +0100
| Newsgroups | org.kernel.vger.linux-perf-users,org.infradead.lists.linux-arm-kernel,org.kernel.vger.bpf,org.kernel.vger.linux-kernel,org.kernel.vger.linux-kselftest |
|---|---|
| Message-ID | <CANk7y0ir1w5zsSajcnaR-QYqM_Y0xKNO2MOcNFAWF3x=padt-g@mail.gmail.com> |
On Wed, Aug 5, 2026 at 9:46 PM Puranjay Mohan <[email protected]> wrote: > > On Wed, Aug 5, 2026 at 5:26 PM James Clark <[email protected]> wrote: > > > > > > > > On 05/08/2026 16:44, Puranjay Mohan wrote: > > > On Wed, Aug 5, 2026 at 4:14 PM James Clark <[email protected]> wrote: > > >> > > >> > > >> > > >> On 05/08/2026 15:43, Puranjay Mohan wrote: > > >>> On Wed, Aug 5, 2026 at 2:58 PM James Clark <[email protected]> wrote: > > >>>> > > >>>> > > >>>> > > >>>> On 05/08/2026 12:47, Puranjay Mohan wrote: > > >>>>> On Wed, Aug 5, 2026 at 11:07 AM James Clark <[email protected]> wrote: > > >>>>>> > > >>>>>> > > >>>>>> > > >>>>>> On 03/08/2026 7:54 pm, Puranjay Mohan wrote: > > >>>>>>> On Mon, Aug 3, 2026 at 12:07 PM James Clark <[email protected]> wrote: > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> On 16/06/2026 16:57, Puranjay Mohan wrote: > > >>>>>>>>> Enable bpf_get_branch_snapshot() on ARM64 by implementing the > > >>>>>>>>> perf_snapshot_branch_stack static call for BRBE. > > >>>>>>>>> > > >>>>>>>>> BRBE is paused before masking exceptions to avoid branch buffer > > >>>>>>>>> pollution from trace_hardirqs_off(). Exceptions are then masked with > > >>>>>>>>> local_daif_save() to prevent PMU overflow pseudo-NMIs from interfering. > > >>>>>>>>> If an overflow between pause and DAIF save re-enables BRBE, the snapshot > > >>>>>>>>> detects this via BRBFCR_EL1.PAUSED and bails out. > > >>>>>>>>> > > >>>>>>>>> Branch records are read using perf_entry_from_brbe_regset() with a NULL > > >>>>>>>>> event pointer to bypass event-specific filtering. The buffer is > > >>>>>>>>> invalidated after reading. > > >>>>>>>>> > > >>>>>>>>> Introduce a for_each_brbe_entry() iterator to deduplicate bank > > >>>>>>>>> iteration between brbe_read_filtered_entries() and the snapshot. > > >>>>>>>>> > > >>>>>>>>> Signed-off-by: Puranjay Mohan <[email protected]> > > >>>>>>>>> Reviewed-by: Rob Herring (Arm) <[email protected]> > > >>>>>>>>> --- > > >>>>>>>>> drivers/perf/arm_brbe.c | 128 ++++++++++++++++++++++++++++++++------- > > >>>>>>>>> drivers/perf/arm_brbe.h | 9 +++ > > >>>>>>>>> drivers/perf/arm_pmuv3.c | 5 +- > > >>>>>>>>> 3 files changed, 120 insertions(+), 22 deletions(-) > > >>>>>>>>> > > >>>>>>>>> diff --git a/drivers/perf/arm_brbe.c b/drivers/perf/arm_brbe.c > > >>>>>>>>> index effbdeacfcbb..a141ad7abcf2 100644 > > >>>>>>>>> --- a/drivers/perf/arm_brbe.c > > >>>>>>>>> +++ b/drivers/perf/arm_brbe.c > > >>>>>>>>> @@ -9,6 +9,7 @@ > > >>>>>>>>> #include <linux/types.h> > > >>>>>>>>> #include <linux/bitmap.h> > > >>>>>>>>> #include <linux/perf/arm_pmu.h> > > >>>>>>>>> +#include <asm/daifflags.h> > > >>>>>>>>> #include "arm_brbe.h" > > >>>>>>>>> > > >>>>>>>>> #define BRBFCR_EL1_BRANCH_FILTERS (BRBFCR_EL1_DIRECT | \ > > >>>>>>>>> @@ -256,6 +257,14 @@ static bool valid_brbe_version(int brbe_version) > > >>>>>>>>> brbe_version == ID_AA64DFR0_EL1_BRBE_BRBE_V1P1; > > >>>>>>>>> } > > >>>>>>>>> > > >>>>>>>>> +static __always_inline bool cpu_has_brbe(void) > > >>>>>>>> > > >>>>>>>> This should be more like cpu_valid_brbe_version(). has_brbe() only > > >>>>>>>> implies that the CPU has BRBE, not that it's a version that the driver > > >>>>>>>> supports. And it's actually just a wrapper around valid_brbe_version() > > >>>>>>>> that accesses the ID reg on that CPU, not a functionally different check. > > >>>>>>>> > > >>>>>>>> But it also looks like valid_brbe_version() isn't called from anywhere > > >>>>>>>> else, so why not delete that function and use its name for the new one? > > >>>>>>> > > >>>>>>> I will do that in next version > > >>>>>>> > > >>>>>>>> > > >>>>>>>>> +{ > > >>>>>>>>> + u64 aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1); > > >>>>>>>>> + int brbe = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT); > > >>>>>>>>> + > > >>>>>>>>> + return valid_brbe_version(brbe); > > >>>>>>>>> +} > > >>>>>>>>> + > > >>>>>>>>> static void select_brbe_bank(int bank) > > >>>>>>>>> { > > >>>>>>>>> u64 brbfcr; > > >>>>>>>>> @@ -271,6 +280,20 @@ static void select_brbe_bank(int bank) > > >>>>>>>>> isb(); > > >>>>>>>>> } > > >>>>>>>>> > > >>>>>>>>> +static inline void __brbe_advance(int *bank, int *idx, int nr_hw) > > >>>>>>>>> +{ > > >>>>>>>>> + if (++(*idx) >= BRBE_BANK_MAX_ENTRIES && > > >>>>>>>>> + *bank * BRBE_BANK_MAX_ENTRIES + *idx < nr_hw) { > > >>>>>>>>> + *idx = 0; > > >>>>>>>>> + select_brbe_bank(++(*bank)); > > >>>>>>>>> + } > > >>>>>>>>> +} > > >>>>>>>>> + > > >>>>>>>>> +#define for_each_brbe_entry(idx, nr_hw) \ > > >>>>>>>>> + for (int __bank = (select_brbe_bank(0), 0), idx = 0; \ > > >>>>>>>>> + __bank * BRBE_BANK_MAX_ENTRIES + idx < (nr_hw); \ > > >>>>>>>>> + __brbe_advance(&__bank, &idx, (nr_hw))) > > >>>>>>>>> + > > >>>>>>>>> static bool __read_brbe_regset(struct brbe_regset *entry, int idx) > > >>>>>>>>> { > > >>>>>>>>> entry->brbinf = get_brbinf_reg(idx); > > >>>>>>>>> @@ -474,11 +497,9 @@ unsigned int brbe_num_branch_records(const struct arm_pmu *armpmu) > > >>>>>>>>> > > >>>>>>>>> void brbe_probe(struct arm_pmu *armpmu) > > >>>>>>>>> { > > >>>>>>>>> - u64 brbidr, aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1); > > >>>>>>>>> - u32 brbe; > > >>>>>>>>> + u64 brbidr; > > >>>>>>>>> > > >>>>>>>>> - brbe = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT); > > >>>>>>>>> - if (!valid_brbe_version(brbe)) > > >>>>>>>>> + if (!cpu_has_brbe()) > > >>>>>>>>> return; > > >>>>>>>>> > > >>>>>>>>> brbidr = read_sysreg_s(SYS_BRBIDR0_EL1); > > >>>>>>>>> @@ -618,10 +639,10 @@ static bool perf_entry_from_brbe_regset(int index, struct perf_branch_entry *ent > > >>>>>>>>> > > >>>>>>>>> brbe_set_perf_entry_type(entry, brbinf); > > >>>>>>>>> > > >>>>>>>>> - if (!branch_sample_no_cycles(event)) > > >>>>>>>>> + if (!event || !branch_sample_no_cycles(event)) > > >>>>>>>>> entry->cycles = brbinf_get_cycles(brbinf); > > >>>>>>>>> > > >>>>>>>>> - if (!branch_sample_no_flags(event)) { > > >>>>>>>>> + if (!event || !branch_sample_no_flags(event)) { > > >>>>>>>>> /* Mispredict info is available for source only and complete branch records. */ > > >>>>>>>>> if (!brbe_record_is_target_only(brbinf)) { > > >>>>>>>>> entry->mispred = brbinf_get_mispredict(brbinf); > > >>>>>>>>> @@ -774,32 +795,97 @@ void brbe_read_filtered_entries(struct perf_branch_stack *branch_stack, > > >>>>>>>>> { > > >>>>>>>>> struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > > >>>>>>>>> int nr_hw = brbe_num_branch_records(cpu_pmu); > > >>>>>>>>> - int nr_banks = DIV_ROUND_UP(nr_hw, BRBE_BANK_MAX_ENTRIES); > > >>>>>>>>> int nr_filtered = 0; > > >>>>>>>>> u64 branch_sample_type = event->attr.branch_sample_type; > > >>>>>>>>> DECLARE_BITMAP(event_type_mask, PERF_BR_ARM64_MAX); > > >>>>>>>>> > > >>>>>>>>> prepare_event_branch_type_mask(branch_sample_type, event_type_mask); > > >>>>>>>>> > > >>>>>>>>> - for (int bank = 0; bank < nr_banks; bank++) { > > >>>>>>>>> - int nr_remaining = nr_hw - (bank * BRBE_BANK_MAX_ENTRIES); > > >>>>>>>>> - int nr_this_bank = min(nr_remaining, BRBE_BANK_MAX_ENTRIES); > > >>>>>>>>> + for_each_brbe_entry(i, nr_hw) { > > >>>>>>>>> + struct perf_branch_entry *pbe = &branch_stack->entries[nr_filtered]; > > >>>>>>>>> > > >>>>>>>>> - select_brbe_bank(bank); > > >>>>>>>>> + if (!perf_entry_from_brbe_regset(i, pbe, event)) > > >>>>>>>>> + break; > > >>>>>>>>> > > >>>>>>>>> - for (int i = 0; i < nr_this_bank; i++) { > > >>>>>>>>> - struct perf_branch_entry *pbe = &branch_stack->entries[nr_filtered]; > > >>>>>>>>> + if (!filter_branch_record(pbe, branch_sample_type, event_type_mask)) > > >>>>>>>>> + continue; > > >>>>>>>>> > > >>>>>>>>> - if (!perf_entry_from_brbe_regset(i, pbe, event)) > > >>>>>>>>> - goto done; > > >>>>>>>>> + nr_filtered++; > > >>>>>>>>> + } > > >>>>>>>>> > > >>>>>>>>> - if (!filter_branch_record(pbe, branch_sample_type, event_type_mask)) > > >>>>>>>>> - continue; > > >>>>>>>>> + branch_stack->nr = nr_filtered; > > >>>>>>>>> +} > > >>>>>>>>> > > >>>>>>>>> - nr_filtered++; > > >>>>>>>>> - } > > >>>>>>>>> +/* > > >>>>>>>>> + * Best-effort BRBE snapshot for BPF tracing. Pause BRBE to avoid > > >>>>>>>>> + * self-recording and return 0 if the snapshot state appears disturbed. > > >>>>>>>>> + */ > > >>>>>>>>> +int arm_brbe_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt) > > >>>>>>>>> +{ > > >>>>>>>>> + unsigned long flags; > > >>>>>>>>> + int nr_hw, nr_copied = 0; > > >>>>>>>>> + u64 brbfcr, brbcr; > > >>>>>>>>> + > > >>>>>>>>> + if (!cnt) > > >>>>>>>>> + return 0; > > >>>>>>>> > > >>>>>>>> If you're trying to avoid branches before pausing BRBE, can't you check > > >>>>>>>> this after the pause? > > >>>>>>> > > >>>>>>> will drop this in the next version and this is just an optimization. > > >>>>>>> > > >>>>>>>> > > >>>>>>>>> + > > >>>>>>>>> + /* Guard against running on a CPU without BRBE (e.g. big.LITTLE). */ > > >>>>>>>>> + if (!cpu_has_brbe()) > > >>>>>>>>> + return 0; > > >>>>>>>>> + > > >>>>>>>>> + /* > > >>>>>>>>> + * Pause BRBE first to avoid recording our own branches. The > > >>>>>>>>> + * sysreg read/write and ISB are branchless, so pausing before > > >>>>>>>>> + * checking BRBCR avoids polluting the buffer with our own > > >>>>>>>>> + * conditional branches. > > >>>>>>>>> + */ > > >>>>>>>>> + brbfcr = read_sysreg_s(SYS_BRBFCR_EL1); > > >>>>>>>>> + brbcr = read_sysreg_s(SYS_BRBCR_EL1); > > >>>>>>>>> + write_sysreg_s(brbfcr | BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1); > > >>>>>>>> > > >>>>>>>> Can this work without first disabling interrupts? Sashiko pointed it > > >>>>>>>> out, but I think it's correct. If you read an active state into brbfcr, > > >>>>>>>> then the PMU event fires and disables BRBE, then you disable interrupts, > > >>>>>>>> then you would restore an active state when it should be inactive. > > >>>>>>>> Surely the only way to do it properly is to disable interrupts before > > >>>>>>>> touching anything at all, if the PMU handler is also touching the same > > >>>>>>>> registers? > > >>>>>>>> > > >>>>>>>> If you want to avoid trace_hardirqs_off() can you make a new > > >>>>>>>> raw_local_daif_save() that disables interrupts and then call > > >>>>>>>> trace_hardirqs_off() yourself after pausing BRBE? Or not call > > >>>>>>>> trace_hardirqs_off() at all? There is a comment mentioning something > > >>>>>>>> like that in arch/arm64/kernel/suspend.c. > > >>>>>>> > > >>>>>>> Yes. I'll add raw_local_daif_save()/raw_local_daif_restore() and mask before > > >>>>>>> touching any BRBE register, which fixes the stale restore. trace_hardirqs_off() > > >>>>>>> then moves below the pause so lockdep still sees a balanced off/on pair while > > >>>>>>> its branches land in an already paused buffer. > > >>>>>>> > > >>>>>>>> > > >>>>>>>> Also, disabling interrupts doesn't stop the PMU event from overflowing > > >>>>>>>> and changing the state of PAUSED either. I think this is another path > > >>>>>>>> that leads to you restoring the wrong state, so don't you also need to > > >>>>>>>> disable the PMU? > > >>>>>>> > > >>>>>>> Hardware only sets PAUSED on a BRBE freeze event (RBHYTD), and a freeze needs > > >>>>>>> BRBE to not already be paused (RNXCWF). So once we have paused, nothing changes > > >>>>>>> underneath us and the PMU does not need disabling. It would not help anyway: > > >>>>>>> armv8pmu_stop() calls brbe_disable(), which zeroes BRBCR_EL1 and discards the > > >>>>>>> records we came to read. > > >>>>>> > > >>>>>> That does mean you throw away real freeze events while paused though, in > > >>>>>> addition to the brbe_invalidate() you need to avoid non contiguous > > >>>>>> buffers. So it takes branches away from PMU events. > > >>>>> > > >>>>> RBHYTD gives a freeze two effects: PAUSED is set, and BRBTS_EL1 captures a > > >>>>> timestamp. Recording has already stopped because we paused, and the driver never > > >>>>> reads BRBTS_EL1. On the way out we check PMOVSCLR_EL0 and leave PAUSED set if a > > >>>>> counter overflowed, so a pending overflow handler still finds a frozen buffer. > > >>>>> > > >>>>>> So it takes branches away from PMU events. > > >>>>> > > >>>>> Yes, but that is brbe_invalidate(), not the pause. Interrupts are masked and > > >>>>> nothing else runs on the CPU, so the only branches the pause suppresses are the > > >>>>> snapshot's own. > > >>>>> > > >>>>>>> > > >>>>>>> You are right that a freeze can still land in the window between reading BRBFCR > > >>>>>>> and setting PAUSED, and restoring the value we read would then clear a PAUSED > > >>>>>>> bit the hardware set. So v6 checks PMOVSCLR_EL0 and leaves BRBE paused if a > > >>>>>>> counter has overflowed. Reads of PMOVSCLR are non-destructive and it stays set > > >>>>>>> until the overflow handler clears it, so it is still visible after we have set > > >>>>>>> PAUSED ourselves: > > >>>>>>> > > >>>>>>> if (!valid_brbe_version()) > > >>>>>>> return 0; > > >>>>>>> > > >>>>>>> flags = raw_local_daif_save(); > > >>>>>>> > > >>>>>>> brbfcr = read_sysreg_s(SYS_BRBFCR_EL1); > > >>>>>>> brbcr = read_sysreg_s(SYS_BRBCR_EL1); > > >>>>>>> > > >>>>>>> write_sysreg_s(brbfcr | BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1); > > >>>>>>> isb(); > > >>>>>>> > > >>>>>>> trace_hardirqs_off(); > > >>>>>>> > > >>>>>>> /* BRBCR_EL1 is zero while the driver has BRBE disabled. */ > > >>>>>>> if (!brbcr) > > >>>>>>> goto restore; > > >>>>>>> > > >>>>>>> ... read the records ... > > >>>>>>> > > >>>>>> > > >>>>>> Up to the point where you read the records you technically don't need > > >>>>>> any branches (if you don't do trace_hardirqs_off()) so you could do the > > >>>>>> whole thing without pausing. Just disable interrupts, read every branch > > >>>>>> entry unconditionally, re-enable interrupts and then find the last valid > > >>>>>> entry and do the branchy stuff after reading. > > >>>>>> > > >>>>>> I'm thinking out loud, but doesn't that make it a lot easier? And it > > >>>>>> avoids the brbe_invalidate() which takes the branches away from the PMU > > >>>>>> event. It also avoids having to think too hard about racing with PMU > > >>>>>> events causing a freeze even after interrupts are disabled and after > > >>>>>> reading the freeze value: > > >>>>>> > > >>>>>> raw_local_daif_save(); > > >>>>>> brbfcr = read_sysreg_s(SYS_BRBFCR_EL1); > > >>>>>> brbcr = read_sysreg_s(SYS_BRBCR_EL1); > > >>>>>> select_bank(0); > > >>>>>> read_record(0); > > >>>>>> read_record(1); > > >>>>>> ... > > >>>>>> select_bank(0); > > >>>>>> read_record(0); > > >>>>>> read_record(1); > > >>>>>> ... > > >>>>>> write_sysreg_s(brbfcr, SYS_BRBFCR_EL1); > > >>>>>> isb(); > > >>>>>> local_daif_restore(); > > >>>>>> > > >>>>>> /* Now do post processing, find last valid record, check if it was > > >>>>>> enabled by looking at brbcr etc. */ > > >>>>> > > >>>>> MRS is not a branch, so that works, but three things would have to change: > > >>>>> > > >>>>> 1. perf_entry_from_brbe_regset() goes through BRBE_REGN_SWITCH, a 32 case > > >>>>> switch, because the register number has to be an immediate. gcc emits a jump > > >>>>> table and the function has 52 branches in the object file. The read would > > >>>>> need full unrolling. > > >>>>> > > >>>> > > >>>> Yes you would have to manually unroll it. I doubt the compiler would > > >>>> emit a branch for BRBE_REGN_SWITCH() because your indexes are static if > > >>>> it's unrolled. But if it does you can change it to a sequence of > > >>>> read_sysreg_s()s. > > >>>> > > >>>> If you really want to be sure there are no branches, write it in a > > >>>> single asm block. > > >>> > > >>> I will try that approach in the next version. > > >>> > > >>>>> 2. RPGDLX needs an ISB before the reads, and Table D19-10 makes it > > >>>>> IMPLEMENTATION DEFINED whether ISB itself generates a record. When we pause, > > >>>>> IZCHRF means the pausing ISB cannot pollute the buffer it is about to read. > > >>>>> > > >>>> > > >>>> I assume one potential extra record from the isb() is acceptible seeing > > >>>> as you already have two or more conditions plus a function call before > > >>>> the pause? Is the problem that you don't know whether to filter it out > > >>>> later because it's IMPDEF and isn't always there? You already don't know > > >>>> exactly how many branches there will be before the pause beause it's > > >>>> written in C. So I'm not sure what the exact issue here is. > > >>>> > > >>>>> 3. 64 record parts still need a bank switch, so BRBFCR_EL1 still has to be > > >>>>> written and restored. > > >>>>> > > >>>> > > >>>> Yes that was included in my pseudo code example but it's still > > >>>> branchless. I did miss that you might need an isb() after disabling > > >>>> interrupts, but you added one for RPGDLX anyway. > > >>>> > > >>>>> Correctness would then depend on the read staying branchless, which is not > > >>>>> checkable at build time and fails silently: a branch mid read shifts the buffer, > > >>>> > > >>>> Why would the compiler insert a branch between two read_sysreg_s()s, > > >>>> which are asm volatile? Is that allowed? > > >>> > > >>> You are right, I just over complicated it! > > >>> > > >>>>> giving a duplicate and a gap. 64 * 24 bytes of records also wants a per-CPU > > >>>>> scratch buffer rather than the stack. > > >>>>> > > >>>> > > >>>> A per-CPU scratch buffer doesn't sound too bad. But aren't you in > > >>>> control of how many entries are available to write to? You can reject > > >>>> any calls that have fewer than 64 and always write directly to *entries. > > >>>> > > >>>> You could also compare with 'cnt' after reading each record and exit the > > >>>> read section. Like you say below, branches not taken don't generate > > >>>> records, and once the branch is taken you stop reading so after that > > >>>> point generating records doesn't matter. > > >>>> > > >>>>> Happy to prototype it. What I would not do is pause without invalidating: > > >>>>> records are from/to pairs, so a consumer walking across the hole reconstructs a > > >>>>> call path that never happened. The invalidate was Mark's request after the RFC, > > >>>>> "to maintain record contiguity for other consumers", so dropping the pause drops > > >>>>> that too. > > >>>> > > >>>> Well the point was to not have to pause at all, so there's no need to > > >>>> invalidate either. Even if you did add a pause, as long as there are no > > >>>> branches between the pause and resume you don't need an invalidate > > >>>> because you didn't miss any branches. > > >>>> > > >>>>> > > >>>>> I was thinking of this for v6: > > >>>>> > > >>>>> flags = raw_local_daif_save(); > > >>>>> > > >>>>> /* The BRBE sysregs below are UNDEFINED without this. */ > > >>>>> if (!valid_brbe_version()) { > > >>>> > > >>>> You don't need to disable interrupts to call this, it's a constant. Or > > >>>> is it to stop migration? > > >>> > > >>> It was to stop migration. > > >>> > > >> > > >> Ah ok, so V5 wasn't correct then. > > > > > > Yes, It was buggy! I missed that one CPU could implement BRBE while > > > another doesn't > > > > > >> > > >> Separately to this, I'm also not sure how this BPF call is invoked. Is > > >> it supposed to target a CPU, or an event or a process? How do you know > > >> you are getting the branches from the CPU that you want? > > > > > > So, A BPF program can attach to a function's entry through ftrace or > > > kprobe and from there we call this helper to find out what branches > > > were taken to reach that function. > > > A BPF program runs on a CPU with migration always disabled, so it > > > > In that case you can check for BRBE support before disabling interrupts. > > > > So I think V5 wasn't buggy and you can carry on doing > > valid_brbe_version() at the same place in V6. > > > > > knows which cpu it is getting the branches from. And It can also > > > filter for a process as it knows which process is called the BPF > > > program, let's say a process does a syscall and we attach a bpf > > > program to it as the example I gave with the retsnoop tool in the > > > cover letter's Usage model section. A BPF program can also attach to a > > > tracepoint, or even to a perf event and this BPF helper can be called > > > from any of those contexts, but the filtering based on cpu or pid or > > > anything else is done by the BPF program itself, this helper should > > > just return the raw branch records. > > > > > > > Thanks for the explanation. > > > > >>>> > > >>>> V5 reads sysregs before disabling interrupts so I assume migration isn't > > >>>> an issue here. > > >>>> > > >>>>> raw_local_daif_restore(flags); > > >>>>> return 0; > > >>>>> } > > >>>>> > > >>>>> brbfcr = read_sysreg_s(SYS_BRBFCR_EL1); > > >>>>> brbcr = read_sysreg_s(SYS_BRBCR_EL1); > > >>>>> > > >>>>> write_sysreg_s(brbfcr | BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1); > > >>>>> isb(); > > >>>>> > > >>>>> /* BRBCR_EL1 is zero while the driver has BRBE disabled. */ > > >>>>> if (!brbcr) { > > >>>>> write_sysreg_s(brbfcr, SYS_BRBFCR_EL1); > > >>>>> isb(); > > >>>>> raw_local_daif_restore(flags); > > >>>>> return 0; > > >>>>> } > > >>>>> > > >>>>> trace_hardirqs_off(); > > >>>>> > > >>>>> ... read the records ... > > >>>>> > > >>>>> if (!(brbfcr & BRBFCR_EL1_PAUSED) && > > >>>>> !(read_pmovsclr() & (ARMV8_PMU_OVSR_P | ARMV8_PMU_OVSR_F))) > > >>>>> brbe_invalidate(); > > >>>>> else > > >>>>> brbfcr |= BRBFCR_EL1_PAUSED; > > >>>>> > > >>>>> write_sysreg_s(brbfcr, SYS_BRBFCR_EL1); > > >>>>> isb(); > > >>>>> local_daif_restore(flags); > > >>>>> > > >>>>> Exceptions are masked before anything is sampled, BRBCR_EL1 is only read and > > >>>>> never written, and BRBFCR_EL1 is written back from the value saved under the > > >>>>> mask. cpu_has_brbe() is now valid_brbe_version(). > > >>>>> > > >>>>> Neither conditional costs a record: valid_brbe_version() falls through when BRBE > > >>>>> is present (RBBNSZ, only taken branches are recorded), and the BRBCR_EL1 test is > > >>>> > > >>>> Isn't the compiler free to invert it and make the happy path a taken > > >>>> branch. I don't think any of that can be assumed without writing in > > >>>> assembly. > > >>>> > > >>>>> after the pause. It cannot move later because BRBFCR_EL1 is UNDEFINED without > > >>>>> FEAT_BRBE. > > >>>>> > > >>>>> PMOVSCLR_EL0 is masked because PMCCNTR_EL0 is bit 31 and PMCR_EL0.N is at most > > >>>>> 31, so the cycle counter is outside every range RNXCWF, RGXGWY, RPKTXQ and > > >>>>> RLDMVK name. Unmasked, a cycle counter overflow looked like a freeze. > > >>>> > > >>>> Sorry I didn't understand this bit. Can you elaborate? > > >>>> > > >>>>> > > >>>>> Do you think this version works? > > >>>>> > > >>>>> Thanks, > > >>>>> Puranjay > > >>>> > > >>>> > > >>>> From your previous reply: > > >>>> > > >>>> "Hardware only sets PAUSED on a BRBE freeze event (RBHYTD), and a > > >>>> freeze needs BRBE to not already be paused (RNXCWF). So once we have > > >>>> paused, nothing changes underneath us and the PMU does not need > > >>>> disabling." > > >>>> > > >>>> I'm still not 100% convinced this is correct. You read BRBFCR_EL1 and > > >>>> write PAUSED to it in separate instructions. The PMU can overflow and > > >>>> change the PAUSED state between those two instructions leading to > > >>>> restoration of the wrong value. > > >>> > > >>> I was checking if the overflow happened and leaving it paused in that case. > > >>> > > >> > > >> But couldn't it overflow after that check? I still think it's vulnerable > > >> to the same issue unless you disable the PMU completely. > > > > > > yeah you are right, I missed that it can overflow after the check. > > > > > >> > > >>>> Isn't this non pausing version way simpler to understand, and also has > > >>>> the benefit of not invalidating someone elses BRBE buffers. The only > > >>>> downside seems to be that it might have some assembly to make sure the > > >>>> if statements are always not taken branches rather than inverted, but > > >>>> personally I don't think that makes it any harder to understand than the > > >>>> branchy pausing version in V5: > > >>>> > > >>>> #define read_record(i) > > >>>> if (i >= cnt) \ > > >>>> goto out; \ > > >>>> isb(); /* Ensure our own exit branch isn't read? */ \ > > >>>> entries[i].inf = read_brbe_inf(i) \ > > >>>> if (!inf) \ > > >>>> goto out; \ > > >>>> entries[i].src = read_brb_src(i) \ > > >>>> entries[i].dst = read_brb_dst(i) \ > > >>>> > > >>>> raw_local_daif_save(); > > >>>> > > >>>> /* disable counters to stop BRBFCR_EL1.PAUSE state changing */ > > >>>> pmcr = armv8pmu_pmcr_read(); > > >>>> armv8pmu_pmcr_write(pmcr & ~ARMV8_PMU_PMCR_E); > > >>>> isb(); > > >>>> > > >>>> brbfcr = read_sysreg_s(SYS_BRBFCR_EL1); > > >>>> brbcr = read_sysreg_s(SYS_BRBCR_EL1); > > >>>> > > >>>> select_bank(0); > > >>>> read_record(0); > > >>>> read_record(1); > > >>>> ... > > >>>> select_bank(1); > > >>>> read_record(32); > > >>>> read_record(33); > > >>>> ... > > >>>> > > >>>> out: > > >>>> write_sysreg_s(brbfcr, SYS_BRBFCR_EL1); > > >>>> isb(); > > >>>> armv8pmu_pmcr_write(pmcr); > > >>>> raw_local_daif_restore(); > > >>>> > > >>>> /* Post process entries[n].inf etc into correct format */ > > >>> > > >>> I will try out this approach and get back. > > Hi James, I tried this approach without the PAUSE but there is a > problem, the hardware that I tested on creates a record for isb() and > when reading the banks, we need to do an isb() after bank selection > and it's recorded, so it shifts every record's index by one between > the two bank reads, leaving the two halves stitched from buffer states > one shift apart. So, I think we have to pause but with your suggestion about disabling counters there is no race left: int brbe_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt) { u64 brbidr, brbfcr, brbcr, pmcr; int nr_hw, nr_copied = 0; unsigned long flags; /* * Other BRBE sysreg accesses are UNDEFINED without this. The caller * runs with migration disabled, so this is the CPU they are read on. */ if (!valid_brbe_version()) return 0; /* * brbe_enable()/brbe_disable() run from the overflow interrupt and * from event add/remove IPIs, so mask before sampling any state. */ flags = raw_local_daif_save(); /* * BRBFCR_EL1 is read here and written back below, and a BRBE freeze * event in between would set BRBFCR_EL1.PAUSED behind that. Stopping * the counters keeps PMOVSCLR_EL0 from gaining a bit, which is what * every freeze condition in Arm ARM (DDI 0487 M.a) D19.3 requires. */ pmcr = read_pmcr(); write_pmcr(pmcr & ~ARMV8_PMU_PMCR_E); isb(); brbcr = read_sysreg_s(SYS_BRBCR_EL1); brbfcr = read_sysreg_s(SYS_BRBFCR_EL1); write_sysreg_s(brbfcr | BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1); isb(); trace_hardirqs_off(); /* * The pause has taken effect: the branches below generate no records, * and every D19.3 freeze condition also requires that generation is * not paused, so the counters can be started again. */ write_pmcr(pmcr); /* Records outlive brbe_disable(), so report nothing while BRBE is off. */ if (brbcr) { brbidr = read_sysreg_s(SYS_BRBIDR0_EL1); nr_hw = min_t(int, FIELD_GET(BRBIDR0_EL1_NUMREC_MASK, brbidr), BRBIDR0_EL1_NUMREC_64); for_each_brbe_entry(i, nr_hw) { if (nr_copied >= cnt) break; if (!perf_entry_from_brbe_regset(i, &entries[nr_copied], NULL)) break; nr_copied++; } } /* * Branches were missed while paused, so discard the buffer rather * than leave a hole in it. BRBE that arrived frozen stopped before * this ran, so its records are still contiguous and belong to * whoever froze it. */ if (!(brbfcr & BRBFCR_EL1_PAUSED)) brbe_invalidate(); write_sysreg_s(brbfcr, SYS_BRBFCR_EL1); isb(); local_daif_restore(flags); return nr_copied; } What do you think about this version? Is this acceptable?