Re: [PATCH v2 5/7] arm64: make arm uprobes code reusable by arm64

[Maciej Slodczyk]

Hi,

Thank you for the review.

> I think that it would be good to move the renaming changes out of this 
> patch.
> 

So, as I understand, you suggest separating renaming from moving and 
putting it in separate patches, right?

>> Â })
>> +#define ARM_COMPAT_LR_OFFSETÂÂÂ 0
> 
> Not sure this should be defined here. What's the meaning of compat for 
> arch/arm ?
> 

Sure, I agree that the name is not very fortunate. I'll change it to 
something like ARM_UPROBES_BRANCH_LR_OFFSET.

>> @@ -39,7 +39,7 @@ struct arch_uprobe {
>> ÂÂÂÂÂ void (*posthandler)(struct arch_uprobe *auprobe,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct arch_uprobe_task *autask,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct pt_regs *regs);
>> -ÂÂÂ struct arch_probes_insn asi;
>> +ÂÂÂ struct arch_probes_insn api;
> 
> It would be easier to follow thing by making this change in its own 
> patch. (Probably before you move arm32 code to lib/probes)
> 

Yup.


>> +enum probes_insn {
>> +ÂÂÂ INSN_REJECTED,
>> +ÂÂÂ INSN_GOOD_NO_SLOT,
>> +ÂÂÂ INSN_GOOD,
>> +};
> 
> Why have two definitions of this enum rather than a common one in 
> lib/probes?
> 

Will fix in v3.

>> -typedef void (probes_handler_t) (u32 opcode,
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct arch_probe_insn *api,
>> +typedef void (probes_insn_handler_t) (u32 opcode,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct arch_probes_insn *api,
> 
> In the previous patch you were already aligning this handler the ARM32's 
> equivalent. Why not fix the name (for the handler and struct 
> arch_probes_insn) in the previous patch?
> 

OK.

>> +
>> +#define link_register(regs)ÂÂÂÂÂÂÂÂÂÂÂ ((regs)->compat_lr)
>> +
>> +static inline void link_register_set(struct pt_regs *regs,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long val)
>> +{
>> +ÂÂÂ link_register(regs) = val;
>> +}
> 
> pstate.h isn't really related to compat mode and whichever compat 
> definition it contains the relations are made explicit through their names.
> 
> I don't think a macro "link_register" defined in arch/arm64 and visible 
> to any file including ptrace.h (which is a lot) should return 
> "compat_lr" instead of the actual link register.
> 
> I'd say have the link_register macro check whether "regs" refers to a 
> compat mode context or not and provide the adequate link register.
> 
> Otherwise maybe you can get away with naming the macro 
> "arm_link_register" and the macro "arm_link_register_set". But I would 
> prefer the previous approach.
> 

OK.

>> +#ifdef CONFIG_ARM64
>> +#include <../../../arm/include/asm/opcodes.h>
> 
> Hmmm not sure this is something that is accepted.
> 

OK, I'll fix it.

>> +/*
>> + * based on arm kprobes implementation
>> + */
>> +static void __kprobes simulate_ldm1stm1(probes_opcode_t insn,
>> +ÂÂÂÂÂÂÂ struct arch_probes_insn *asi,
> 
> The whole asi/api mix become a bit confusing IMO.
> Should we have api when the argument is of type "arch_probes_insn" and 
> asi when the type is "arch_specific_insn"?
> Should we have more coherent definitions of those structures between arm 
> and arm64 if we are going to share functions between them?
> 

OK, I'll try to figure something out that's less confusing.

> 
> #ifdef CONFIG_ARM64
> 
>> +enum probes_insn
>> +uprobe_decode_ldmstm_aarch64(probes_opcode_t insn,
>> +ÂÂÂÂÂÂÂÂ struct arch_probes_insn *asi,
>> +ÂÂÂÂÂÂÂÂ const struct decode_header *d)
> 
> Should be static.
> 

OK.

Thanks again for the review. I'll rework the whole patchset to include 
your remarks.


Thank you,
Maciej