[PATCH v4 14/16] powerpc64: Add prefixed instructions to instruction data type
Jordan Niethe
jniethe5 at gmail.com
Tue Mar 24 10:45:50 AEDT 2020
On Mon, Mar 23, 2020 at 6:37 PM Nicholas Piggin <npiggin at gmail.com> wrote:
>
> Jordan Niethe's on March 20, 2020 3:18 pm:
> > For powerpc64, redefine the ppc_inst type so both word and prefixed
> > instructions can be represented. On powerpc32 the type will remain the
> > same. Update places which had assumed instructions to be 4 bytes long.
> >
> > Signed-off-by: Jordan Niethe <jniethe5 at gmail.com>
> > ---
> > v4: New to series
> > ---
> > arch/powerpc/include/asm/code-patching.h | 10 +--
> > arch/powerpc/include/asm/inst.h | 90 ++++++++++++++++++++++++
> > arch/powerpc/include/asm/kprobes.h | 2 +-
> > arch/powerpc/include/asm/sstep.h | 4 ++
> > arch/powerpc/include/asm/uaccess.h | 22 ++++++
> > arch/powerpc/include/asm/uprobes.h | 2 +-
> > arch/powerpc/kernel/align.c | 5 +-
> > arch/powerpc/kernel/hw_breakpoint.c | 2 +-
> > arch/powerpc/kernel/kprobes.c | 7 +-
> > arch/powerpc/kernel/optprobes.c | 42 ++++++-----
> > arch/powerpc/kernel/optprobes_head.S | 3 +
> > arch/powerpc/kernel/trace/ftrace.c | 19 ++++-
> > arch/powerpc/kernel/uprobes.c | 2 +-
> > arch/powerpc/lib/code-patching.c | 22 ++++--
> > arch/powerpc/lib/sstep.c | 4 +-
> > arch/powerpc/xmon/xmon.c | 38 +++++++---
> > 16 files changed, 221 insertions(+), 53 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
> > index 68bd9db334bd..bd41e1558707 100644
> > --- a/arch/powerpc/include/asm/code-patching.h
> > +++ b/arch/powerpc/include/asm/code-patching.h
> > @@ -25,11 +25,11 @@
> > bool is_offset_in_branch_range(long offset);
> > ppc_inst create_branch(const ppc_inst *addr,
> > unsigned long target, int flags);
> > -unsigned int create_cond_branch(const ppc_inst *addr,
> > +ppc_inst create_cond_branch(const void *addr,
> > unsigned long target, int flags);
> > -int patch_branch(ppc_inst *addr, unsigned long target, int flags);
> > -int patch_instruction(ppc_inst *addr, ppc_inst instr);
> > -int raw_patch_instruction(ppc_inst *addr, ppc_inst instr);
> > +int patch_branch(void *addr, unsigned long target, int flags);
> > +int patch_instruction(void *addr, ppc_inst instr);
> > +int raw_patch_instruction(void *addr, ppc_inst instr);
> >
> > static inline unsigned long patch_site_addr(s32 *site)
> > {
> > @@ -60,7 +60,7 @@ static inline int modify_instruction_site(s32 *site, unsigned int clr, unsigned
> > int instr_is_relative_branch(ppc_inst instr);
> > int instr_is_relative_link_branch(ppc_inst instr);
> > int instr_is_branch_to_addr(const ppc_inst *instr, unsigned long addr);
> > -unsigned long branch_target(const ppc_inst *instr);
> > +unsigned long branch_target(const void *instr);
> > ppc_inst translate_branch(const ppc_inst *dest,
> > const ppc_inst *src);
> > extern bool is_conditional_branch(ppc_inst instr);
> > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
> > index 7c8596ee411e..1a40b0a71128 100644
> > --- a/arch/powerpc/include/asm/inst.h
> > +++ b/arch/powerpc/include/asm/inst.h
> > @@ -6,6 +6,95 @@
> > * Instruction data type for POWER
> > */
> >
> > +#ifdef __powerpc64__
> > +
> > +typedef struct ppc_inst {
> > + union {
> > + struct {
> > + u32 word;
> > + u32 pad;
> > + } __packed;
> > + struct {
> > + u32 prefix;
> > + u32 suffix;
> > + } __packed;
> > + };
> > +} ppc_inst;
> > +
> > +#define PPC_INST(x) ((ppc_inst) { .word = (x), .pad = 0 })
> > +#define PPC_INST_PREFIXED(x, y) ((ppc_inst) { .prefix = (x), .suffix = (y) })
> > +
> > +static inline int ppc_inst_opcode(ppc_inst x)
> > +{
> > + return x.word >> 26;
> > +}
> > +
> > +static inline bool ppc_inst_prefixed(ppc_inst x) {
> > + return ppc_inst_opcode(x) == 1;
> > +}
> > +
> > +static inline int ppc_inst_len(ppc_inst x)
> > +{
> > + if (ppc_inst_prefixed(x))
> > + return 8;
> > + else
> > + return 4;
> > +}
> > +
> > +static inline u32 ppc_inst_word(ppc_inst x)
> > +{
> > + return x.word;
> > +}
>
> I guess a concern could be that code using ppc_inst_word could now get a
> prefix unexpectedly and not handle it properly. The reason it should
> generally be okay is that prefix won't match any existing valid
> instruction words, so callers won't match or think it's an unknown
> instruction. Am I right? Possibly a small comment?
Yes all prefixes are illegal as word instructions so I thought that
should be okay.
>
> > +
> > +static inline u32 ppc_inst_prefix(ppc_inst x)
> > +{
> > + return x.prefix;
> > +}
> > +
> > +static inline u32 ppc_inst_suffix(ppc_inst x)
> > +{
> > + return x.suffix;
> > +}
> > +
> > +
> > +static inline ppc_inst ppc_inst_read(const void *ptr)
> > +{
> > + ppc_inst inst;
> > + inst.word = *(u32 *)ptr;
> > + if (ppc_inst_prefixed(inst))
> > + inst.suffix = *((u32 *)ptr + 1);
> > + else
> > + inst.pad = 0;
>
> I'm a bit against using partially constructed opaque type for things
> like this, even if it is in the code that knows about the type. We
> could modify ppc_inst_prefixed() to assert that pad is equal to zero
> (or some poisoned value) if it's not prefixed. Or do some validation
> on the suffix if it is.
Okay what about something like:
+static inline ppc_inst ppc_inst_read(const void *ptr)
+{
+ u32 prefix, suffix;
+ prefix = *(u32 *)ptr;
+ if (prefix >> 26 == 1)
+ suffix = *((u32 *)ptr + 1);
+ else
+ suffix = 0;
+ return PPC_INST_PREFIX(prefix, suffix);
+}
>
>
>
> > +static inline bool ppc_inst_equal(ppc_inst x, ppc_inst y)
> > +{
> > + return !memcmp(&x, &y, sizeof(struct ppc_inst));
> > +}
>
> I guess a variable length memcmp will make terrible code, so you're
> requiring pad to equal 0 to match non-prefixed. Fine.
Yes this assumes pad always gets set to 0.
>
> > +
> > +static inline bool ppc_inst_null(ppc_inst x)
> > +{
> > + return x.word == 0 && x.pad == 0;
> > +}
>
> In this case you shouldn't need x.pad == 0. If x.word == 0, then
> WARN_ON_ONCE(x.pad != 0) ?
True.
>
> > if ((regs->msr & MSR_LE) != (MSR_KERNEL & MSR_LE)) {
> > /* We don't handle PPC little-endian any more... */
> > if (cpu_has_feature(CPU_FTR_PPC_LE))
> > return -EIO;
> > - instr = PPC_INST(swab32(ppc_inst_word(instr)));
> > + instr = PPC_INST_PREFIXED(swab32(ppc_inst_word(instr)),
> > + swab32(ppc_inst_suffix(instr)));
>
> Ugly, don't suppose you'd bother to do a ppc_inst_bswap function for
> this one case?
I can do that.
>
> [snip probes stuff]
>
> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> > index fa7f32adf029..3b8277a64b8f 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -24,17 +24,27 @@ static int __patch_instruction(ppc_inst *exec_addr, ppc_inst instr,
> > {
> > int err = 0;
> >
> > - __put_user_asm(instr, patch_addr, err, "stw");
> > + __put_user_asm(ppc_inst_word(instr), patch_addr, err, "stw");
> > if (err)
> > return err;
> >
> > asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
> > "r" (exec_addr));
> >
> > + if (!ppc_inst_prefixed(instr))
> > + return 0;
> > +
> > + __put_user_asm(ppc_inst_suffix(instr), patch_addr + 4, err, "stw");
> > + if (err)
> > + return err;
> > +
> > + asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr + 4),
> > + "r" (exec_addr + 4));
>
> Although there's proably no real performance or atomicity issues here,
> I'd be pleased if we could do a case for prefixed and a case for non
> prefixed, and store the non-prefixed with "std". Just for the principle
> of not having half-written instructions in the image.
Do you mean store the prefixed with std?
>
> You could skip the dcbst and icbi for the second address if you happen
> to know this future CPU does not store prefix insns across a CL
> boundary. But probably not necessary to make that assumption in non
> perf critical code here, so I'd leave it as you have.
>
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index ee084411f2f5..c5536e1a3356 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -110,7 +110,7 @@ struct bpt {
> > #define BP_DABR 4
> >
> > #define NBPTS 256
> > -#define BPT_WORDS 2
> > +#define BPT_WORDS 4
>
> (2 * sizeof(ppc_inst) / sizeof(u32)) ?
>
> > static struct bpt bpts[NBPTS];
> > static struct bpt dabr;
> > static struct bpt *iabr;
> > @@ -118,12 +118,13 @@ static unsigned bpinstr = 0x7fe00008; /* trap */
> >
> > #define BP_NUM(bp) ((bp) - bpts + 1)
> >
> > -static unsigned int __section(.text.xmon_bpts) bpt_table[NBPTS * BPT_WORDS];
> > +static unsigned int __section(.text.xmon_bpts) bpt_table[NBPTS * BPT_WORDS] __aligned(64);
>
> Should have a define somewhere for this magical 64.
Sure.
>
> > /* Prototypes */
> > static int cmds(struct pt_regs *);
> > static int mread(unsigned long, void *, int);
> > static int mwrite(unsigned long, void *, int);
> > +static int mread_instr(unsigned long, ppc_inst *);
>
> In some cases you've addd helpers like this as separate patches,
> others you've bundled them together. NBD but I liked the prep patches
> which then made the more important changes easier to see.
I will make some more prep patches.
>
> > @@ -759,8 +760,8 @@ static int xmon_bpt(struct pt_regs *regs)
> >
> > /* Are we at the trap at bp->instr[1] for some bp? */
> > bp = in_breakpoint_table(regs->nip, &offset);
> > - if (bp != NULL && offset == 4) {
> > - regs->nip = bp->address + 4;
> > + if (bp != NULL && (offset == 4 || offset == 8)) {
> > + regs->nip = bp->address + offset;
> > atomic_dec(&bp->ref_count);
> > return 1;
> > }
> > @@ -862,7 +863,7 @@ static struct bpt *in_breakpoint_table(unsigned long nip, unsigned long *offp)
> > if (off >= sizeof(bpt_table))
> > return NULL;
> > bp_off = off % (sizeof(unsigned int) * BPT_WORDS);
> > - if (bp_off != 0 && bp_off != 4)
> > + if (bp_off != 0 && bp_off != 4 && bp_off != 8)
> > return NULL;
> > *offp = bp_off;
> > return bpts + ((off - bp_off) / (sizeof(unsigned int) * BPT_WORDS));
> > @@ -881,7 +882,6 @@ static struct bpt *new_breakpoint(unsigned long a)
> > if (!bp->enabled && atomic_read(&bp->ref_count) == 0) {
> > bp->address = a;
> > bp->instr = bpt_table + ((bp - bpts) * BPT_WORDS);
> > - patch_instruction(bp->instr + 1, PPC_INST(bpinstr));
> > return bp;
> > }
> > }
>
> Why is this okay to remove?
When we only had word instructions the bpt was just patched in here
once and that was that.
With prefixed instructions bp->instr + 1 might be the suffix. So I
moved putting the breakpoint to insert_bpts():
patch_instruction(bp->instr + ppc_inst_len(instr), PPC_INST(bpinstr));
>
> Thanks,
> Nick
>
More information about the Linuxppc-dev
mailing list