Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 19 Oct 2020 18:41:15 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     =?iso-8859-1?Q?Andr=E9?= Przywara <andre.przywara@arm.com>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Wei Li <liwei391@huawei.com>,
        James Clark <james.clark@arm.com>,
        Dave Martin <Dave.Martin@arm.com>,
        linux-kernel@vger.kernel.org, Al Grant <Al.Grant@arm.com>
Subject: Re: [PATCH v2 07/14] perf arm-spe: Refactor address packet handling
Message-ID: <20201019104115.GA7187@leoy-ThinkPad-X240s>
References: <20200929133917.9224-1-leo.yan@linaro.org>
 <20200929133917.9224-8-leo.yan@linaro.org>
 <2e9a9499-4596-2a07-4d44-30e319206c12@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2e9a9499-4596-2a07-4d44-30e319206c12@arm.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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