From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 47971C55174 for ; Wed, 5 Aug 2026 16:27:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=daeifs2uc9oo7Dw8xYA7KxbaF/u580/TXeKl51imcRI=; b=kjMpC3a/qfzZH3gpDkkXfBNxNh UavGUv5E4cFjutefGuO9QCxP0lj7NXS+6CBnBD1SZ6NOIzAyAIyib2gG6e9gW6ZLVE90aXE2R12bg HbKSh35aEywGlQqYOtIrzBj3uAS/duUmT+gfiI8e+WWvKExQGrsTGyIZIvVTFxAmV5rYQ8EUiqjTI eRlrYMzOoJpeorU9rr3ywijh0TuGtGrQB6xWujx6c05Kg6no5FwFGWCuAkXqoc/4n2DsmMc0cSRcm ALcSJRHPqQj3/J7lIbf9+Kcmi/OZIEPlUEF84JvZM1dyB9ArPZD5i1j3Z/jjSYaTO+W3dAqjsZsK2 Lxg1i4Lw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1wreSL-00000004HBs-1gM1; Wed, 05 Aug 2026 16:27:01 +0000 Received: from mail-wr1-x42d.google.com ([2a00:1450:4864:20::42d]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1wreSI-00000004HAt-1YBh for linux-arm-kernel@lists.infradead.org; Wed, 05 Aug 2026 16:27:00 +0000 Received: by mail-wr1-x42d.google.com with SMTP id ffacd0b85a97d-47f84023916so1056091f8f.3 for ; Wed, 05 Aug 2026 09:26:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1785947216; x=1786552016; darn=lists.infradead.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=daeifs2uc9oo7Dw8xYA7KxbaF/u580/TXeKl51imcRI=; b=tzPOcpmXjWpzossYGgwgCEl98Q2XBuEvoCS/Q97wa2yychIzmsTN96tvdVAQGJsu9h vepEoKJ/hBganfVdXMlq2e5H/8BsmqucjqZaNSwEaOi6JA08QDTK1B/3uIvKyfuX/7c1 WgUth+VPpntYUvjOWNTEx/q8NGtQc2YwakVZOhhypflLT8i9uQmQRlaF4/jOyBS3wjYO nx4JP0UzMlATzH+libzQxGWQ/q0D1xr7IdDpOhNIWlY7JJVdXPiGSg30yAyY/fg+bLEd CBiEXTIUIJwXvprSgCDzEa0b8LsWbJzZniabZvpWyGqDbTwtsoQKHwan6ZHU8tFm/PV+ o68g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1785947216; x=1786552016; 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=daeifs2uc9oo7Dw8xYA7KxbaF/u580/TXeKl51imcRI=; b=G0K7sLPJCMYnZcezmKhYFm6AOOSlrhVzvyauSIwXIEdKX6j28NXqzbrIXPmlS34x+g 4KpslZGb6LqxV8AdShOLNla17+gVmhxtGuL0flop+idK53AgpYWPLBU5tQ2QVGlXsouo TbmED0v5QnShlDQc79PuuJwtwJNvcPvG3C0BqUbH+q7nhW4WGQB0OjJibo7kz2qS9XoN Dft5+h0xWCmiYEJuY158ZBGBI+iGtn/be/XsLEZRU6FzaNDQinQPif2wk29m6exmR0+M syubPH3Vth9Ytia7G17tzatSAjFMRK3vMu7XzpI7Ho3Wv7ILgQhxO5h4pCgyuMnQ8Vit 4bUg== X-Forwarded-Encrypted: i=1; AHgh+RoS3L58y/5mDFGenJAhZeJfHeYrpu5998fFGdtZ1bye5I6vrFwfruarj4Uwqf0hXj99/we57HLYtfnrZR5GRghd@lists.infradead.org X-Gm-Message-State: AOJu0YzXZ42n91TkakzVyn2iZ4mJwG2Y0pjyHeFzJ/WeVjY67zfyEdr0 4BzdqugFRwsEG5DkDUanocbdO+yJVAa3zN+tPH3B9h/5gbzI3i1TPklvrh6PcMGUQWI= X-Gm-Gg: AR+sD10yJME8+RYZ64/q22eg+8Qb/aF8ephyLr93S5lPLKFfYZgxb7wfOa6j6eRaebA f9JOvGC6zrkQlLPcEUph6tpbYLtIqEV1kad+2suBZSz8al66xVl9ubEPE/ulJbDDSKqnYQbAsP3 SQvXUqp5DN6FuuTc+XoitOxUfo2m/lu0RnNMjD15ZtuUx34l4fKZHerZ4U9nQBtH07vLp7Hj+30 3KO2cFQqidE6AsKtR/8gogCv+RKGJJR+M4Ljys73sBo9vIbMN79T6QIjWkv8jYKazgHiZamphfW UuAo14iMjGJBkxXvGlFHKvf7tZVTLJTdNdQlTfnhdWRFMVu9SIRcGLWSkNo3Im1YIkz+QYAbgxq yuyV48uNOJMieA6T0MCv3rUK8mZXTOWIjyt1yzfDvUqdUXQabRSN3KaoT0U96A5WiLLzp0ziZGU dEWosTDDBz+i7JeCW0dGSdjGlkPSL322Cq6QAToPLh1MsrY38Y/t7vXu2Klc2YAcuTAw== X-Received: by 2002:a05:6000:2c0d:b0:47f:6f9e:1e82 with SMTP id ffacd0b85a97d-47fec4f11cbmr14253267f8f.9.1785947215811; Wed, 05 Aug 2026 09:26:55 -0700 (PDT) Received: from [192.168.1.3] ([37.18.141.193]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-47febfdd383sm9334155f8f.8.2026.08.05.09.26.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 05 Aug 2026 09:26:55 -0700 (PDT) Message-ID: <2b402e4b-b916-4dba-bcb3-634ad58f1710@linaro.org> Date: Wed, 5 Aug 2026 17:26:53 +0100 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 Cc: bpf@vger.kernel.org, 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> <8a24abbc-10bc-42bb-8fee-0a174d8f1751@linaro.org> <6ee32efc-01be-4532-aa59-3948f59156c0@linaro.org> <9fa68534-2b12-420a-93b7-a6ada21c180e@linaro.org> Content-Language: en-US From: James Clark In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.9.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20260805_092658_607584_C4A536F3 X-CRM114-Status: GOOD ( 27.96 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 05/08/2026 16:44, Puranjay Mohan wrote: > On Wed, Aug 5, 2026 at 4:14 PM James Clark wrote: >> >> >> >> On 05/08/2026 15:43, Puranjay Mohan wrote: >>> On Wed, Aug 5, 2026 at 2:58 PM James Clark wrote: >>>> >>>> >>>> >>>> On 05/08/2026 12:47, Puranjay Mohan wrote: >>>>> On Wed, Aug 5, 2026 at 11:07 AM James Clark wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 03/08/2026 7:54 pm, Puranjay Mohan wrote: >>>>>>> On Mon, Aug 3, 2026 at 12:07 PM James Clark 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 >>>>>>>>> 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? >>>>>>> >>>>>>> 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. >>> >>> Thanks, >>> Puranjay >> >> Nice, hope it can work. >>