Re: [PATCH v2 07/14] perf arm-spe: Refactor address packet handling

[Leo Yan <leo.yan@linaro.org>]

Hi Andre,

On Mon, Oct 19, 2020 at 10:01:28AM +0100, André Przywara wrote:

[...]

> > diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> > index cc18a1e8c212..9d3de163d47c 100644
> > --- a/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> > +++ b/tools/perf/util/arm-spe-decoder/arm-spe-decoder.c
> > @@ -24,36 +24,37 @@
> >  
> >  static u64 arm_spe_calc_ip(int index, u64 payload)
> >  {
> > -	u8 *addr = (u8 *)&payload;
> > -	int ns, el;
> > +	u64 ns, el;
> 
> This (and the "u64 vs. u8[]" changes below) looks like a nice cleanup.
>
> >  	/* Instruction virtual address or Branch target address */
> >  	if (index == SPE_ADDR_PKT_HDR_INDEX_INS ||
> >  	    index == SPE_ADDR_PKT_HDR_INDEX_BRANCH) {
> > -		ns = addr[7] & SPE_ADDR_PKT_NS;
> > -		el = (addr[7] & SPE_ADDR_PKT_EL_MASK) >> SPE_ADDR_PKT_EL_OFFSET;
> > +		ns = payload & SPE_ADDR_PKT_INST_VA_NS;
> > +		el = (payload & SPE_ADDR_PKT_INST_VA_EL_MASK)
> > +			>> SPE_ADDR_PKT_INST_VA_EL_SHIFT;
> 
> So if I see this correctly, this _EL_SHIFT and _EL_MASK are only used
> together, and only to read values, not to construct them.
> So can you fuse them together in the header file below, like:
> 	el = SPE_ADDR_PKT_INST_VA_GET_EL(payload);

Agreed that this is more neat.

> That should help readablity, I guess, while still keeping the actual
> numbers in one place. _SHIFT and _MASK are useful when we use them to
> both extract *and construct* values, but here we only parse the buffer.
> 
> Similar for other places where you just extract bits from a bitfield or
> integer.

Will apply the suggestion crossing the patch set.

Thanks a lot for your reviewing and suggestions.

Leo