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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <beabfb2a-e9ec-fe8e-a6ae-a7d4935a421e@huawei.com>
Date:   Wed, 29 Jul 2020 15:42:46 +0800
From:   "liwei (GF)" <liwei391@...wei.com>
To:     Leo Yan <leo.yan@...aro.org>
CC:     Adrian Hunter <adrian.hunter@...el.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        James Clark <james.clark@....com>,
        Jiri Olsa <jolsa@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Namhyung Kim <namhyung@...nel.org>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Will Deacon <will@...nel.org>, <zhangshaokun@...ilicon.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...hat.com>,
        "Peter Zijlstra" <peterz@...radead.org>, <guohanjun@...wei.com>
Subject: Re: [PATCH 2/4] perf: arm-spe: Add support for ARMv8.3-SPE

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