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

From: Leo Yan <leo.yan@linaro.org>
To: "liwei (GF)" <liwei391@huawei.com>
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:28:44 +0800	[thread overview]
Message-ID: <20200729072844.GH4343@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <a8f9df67-adf1-09e7-b117-ff4163cc2a03@huawei.com>

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

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