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]

Date:   Wed, 29 Jul 2020 15:28:44 +0800
From:   Leo Yan <leo.yan@...aro.org>
To:     "liwei (GF)" <liwei391@...wei.com>
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

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.

Thanks,
Leo

> >> +					ret = snprintf(buf, buf_len, " EVL %d",
> >> +						32 << ((payload & 0x70) >> 4));
> >> +					buf += ret;
> >> +					blen -= ret;
> >> +				}
> >> +				if (ret < 0)
> >> +					return ret;
> >> +				blen -= ret;
> >> +				return buf_len - blen;
> >> +			} else {
> > 
> > Here we can check with more accurate format as defined in ARMv8 ARM:
> > 
> >   /* Other operation subclass is: 0b0000000x */
> >   if ((payload & 0xfe) == 0x0) {
> >      ....
> >   }
> > 
> >> +				return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> >> +						"COND-SELECT" : "INSN-OTHER");
> >> +			}
> >> +		}
> >>  		case 1:	{
> >>  			size_t blen = buf_len;
> >>  
> >> @@ -385,6 +433,23 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> >>  				ret = snprintf(buf, buf_len, " SIMD-FP");
> >>  				buf += ret;
> >>  				blen -= ret;
> >> +			} else if (payload & 0x8) {
> >> +				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.
> 
> Refer to the ARM DDI 0487F.b (ID040120), page D10-2832, if bits[6:4] is zero,
> it presents vector length is 32 bits indeed.
> 
> >> +					ret = snprintf(buf, buf_len, " EVL %d",
> >> +						32 << ((payload & 0x70) >> 4));
> >> +					buf += ret;
> >> +					blen -= ret;
> >> +				}
> >> +				if (payload & 0x80) {
> >> +					ret = snprintf(buf, buf_len, " SG");
> > 
> > Here should express as binary results: " SG" or " NOT-SG".
> 
> 
> Thanks,
> Wei