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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0B970C07548 for ; Wed, 15 Nov 2023 09:37:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234798AbjKOJho (ORCPT ); Wed, 15 Nov 2023 04:37:44 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37200 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234688AbjKOJhn (ORCPT ); Wed, 15 Nov 2023 04:37:43 -0500 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 60AD4E5; Wed, 15 Nov 2023 01:37:39 -0800 (PST) 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 A4A97DA7; Wed, 15 Nov 2023 01:38:24 -0800 (PST) Received: from [192.168.1.3] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6A7653F7B4; Wed, 15 Nov 2023 01:37:37 -0800 (PST) Message-ID: <4fa8ca52-b40c-65ec-59a0-7ad14c4e93b7@arm.com> Date: Wed, 15 Nov 2023 09:37:36 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Subject: Re: [V14 3/8] drivers: perf: arm_pmuv3: Enable branch stack sampling framework Content-Language: en-US To: Anshuman Khandual Cc: Mark Brown , Rob Herring , Marc Zyngier , Suzuki Poulose , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , linux-perf-users@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, will@kernel.org, catalin.marinas@arm.com, mark.rutland@arm.com References: <20231114051329.327572-1-anshuman.khandual@arm.com> <20231114051329.327572-4-anshuman.khandual@arm.com> <0020aa0d-e9a5-aef6-f33d-817da56411a3@arm.com> <5bf73672-4094-490b-b0a9-b983d512c904@arm.com> From: James Clark In-Reply-To: <5bf73672-4094-490b-b0a9-b983d512c904@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 15/11/2023 05:44, Anshuman Khandual wrote: > On 11/14/23 15:28, James Clark wrote: >> >> >> On 14/11/2023 05:13, Anshuman Khandual wrote: >>> Branch stack sampling support i.e capturing branch records during execution >>> in core perf, rides along with normal HW events being scheduled on the PMU. >>> This prepares ARMV8 PMU framework for branch stack support on relevant PMUs >>> with required HW implementation. >>> >> >> [...] >> >>> - All armv8pmu_branch_xxxx() stub definitions have been moved inside >>> include/linux/perf/arm_pmuv3.h for easy access from both arm32 and >>> arm64 platforms >>> >> >> This causes lots of W=1 build errors because the prototypes are in >> arm_pmuv3.h, but arm_brbe.c doesn't include it. > > I guess these are the W=1 warnings you mentioned above. > > drivers/perf/arm_brbe.c:11:6: warning: no previous prototype for ‘armv8pmu_branch_reset’ [-Wmissing-prototypes] > 11 | void armv8pmu_branch_reset(void) > | ^~~~~~~~~~~~~~~~~~~~~ > drivers/perf/arm_brbe.c:190:6: warning: no previous prototype for ‘armv8pmu_branch_save’ [-Wmissing-prototypes] > 190 | void armv8pmu_branch_save(struct arm_pmu *arm_pmu, void *ctx) > | ^~~~~~~~~~~~~~~~~~~~ > drivers/perf/arm_brbe.c:236:6: warning: no previous prototype for ‘armv8pmu_branch_attr_valid’ [-Wmissing-prototypes] > 236 | bool armv8pmu_branch_attr_valid(struct perf_event *event) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/perf/arm_brbe.c:269:5: warning: no previous prototype for ‘armv8pmu_task_ctx_cache_alloc’ [-Wmissing-prototypes] > 269 | int armv8pmu_task_ctx_cache_alloc(struct arm_pmu *arm_pmu) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/perf/arm_brbe.c:279:6: warning: no previous prototype for ‘armv8pmu_task_ctx_cache_free’ [-Wmissing-prototypes] > 279 | void armv8pmu_task_ctx_cache_free(struct arm_pmu *arm_pmu) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/perf/arm_brbe.c:303:6: warning: no previous prototype for ‘armv8pmu_branch_probe’ [-Wmissing-prototypes] > 303 | void armv8pmu_branch_probe(struct arm_pmu *armpmu) > | ^~~~~~~~~~~~~~~~~~~~~ > drivers/perf/arm_brbe.c:449:6: warning: no previous prototype for ‘armv8pmu_branch_enable’ [-Wmissing-prototypes] > 449 | void armv8pmu_branch_enable(struct arm_pmu *arm_pmu) > | ^~~~~~~~~~~~~~~~~~~~~~ > drivers/perf/arm_brbe.c:474:6: warning: no previous prototype for ‘armv8pmu_branch_disable’ [-Wmissing-prototypes] > 474 | void armv8pmu_branch_disable(void) > | ^~~~~~~~~~~~~~~~~~~~~~~ > drivers/perf/arm_brbe.c:717:6: warning: no previous prototype for ‘armv8pmu_branch_read’ [-Wmissing-prototypes] > 717 | void armv8pmu_branch_read(struct pmu_hw_events *cpuc, struct perf_event *event) > > Branch helpers are used in ARM PMU V3 driver i.e drivers/perf/arm_pmuv3.c. > Whether the actual BRBE helper definitions, or their fallback stubs (when > CONFIG_ARM64_BRBE is not enabled), need to be accessible from arm_pmuv3.c > driver not from brbe.c implementations itself. > >> >> It seems like the main reason you can't include arm_brbe.h in arm32 code >> is because there are a load of inline functions and references to >> registers in there. But these are only used in arm_brbe.c, so they don't > > Right, arm32 should not be exposed to BRBE internals via arm_brbe.h header. > >> need to be in the header anyway. > > Right, these are only used in arm_brbe.c > >> >> If you removed the code from the header and moved it to the source file >> you could move the brbe prototypes to the brbe header and it would be a >> bit cleaner and more idiomatic. > > Alight, how about the following changes - build tested on arm32 and arm64. > > - Move BRBE helpers from arm_brbe.h into arm_brbe.c > - Move armv8_pmu_xxx() declaration inside arm_brbe.h for arm64 (CONFIG_ARM64_BRBE) > - Move armv8_pmu_xxx() stub definitions inside arm_pmuv3.c for arm32 (!CONFIG_ARM64_BRBE) > - Include arm_brbe.h header both in arm_pmuv3.c and arm_brbe.c Agree to them all except: - Move armv8_pmu_xxx() stub definitions inside arm_pmuv3.c for arm32 (!CONFIG_ARM64_BRBE) Normally you put the stubs right next to the prototypes with #else, so in this case both would be in arm_brbe.h. Not sure what the reason for splitting them here is? You already said "include arm_brbe.h in arm_pmuv3.c", so that covers arm32 too.