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
