Re: [PATCH 2/4] perf: arm-spe: Add support for ARMv8.3-SPE

From: "liwei (GF)" <liwei391@huawei.com>
To: Leo Yan <leo.yan@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Will Deacon <will@kernel.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	linux-kernel@vger.kernel.org, zhangshaokun@hisilicon.com,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, James Clark <james.clark@arm.com>,
	guohanjun@huawei.com, Namhyung Kim <namhyung@kernel.org>,
	Jiri Olsa <jolsa@redhat.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/4] perf: arm-spe: Add support for ARMv8.3-SPE
Date: Wed, 29 Jul 2020 15:42:46 +0800	[thread overview]
Message-ID: <beabfb2a-e9ec-fe8e-a6ae-a7d4935a421e@huawei.com> (raw)
In-Reply-To: <20200729072844.GH4343@leoy-ThinkPad-X240s>

Hi Leo,

On 2020/7/29 15:28, Leo Yan wrote:
> On Wed, Jul 29, 2020 at 03:21:20PM +0800, liwei (GF) wrote:
> 
> [...]
> 
>>>> @@ -354,8 +372,38 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>>>>  	}
>>>>  	case ARM_SPE_OP_TYPE:
>>>>  		switch (idx) {
>>>> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>>>> -					"COND-SELECT" : "INSN-OTHER");
>>>> +		case 0:	{
>>>> +			if (payload & 0x8) {
>>>
>>> Some nitpicks for packet format checking ...
>>>
>>> For SVE operation, the payload partten is: 0b0xxx1xx0.
>>>
>>> So it's good to check the partten like:
>>>
>>>   /* SVE operation subclass is: 0b0xxx1xx0 */
>>>   if ((payload & 0x8081) == 0x80) {
>>>      ....
>>>   }
>>>
>>> If later the packet format is extended, this will not introduce any
>>> confliction.
>>
>> Get it, but i think what you are really meaning is:
>> if ((payload & 0x89) == 0x80) {
>> 	...
>> }
> 
> Yes.
> 
>>>
>>>> +				size_t blen = buf_len;
>>>> +
>>>> +				ret = snprintf(buf, buf_len, "SVE-OTHER");
>>>> +				buf += ret;
>>>> +				blen -= ret;
>>>> +				if (payload & 0x2) {
>>>
>>> Here should express as binary results: " FP" or " INT".
>>
>> I think this is a style choice, i add these just like the current code where
>> processing "AT", "EXCL", "AR", "COND" and so on. So should we modify all the corresponding code together?
> 
> Okay, understood.  Let's just follow the existed style and later can
> enhance the output log with more readable format.
> 
> [...]
> 
>>>
>>>> +					ret = snprintf(buf, buf_len, " FP");
>>>> +					buf += ret;
>>>> +					blen -= ret;
>>>> +				}
>>>> +				if (payload & 0x4) {
>>>> +					ret = snprintf(buf, buf_len, " PRED");
>>>
>>> Here should express as binary results: " PRED" or " NOT-PRED".
>>
>> Ditto.
>>
>>>
>>>> +					buf += ret;
>>>> +					blen -= ret;
>>>> +				}
>>>> +				if (payload & 0x70) {
>>>
>>> This is incorrect.  If bits[6:4] is zero, it presents vector length is 32 bits.
>>>
>>
>> I am a little confused here.
>> Refer to the ARM DDI 0487F.b (ID040120), page D10-2830, if bits[6:4] is zero,
>> it presents vector length is 32 bits indeed.
> 
> Yes, if bits[6:4] is zero, your current code will not output any info.
> 

Yes, thanks for spotting this.
And thanks for you review.

Thanks,
Wei

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