Message-ID: <20191030141950.GB21153@leoy-ThinkPad-X240s>
Date:   Wed, 30 Oct 2019 22:19:50 +0800
From:   Leo Yan <leo.yan@...aro.org>
To:     Peter Zijlstra <peterz@...radead.org>,
        Mike Leach <mike.leach@...aro.org>
Cc:     Adrian Hunter <adrian.hunter@...el.com>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 1/6] perf/x86: Add perf text poke event

Hi Peter,

[ + Mike Leach ]

On Wed, Oct 30, 2019 at 01:46:59PM +0100, Peter Zijlstra wrote:
> On Wed, Oct 30, 2019 at 06:47:47PM +0800, Leo Yan wrote:
> > Hi Adrian,
> > 
> > On Fri, Oct 25, 2019 at 03:59:55PM +0300, Adrian Hunter wrote:
> > > Record changes to kernel text (i.e. self-modifying code) in order to
> > > support tracers like Intel PT decoding through jump labels.
> > > 
> > > A copy of the running kernel code is needed as a reference point
> > > (e.g. from /proc/kcore). The text poke event records the old bytes
> > > and the new bytes so that the event can be processed forwards or backwards.
> > > 
> > > The text poke event has 'flags' to describe when the event is sent. At
> > > present the only flag is PERF_TEXT_POKE_UPDATE which indicates the
> > > point at which tools should update their reference kernel executable to
> > > change the old bytes to the new bytes.
> > > 
> > > Other architectures may wish to emit a pair of text poke events. One before
> > > and one after a text poke. In that case, PERF_TEXT_POKE_UPDATE flag would
> > > be set on only one of the pair.
> > 
> > Thanks a lot for the patch set.
> > 
> > Below is my understanding for implementation 'emit a pair of text poke
> > events' as you mentioned:
> > 
> > Since Arm64 instruction is fixed size, it doesn't need to rely on INT3
> > liked mechanism to replace instructions and directly uses two operations
> > to alter instruction (modify instruction and flush icache line).  So
> > Arm64 has no chance to send perf event in the middle of altering
> > instruction.
> 
> Right.
> 
> > Thus we can send pair of text poke events in the kernel:
> > 
> >   perf_event_text_poke(PERF_TEXT_POKE_UPDATE_PREV)
> > 
> >     Change instruction
> >     Flush icache
> > 
> >   perf_event_text_poke(PERF_TEXT_POKE_UPDATE_POST)
> > 
> > In the userspace, if perf tool detects the instruction is changed
> > from nop to branch,
> 
> There is _far_ more text poking than just jump_label's NOP/JMP
> transitions. Ftrace also does NOP/CALL, CALL/CALL, and the static_call
> infrastructure that I posted here:
> 
>   https://lkml.kernel.org/r/20191007082708.013939311@infradead.org
> 
> Has: JMP/RET, JMP/JMP and if it has inline patching support: NOP/CALL,
> CALL/CALL, patching.

Thanks for the info.  I took a bit time to look your patch set and
Steven's patch set for dynamic function, though don't completely
understand, but get more sense for the context.

> Anyway, the below argument doesn't care much, it works for NOP/JMP just
> fine.

We can support NOP/JMP case as the first step, but later should can
extend to support other transitions.

> > we need to update dso cache for the
> > 'PERF_TEXT_POKE_UPDATE_PREV' event; if detect the instruction is
> > changed from branch to nop, we need to update dso cache for
> > 'PERF_TEXT_POKE_UPDATE_POST' event.  The main idea is to ensure the
> > branch instructions can be safely contained in the dso file and any
> > branch samples can read out correct branch instruction.
> > 
> > Could you confirm this is the same with your understanding?  Or I miss
> > anything?  I personally even think the pair events can be used for
> > different arches (e.g. the solution can be reused on Arm64/x86, etc).
> 
> So the problem we have with PT is that it is a bit-stream of
> branch taken/not-taken decisions. In order to decode that we need to
> have an accurate view of the unconditional code flow.
> 
> Both NOP/JMP are unconditional and we need to exactly know which of the
> two was encountered.

If I understand correctly, PT decoder needs to read out instructions
from dso and decide the instruction type (NOP or JMP), and finally
generate the accurate code flow.

So PT decoder relies on (cached) DSO for decoding.  As I know, this
might be different from Arm CS, since Arm CS decoder is merely
generate packets and it doesn't need to rely on DSO for decoding.

I think my answer is not very convinced, in case I mislead, loop Mike
to confirm for this.

> With your scheme, I don't see how we can ever actually know that. When
> we get the PRE event, all we really know is that we're going to change
> a specific instruction into another. And at the POST event we know it
> has been done. But in between these two events, we have no clue which of
> the two instructions is live on which CPU (two CPUs might in fact have a
> different live instruction at the same time).
>
> This means we _cannot_ unambiguously decode a taken/not-taken decision
> stream.
> 
> Does CS have this same problem, and how would the PRE/POST events help
> with that?

My purpose is to use PRE event and POST event to update cached DSO,
thus perf tool can read out 'correct' instructions and fill them into
instruction/branch samples.  On the other hand, as mentioned, I don't
aware the instruction read out from DSO will be used for Arm CS decoder;
Arm CS also reads the instructions from DSO, usually it's used to decide
instruction size or decide the sample flags (flags for CALL/RETURN/hw
int, etc...), but it isn't used for decoder.

@Mike, please help confirm for this as well, from Arm CoreSight's
decoder pespective.

> So our (x86) horrible (variable instruction length induced) complexity
> for text poking is actually in our advantage this one time. The
> exception and exception-return paths allow us to unambiguously know what
> happens around the time of poking.

Thanks a lot for the info!
Leo.

Powered by blists - more mailing lists