Re: [PATCH v4 2/2] powerpc: Uprobes port to powerpc

[Srikar Dronamraju <srikar@linux.vnet.ibm.com>]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> [2012-08-23 20:06:18]:

> On Thu, 2012-08-23 at 11:02 +0530, Srikar Dronamraju wrote:
> > > 
> > 
> > insn is updated/accessed in the arch independent code. Size of
> > uprobe_opcode_t could be different for different archs.
> > uprobe_opcode_t
> > represents the size of the smallest breakpoint instruction for an
> > arch.
> > 
> > Hence u8 works out the best. I know we could still use uprobe_opcode_t
> > and achieve the same. In which case, we would have to interpret
> > MAX_UINSN_BYTES differently. Do you see any advantages of using
> > uprobe_opcode_t instead of u8 across archs?
> 
> But don't you actively rely on the fact that on powerpc, unlike x86, you
> -can- atomically replace an instruction with a single 32-bit store ?
> 

We are not doing a replace here, we are only copying from the ->vm_file
for the largest size instruction possible for that instruction. For
powerpc, this is easy because of fixed size instructions.  

On other archs, at this point, we dont even know the length of the
underlying instruction.

Now there are 3 ways to handle this:
1. use arch independent copy_insn() (current.) (handles if the
instruction spreads across multiple pages on non fixed instruction
archs). 

2. make the copy_insn() arch specific, that would mean every arch will
have to do read_mapping_page etc.

3. have a arch specific hook in arch independent copy_insn code that
either does a memcpy for non fixed instruction archs or does an
assignment in archs like powerpc.

I think you are suggesting option 3.
But instead of adding another call that does the arch specific stuff, we
are probably be better of doing a memcpy. Right?

For all powerpc references to insn we could refer to it as u32 as
suggested by Oleg.

-- 
Thanks and Regards
Srikar


> If you don't you should consider it, and that makes defining this as a
> u8 array non-sensical (as is using memcpy)
> 
> Ben.
> 
>