On Wed, May 21, 2025 at 11:03 AM James Clark <james.clark@xxxxxxxxxx> wrote:
> On 20/05/2025 11:27 pm, Rob Herring (Arm) wrote:
> > The ARMv9.2 architecture introduces the optional Branch Record Buffer
> > Extension (BRBE), which records information about branches as they are
> > executed into set of branch record registers. BRBE is similar to x86's
> > Last Branch Record (LBR) and PowerPC's Branch History Rolling Buffer
> > (BHRB).
[...]
> > +/*
> > + * BRBE is configured with an OR of permissions from all events, so there may
> > + * be events which have to be dropped or events where just the source or target
> > + * address has to be zeroed.
> > + */
> > +static bool filter_branch_privilege(struct perf_branch_entry *entry, u64 branch_sample_type)
> > +{
> > + /* We can only have a half record if permissions have not been expanded */
> > + if (!entry->from || !entry->to)
> > + return true;
> > +
> > + bool from_user = access_ok((void __user *)(unsigned long)entry->from, 4);
> > + bool to_user = access_ok((void __user *)(unsigned long)entry->to, 4);
> > + bool exclude_kernel = !((branch_sample_type & PERF_SAMPLE_BRANCH_KERNEL) ||
> > + (is_kernel_in_hyp_mode() && (branch_sample_type & PERF_SAMPLE_BRANCH_HV)));
> > +
> > + /*
> > + * If record is within a single exception level, just need to either
> > + * drop or keep the entire record.
> > + */
> > + if (from_user == to_user)
> > + return ((entry->priv == PERF_BR_PRIV_KERNEL) && !exclude_kernel) ||
> > + ((entry->priv == PERF_BR_PRIV_USER) &&
> > + (branch_sample_type & PERF_SAMPLE_BRANCH_USER));
> > +
> > + // Fixup calls which are syscalls
> > + if (entry->type == PERF_BR_CALL && from_user && !to_user)
> > + entry->type = PERF_BR_SYSCALL;
> > +
> > + /*
> > + * Record is across exception levels, mask addresses for the exception
> > + * level we're not capturing.
> > + */
> > + if (!(branch_sample_type & PERF_SAMPLE_BRANCH_USER)) {
> > + if (from_user)
> > + entry->from = 0;
> > + if (to_user)
> > + entry->to = 0;
> > + }
> > +
> > + if (exclude_kernel) {
> > + if (!from_user)
> > + entry->from = 0;
> > + if (!to_user)
> > + entry->to = 0;
> > + }
> > + return true;
> > +}
> > +
> > +static bool filter_branch_type(struct perf_branch_entry *entry,
> > + const unsigned long *event_type_mask)
> > +{
> > + if (entry->type == PERF_BR_EXTEND_ABI)
> > + return test_bit(PERF_BR_MAX + entry->new_type, event_type_mask);
> > + else
> > + return test_bit(entry->type, event_type_mask);
> > +}
> > +
> > +static bool filter_branch_record(struct perf_branch_entry *entry,
> > + u64 branch_sample,
> > + const unsigned long *event_type_mask)
> > +{
> > + return filter_branch_type(entry, event_type_mask) &&
> > + filter_branch_privilege(entry, branch_sample);
>
> filter_branch_privilege() sometimes changes the branch type for
> PERF_BR_SYSCALL so I think it should come before filter_branch_type(). I
> didn't see any actual issue caused by this, but it's a bit hard to
> review to see if it's working correctly.
Looking at this again, I think we can drop that with this change:
diff --git a/drivers/perf/arm_brbe.c b/drivers/perf/arm_brbe.c
index 2f254bd40af3..acdde61a8559 100644
--- a/drivers/perf/arm_brbe.c
+++ b/drivers/perf/arm_brbe.c
@@ -546,7 +546,7 @@ static const int
brbe_type_to_perf_type_map[BRBINFx_EL1_TYPE_DEBUG_EXIT + 1][2]
[BRBINFx_EL1_TYPE_INDIRECT_LINK] = { PERF_BR_IND_CALL, 0 },
[BRBINFx_EL1_TYPE_RET] = { PERF_BR_RET, 0 },
[BRBINFx_EL1_TYPE_DIRECT_COND] = { PERF_BR_COND, 0 },
- [BRBINFx_EL1_TYPE_CALL] = { PERF_BR_CALL, 0 },
+ [BRBINFx_EL1_TYPE_CALL] = { PERF_BR_SYSCALL, 0 },
[BRBINFx_EL1_TYPE_ERET] = { PERF_BR_ERET, 0 },
[BRBINFx_EL1_TYPE_IRQ] = { PERF_BR_IRQ, 0 },
[BRBINFx_EL1_TYPE_TRAP] = { PERF_BR_IRQ, 0 },
AFAICT, the only cases for BRBINFx_EL1_TYPE_CALL are SVC, SMC, and HVC.
Rob
