Re: [PATCH v2 10/14] perf arm-spe: Refactor event type handling

[Leo Yan]

Hi Andre,

On Wed, Oct 21, 2020 at 10:20:24AM +0100, André Przywara wrote:

[...]

> >>> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >>> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >>> @@ -284,58 +284,58 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> >>>  		if (ret < 0)
> >>>  			return ret;
> >>>  
> >>> -		if (payload & 0x1) {
> >>> +		if (payload & SPE_EVT_PKT_GEN_EXCEPTION) {
> >>
> >> Having the bitmask here directly is indeed not very nice and error
> >> prone. But I would rather see the above solution:
> >> 		if (payload & BIT(EV_EXCEPTION_GEN)) {
> > 
> > Will do.
> > 
> >>>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " EXCEPTION-GEN");
> >>>  			if (ret < 0)
> >>>  				return ret;
> >>>  		}
> >>> -		if (payload & 0x2) {
> >>> +		if (payload & SPE_EVT_PKT_ARCH_RETIRED) {
> >>>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " RETIRED");
> >>>  			if (ret < 0)
> >>>  				return ret;
> >>>  		}
> >>> -		if (payload & 0x4) {
> >>> +		if (payload & SPE_EVT_PKT_L1D_ACCESS) {
> >>>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-ACCESS");
> >>>  			if (ret < 0)
> >>>  				return ret;
> >>>  		}
> >>> -		if (payload & 0x8) {
> >>> +		if (payload & SPE_EVT_PKT_L1D_REFILL) {
> >>>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " L1D-REFILL");
> >>>  			if (ret < 0)
> >>>  				return ret;
> >>>  		}
> >>> -		if (payload & 0x10) {
> >>> +		if (payload & SPE_EVT_PKT_TLB_ACCESS) {
> >>>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-ACCESS");
> >>>  			if (ret < 0)
> >>>  				return ret;
> >>>  		}
> >>> -		if (payload & 0x20) {
> >>> +		if (payload & SPE_EVT_PKT_TLB_WALK) {
> >>>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " TLB-REFILL");
> >>>  			if (ret < 0)
> >>>  				return ret;
> >>>  		}
> >>> -		if (payload & 0x40) {
> >>> +		if (payload & SPE_EVT_PKT_NOT_TAKEN) {
> >>>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " NOT-TAKEN");
> >>>  			if (ret < 0)
> >>>  				return ret;
> >>>  		}
> >>> -		if (payload & 0x80) {
> >>> +		if (payload & SPE_EVT_PKT_MISPREDICTED) {
> >>>  			ret = arm_spe_pkt_snprintf(&buf, &blen, " MISPRED");
> >>>  			if (ret < 0)
> >>>  				return ret;
> >>>  		}
> >>>  		if (idx > 1) {
> >>
> >> Do you know what the purpose of this comparison is? Surely payload would
> >> not contain more bits than would fit in "idx" bytes? So is this some
> >> attempt of an optimisation?
> > 
> > Here "idx" is for payload size (in bytes); you could see function
> > arm_spe_get_events() calculate the payload size:
> > 
> >   packet->index = PAYLOAD_LEN(buf[0]);
> > 
> > Please note, the raw payload size (field "sz" in header) value is:
> > 
> >   0b00 Byte.
> >   0b01 Halfword.
> >   0b10 Word.
> >   0b11 Doubleword.
> > 
> > After using PAYLOAD_LEN(), the payload size is converted to value in
> > byte, so:
> > 
> >   packet->index = 1 << "sz";
> > 
> >   1  Byte
> >   2  Halfword
> >   4  Word
> >   8  Doubleword
> > 
> > In Armv8 ARM, chapter "D10.2.6 Events packet", we can see the events
> > "Remote access", "Last Level cache miss" and "Last Level cache access"
> > are only valid when "sz" is equal or longer than Halfword, thus idx is
> > 2/4/8; this is why here checks the condition "if (idx > 1)".
> 
> Right, thanks for the explanation. But in the end this is just a lot of
> words for: "You can only fit n*8 bits in n bytes.", isn't it?
> So if the payload size is 1 bytes, we can't have bits 8 or higher.
> 
> And in arm_spe_get_payload() we load payload with casts, so the upper
> bits, beyond the payload size, must always be 0? Regardless of what was
> in the buffer. Or am I looking at the wrong function?

You are right!  Sorry I didn't connect with the function
arm_spe_get_payload(), so packet->payload has been been casted, so we
don't need to do extra checking for payload size at here.

Will remove the condition checking in next spin.

Thanks a lot!

Leo

> Even if that wouldn't be the case, I'd rather mask it here again, so
> that we can rely on this, and lose the extra check.
> 
> > 
> >> If so, I doubt it's really useful, the
> >> compiler might find a smarter solution to the problem. Just continuing
> >> with the bit mask comparison would make it look nicer, I think.
> > 
> > ARMv8 ARM gives out "Otherwise this bit reads-as-zero.", IIUC this
> > suggests to firstly check the size, if cannot meet the size requirement,
> > then the Event bit should be reads-as-zero.
> 
> But as mentioned above, we take care of this already:
>         switch (payload_len) {
>         case 1: packet->payload = *(uint8_t *)buf; break;
>         case 2: packet->payload = le16_to_cpu(*(uint16_t *)buf); break;
> 	...
> 
> Thanks,
> Andre