[PATCH v4 05/16] ARM: use a function table for determining instruction interpreter actions

[dave.long@linaro.org (David Long)]

On 12/20/13 07:45, Jon Medhurst (Tixy) wrote:
> On Sun, 2013-12-15 at 23:08 -0500, David Long wrote:
>> From: "David A. Long" <dave.long@linaro.org>
>>
>> Make the instruction interpreter call back to semantic action functions
>> through a function pointer array provided by the invoker.  The interpreter
>> decodes the instructions into groups and uses the group number to index
>> into the supplied array.  kprobes and uprobes code will each supply their
>> own array of functions.
>>
>> Signed-off-by: David A. Long <dave.long@linaro.org>
>> ---
>
> Because I've been very slow in reviewing these I've only just noticed
> that some of the the comments I made on version one of this patch didn't
> get a response. I've copied them again below (slightly edited)  and
> heavily trimmed the patch...
>
>>   arch/arm/kernel/kprobes-arm.c    |  41 +++++++++++
>>   arch/arm/kernel/kprobes-common.c |   3 +-
>>   arch/arm/kernel/kprobes-thumb.c  |  92 ++++++++++++++++++------
>>   arch/arm/kernel/kprobes.c        |  10 ++-
>>   arch/arm/kernel/kprobes.h        |  14 ++--
>>   arch/arm/kernel/probes-arm.c     | 114 +++++++++++++++---------------
>>   arch/arm/kernel/probes-arm.h     |  37 ++++++++++
>>   arch/arm/kernel/probes-thumb.c   | 149 +++++++++++++++++++--------------------
>>   arch/arm/kernel/probes-thumb.h   |  14 ++--
>>   arch/arm/kernel/probes.c         |  13 ++--
>>   arch/arm/kernel/probes.h         |  15 ++--
>>   11 files changed, 325 insertions(+), 177 deletions(-)
>>
>> diff --git a/arch/arm/kernel/kprobes-arm.c b/arch/arm/kernel/kprobes-arm.c
>> index a359475..ee329ff 100644
>> --- a/arch/arm/kernel/kprobes-arm.c
>> +++ b/arch/arm/kernel/kprobes-arm.c
>> @@ -299,3 +299,44 @@ emulate_rdlo12rdhi16rn0rm8_rwflags_nopc(struct kprobe *p, struct pt_regs *regs)
>>   	regs->uregs[rdhi] = rdhiv;
>>   	regs->ARM_cpsr = (regs->ARM_cpsr & ~APSR_MASK) | (cpsr & APSR_MASK);
>>   }
>> +
>> +const union decode_item kprobes_arm_actions[] = {
>
> I think it's best if we don't reuse the decode_item type here, this is a
> different sort of table so probably best to have it's own union. Also,
> if we do that, then decode_item can be simplified as it won't need to
> have function pointers in it, i.e. we could end up with...
>
> union decode_action {
>          kprobe_insn_handler_t   *handler;
>          kprobe_custom_decode_t  *decoder;
> };
>
> union decode_item {
>          u32                     bits;
>          const union decode_item *table;
> };
>
> typedef enum kprobe_insn (kprobe_custom_decode_t)(kprobe_opcode_t,
>                                                    struct arch_specific_insn *,
>                                                    union decode_action *actions);
>
>

I've added the following:

typedef enum kprobe_insn (probes_custom_decode_t)(kprobe_opcode_t,
		struct arch_specific_insn *,
		struct decode_header *);

union decode_action {
	kprobe_insn_handler_t	*handler;
	probes_custom_decode_t	*decoder;
};

Note the third argument actually passed into the decoder functions is 
the decode table entry.  decode_action is only used to select a 
decode/emulate/simullate function.

> A second point, I think it would be a good idea to make sure these
> action arrays are the size we expect by adding an entry at the end of
> the relevant enumeration and using that to set the size of the arrays.
> E.g. for this one
>
> enum probes_arm_action {
>          ...
>          ...
>          NUM_PROBES_ARM_ACTIONS
> };
>
> and then use it like:
>
> const union decode_action kprobes_arm_actions[NUM_PROBES_ARM_ACTIONS] = {
>
> That way, we at least make any uninitialised entries are null (I
> assume?) which is safer than accidentally indexing beyond the array.
>
>

Done.

>> +	[PROBES_EMULATE_NONE] = {.handler = kprobe_emulate_none},
>> +	[PROBES_SIMULATE_NOP] = {.handler = kprobe_simulate_nop},
>> +	[PROBES_PRELOAD_IMM] = {.handler = kprobe_simulate_nop},
>
> [...]
>
>
>> diff --git a/arch/arm/kernel/probes.h b/arch/arm/kernel/probes.h
>> index d14d224..2238972 100644
>> --- a/arch/arm/kernel/probes.h
>> +++ b/arch/arm/kernel/probes.h
>> @@ -131,7 +131,8 @@ void __kprobes kprobe_simulate_nop(struct kprobe *p, struct pt_regs *regs);
>>   void __kprobes kprobe_emulate_none(struct kprobe *p, struct pt_regs *regs);
>>
>>   enum kprobe_insn __kprobes
>> -kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi);
>> +kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi,
>> +		struct decode_header *h);
>>
>>   /*
>>    * Test if load/store instructions writeback the address register.
>> @@ -334,7 +335,7 @@ struct decode_custom {
>>
>>   #define DECODE_CUSTOM(_mask, _value, _decoder)			\
>>   	DECODE_HEADER(DECODE_TYPE_CUSTOM, _mask, _value, 0),	\
>> -	{.decoder = (_decoder)}
>> +	{.bits = (_decoder)}
>>
>
> This third and final comment is probably just bike shedding...
>
> 'bits' looks a bit funny here. I've been trying to think of a way of
> making it nicer but it's difficult. The actual value is one of three
> different enums, so if we were to add another members to decode_item it
> would just have to be "int action", at least that would read nicer in
> these macros and where it gets read out in probes_decode_insn.
>

I agree.  I've added an "int action" to the decode_item union, and use 
it instead of "bits" for the action array index.

I've also updated the description of how this all works, in the comments.

-dl