lkml.org 
[lkml]   [2018]   [Oct]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 5/7] arm64: make arm uprobes code reusable by arm64
From
Date
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

\
 
 \ /
  Last update: 2018-10-01 15:29    [W:0.235 / U:0.336 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site