linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jamie Lokier <jamie@shareable.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Will Deacon <will.deacon@arm.com>,
	"Jon Medhurst (Tixy)" <tixy@linaro.org>,
	Russell King - ARM Linux <linux@arm.linux.org.uk>,
	Frederic Weisbecker <fweisbec@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Rabin Vincent <rabin@rab.in>, Ingo Molnar <mingo@redhat.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] ARM: ftrace: Ensure code modifications are synchronised across all cpus
Date: Mon, 10 Dec 2012 13:40:01 +0000	[thread overview]
Message-ID: <20121210134000.GQ26936@jl-vm1.vm.bytemark.co.uk> (raw)
In-Reply-To: <1355144537.17101.155.camel@gandalf.local.home>

Steven Rostedt wrote:
> > Yes, and I think if you do use two 16-bit nops, you can even get rid of all
> > the intermediate `sync' operations (I guess you might want one at the end if
> > you want the call to become visible at a particular point).
> 
> Wont work. We are replacing a 32bit call with a nop. That nop must also
> be 32bits, because we could eventually replace the nop(s) with a 32bit
> call. Basically, we can never allow the second 16bit part ever be the
> next instruction. If the first 16bit nop is executed, and then the task
> gets preempted. The nops get converted to a 32bit call. The task gets
> scheduled again and now is executing the second 16bits of the 32bit call
> and we get unexpected (probably crashing) results.
> 
> By having either a 16bit breakpoint whose handler returns after the
> second 16bit part, or a 16bit jump that simply jumps over the second
> half, then all this should work. When the CPU processes a 32bit
> instruction, it either processes all or non of it, correct?

Sounds good, except what Will wrote a few days ago:

On Fri, 2012-12-07 at 19:02 +0000, Will Deacon wrote:
> For ARMv7, there are small subsets of instructions for ARM and Thumb which
> are guaranteed to be atomic wrt concurrent modification and execution of
> the instruction stream between different processors:
>
> Thumb:      The 16-bit encodings of the B, NOP, BKPT, and SVC instructions.
> ARM:        The B, BL, NOP, BKPT, SVC, HVC, and SMC instructions.

Thumb 32-bit ftrace call isn't in the above list.

Questions: does the above concurrent modification guarantee require
both the old instruction _and_ the new one to be among those listed,
or is it enough to be just the new one (for example when setting a
normal software breakpoint, that would be useful)?  Can it be the old
one and not the new (for example when removing a software breakpoint,
that would be useful)?  Does that subset mean replacing any of the
listed instructions by any of the others is ok, or any of the listed
with another of the same type?

(I guess as a matter of architecture design, it makes sense to
guarantee only a short list, because of occasions when the hardware,
or a software emulation through traps, or a simulation, might read the
instruction memory more than once.)

This is what makes me wonder, if it's safe to replace the 32-bit
mcount call with a 16-bit short jump:

> On Mon, Dec 10, 2012 at 11:04:05AM +0000, Jon Medhurst (Tixy) wrote:
> > So this means for things like kprobes which can modify arbitrary kernel
> > code we are going to need to continue to always use some form of
> > stop_the_whole_system() function?
> >
> > Also, kprobes currently uses patch_text() which only uses stop_machine
> > for Thumb2 instructions which straddle a word boundary, so this needs
> > changing?

Will Deacon replied:
> Yes; if you're modifying instructions other than those mentioned above, then
> you'll need to synchronise the CPUs, update the instructions, perform
> cache-maintenance on the writing CPU and then execute an isb on the
> executing core (this last bit isn't needed if you're going to go through an
> exception return to get back to the new code -- depends on how your
> stop/resume code works).

If I've understood that exchange, it implies that using patch_text()
to replace an instruction not in the list of special ones, with a trap
or jump, isn't ok?  And so it's ok to replace the NOP with a short
branch (since 16-bit "B" is in the list), but it's not ok to replace
16-bit "B" with the 32-bit ftrace call; and the same going the other way?

Best,
-- Jamie

  parent reply	other threads:[~2012-12-10 13:40 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-12-06 18:11 [PATCH] ARM: ftrace: Ensure code modifications are synchronised across all cpus Jon Medhurst (Tixy)
2012-12-06 19:19 ` Steven Rostedt
2012-12-07  9:22   ` Jon Medhurst (Tixy)
2012-12-07 14:03     ` Steven Rostedt
2012-12-07 14:55       ` Jon Medhurst (Tixy)
2012-12-07 15:28         ` Steven Rostedt
2012-12-07 15:40           ` Jon Medhurst (Tixy)
2012-12-07 16:09             ` Steven Rostedt
2012-12-07 16:23           ` Russell King - ARM Linux
2012-12-07 16:36             ` Steven Rostedt
2012-12-07 16:45               ` Russell King - ARM Linux
2012-12-07 17:13                 ` Steven Rostedt
2012-12-07 17:45                   ` Jon Medhurst (Tixy)
2012-12-07 18:06                     ` Steven Rostedt
2012-12-07 18:17                       ` Steven Rostedt
2012-12-07 18:18                       ` Jon Medhurst (Tixy)
2012-12-10 10:04                     ` Will Deacon
2012-12-10 13:02                       ` Steven Rostedt
2012-12-10 13:33                         ` Will Deacon
2012-12-10 13:40                         ` Jamie Lokier [this message]
2012-12-10 14:56                           ` Will Deacon
2012-12-10 13:57                         ` Russell King - ARM Linux
2012-12-10 14:06                           ` Steven Rostedt
2012-12-10 14:07                             ` Russell King - ARM Linux
2012-12-10 14:46                               ` Steven Rostedt
2012-12-10 15:25                                 ` Russell King - ARM Linux
2012-12-10 16:31                                   ` Steven Rostedt
2012-12-10 16:45                       ` Jon Medhurst (Tixy)
2012-12-07 18:13                   ` Russell King - ARM Linux
2012-12-07 18:43                     ` Steven Rostedt
2012-12-07 19:02                       ` Will Deacon
2012-12-07 20:01                         ` Steven Rostedt
2012-12-10 11:04                         ` Jon Medhurst (Tixy)
2012-12-10 11:24                           ` Will Deacon
2012-12-10 14:02                             ` Steven Rostedt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20121210134000.GQ26936@jl-vm1.vm.bytemark.co.uk \
    --to=jamie@shareable.org \
    --cc=fweisbec@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=mingo@redhat.com \
    --cc=rabin@rab.in \
    --cc=rostedt@goodmis.org \
    --cc=tixy@linaro.org \
    --cc=will.deacon@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).