Jordan Niethe's on February 27, 2020 10:58 am: > On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npig...@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