Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 16 Feb 2022 21:22:57 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     German Gomez <german.gomez@arm.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        will@kernel.org, mark.rutland@arm.com, james.clark@arm.com
Subject: Re: [RFC PATCH 1/2] perf: arm_spe: Fix consistency of PMSCR register
 bit CX
Message-ID: <20220216132257.GB56419@leoy-ThinkPad-X240s>
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>
 <e68839bc-b4f0-1fe8-1748-484254ded37a@arm.com>
 <20220211104528.GA475776@leoy-ThinkPad-X240s>
 <d59e99ad-dfc6-27ee-d952-7455d8e9fda6@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <d59e99ad-dfc6-27ee-d952-7455d8e9fda6@arm.com>
Precedence: bulk

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: <stack trace>

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);

  ...

  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