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

[Nicholas Piggin <npiggin@gmail.com>]

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?

> 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?

> +} __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):

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.

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

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.

> +
> +#define DEREF_PPC_INST_PTR(ptr)                \
> +({                            \
> +    ppc_inst __inst;                \
> +    __inst.w = *(unsigned int *)(ptr);        \
> +    if (PPC_INST_IS_PREFIXED(__inst))        \
> +        __inst.p = *(ppc_prefixed_inst *)(ptr);    \
> +    __inst;                        \
> +})

I'd go an inline with shorter lowercase names. ppc_inst_read(&inst);

> +#define PPC_INST_NEXT(ptr) ((ptr) += PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))
> +#define PPC_INST_PREV(ptr) ((ptr) -= PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))

Wouldn't bother with this accessor, caller could ptr += ppc_inst_len(inst)


> +#define PPC_INST_EQ(x, y)                \
> +({                            \
> +    long pic_ret = 0;                \
> +    pic_ret = (PPC_INST_PREFIX(x) == PPC_INST_PREFIX(y));    \
> +    if (pic_ret) {                    \
> +        if (PPC_INST_IS_PREFIXED(x) && PPC_INST_IS_PREFIXED(y)) {    \
> +            pic_ret = (PPC_INST_SUFFIX(x) == PPC_INST_SUFFIX(y));    \
> +        } else {                \
> +            pic_ret = 0;            \
> +        }                    \
> +    }                        \
> +    pic_ret;                    \
> +})

static inline bool ppc_inst_eq(struct ppc_inst x, struct ppc_inst y)
{
	return !memcmp(&x, &y, sizeof(struct ppc_inst));
}

If all accessors and initalisers are such that the padding gets set,
that'll work.

Thanks,
Nick