From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f43.google.com (mail-wm1-f43.google.com [209.85.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 16DAB3E0090 for ; Mon, 3 Aug 2026 11:07:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.43 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785755230; cv=none; b=goA+QJlNVfBLxXChthOZiwrUtwFpuIh7Mi5Rtpzbin8ymw9//34TgyN0vCApFq0R/JyYWQtc3FuUxa+pEbGtCS+mbOvYXCKRBKevhK24muSaS0Vw1m7BGSmQFCXd7e9+YmU0CKqZn+dRbfveb23ZNSEINUHAmf0I9qRww3isoj0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785755230; c=relaxed/simple; bh=VBR60XG9y2Uk6RCxjNGiPAYXZH8PpzPBAAF3gDrOcTg=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=TFf9LWVw6nrrZDqP569FLp7/cvjIwBw/fCFFagd9pm8RY2GelzwlM7/RZ5RUZYqO3BDFXXKpCVomhkjkQyAPFIHF0CqTAg+Nk7/jybSUaTqH9tJ+YHSUr6Z5lZm+MXg4yi03f26V9gfbH077EfjhHVmWFWO6lEcetqlS92VSLaE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=yC2Z7kv7; arc=none smtp.client-ip=209.85.128.43 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="yC2Z7kv7" Received: by mail-wm1-f43.google.com with SMTP id 5b1f17b1804b1-495437bb891so15902535e9.1 for ; Mon, 03 Aug 2026 04:07:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1785755225; x=1786360025; darn=vger.kernel.org; h=content-transfer-encoding:content-type:in-reply-to:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:from:to:cc:subject:date:message-id:reply-to :content-type; bh=uSQmwTgmqqEI3Rld3bh1K2CmFtffeneeKX7l2+wWsvo=; b=yC2Z7kv76wwImAZjnRBOfwMNxmXiH1AoJioPafFIjVUqPIWoMWvQEwvbOvtBgf+COc hPRVg1eQiNA0EaNX5KKEFNloORTOQuxqy8+ql46WljgcKwdLbmWpkxp/7zgAxXnLxOH6 IBcLiq9sCVBnszjZ4JpF2wO6epou6/DvgNNWX4GsLsL74kUVqNfSyqHB5BVRskqysmWb VT/+L/9RnxQCAPOhxA3IPDqfas6ghdqsoByvp2x8K0nQa+hqfiUSOvPQB1E9xP4QZUak fb9jySgDlUItQjT2XEBMIvb83fZ7hCQBSr9gkaMYLQ0uFfJsRqx5QwzsPmTqXsGEWemi vFNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1785755225; x=1786360025; h=content-transfer-encoding:content-type:in-reply-to:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to:content-type; bh=uSQmwTgmqqEI3Rld3bh1K2CmFtffeneeKX7l2+wWsvo=; b=Q7fH1VuXRnXadvSblTIyDtAJnsCXff34RL7yuyzehjqCVmofSY8zfXpzAECYzxEnBb JsWHhoSKlXCMllZhRYYR95wD8iidNUIupatFCfKYTTmKdlre+g54MRSv5hFDLwPvdRyq jYhiU0Mz2BjUUh1Jeaa/kxXGyMihp+SW7xlL2TnM2cSprc5EDFYjC9Ix+o0tZBXJoeD7 EBQ/PcUVA8coiH/ch0CSNr7X2YpqST7QqST5Dfdt8CQZooy940uqYPIq2BR2Qul+gvgG 5z07gzcyB9F+E6Zi29BSUXVW+NZWyKzlLo2Pe0n2yDepFJQ+oHme/JKZmUE5PzQvGsse qqOA== X-Forwarded-Encrypted: i=1; AHgh+RprsDf8fta1zGVi6eLLlu2eFizt+LQQd+DPQlZz5EOcSckiGjcxzXMDJH1Cu7ZJtXopnDxZ8xVAwolTo+M=@vger.kernel.org X-Gm-Message-State: AOJu0Yzt8JaT9MymcDzSliTM1vbDoY6/0wFs4UVAUbh1HXTfmLnkaNTY ygqBQF/S7XzoWAtDlzewKCIlO74udo1KyWC70+v51YUnOaOpaUfGaKMd9mntRyK8lsc= X-Gm-Gg: AR+sD10VyG3g2oJti9wKE0oY3UWmo+M5TSlVp1gKSlDE94owF8KHnECyxBtCAG46dBs FIJV8TbLt2uOiLod3bCe6nDaRZ/qH8B7mWtz6g0QjkdacuwuyGsmZdn8WVRuRd7IFBmEZKzLGxL K/2TdqmlVEQTfL4h9bfr7KQ83/ruBRay5oHBKQe/DwwPT5iSvLXzahJVluKr/J/qPEJqpHf6joa 3WoSYjAOrDTzGciJ3tuVtZOyCJ6pb4rjRpb+AO9OukPhd1S5Pqii9ywBKTz4IIt1ifiihIii5NL AvQB8ALAls5tPr+IKWEqRWxVC82YhAkL3dvV/I4XhEf/eRjC+k9h5lFK9CGHW5AbkF4Ko5r1VBV fvT/lZas/4W/tVeYFyuNXgK/FcUjis1PC5pTnBspRN4YOxcW1DD44SmxfVaynTK6UJEtV6dF7nq m5WDkgyeGZB6pRlPA4Fw45BzVpBGHVLmf/BGFvlGDmeshxr1TY41IhyULzuUBAj0qrGoM= X-Received: by 2002:a05:600c:5249:b0:495:3a52:71b1 with SMTP id 5b1f17b1804b1-4980eb9b2eemr139526335e9.5.1785755224975; Mon, 03 Aug 2026 04:07:04 -0700 (PDT) Received: from [192.168.1.183] ([37.18.141.193]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4980878dbaesm375587785e9.12.2026.08.03.04.07.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 03 Aug 2026 04:07:04 -0700 (PDT) Message-ID: <8a24abbc-10bc-42bb-8fee-0a174d8f1751@linaro.org> Date: Mon, 3 Aug 2026 12:07:02 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 3/4] perf/arm64: Add BRBE support for bpf_get_branch_snapshot() To: Puranjay Mohan , bpf@vger.kernel.org Cc: Puranjay Mohan , Alexei Starovoitov , Daniel Borkmann , John Fastabend , Andrii Nakryiko , Martin KaFai Lau , Eduard Zingerman , Song Liu , Yonghong Song , Will Deacon , Mark Rutland , Catalin Marinas , Leo Yan , Rob Herring , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Namhyung Kim , Ian Rogers , Adrian Hunter , Shuah Khan , Breno Leitao , Ravi Bangoria , Stephane Eranian , Kumar Kartikeya Dwivedi , Usama Arif , linux-arm-kernel@lists.infradead.org, linux-perf-users@vger.kernel.org, linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@meta.com References: <20260616155716.2631508-1-puranjay@kernel.org> <20260616155716.2631508-4-puranjay@kernel.org> Content-Language: en-US From: James Clark In-Reply-To: <20260616155716.2631508-4-puranjay@kernel.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 > Reviewed-by: Rob Herring (Arm) > --- > 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 > #include > #include > +#include > #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? > +{ > + 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? > + > + /* 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. 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? > + isb(); > + > + /* Bail out if BRBE is not enabled (BRBCR_EL1 == 0). */ > + if (!brbcr) { > + write_sysreg_s(brbfcr, SYS_BRBFCR_EL1); > + isb(); > + return 0; > } > > -done: > - branch_stack->nr = nr_filtered; > + /* Block local exception delivery while reading the buffer. */ > + flags = local_daif_save(); > + > + /* > + * A PMU overflow before local_daif_save() could have re-enabled > + * BRBE, clearing the PAUSED bit. The overflow handler already > + * restored BRBE to its correct state, so just bail out. > + */ > + if (!(read_sysreg_s(SYS_BRBFCR_EL1) & BRBFCR_EL1_PAUSED)) { > + local_daif_restore(flags); > + return 0; > + } > + > + nr_hw = FIELD_GET(BRBIDR0_EL1_NUMREC_MASK, > + read_sysreg_s(SYS_BRBIDR0_EL1)); > + > + 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++; > + } > + > + brbe_invalidate(); > + > + /* Restore BRBCR before unpausing via BRBFCR, matching brbe_enable(). */ > + write_sysreg_s(brbcr, SYS_BRBCR_EL1); > + isb(); > + write_sysreg_s(brbfcr, SYS_BRBFCR_EL1); > + /* Ensure BRBE is unpaused before returning to the caller. */ > + isb(); > + local_daif_restore(flags); > + > + return nr_copied; > } > diff --git a/drivers/perf/arm_brbe.h b/drivers/perf/arm_brbe.h > index b7c7d8796c86..c2a1824437fb 100644 > --- a/drivers/perf/arm_brbe.h > +++ b/drivers/perf/arm_brbe.h > @@ -10,6 +10,7 @@ > struct arm_pmu; > struct perf_branch_stack; > struct perf_event; > +struct perf_branch_entry; > > #ifdef CONFIG_ARM64_BRBE > void brbe_probe(struct arm_pmu *arm_pmu); > @@ -22,6 +23,8 @@ void brbe_disable(void); > bool brbe_branch_attr_valid(struct perf_event *event); > void brbe_read_filtered_entries(struct perf_branch_stack *branch_stack, > const struct perf_event *event); > +int arm_brbe_snapshot_branch_stack(struct perf_branch_entry *entries, > + unsigned int cnt); > #else > static inline void brbe_probe(struct arm_pmu *arm_pmu) { } > static inline unsigned int brbe_num_branch_records(const struct arm_pmu *armpmu) > @@ -44,4 +47,10 @@ static void brbe_read_filtered_entries(struct perf_branch_stack *branch_stack, > const struct perf_event *event) > { > } > + > +static inline int arm_brbe_snapshot_branch_stack(struct perf_branch_entry *entries, > + unsigned int cnt) > +{ > + return 0; > +} > #endif > diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c > index 8014ff766cff..1a9f129a0f94 100644 > --- a/drivers/perf/arm_pmuv3.c > +++ b/drivers/perf/arm_pmuv3.c > @@ -1449,8 +1449,11 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name, > cpu_pmu->set_event_filter = armv8pmu_set_event_filter; > > cpu_pmu->pmu.event_idx = armv8pmu_user_event_idx; > - if (brbe_num_branch_records(cpu_pmu)) > + if (brbe_num_branch_records(cpu_pmu)) { > cpu_pmu->pmu.sched_task = armv8pmu_sched_task; > + static_call_update(perf_snapshot_branch_stack, > + arm_brbe_snapshot_branch_stack); > + } > > cpu_pmu->name = name; > cpu_pmu->map_event = map_event;