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 F0E56C433EF for ; Wed, 16 Feb 2022 15:16:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235482AbiBPPRF (ORCPT ); Wed, 16 Feb 2022 10:17:05 -0500 Received: from mxb-00190b01.gslb.pphosted.com ([23.128.96.19]:40880 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233873AbiBPPRB (ORCPT ); Wed, 16 Feb 2022 10:17:01 -0500 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 069B39F391 for ; Wed, 16 Feb 2022 07:16:49 -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 AB8FCD6E; Wed, 16 Feb 2022 07:16:48 -0800 (PST) Received: from [10.1.31.148] (e127744.cambridge.arm.com [10.1.31.148]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 550963F70D; Wed, 16 Feb 2022 07:16:47 -0800 (PST) Subject: Re: [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register bit CX To: Leo Yan Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, will@kernel.org, mark.rutland@arm.com, james.clark@arm.com References: <20220117124432.3119132-1-german.gomez@arm.com> <20220117124432.3119132-2-german.gomez@arm.com> <20220205153940.GB391033@leoy-ThinkPad-X240s> <4d5951ee-d7d2-1e76-eb24-5f3c46d1662c@arm.com> <20220208130047.GA273989@leoy-ThinkPad-X240s> <20220211104528.GA475776@leoy-ThinkPad-X240s> <20220216132257.GB56419@leoy-ThinkPad-X240s> From: German Gomez Message-ID: <7d50f7ba-d8e5-1fbc-e9e6-749a24e26555@arm.com> Date: Wed, 16 Feb 2022 15:16:01 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <20220216132257.GB56419@leoy-ThinkPad-X240s> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-US Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Leo, Thanks for the review On 16/02/2022 13:22, Leo Yan wrote: > On Tue, Feb 15, 2022 at 02:29:27PM +0000, German Gomez wrote: > > [...] > >> Thanks for the suggestion. I recorded the following stacktrace: >> >> perf-323841 [052] d.... 3996.528812: arm_spe_pmu_setup_aux: (arm_spe_pmu_setup_aux+0x60/0x1c0 [arm_spe_pmu]) >> perf-323841 [052] d.... 3996.528813: > Yeah, this show arm_spe_pmu_setup_aux() is called in perf process. > >> => kprobe_dispatcher >> => kprobe_breakpoint_handler >> => call_break_hook >> => brk_handler >> => do_debug_exception >> => el1_dbg >> => el1h_64_sync_handler >> => el1h_64_sync >> => arm_spe_pmu_setup_aux >> => perf_mmap >> => mmap_region >> => do_mmap >> => vm_mmap_pgoff >> => ksys_mmap_pgoff >> => __arm64_sys_mmap >> => invoke_syscall >> => el0_svc_common.constprop.0 >> => do_el0_svc >> => el0_svc >> => el0t_64_sync_handler >> => el0t_64_sync >> >> So for a v2 I may include something like this: > The change looks good to me, please see below minor comment. > >> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c >> index d44bcc29d..aadec5a0e 100644 >> --- a/drivers/perf/arm_spe_pmu.c >> +++ b/drivers/perf/arm_spe_pmu.c >> @@ -45,6 +45,7 @@ struct arm_spe_pmu_buf { >> int nr_pages; >> bool snapshot; >> void *base; >> + u64 pmscr; >> }; >> >> struct arm_spe_pmu { >> @@ -748,7 +749,7 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags) >> write_sysreg_s(reg, SYS_PMSICR_EL1); >> } >> >> - reg = arm_spe_event_to_pmscr(event); >> + reg = ((struct arm_spe_pmu_buf *) perf_get_aux(handle))->pmscr; >> isb(); >> write_sysreg_s(reg, SYS_PMSCR_EL1); > Just nitpick (or it's just my preferring coding style), we can define > a local pointer variable 'buf': > > struct arm_spe_pmu_buf *buf = perf_get_aux(handle); I need to make sure perf_get_aux(..) is called between perf_aux_output_begin and *_end though (so, after arm_spe_perf_aux_output_begin(..)):  buf = perf_get_aux(handle);  reg = buf->pmscr;  isb();  write_sysreg_s(buf, SYS_PMSCR_EL1); Alternatively, we set the register inside of perf_aux_output_begin. It might be confusing for casual readers because the function handles a case where perf_get_aux(..) returns NULL. Alternatively, we could also wrap perf_get_aux(..) in a static inline function that returns the correct type and do:  reg = arm_spe_get_aux(handle)->pmscr;  isb();  write_sysreg_s(reg, SYS_PMSCR_EL1); So that it looks cleaner. > > ... > > isb(); > write_sysreg_s(buf->pmscr, SYS_PMSCR_EL1); > > Thanks, > Leo > >> } >> @@ -855,6 +856,8 @@ static void *arm_spe_pmu_setup_aux(struct perf_event *event, void **pages, >> if (!pglist) >> goto out_free_buf; >> >> + buf->pmscr = arm_spe_event_to_pmscr(event); >> + >> for (i = 0; i < nr_pages; ++i) >> pglist[i] = virt_to_page(pages[i]); >> >>> Thanks, >>> Leo