Messages in this thread | | | Date | Mon, 19 Oct 2020 18:41:15 +0800 | From | Leo Yan <> | Subject | Re: [PATCH v2 07/14] perf arm-spe: Refactor address packet handling |
| |
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
| |