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 AEDB2C30658 for ; Tue, 2 Jul 2024 16:20:16 +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:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=qZzJwoboFasF6mEm2sMsg9dcasHEHWBjJtX5h0onBNI=; b=SSc+1T7DH5FTRi9DbxWRvsaob2 IQbmKDtKXNg0at5StOiliTNasmYJHf23EGiNmpU4xhoAgu40e2qW8glQ1SXVZ1RfRZCwI653LwtP1 sQXIqcywNPGWEcavCSf2G5+Vbm0DIF6Sp9Ipu5O9NmBPsDHd/HuOuBg93VFpQ1W5mrJCTwG3xvRed XZJAJVvst0RrOhX6ksxEEoTW13aOWgGIiNkTg8hrgEV90rjvpHUcqw7uiG2TXB9WKxvLoOD3pGfYt 1N/B7zJn1mr+Tvx4xd7zcfWNNH5Cws97Tra0ThkUQ7uEXeThKIwuYVVW6fo+ymrszd7QvXkFEiyrP vZjDB+CA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sOgEZ-00000007Map-3RPx; Tue, 02 Jul 2024 16:19:59 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sOgEO-00000007MYb-2fAV for linux-arm-kernel@lists.infradead.org; Tue, 02 Jul 2024 16:19:50 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id DC26561E8F; Tue, 2 Jul 2024 16:19:47 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D6965C116B1; Tue, 2 Jul 2024 16:19:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1719937187; bh=lVQmHI+LCB+7I2btTVAeudIE1E/R0zerK0QF7WkCDJg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=KFZN/teI9CkmW0iOWqSmXr/ZYdGvy+z/jP2qp+ujTEwwY4xzOjGY3b15+KbrRXlQ4 +Cxy33ZcFI90gE+8LtOphor/Iqn3TU6pCQ7ojazeBHMFkhxWasd2Bts5Ty7i75LSnu +1LfDYpniU+5H+dDLj14xL+K26hfYqjNgipBlMEOoOFfdg01Cv8+ORHoDGs4+Pm1dg cC2P+1KG5KIgkezBQLiR98sJ15yvnPB7hgK92wvYXZItkgfsYvNuYz7RAZH9XP4UdK HjCRxp4GoLEGcnaZM2GrSN3SWHBQfkqAxEF8EhVitwV4QocRoS9UukgANWfn0m7eA0 hKssnf0roWL6g== Date: Tue, 2 Jul 2024 17:19:40 +0100 From: Will Deacon To: Rob Herring Cc: Marc Zyngier , Russell King , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Namhyung Kim , Mark Rutland , Alexander Shishkin , Jiri Olsa , Ian Rogers , Adrian Hunter , Oliver Upton , James Morse , Suzuki K Poulose , Zenghui Yu , Catalin Marinas , James Clark , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, kvmarm@lists.linux.dev Subject: Re: [PATCH v2 06/12] perf: arm_pmu: Remove event index to counter remapping Message-ID: <20240702161940.GA4460@willie-the-truck> References: <20240626-arm-pmu-3-9-icntr-v2-0-c9784b4f4065@kernel.org> <20240626-arm-pmu-3-9-icntr-v2-6-c9784b4f4065@kernel.org> <86ikxuir2k.wl-maz@kernel.org> <20240701135216.GD2250@willie-the-truck> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240702_091948_834429_D1B74327 X-CRM114-Status: GOOD ( 41.40 ) 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 Mon, Jul 01, 2024 at 09:49:29AM -0600, Rob Herring wrote: > On Mon, Jul 1, 2024 at 7:52 AM Will Deacon wrote: > > > > On Thu, Jun 27, 2024 at 12:05:23PM +0100, Marc Zyngier wrote: > > > On Wed, 26 Jun 2024 23:32:30 +0100, > > > "Rob Herring (Arm)" wrote: > > > > > > > > Xscale and Armv6 PMUs defined the cycle counter at 0 and event counters > > > > starting at 1 and had 1:1 event index to counter numbering. On Armv7 and > > > > later, this changed the cycle counter to 31 and event counters start at > > > > 0. The drivers for Armv7 and PMUv3 kept the old event index numbering > > > > and introduced an event index to counter conversion. The conversion uses > > > > masking to convert from event index to a counter number. This operation > > > > relies on having at most 32 counters so that the cycle counter index 0 > > > > can be transformed to counter number 31. > > > > > > > > Armv9.4 adds support for an additional fixed function counter > > > > (instructions) which increases possible counters to more than 32, and > > > > the conversion won't work anymore as a simple subtract and mask. The > > > > primary reason for the translation (other than history) seems to be to > > > > have a contiguous mask of counters 0-N. Keeping that would result in > > > > more complicated index to counter conversions. Instead, store a mask of > > > > available counters rather than just number of events. That provides more > > > > information in addition to the number of events. > > > > > > > > No (intended) functional changes. > > > > > > > > Signed-off-by: Rob Herring (Arm) > > > > > > [...] > > > > > > > diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h > > > > index b3b34f6670cf..e5d6d204beab 100644 > > > > --- a/include/linux/perf/arm_pmu.h > > > > +++ b/include/linux/perf/arm_pmu.h > > > > @@ -96,7 +96,7 @@ struct arm_pmu { > > > > void (*stop)(struct arm_pmu *); > > > > void (*reset)(void *); > > > > int (*map_event)(struct perf_event *event); > > > > - int num_events; > > > > + DECLARE_BITMAP(cntr_mask, ARMPMU_MAX_HWEVENTS); > > > > > > I'm slightly worried by this, as this size is never used, let alone > > > checked by the individual drivers. I can perfectly picture some new > > > (non-architectural) PMU driver having more counters than that, and > > > blindly setting bits outside of the allowed range. > > > > I tend to agree. > > > > > One way to make it a bit safer would be to add a helper replacing the > > > various bitmap_set() calls, and enforcing that we never overflow this > > > bitmap. > > > > Or perhaps wd could leave the 'num_events' field intact and allocate the > > new bitmap dynamically? > > > > Rob -- what do you prefer? I think the rest of the series is ready to go. > > I think the list of places we're initializing cntr_mask is short > enough to check and additions to arm_pmu users are rare enough I would > not be too worried about it. > > If anything, I think the issue is with the bitmap API in that it has > no bounds checking. I'm sure it will get on someone's radar to fix at > some point. > > But if we want to have something check, this is what I have: > > static inline void armpmu_set_counter_mask(struct arm_pmu *pmu, > unsigned int start, unsigned int nr) > { > if (WARN_ON(start + nr > ARMPMU_MAX_HWEVENTS)) > return; > bitmap_set(pmu->cntr_mask, start, nr); > } Fair enough, for the sake of consistency, let's leave the series as-is and we can add helpers for all the counter-bound structures later, if we want to. Will