| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190905015032.GA10141@leoy-ThinkPad-X240s>
Date: Thu, 5 Sep 2019 09:50:32 +0800
From: Leo Yan <leo.yan@...aro.org>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
Suzuki K Poulose <suzuki.poulose@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...hat.com>,
Namhyung Kim <namhyung@...nel.org>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Mike Leach <mike.leach@...aro.org>,
Robert Walker <Robert.Walker@....com>,
Adrian Hunter <adrian.hunter@...el.com>
Subject: Re: [PATCH v1 1/3] perf cs-etm: Refactor instruction size handling
Hi Mathieu,
On Wed, Sep 04, 2019 at 11:06:10AM -0600, Mathieu Poirier wrote:
> On Wed, 4 Sep 2019 at 03:19, Leo Yan <leo.yan@...aro.org> wrote:
> >
> > Hi Mathieu,
> >
> > On Tue, Sep 03, 2019 at 04:22:15PM -0600, Mathieu Poirier wrote:
> > > On Fri, Aug 30, 2019 at 02:24:19PM +0800, Leo Yan wrote:
> > > > There has several code pieces need to know the instruction size, but
> > > > now every place calculates the instruction size separately.
> > > >
> > > > This patch refactors to create a new function cs_etm__instr_size() as
> > > > a central place to analyze the instruction length based on ISA type
> > > > and instruction value.
> > > >
> > > > Signed-off-by: Leo Yan <leo.yan@...aro.org>
> > > > ---
> > > > tools/perf/util/cs-etm.c | 44 +++++++++++++++++++++++++++-------------
> > > > 1 file changed, 30 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > > > index b3a5daaf1a8f..882a0718033d 100644
> > > > --- a/tools/perf/util/cs-etm.c
> > > > +++ b/tools/perf/util/cs-etm.c
> > > > @@ -914,6 +914,26 @@ static inline int cs_etm__t32_instr_size(struct cs_etm_queue *etmq,
> > > > return ((instrBytes[1] & 0xF8) >= 0xE8) ? 4 : 2;
> > > > }
> > > >
> > > > +static inline int cs_etm__instr_size(struct cs_etm_queue *etmq,
> > > > + u8 trace_chan_id,
> > > > + enum cs_etm_isa isa,
> > > > + u64 addr)
> > > > +{
> > > > + int insn_len;
> > > > +
> > > > + /*
> > > > + * T32 instruction size might be 32-bit or 16-bit, decide by calling
> > > > + * cs_etm__t32_instr_size().
> > > > + */
> > > > + if (isa == CS_ETM_ISA_T32)
> > > > + insn_len = cs_etm__t32_instr_size(etmq, trace_chan_id, addr);
> > > > + /* Otherwise, A64 and A32 instruction size are always 32-bit. */
> > > > + else
> > > > + insn_len = 4;
> > > > +
> > > > + return insn_len;
> > > > +}
> > > > +
> > > > static inline u64 cs_etm__first_executed_instr(struct cs_etm_packet *packet)
> > > > {
> > > > /* Returns 0 for the CS_ETM_DISCONTINUITY packet */
> > > > @@ -938,19 +958,23 @@ static inline u64 cs_etm__instr_addr(struct cs_etm_queue *etmq,
> > > > const struct cs_etm_packet *packet,
> > > > u64 offset)
> > > > {
> > > > + int insn_len;
> > > > +
> > > > if (packet->isa == CS_ETM_ISA_T32) {
> > > > u64 addr = packet->start_addr;
> > > >
> > > > while (offset > 0) {
> > > > - addr += cs_etm__t32_instr_size(etmq,
> > > > - trace_chan_id, addr);
> > > > + addr += cs_etm__instr_size(etmq, trace_chan_id,
> > > > + packet->isa, addr);
> > > > offset--;
> > > > }
> > > > return addr;
> > > > }
> > > >
> > > > - /* Assume a 4 byte instruction size (A32/A64) */
> > > > - return packet->start_addr + offset * 4;
> > > > + /* Return instruction size for A32/A64 */
> > > > + insn_len = cs_etm__instr_size(etmq, trace_chan_id,
> > > > + packet->isa, packet->start_addr);
> > > > + return packet->start_addr + offset * insn_len;
> > >
> > > This patch will work but from where I stand it makes things difficult to
> > > understand more than anything else. It is also adding coupling between function
> > > cs_etm__instr_addr() and cs_etm__instr_size(), meaning the code needs to be
> > > carefully inspected in order to make changes to either one.
> >
> > My purpose is to use a same place to calculate the instruction
> > size, rather than to spread the duplicate codes in several different
> > functions.
> >
> > > Last but not least function cs_etm__instr_size() isn't used in the upcoming
> > > patches. I really don't see what is gained here.
> >
> > Sorry that I forgot to commit my final change into patch 02.
> >
> > I planed to use cs_etm__instr_size() in patch 02; patch 02 has
> > function cs_etm__add_stack_event(), which also needs to get the
> > instruction size when it sends stack event.
> >
> > After apply patch 02, tools/perf/util/cs-etm.c will have below three
> > functions to caculate instruction size; this is the main reason I want
> > to refactor the code for instruction size.
> >
> > cs_etm__instr_addr()
> > cs_etm__copy_insn()
> > cs_etm__add_stack_event()
> >
> > If this lets code more difficult to understand, will drop it.
> >
>
> I agree with the consolidation but for that to work function
> cs_etm__instr_addr() needs to be refactored. Since
> cs_etm__instr_size() is already taking care of checking the ISA type
> the while() loop in cs_etm__instr_addr() can be done regardless of the
> operation mode. That way cs_etm__instr_size() can be changed at will
> without breaking anything.
>
> The downside is that we are doing a few more iterations... Which isn't
> that big a deal considering we are in user space. We can think about
> some optimisation if it is ever proven to be a bottleneck.
>
> Let me know if you see a problem with that approach.
Yes, your approach is neat; I will try it in next patch version.
Thanks a lot for suggestion!
[...]
Powered by blists - more mailing lists