lkml.org 
[lkml]   [2013]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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;
    >
    > [...]
    >



    \
     
     \ /
      Last update: 2013-11-15 18:01    [W:4.000 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site