linux-kernel - Re: [PATCH v22 5/5] perf: arm_pmuv3: Add support for the Branch Record Buffer Extension (BRBE)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAL_JsqJ3t5UvWMBgLEkm_-sD_S7NEor+vWfVkU-d7va6u9xsmQ@mail.gmail.com>
Date: Thu, 29 May 2025 12:27:18 -0500
From: Rob Herring <robh@...nel.org>
To: James Clark <james.clark@...aro.org>
Cc: linux-arm-kernel@...ts.infradead.org, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, 
	kvmarm@...ts.linux.dev, Will Deacon <will@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, Catalin Marinas <catalin.marinas@....com>, 
	Jonathan Corbet <corbet@....net>, Marc Zyngier <maz@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>, 
	Joey Gouly <joey.gouly@....com>, Suzuki K Poulose <suzuki.poulose@....com>, 
	Zenghui Yu <yuzenghui@...wei.com>, Anshuman Khandual <anshuman.khandual@....com>, 
	Leo Yan <leo.yan@....com>
Subject: Re: [PATCH v22 5/5] perf: arm_pmuv3: Add support for the Branch
 Record Buffer Extension (BRBE)

On Wed, May 21, 2025 at 11:03 AM James Clark <james.clark@...aro.org> 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