[v2,08/13] powerpc/xmon: Add initial support for prefixed instructions
diff mbox series

Message ID 20200211053355.21574-9-jniethe5@gmail.com
State New
Headers show
Series
  • Initial Prefixed Instruction support
Related show

Checks

Context Check Description
snowpatch_ozlabs/needsstable success Patch has no Fixes tags
snowpatch_ozlabs/checkpatch warning total: 0 errors, 3 warnings, 3 checks, 161 lines checked
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (a5bc6e124219546a81ce334dc9b16483d55e9abf)

Commit Message

Jordan Niethe Feb. 11, 2020, 5:33 a.m. UTC
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@gmail.com>
---
v2: Rename sufx to suffix
---
 arch/powerpc/xmon/xmon.c | 82 ++++++++++++++++++++++++++++++++++------
 1 file changed, 71 insertions(+), 11 deletions(-)

Comments

Christophe Leroy Feb. 11, 2020, 6:32 a.m. UTC | #1
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@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
Jordan Niethe Feb. 12, 2020, 12:28 a.m. UTC | #2
On Tue, Feb 11, 2020 at 5:32 PM Christophe Leroy
<christophe.leroy@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@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

Patch
diff mbox series

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 */
 
 #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
+			 * 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;
 		}
 	}
@@ -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)
+{
+	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[] = { "--", "**", "##" };