Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 30 Oct 2018 11:32:26 -0300
From:   Arnaldo Carvalho de Melo <acme@kernel.org>
To:     Leo Yan <leo.yan@linaro.org>
Cc:     Mathieu Poirier <mathieu.poirier@linaro.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Coresight ML <coresight@lists.linaro.org>
Subject: Re: [PATCH] perf cs-etm: Correct CPU mode for samples
Message-ID: <20181030143226.GA23310@kernel.org>
References: <1540883908-17018-1-git-send-email-leo.yan@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1540883908-17018-1-git-send-email-leo.yan@linaro.org>
User-Agent: Mutt/1.9.2 (2017-12-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Em Tue, Oct 30, 2018 at 03:18:28PM +0800, Leo Yan escreveu:
> Since commit 9042f5e3539e ("perf tools: Stop fallbacking to kallsyms
> for vdso symbols lookup"), the kernel address cannot be properly parsed
> to kernel symbol with command 'perf script -k vmlinux'.  The reason is
> CoreSight samples is always to set CPU mode as PERF_RECORD_MISC_USER,
> thus it fails to find corresponding map/dso in below flows:
> 
>   process_sample_event()
>     `-> machine__resolve()
> 	  `-> thread__find_map(thread, sample->cpumode, sample->ip, al);
> 
> In this flow it needs to pass argument 'sample->cpumode' to tell what's
> the CPU mode, before it always passed PERF_RECORD_MISC_USER but without
> any failure until the commit 9042f5e3539e ("perf tools: Stop fallbacking
> to kallsyms for vdso symbols lookup") has been merged.  The reason is
> even with the wrong CPU mode the function thread__find_map() firstly
> fails to find map but it will rollback to find kernel map for vdso
> symbols lookup.  In the latest code it has removed the fallback code,
> thus if CPU mode is PERF_RECORD_MISC_USER then it cannot find map
> anymore with kernel address.
> 
> This patch is to correct samples CPU mode setting, it creates a new
> helper function cs_etm__cpu_mode() to tell what's the CPU mode based on
> the address with the info from machine structure; this patch has a bit
> extension to check not only kernel and user mode, but also check for
> host/guest and hypervisor mode.  Finally this patch uses the function
> in instruction and branch samples and also apply in cs_etm__mem_access()
> for a minor polishing.

Mathieu, can I have your Acked-by, please? Leo, thanks for acting so
quickly on this one! 

Now processing coresight traces should be faster, less lookups :-)

- Arnaldo
 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  tools/perf/util/cs-etm.c | 39 ++++++++++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 3b37d66..73430b7 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -244,6 +244,27 @@ static void cs_etm__free(struct perf_session *session)
>  	zfree(&aux);
>  }
>  
> +static u8 cs_etm__cpu_mode(struct cs_etm_queue *etmq, u64 address)
> +{
> +	struct machine *machine;
> +
> +	machine = etmq->etm->machine;
> +
> +	if (address >= etmq->etm->kernel_start) {
> +		if (machine__is_host(machine))
> +			return PERF_RECORD_MISC_KERNEL;
> +		else
> +			return PERF_RECORD_MISC_GUEST_KERNEL;
> +	} else {
> +		if (machine__is_host(machine))
> +			return PERF_RECORD_MISC_USER;
> +		else if (perf_guest)
> +			return PERF_RECORD_MISC_GUEST_USER;
> +		else
> +			return PERF_RECORD_MISC_HYPERVISOR;
> +	}
> +}
> +
>  static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u64 address,
>  			      size_t size, u8 *buffer)
>  {
> @@ -258,10 +279,7 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u64 address,
>  		return -1;
>  
>  	machine = etmq->etm->machine;
> -	if (address >= etmq->etm->kernel_start)
> -		cpumode = PERF_RECORD_MISC_KERNEL;
> -	else
> -		cpumode = PERF_RECORD_MISC_USER;
> +	cpumode = cs_etm__cpu_mode(etmq, address);
>  
>  	thread = etmq->thread;
>  	if (!thread) {
> @@ -653,7 +671,7 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
>  	struct perf_sample sample = {.ip = 0,};
>  
>  	event->sample.header.type = PERF_RECORD_SAMPLE;
> -	event->sample.header.misc = PERF_RECORD_MISC_USER;
> +	event->sample.header.misc = cs_etm__cpu_mode(etmq, addr);
>  	event->sample.header.size = sizeof(struct perf_event_header);
>  
>  	sample.ip = addr;
> @@ -665,7 +683,7 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
>  	sample.cpu = etmq->packet->cpu;
>  	sample.flags = 0;
>  	sample.insn_len = 1;
> -	sample.cpumode = event->header.misc;
> +	sample.cpumode = event->sample.header.misc;
>  
>  	if (etm->synth_opts.last_branch) {
>  		cs_etm__copy_last_branch_rb(etmq);
> @@ -706,12 +724,15 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq)
>  		u64			nr;
>  		struct branch_entry	entries;
>  	} dummy_bs;
> +	u64 ip;
> +
> +	ip = cs_etm__last_executed_instr(etmq->prev_packet);
>  
>  	event->sample.header.type = PERF_RECORD_SAMPLE;
> -	event->sample.header.misc = PERF_RECORD_MISC_USER;
> +	event->sample.header.misc = cs_etm__cpu_mode(etmq, ip);
>  	event->sample.header.size = sizeof(struct perf_event_header);
>  
> -	sample.ip = cs_etm__last_executed_instr(etmq->prev_packet);
> +	sample.ip = ip;
>  	sample.pid = etmq->pid;
>  	sample.tid = etmq->tid;
>  	sample.addr = cs_etm__first_executed_instr(etmq->packet);
> @@ -720,7 +741,7 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq)
>  	sample.period = 1;
>  	sample.cpu = etmq->packet->cpu;
>  	sample.flags = 0;
> -	sample.cpumode = PERF_RECORD_MISC_USER;
> +	sample.cpumode = event->sample.header.misc;
>  
>  	/*
>  	 * perf report cannot handle events without a branch stack
> -- 
> 2.7.4