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 2B6E4C27C55 for ; Mon, 10 Jun 2024 10:45:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To: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=GWM+ag9Rv99MebpzBHVQnRHMsAux2rdmmd/W25jfPpA=; b=0jivwkoT9VRtUb ouMZGD2kQPDYuh7YdRM9rnjV3bHO8J9g+F5rkaGgTONyVTc6IYjamzdRnTUg6BFtyFx2+STQf3df8 87/kqv2KndcOikHuvXlp2fiLRqIMtsbIjnmtUVJ16hCAPFQz1/cy4YAPPWexLeLEEYgGNFwdT8jOo 92W3LmQ5Rb0/rhpcxaBGPX+UqexQ7jvbOgEXSGaUZsIE0YxPcd6gbsvFRMlgZWpZLVp7mtYBEPYUu 19h44EeAQvYFxRRig/ifnwtRCxqNVSgQy9BeyI1IoKxhL1LXa6r04ehkEIMBNPkwbOGYEQrVjoUOV YzFf8K061h7CSH0rfAnQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sGcWQ-00000004hRr-3Cw8; Mon, 10 Jun 2024 10:45:07 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sGcWA-00000004hGk-48va for linux-arm-kernel@lists.infradead.org; Mon, 10 Jun 2024 10:45:00 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B17B112FC; Mon, 10 Jun 2024 03:45:14 -0700 (PDT) Received: from J2N7QTR9R3 (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D641F3F73B; Mon, 10 Jun 2024 03:44:46 -0700 (PDT) Date: Mon, 10 Jun 2024 11:44:43 +0100 From: Mark Rutland To: "Rob Herring (Arm)" Cc: Russell King , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Namhyung Kim , Alexander Shishkin , Jiri Olsa , Ian Rogers , Adrian Hunter , Will Deacon , Marc Zyngier , Oliver Upton , James Morse , Suzuki K Poulose , Zenghui Yu , Catalin Marinas , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, kvmarm@lists.linux.dev Subject: Re: [PATCH 3/9] perf: arm_pmu: Remove event index to counter remapping Message-ID: References: <20240607-arm-pmu-3-9-icntr-v1-0-c7bd2dceff3b@kernel.org> <20240607-arm-pmu-3-9-icntr-v1-3-c7bd2dceff3b@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240607-arm-pmu-3-9-icntr-v1-3-c7bd2dceff3b@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240610_034451_503034_BA6DA952 X-CRM114-Status: GOOD ( 36.72 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Jun 07, 2024 at 02:31:28PM -0600, 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) > --- > arch/arm64/kvm/pmu-emul.c | 6 ++-- > drivers/perf/arm_pmu.c | 11 ++++--- > drivers/perf/arm_pmuv3.c | 57 ++++++++++---------------------- > drivers/perf/arm_v6_pmu.c | 6 ++-- > drivers/perf/arm_v7_pmu.c | 77 ++++++++++++++++--------------------------- > drivers/perf/arm_xscale_pmu.c | 12 ++++--- > include/linux/perf/arm_pmu.h | 2 +- > 7 files changed, 69 insertions(+), 102 deletions(-) This looks like a nice cleanup! As the test robot reports, it looks like this missed drivers/perf/apple_m1_cpu_pmu.c, but IIUC that's simple enough to fix up. Otherwise, I have a few minor comments below, > diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c > index 23fa6c5da82c..80202346fc7a 100644 > --- a/drivers/perf/arm_pmuv3.c > +++ b/drivers/perf/arm_pmuv3.c > @@ -451,9 +451,7 @@ static const struct attribute_group armv8_pmuv3_caps_attr_group = { > /* > * Perf Events' indices > */ > -#define ARMV8_IDX_CYCLE_COUNTER 0 > -#define ARMV8_IDX_COUNTER0 1 > -#define ARMV8_IDX_CYCLE_COUNTER_USER 32 > +#define ARMV8_IDX_CYCLE_COUNTER 31 I was going to ask whether this affected the ABI, but I see from below that armv8pmu_user_event_idx() will now always offset the counter by one rather than special-casing the cycle counter, and this gives us the same behavior as before. [...] > @@ -783,7 +767,7 @@ static void armv8pmu_enable_user_access(struct arm_pmu *cpu_pmu) > struct pmu_hw_events *cpuc = this_cpu_ptr(cpu_pmu->hw_events); > > /* Clear any unused counters to avoid leaking their contents */ > - for_each_clear_bit(i, cpuc->used_mask, cpu_pmu->num_events) { > + for_each_clear_bit(i, cpuc->used_mask, ARMPMU_MAX_HWEVENTS) { > if (i == ARMV8_IDX_CYCLE_COUNTER) > write_pmccntr(0); > else IIUC this will now hit all unimplemented counters; e.g. for N counters the body will run for counters N..31, and the else case has: armv8pmu_write_evcntr(i, 0); ... where the resulting write to PMEVCNTR_EL0 for unimplemented counters is CONSTRAINED UNPREDICTABLE and might be UNDEFINED. We can fix that with for_each_andnot_bit(), e.g. for_each_andnot_bit(i, cpu_pmu->cntr_mask, cpuc->used_mask, ARMPMU_MAX_HWEVENTS) { if (i == ARMV8_IDX_CYCLE_COUNTER) write_pmccntr(0); else armv8pmu_write_evcntr(i, 0); } [...] > @@ -905,7 +889,7 @@ static int armv8pmu_get_single_idx(struct pmu_hw_events *cpuc, > { > int idx; > > - for (idx = ARMV8_IDX_COUNTER0; idx < cpu_pmu->num_events; idx++) { > + for_each_set_bit(idx, cpu_pmu->cntr_mask, 31) { > if (!test_and_set_bit(idx, cpuc->used_mask)) > return idx; > } > @@ -921,7 +905,9 @@ static int armv8pmu_get_chain_idx(struct pmu_hw_events *cpuc, > * Chaining requires two consecutive event counters, where > * the lower idx must be even. > */ > - for (idx = ARMV8_IDX_COUNTER0 + 1; idx < cpu_pmu->num_events; idx += 2) { > + for_each_set_bit(idx, cpu_pmu->cntr_mask, 31) { > + if (!(idx & 0x1)) > + continue; > if (!test_and_set_bit(idx, cpuc->used_mask)) { > /* Check if the preceding even counter is available */ > if (!test_and_set_bit(idx - 1, cpuc->used_mask)) It would be nice to replace those instances of '31' with something indicating that this was only covering the generic/programmable counters, but I wasn't able to come up with a nice mnemonic for that. The best I could think of was: #define ARMV8_MAX_NR_GENERIC_COUNTERS 31 Maybe it makes sense to define that along with ARMV8_IDX_CYCLE_COUNTER. > @@ -974,15 +960,7 @@ static int armv8pmu_user_event_idx(struct perf_event *event) > if (!sysctl_perf_user_access || !armv8pmu_event_has_user_read(event)) > return 0; > > - /* > - * We remap the cycle counter index to 32 to > - * match the offset applied to the rest of > - * the counter indices. > - */ > - if (event->hw.idx == ARMV8_IDX_CYCLE_COUNTER) > - return ARMV8_IDX_CYCLE_COUNTER_USER; > - > - return event->hw.idx; > + return event->hw.idx + 1; > } [...] > static void armv7_read_num_pmnc_events(void *info) > { > - int *nb_cnt = info; > + int nb_cnt; > + struct arm_pmu *cpu_pmu = info; > > /* Read the nb of CNTx counters supported from PMNC */ > - *nb_cnt = (armv7_pmnc_read() >> ARMV7_PMNC_N_SHIFT) & ARMV7_PMNC_N_MASK; > + nb_cnt = (armv7_pmnc_read() >> ARMV7_PMNC_N_SHIFT) & ARMV7_PMNC_N_MASK; > + bitmap_set(cpu_pmu->cntr_mask, 0, nb_cnt); > > /* Add the CPU cycles counter */ > - *nb_cnt += 1; > + bitmap_set(cpu_pmu->cntr_mask, ARMV7_IDX_CYCLE_COUNTER, 1); This can be: set_bit(cpu_pmu->cntr_mask, ARMV7_IDX_CYCLE_COUNTER); ... and likewise for the PMUv3 version. [...] > diff --git a/drivers/perf/arm_xscale_pmu.c b/drivers/perf/arm_xscale_pmu.c > index 3d8b72d6b37f..e075df521350 100644 > --- a/drivers/perf/arm_xscale_pmu.c > +++ b/drivers/perf/arm_xscale_pmu.c > @@ -52,6 +52,8 @@ enum xscale_counters { > XSCALE_COUNTER1, > XSCALE_COUNTER2, > XSCALE_COUNTER3, > + XSCALE2_NUM_COUNTERS, > + XSCALE_NUM_COUNTERS = 3, > }; Minor nit, but for consistency with other xscale1-only definitions, it'd be good to s/XSCALE_NUM_COUNTERS/XSCALE1_NUM_COUNTERS/. While it'd be different fro mthe other PMU drivers, I reckon it's clearer to pull those out as: #define XSCALE1_NUM_COUNTERS 3 #define XSCALE2_NUM_COUNTERS 5 Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel