[PATCH v2 08/13] powerpc/xmon: Add initial support for prefixed instructions
Christophe Leroy
christophe.leroy at c-s.fr
Tue Feb 11 17:32:24 AEDT 2020
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 ?
>
> #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
> + * 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 ?
> }
> }
> @@ -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()
> + 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