Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions

[Nicholas Piggin <npiggin@gmail.com>]

Jordan Niethe's on February 28, 2020 1:23 pm:
> On Fri, Feb 28, 2020 at 12:48 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Jordan Niethe's on February 27, 2020 10:58 am:
>> > On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>> >>
>> >> Jordan Niethe's on February 26, 2020 2:07 pm:
>> >> > @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
>> >> >       }
>> >> >
>> >> >       if (!ret) {
>> >> > -             patch_instruction(p->ainsn.insn, *p->addr);
>> >> > +             patch_instruction(&p->ainsn.insn[0], p->addr[0]);
>> >> > +             if (IS_PREFIX(insn))
>> >> > +                     patch_instruction(&p->ainsn.insn[1], p->addr[1]);
>> >> >               p->opcode = *p->addr;
>> >>
>> >> Not to single out this hunk or this patch even, but what do you reckon
>> >> about adding an instruction data type, and then use that in all these
>> >> call sites rather than adding the extra arg or doing the extra copy
>> >> manually in each place depending on prefix?
>> >>
>> >> instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
>> >> etc., would all take this new instr. Places that open code a memory
>> >> access like your MCE change need some accessor
>> >>
>> >>                instr = *(unsigned int *)(instr_addr);
>> >> -               if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
>> >> +               if (IS_PREFIX(instr))
>> >> +                       suffix = *(unsigned int *)(instr_addr + 4);
>> >>
>> >> Becomes
>> >>                read_instr(instr_addr, &instr);
>> >>                if (!analyse_instr(&op, &tmp, instr)) ...
>> >>
>> >> etc.
>> > Daniel Axtens also talked about this and my reasons not to do so were
>> > pretty unconvincing, so I started trying something like this. One
>>
>> Okay.
>>
>> > thing I have been wondering is how pervasive should the new type be.
>>
>> I wouldn't mind it being quite pervasive. We have to be careful not
>> to copy it directly to/from memory now, but if we have accessors to
>> do all that with, I think it should be okay.
>>
>> > Below is data type I have started using, which I think works
>> > reasonably for replacing unsigned ints everywhere (like within
>> > code-patching.c). In a few architecture independent places such as
>> > uprobes which want to do ==, etc the union type does not work so well.
>>
>> There will be some places you have to make the boundary. I would start
>> by just making it a powerpc thing, but possibly there is or could be
>> some generic helpers. How does something like x86 cope with this?
> 
> One of the places I was thinking of was is_swbp_insn() in
> kernel/events/uprobes.c. The problem was I wanted to typedef
> uprobe_opcode_t as ppc_insn type which was probably the wrong thing to
> do. x86 typedef's it as u8 (size of the trap they use). So we probably
> can do the same thing and just keep it as a u32.

Sounds good.

>> > I will have the next revision of the series start using a type.
>>
>> Thanks for doing that.
>>
>> >
>> > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
>> > new file mode 100644
>> > index 000000000000..50adb3dbdeb4
>> > --- /dev/null
>> > +++ b/arch/powerpc/include/asm/inst.h
>> > @@ -0,0 +1,87 @@
>> > +
>> > +#ifndef _ASM_INST_H
>> > +#define _ASM_INST_H
>> > +
>> > +#ifdef __powerpc64__
>> > +
>> > +/* 64  bit Instruction */
>> > +
>> > +typedef struct {
>> > +    unsigned int prefix;
>> > +    unsigned int suffix;
>>
>> u32?
> Sure.
>>
>> > +} __packed ppc_prefixed_inst;
>> > +
>> > +typedef union ppc_inst {
>> > +    unsigned int w;
>> > +    ppc_prefixed_inst p;
>> > +} ppc_inst;
>>
>> I'd make it a struct and use nameless structs/unions inside it (with
>> appropriate packed annotation):
> Yeah that will be nicer.
>>
>> struct ppc_inst {
>>     union {
>>         struct {
>>             u32 word;
>>             u32 pad;
>>         };
>>         struct {
>>             u32 prefix;
>>             u32 suffix;
>>         };
>>     };
>> };
>>
>> > +
>> > +#define PPC_INST_IS_PREFIXED(inst) (((inst).w >> 26) == 1)
>> > +#define PPC_INST_LEN(inst) (PPC_INST_IS_PREFIXED((inst)) ?
>> > sizeof((inst).p) : sizeof((inst).w))
>>
>> Good accessors, I'd make them all C inline functions and lower case.
> Will change.
>>
>> > +
>> > +#define PPC_INST_NEW_WORD(x) ((ppc_inst) { .w = (x) })
>> > +#define PPC_INST_NEW_WORD_PAD(x) ((ppc_inst) { .p.prefix = (x),
>> > .p.suffix = (0x60000000) })
>> > +#define PPC_INST_NEW_PREFIXED(x, y) ((ppc_inst) { .p.prefix = (x),
>> > .p.suffix = (y) })
>>
>> If these are widely used, I'd make them a bit shorter
>>
>> #define PPC_INST(x)
>> #define PPC_INST_PREFIXED(x)
> Good idea, they do end up used quite widely.
>>
>> I'd also set padding to something invalid 0 or 0xffffffff for the first
>> case, and have your accessors check that rather than carrying around
>> another type of ppc_inst (prefixed, padded, non-padded).
>>
>> > +
>> > +#define PPC_INST_WORD(x) ((x).w)
>> > +#define PPC_INST_PREFIX(x) (x.p.prefix)
>> > +#define PPC_INST_SUFFIX(x) (x.p.suffix)
>> > +#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
>>
>> I'd avoid simple accessors like this completely. If they have any use
>> it would be to ensure you don't try to use prefix/suffix on a
>> non-prefixed instruction for example. If you want to do that I'd use
>> inline functions for it.
> What I was thinking with these macros was that they would let us keep
> the instruction type as a simple u32 on 32 bit ppc. Is it worth trying
> to do that?

Hmm, it might be. On the other hand if ppc32 can use the same
struct ppc_insn struct it might help share code without too many
macros.

I guess it's up to Christophe and Michael and you I won't complain
any more (so long as they're lower case and inlines where possible :))

Thanks,
Nick