| 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: <CANLsYkwZXyzWqK1oiEg0hb99J89KfdNPsFOmS7G1XJ0xvoUc0Q@mail.gmail.com>
Date: Wed, 4 Sep 2019 11:06:10 -0600
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Leo Yan <leo.yan@...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
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.
Regards,
Mathieu
> Thanks,
> Leo Yan
>
> > > }
> > >
> > > static void cs_etm__update_last_branch_rb(struct cs_etm_queue *etmq,
> > > @@ -1090,16 +1114,8 @@ static void cs_etm__copy_insn(struct cs_etm_queue *etmq,
> > > return;
> > > }
> > >
> > > - /*
> > > - * T32 instruction size might be 32-bit or 16-bit, decide by calling
> > > - * cs_etm__t32_instr_size().
> > > - */
> > > - if (packet->isa == CS_ETM_ISA_T32)
> > > - sample->insn_len = cs_etm__t32_instr_size(etmq, trace_chan_id,
> > > - sample->ip);
> > > - /* Otherwise, A64 and A32 instruction size are always 32-bit. */
> > > - else
> > > - sample->insn_len = 4;
> > > + sample->insn_len = cs_etm__instr_size(etmq, trace_chan_id,
> > > + packet->isa, sample->ip);
> > >
> > > cs_etm__mem_access(etmq, trace_chan_id, sample->ip,
> > > sample->insn_len, (void *)sample->insn);
> > > --
> > > 2.17.1
> > >
Powered by blists - more mailing lists