Re: [PATCH RFC 1/6] perf/x86: Add perf text poke event

[Will Deacon]

As a disclaimer: I'm having a hard time understanding what this thread is
about.

On Mon, Nov 11, 2019 at 03:46:42PM +0100, Peter Zijlstra wrote:
> On Fri, Nov 08, 2019 at 11:05:30PM +0800, Leo Yan wrote:
> 
> > I will update some status for prototype (the code has been uploaded into
> > git server [1]) and found some issues for text poke perf event on arm64.
> > These issues are mainly related with arch64 architecture.
> > 
> > - The first issue is for the nosync instruction emulation.  On arm64,
> >   some instructions need to be single stepped and some instructions
> >   is emulated.  Especially, after I read the code for kprobe
> >   implementation on Arm64.  So the main idea for prototyping is to use
> >   the almos same method with kprobe for nosync instruction.
> 
> This makes no sense to me what so ever. What actual instructions are
> patched with _nosync() ? ftrace/jump_label only use 'NOP/JMP/CALL'

'_nosync()' can be used to patch the following instructions:

	B		(JMP)
	BL		(CALL)
	BRK		(Breakpoint)
	SVC, HVC, SMC	(System calls)
	NOP
	ISB		(Pipe flush)

Our kprobes implementation prefers to single-step the instruction in an
XOL buffer (we should do this by placing a BRK at the end of the buffer,
but we currently use hardware step which is overkill imo). For instructions
that are performing a PC-relative operation (e.g. an immediate branch),
we can't run from the XOL buffer because we'd go to the wrong place; these
instructions are therefore emulated in software...

> For NOP you can advance regs->ip, for JMP you can adjust regs->ip, for
> CALL you adjust regs->ip and regs->r14 (IIUC you do something like:
> regs->r14 = regs->ip+4; regs->ip = func;)

... in a manner similar to what you describe here. See simulate_b_bl(),
although it's thankfully simpler than what x86 seems to have to do. This
approach means we can avoid emulating the vast majority of the instruction
set.

> (FWIW emulating CALL on x86 is fun because we get to PUSH something on
> the stack, let me know if you want to see the patches that were required
> to make that happen :-)

No thanks ;)

> > - The second issue is race condition between the CPU alters
> >   instructions and other CPUs hit the altered instructions (and
> >   breakpointed).
> > 
> >   Peter's suggestion uses global variables 'nosync_insn' and
> >   'nosync_addr' to record the altered instruction.  But from the
> >   testing I found for single static key, usually it will change for
> >   multiple address at once.
> > 
> >   So this might cause the issue is: CPU(a) will loop to alter
> >   instructions for different address (sometimes the opcode also is
> >   different for different address), at the meantime, CPU(b) hits an
> >   altered instruction and handle exception for the breakpoint, if
> >   CPU(a) is continuing to alter instruction for the next address, thne
> >   CPU(a) might wrongly to use the value from 'nosync_insn' and
> >   'nosync_addr'.
> > 
> >   Simply to say, we cannot only support single nosync instruction but
> >   need to support multiple nosync instructions in the loop.
> 
> On x86 all actual text poking is serialized by text_mutex.

On arm64, we patch a *lot* and I think stop_machine() is the only safe
choice for things like the alternatives, where we patch all sorts of things
(cache maintenance, I/O accessors, atomics (until recently), user accessors.
Even then, we have to provide our own synchronisation in
__apply_alternatives_multi_stop() and use special cache-flushing
(clean_dcache_range_nopatch()) to avoid the possibility of running the code
that we're modifying. Outside of the alternatives, I'd still be wary about
recursive debug exceptions if we were to patch jump labels with breakpoints.

Backing up though, I think I'm missing details about what this thread is
trying to achieve. You're adding perf events so that coresight trace can
take into account modifications of the kernel text, right? If so:

  * Does this need to take into account more than just jump_label()?
  * What consistency guarantees are needed by userspace? It's not clear to
    me how it correlates the new event with execution on other CPUs. Is this
    using timestamps or something else?
  * What about module loading?

Finally, the whole point of the '_nosync()' stuff is that we don't have
to synchronise. Therefore, there is a window where you really can't tell
whether the instruction has been updated from the perspective of another
CPU.

Will