[PATCH v2 08/13] powerpc/xmon: Add initial support for prefixed instructions
Jordan Niethe
jniethe5 at gmail.com
Wed Feb 12 11:28:42 AEDT 2020
On Tue, Feb 11, 2020 at 5:32 PM Christophe Leroy
<christophe.leroy at c-s.fr> wrote:
>
>
>
> Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> > A prefixed instruction is composed of a word prefix and a word suffix.
> > It does not make sense to be able to have a breakpoint on the suffix of
> > a prefixed instruction, so make this impossible.
> >
> > When leaving xmon_core() we check to see if we are currently at a
> > breakpoint. If this is the case, the breakpoint needs to be proceeded
> > from. Initially emulate_step() is tried, but if this fails then we need
> > to execute the saved instruction out of line. The NIP is set to the
> > address of bpt::instr[] for the current breakpoint. bpt::instr[]
> > contains the instruction replaced by the breakpoint, followed by a trap
> > instruction. After bpt::instr[0] is executed and we hit the trap we
> > enter back into xmon_bpt(). We know that if we got here and the offset
> > indicates we are at bpt::instr[1] then we have just executed out of line
> > so we can put the NIP back to the instruction after the breakpoint
> > location and continue on.
> >
> > Adding prefixed instructions complicates this as the bpt::instr[1] needs
> > to be used to hold the suffix. To deal with this make bpt::instr[] big
> > enough for three word instructions. bpt::instr[2] contains the trap,
> > and in the case of word instructions pad bpt::instr[1] with a noop.
> >
> > No support for disassembling prefixed instructions.
> >
> > Signed-off-by: Jordan Niethe <jniethe5 at gmail.com>
> > ---
> > v2: Rename sufx to suffix
> > ---
> > arch/powerpc/xmon/xmon.c | 82 ++++++++++++++++++++++++++++++++++------
> > 1 file changed, 71 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index 897e512c6379..0b085642bbe7 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -97,7 +97,8 @@ static long *xmon_fault_jmp[NR_CPUS];
> > /* Breakpoint stuff */
> > struct bpt {
> > unsigned long address;
> > - unsigned int instr[2];
> > + /* Prefixed instructions can not cross 64-byte boundaries */
> > + unsigned int instr[3] __aligned(64);
> > atomic_t ref_count;
> > int enabled;
> > unsigned long pad;
> > @@ -113,6 +114,7 @@ static struct bpt bpts[NBPTS];
> > static struct bpt dabr;
> > static struct bpt *iabr;
> > static unsigned bpinstr = 0x7fe00008; /* trap */
> > +static unsigned nopinstr = 0x60000000; /* nop */
>
> Use PPC_INST_NOP instead of 0x60000000
>
> And this nopinstr variable will never change. Why not use directly
> PPC_INST_NOP in the code ?
True, I will do that.
>
> >
> > #define BP_NUM(bp) ((bp) - bpts + 1)
> >
> > @@ -120,6 +122,7 @@ static unsigned bpinstr = 0x7fe00008; /* trap */
> > static int cmds(struct pt_regs *);
> > static int mread(unsigned long, void *, int);
> > static int mwrite(unsigned long, void *, int);
> > +static int read_instr(unsigned long, unsigned int *, unsigned int *);
> > static int handle_fault(struct pt_regs *);
> > static void byterev(unsigned char *, int);
> > static void memex(void);
> > @@ -706,7 +709,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
> > bp = at_breakpoint(regs->nip);
> > if (bp != NULL) {
> > int stepped = emulate_step(regs, bp->instr[0],
> > - PPC_NO_SUFFIX);
> > + bp->instr[1]);
> > if (stepped == 0) {
> > regs->nip = (unsigned long) &bp->instr[0];
> > atomic_inc(&bp->ref_count);
> > @@ -761,8 +764,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;
> > }
> > @@ -864,7 +867,8 @@ static struct bpt *in_breakpoint_table(unsigned long nip, unsigned long *offp)
> > return NULL;
> > off %= sizeof(struct bpt);
> > if (off != offsetof(struct bpt, instr[0])
> > - && off != offsetof(struct bpt, instr[1]))
> > + && off != offsetof(struct bpt, instr[1])
> > + && off != offsetof(struct bpt, instr[2]))
> > return NULL;
> > *offp = off - offsetof(struct bpt, instr[0]);
> > return (struct bpt *) (nip - off);
> > @@ -881,9 +885,18 @@ static struct bpt *new_breakpoint(unsigned long a)
> >
> > for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
> > if (!bp->enabled && atomic_read(&bp->ref_count) == 0) {
> > + /*
> > + * Prefixed instructions are two words, but regular
> > + * instructions are only one. Use a nop to pad out the
> > + * regular instructions so that we can place the trap
> > + * at the same plac. For prefixed instructions the nop
>
> plac ==> place
thanks.
>
> > + * will get overwritten during insert_bpts().
> > + */
> > bp->address = a;
> > - bp->instr[1] = bpinstr;
> > + bp->instr[1] = nopinstr;
> > store_inst(&bp->instr[1]);
> > + bp->instr[2] = bpinstr;
> > + store_inst(&bp->instr[2]);
> > return bp;
>
> Not directly related to this patch, but shouldn't we use
> patch_instruction() instead ?
It does seem like that. I will try that.
>
> > }
> > }
> > @@ -895,13 +908,15 @@ static struct bpt *new_breakpoint(unsigned long a)
> > static void insert_bpts(void)
> > {
> > int i;
> > - struct bpt *bp;
> > + unsigned int prefix;
> > + struct bpt *bp, *bp2;
> >
> > bp = bpts;
> > for (i = 0; i < NBPTS; ++i, ++bp) {
> > if ((bp->enabled & (BP_TRAP|BP_CIABR)) == 0)
> > continue;
> > - if (mread(bp->address, &bp->instr[0], 4) != 4) {
> > + if (!read_instr(bp->address, &bp->instr[0],
> > + &bp->instr[1])) {
> > printf("Couldn't read instruction at %lx, "
> > "disabling breakpoint there\n", bp->address);
> > bp->enabled = 0;
> > @@ -913,7 +928,34 @@ static void insert_bpts(void)
> > bp->enabled = 0;
> > continue;
> > }
> > + /*
> > + * Check the address is not a suffix by looking for a prefix in
> > + * front of it.
> > + */
> > + if ((mread(bp->address - 4, &prefix, 4) == 4) &&
> > + IS_PREFIX(prefix)) {
> > + printf("Breakpoint at %lx is on the second word of a "
> > + "prefixed instruction, disabling it\n",
> > + bp->address);
> > + bp->enabled = 0;
> > + continue;
> > + }
> > + /*
> > + * We might still be a suffix - if the prefix has already been
> > + * replaced by a breakpoint we won't catch it with the above
> > + * test.
> > + */
> > + bp2 = at_breakpoint(bp->address - 4);
> > + if (bp2 && IS_PREFIX(bp2->instr[0])) {
> > + printf("Breakpoint at %lx is on the second word of a "
> > + "prefixed instruction, disabling it\n",
> > + bp->address);
> > + bp->enabled = 0;
> > + continue;
> > + }
> > store_inst(&bp->instr[0]);
> > + if (IS_PREFIX(bp->instr[0]))
> > + store_inst(&bp->instr[1]);
> > if (bp->enabled & BP_CIABR)
> > continue;
> > if (patch_instruction((unsigned int *)bp->address,
> > @@ -1164,14 +1206,14 @@ static int do_step(struct pt_regs *regs)
> > */
> > static int do_step(struct pt_regs *regs)
> > {
> > - unsigned int instr;
> > + unsigned int instr, suffix;
> > int stepped;
> >
> > force_enable_xmon();
> > /* check we are in 64-bit kernel mode, translation enabled */
> > if ((regs->msr & (MSR_64BIT|MSR_PR|MSR_IR)) == (MSR_64BIT|MSR_IR)) {
> > - if (mread(regs->nip, &instr, 4) == 4) {
> > - stepped = emulate_step(regs, instr, PPC_NO_SUFFIX);
> > + if (read_instr(regs->nip, &instr, &suffix)) {
> > + stepped = emulate_step(regs, instr, suffix);
> > if (stepped < 0) {
> > printf("Couldn't single-step %s instruction\n",
> > (IS_RFID(instr)? "rfid": "mtmsrd"));
> > @@ -2130,6 +2172,24 @@ mwrite(unsigned long adrs, void *buf, int size)
> > return n;
> > }
> >
> > +static int read_instr(unsigned long addr, unsigned int *instr,
> > + unsigned int *suffix)
> > +{
>
> Don't know if it is worth it, but wouldn't it be better to define a
> mread_inst() based on mread() instead of doing something that chain
> calls to mread()
I will give it go.
>
> > + int r;
> > +
> > + r = mread(addr, instr, 4);
> > + if (r != 4)
> > + return 0;
> > + if (!IS_PREFIX(*instr))
> > + return 4;
> > + r = mread(addr + 4, suffix, 4);
> > + if (r != 4)
> > + return 0;
> > +
> > + return 8;
> > +}
> > +
> > +
> > static int fault_type;
> > static int fault_except;
> > static char *fault_chars[] = { "--", "**", "##" };
> >
>
> Christophe
More information about the Linuxppc-dev
mailing list