Messages in this thread |  | | Date | Fri, 15 Nov 2013 10:45:59 -0500 | From | David Long <> | Subject | Re: [PATCH v2 11/13] ARM: Finish renaming ARM kprobes APIs for sharing with uprobes |
| |
On 11/13/13 12:16, Jon Medhurst (Tixy) wrote: > On Tue, 2013-10-15 at 17:04 -0400, David Long wrote: >> From: "David A. Long" <dave.long@linaro.org> >> >> Use the prefix "probes_" for APIs and definitions to be shared between >> kprobes and uprobes. Pass additional information into the lower-level decoding >> functions to avoid having to duplicate architecture-specfific data structures. >> Make susbsystem-specific APIs static (non-global) again. Make a couple of utility >> functions (probes_simulate_nop and probes_emulate_none) sharable and in a common >> file. Keep the current "action" tables specific to kprobes, as this is where >> uprobes functionality will be connected up to the instruction interpreter. > > That's a rather long list of things this patch is reorganising and makes > it very difficult to review. For those class of changes which are > independent of each other, can you do them as separate patches? > > Particularly, the change from using the function arguments > > struct kprobe *p > > to > probes_opcode_t opcode, probes_opcode_t *addr, struct arch_specific_insn *asi > > should be in it's own separate patch as that is spread out across multiple files > and functions.
OK, I will break this down further into separate patches.
> I have a few general comments on this patch... > > 1. I don't think we actually need to pass 'probes_opcode_t *addr' to the > simulation functions, this will be the same as regs->ARM_pc (except that > for thumb code 'addr' has bit0 set, which we don't actually care about). > So drop the new 'addr' argument, and we can also delete the definition of > thumb_probe_pc() and replace its use with 'regs->ARM_pc + 4'
I had some trouble getting that to work before, but I'll try again.
> 2. Now we pass 'probes_opcode_t opcode' to simulation functions a lot of > them end up doing the redundant... > > kprobe_opcode_t insn = opcode > > I would suggest deleting all of these assignments and either rename all > use of 'insn' to 'opcode' or just changing the function argument name to > 'insn'.
OK, I'll have a go at that.
> 3. I've a comment about the change to probes_decode_insn, I've snipped > the rest of the patch... > > [...] >> int __kprobes >> -kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi, >> +probes_decode_insn(probes_opcode_t insn, struct arch_specific_insn *asi, >> const union decode_item *table, bool thumb, >> - const union decode_item *actions) >> + bool usermode, const union decode_item *actions) >> { > > The new argument 'usermode' is named for the use you are using it for, > i.e. uprobes, I think the code is more intuitive if it was called > something like 'no_emulate', because what the argument does is force all > emulate operations to become 'custom'. > > Perhaps to avoid double negatives like !no_emulate, the logic of the > argument could be inverted and it called 'use_emulation' or something.
Or maybe just "emulate".
>> - const struct decode_header *h = (struct decode_header *)table; >> - const struct decode_header *next; >> + struct decode_header *h = (struct decode_header *)table; >> + struct decode_header *next; >> bool matched = false; >> >> - insn = prepare_emulated_insn(insn, asi, thumb); >> + if (!usermode) >> + insn = prepare_emulated_insn(insn, asi, thumb); >> >> for (;; h = next) { >> enum decode_type type = h->type_regs.bits & DECODE_TYPE_MASK; >> @@ -401,7 +412,7 @@ kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi, >> if (!matched && (insn & h->mask.bits) != h->value.bits) >> continue; >> >> - if (!decode_regs(&insn, regs)) >> + if (!decode_regs(&insn, regs, !usermode)) >> return INSN_REJECTED; >> >> switch (type) { >> @@ -414,7 +425,8 @@ kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi, >> >> case DECODE_TYPE_CUSTOM: { >> struct decode_custom *d = (struct decode_custom *)h; >> - return actions[d->decoder.bits].decoder(insn, asi, h); >> + return actions[d->decoder.bits].decoder(insn, >> + asi, h); >> } >> >> case DECODE_TYPE_SIMULATE: { >> @@ -425,6 +437,11 @@ kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi, >> >> case DECODE_TYPE_EMULATE: { >> struct decode_emulate *d = (struct decode_emulate *)h; >> + >> + if (usermode) >> + return actions[d->handler.bits].decoder(insn, >> + asi, h); >> + >> asi->insn_handler = actions[d->handler.bits].handler; >> set_emulated_insn(insn, asi, thumb); >> return INSN_GOOD; > > [...] >
|  |