Re: [PATCH 3/3] rfc: perf: cs_etm: Detect pid in VMID for kernel running at EL2

From: Suzuki K Poulose <suzuki.poulose@arm.com>
To: Leo Yan <leo.yan@linaro.org>
Cc: Al Grant <Al.Grant@arm.com>,
	"mathieu.poirier@linaro.org" <mathieu.poirier@linaro.org>,
	Anshuman Khandual <Anshuman.Khandual@arm.com>,
	"coresight@lists.linaro.org" <coresight@lists.linaro.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"mike.leach@linaro.org" <mike.leach@linaro.org>
Subject: Re: [PATCH 3/3] rfc: perf: cs_etm: Detect pid in VMID for kernel running at EL2
Date: Fri, 13 Nov 2020 09:47:42 +0000	[thread overview]
Message-ID: <7e45fd42-0869-cd4f-1eaf-5d48e4464482@arm.com> (raw)
In-Reply-To: <20201113001105.GA22708@leoy-ThinkPad-X240s>

Hi Leo

On 11/13/20 12:11 AM, Leo Yan wrote:
> On Wed, Nov 11, 2020 at 11:40:03AM +0000, Suzuki Kuruppassery Poulose wrote:
>> On 11/11/20 11:03 AM, Al Grant wrote:
> 
> [...]
> 
>>> But if you must do it heuristically at decode time based on what
>>> you see in packets, then it would be safer to say that if you see both
>>> VMID and CONTEXIDR, the TID will be in VMID.
>>
>> This may not be entirely correct, unless we correlate the VMID_OPT. Because,
>> when we add Virtualization support (vmid == actual vmid).
>>
>> So we should do:
>>         if (VMID && VMIDOPT) {
>> 	   tid = pid;
>> 	} else if (CID) {
>>
>>          }
> 
> Rather than heuristic method, I think we can use metadata to store
> "pid" entry in the function cs_etm_get_metadata(), so that in the
> record phase, the "pid" is stored into perf data file.

True, that is a good idea. That makes it future proof.

> 
> When decode the trace data, we can retrieve "pid" value from the
> metadata, and can base on the "pid" value to make decision for using
> context_id or VMID.

> 
> The metadata can be accessed by referring
> cs_etm_queue::cs_etm_auxtrace::metadata; but the file
> util/cs-etm-decoder/cs-etm-decoder.c cannot directly access the
> metadata structure due to "cs_etm_queue" and "cs_etm_auxtrace" both
> are defined in util/cs-etm.c.  It's better to introduce a helper
> function in util/cs-etm.c to retrieve "pid" value, like:
> 
>    u64 cs_etm__etmq_get_pid_type(struct cs_etm_queue *etmq);

I am not an expert in that side, how it interacts with the OpenCSD.
thus left this patch as an RFC.

I appreciate your feedback and thoughts. If you feel like you could
cleanup and implement this, please feel free to do so. Otherwise, I
might try it out whenever I have some spare cycles to understand
this side of the code (and this might be delayed).

Cheers
Suzuki

> 
> Thanks,
> Leo
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel