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

[Julien Thierry]

Hi,

On 01/10/18 14:28, Maciej Slodczyk wrote:
> 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?

Yes, I just feel this patch has a lot of changes and things like 
renaming/adapting the callback handler and renaming the arm32 stucture 
field could be put in their own patches.

Thanks,

> > >  Â })
> > > +#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

--
Julien Thierry