| Message ID | 20200501034220.8982-3-jniethe5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Initial Prefixed Instruction support | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (54dc28ff5e0b3585224d49a31b53e030342ca5c3) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 1 checks, 47 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
On Friday, 1 May 2020 1:41:54 PM AEST Jordan Niethe wrote: > To execute an instruction out of line after a breakpoint, the NIP is set > to the address of struct bpt::instr. Here a copy of the instruction that > was replaced with a breakpoint is kept, along with a trap so normal flow > can be resumed after XOLing. The struct bpt's are located within the > data section. This is problematic as the data section may be marked as > no execute. > > Instead of each struct bpt holding the instructions to be XOL'd, make a > new array, bpt_table[], with enough space to hold instructions for the > number of supported breakpoints. A later patch will move this to the > text section. > Make struct bpt::instr a pointer to the instructions in bpt_table[] > associated with that breakpoint. This association is a simple mapping: > bpts[n] -> bpt_table[n * words per breakpoint]. Currently we only need > the copied instruction followed by a trap, so 2 words per breakpoint. > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > --- > v4: New to series > v5: - Do not use __section(), use a .space directive in .S file > - Simplify in_breakpoint_table() calculation > - Define BPT_SIZE > v6: - Seperate moving to text section > --- > arch/powerpc/xmon/xmon.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > index f91ae2c9adbe..6ba7f66c1dd0 100644 > --- a/arch/powerpc/xmon/xmon.c > +++ b/arch/powerpc/xmon/xmon.c > @@ -98,7 +98,7 @@ static long *xmon_fault_jmp[NR_CPUS]; > /* Breakpoint stuff */ > struct bpt { > unsigned long address; > - unsigned int instr[2]; > + unsigned int *instr; > atomic_t ref_count; > int enabled; > unsigned long pad; > @@ -117,6 +117,10 @@ static unsigned bpinstr = 0x7fe00008; /* trap */ > > #define BP_NUM(bp) ((bp) - bpts + 1) > > +#define BPT_SIZE (sizeof(unsigned int) * 2) > +#define BPT_WORDS (BPT_SIZE / sizeof(unsigned int)) Minor nit-pick but IMHO this would be more logical if you defined BPT_WORDS first like so: #define BPT_WORDS (2) #define BPT_SIZE (sizeof(unsigned int) * BPT_WORDS) Otherwise this looks good and I think the offset calculations below are correct so: Reviewed-by: Alistair Popple <alistair@popple.id.au> > +static unsigned int bpt_table[NBPTS * BPT_WORDS]; > + > /* Prototypes */ > static int cmds(struct pt_regs *); > static int mread(unsigned long, void *, int); > @@ -854,15 +858,13 @@ static struct bpt *in_breakpoint_table(unsigned long > nip, unsigned long *offp) { > unsigned long off; > > - off = nip - (unsigned long) bpts; > - if (off >= sizeof(bpts)) > + off = nip - (unsigned long) bpt_table; > + if (off >= sizeof(bpt_table)) > return NULL; > - off %= sizeof(struct bpt); > - if (off != offsetof(struct bpt, instr[0]) > - && off != offsetof(struct bpt, instr[1])) > + *offp = off % BPT_SIZE; > + if (*offp != 0 && *offp != 4) > return NULL; > - *offp = off - offsetof(struct bpt, instr[0]); > - return (struct bpt *) (nip - off); > + return bpts + (off / BPT_SIZE); > } > > static struct bpt *new_breakpoint(unsigned long a) > @@ -877,7 +879,8 @@ static struct bpt *new_breakpoint(unsigned long a) > for (bp = bpts; bp < &bpts[NBPTS]; ++bp) { > if (!bp->enabled && atomic_read(&bp->ref_count) == 0) { > bp->address = a; > - patch_instruction(&bp->instr[1], bpinstr); > + bp->instr = bpt_table + ((bp - bpts) * BPT_WORDS); > + patch_instruction(bp->instr + 1, bpinstr); > return bp; > } > }
On Mon, May 4, 2020 at 3:41 PM Alistair Popple <alistair@popple.id.au> wrote: > > On Friday, 1 May 2020 1:41:54 PM AEST Jordan Niethe wrote: > > To execute an instruction out of line after a breakpoint, the NIP is set > > to the address of struct bpt::instr. Here a copy of the instruction that > > was replaced with a breakpoint is kept, along with a trap so normal flow > > can be resumed after XOLing. The struct bpt's are located within the > > data section. This is problematic as the data section may be marked as > > no execute. > > > > Instead of each struct bpt holding the instructions to be XOL'd, make a > > new array, bpt_table[], with enough space to hold instructions for the > > number of supported breakpoints. A later patch will move this to the > > text section. > > Make struct bpt::instr a pointer to the instructions in bpt_table[] > > associated with that breakpoint. This association is a simple mapping: > > bpts[n] -> bpt_table[n * words per breakpoint]. Currently we only need > > the copied instruction followed by a trap, so 2 words per breakpoint. > > > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > > --- > > v4: New to series > > v5: - Do not use __section(), use a .space directive in .S file > > - Simplify in_breakpoint_table() calculation > > - Define BPT_SIZE > > v6: - Seperate moving to text section > > --- > > arch/powerpc/xmon/xmon.c | 21 ++++++++++++--------- > > 1 file changed, 12 insertions(+), 9 deletions(-) > > > > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > > index f91ae2c9adbe..6ba7f66c1dd0 100644 > > --- a/arch/powerpc/xmon/xmon.c > > +++ b/arch/powerpc/xmon/xmon.c > > @@ -98,7 +98,7 @@ static long *xmon_fault_jmp[NR_CPUS]; > > /* Breakpoint stuff */ > > struct bpt { > > unsigned long address; > > - unsigned int instr[2]; > > + unsigned int *instr; > > atomic_t ref_count; > > int enabled; > > unsigned long pad; > > @@ -117,6 +117,10 @@ static unsigned bpinstr = 0x7fe00008; /* trap */ > > > > #define BP_NUM(bp) ((bp) - bpts + 1) > > > > +#define BPT_SIZE (sizeof(unsigned int) * 2) > > +#define BPT_WORDS (BPT_SIZE / sizeof(unsigned int)) > > Minor nit-pick but IMHO this would be more logical if you defined BPT_WORDS > first like so: > > #define BPT_WORDS (2) > #define BPT_SIZE (sizeof(unsigned int) * BPT_WORDS) > > Otherwise this looks good and I think the offset calculations below are correct > so: What I was thinking was BPT_SIZE would later be defined in terms the instruction type and by doing it this way BPT_WORDS would be correct for the the instruction type if like on ppc32, it did not include a suffix. > > Reviewed-by: Alistair Popple <alistair@popple.id.au> > > > +static unsigned int bpt_table[NBPTS * BPT_WORDS]; > > + > > /* Prototypes */ > > static int cmds(struct pt_regs *); > > static int mread(unsigned long, void *, int); > > @@ -854,15 +858,13 @@ static struct bpt *in_breakpoint_table(unsigned long > > nip, unsigned long *offp) { > > unsigned long off; > > > > - off = nip - (unsigned long) bpts; > > - if (off >= sizeof(bpts)) > > + off = nip - (unsigned long) bpt_table; > > + if (off >= sizeof(bpt_table)) > > return NULL; > > - off %= sizeof(struct bpt); > > - if (off != offsetof(struct bpt, instr[0]) > > - && off != offsetof(struct bpt, instr[1])) > > + *offp = off % BPT_SIZE; > > + if (*offp != 0 && *offp != 4) > > return NULL; > > - *offp = off - offsetof(struct bpt, instr[0]); > > - return (struct bpt *) (nip - off); > > + return bpts + (off / BPT_SIZE); > > } > > > > static struct bpt *new_breakpoint(unsigned long a) > > @@ -877,7 +879,8 @@ static struct bpt *new_breakpoint(unsigned long a) > > for (bp = bpts; bp < &bpts[NBPTS]; ++bp) { > > if (!bp->enabled && atomic_read(&bp->ref_count) == 0) { > > bp->address = a; > > - patch_instruction(&bp->instr[1], bpinstr); > > + bp->instr = bpt_table + ((bp - bpts) * BPT_WORDS); > > + patch_instruction(bp->instr + 1, bpinstr); > > return bp; > > } > > } > > > >
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c index f91ae2c9adbe..6ba7f66c1dd0 100644 --- a/arch/powerpc/xmon/xmon.c +++ b/arch/powerpc/xmon/xmon.c @@ -98,7 +98,7 @@ static long *xmon_fault_jmp[NR_CPUS]; /* Breakpoint stuff */ struct bpt { unsigned long address; - unsigned int instr[2]; + unsigned int *instr; atomic_t ref_count; int enabled; unsigned long pad; @@ -117,6 +117,10 @@ static unsigned bpinstr = 0x7fe00008; /* trap */ #define BP_NUM(bp) ((bp) - bpts + 1) +#define BPT_SIZE (sizeof(unsigned int) * 2) +#define BPT_WORDS (BPT_SIZE / sizeof(unsigned int)) +static unsigned int bpt_table[NBPTS * BPT_WORDS]; + /* Prototypes */ static int cmds(struct pt_regs *); static int mread(unsigned long, void *, int); @@ -854,15 +858,13 @@ static struct bpt *in_breakpoint_table(unsigned long nip, unsigned long *offp) { unsigned long off; - off = nip - (unsigned long) bpts; - if (off >= sizeof(bpts)) + off = nip - (unsigned long) bpt_table; + if (off >= sizeof(bpt_table)) return NULL; - off %= sizeof(struct bpt); - if (off != offsetof(struct bpt, instr[0]) - && off != offsetof(struct bpt, instr[1])) + *offp = off % BPT_SIZE; + if (*offp != 0 && *offp != 4) return NULL; - *offp = off - offsetof(struct bpt, instr[0]); - return (struct bpt *) (nip - off); + return bpts + (off / BPT_SIZE); } static struct bpt *new_breakpoint(unsigned long a) @@ -877,7 +879,8 @@ static struct bpt *new_breakpoint(unsigned long a) for (bp = bpts; bp < &bpts[NBPTS]; ++bp) { if (!bp->enabled && atomic_read(&bp->ref_count) == 0) { bp->address = a; - patch_instruction(&bp->instr[1], bpinstr); + bp->instr = bpt_table + ((bp - bpts) * BPT_WORDS); + patch_instruction(bp->instr + 1, bpinstr); return bp; } }
To execute an instruction out of line after a breakpoint, the NIP is set to the address of struct bpt::instr. Here a copy of the instruction that was replaced with a breakpoint is kept, along with a trap so normal flow can be resumed after XOLing. The struct bpt's are located within the data section. This is problematic as the data section may be marked as no execute. Instead of each struct bpt holding the instructions to be XOL'd, make a new array, bpt_table[], with enough space to hold instructions for the number of supported breakpoints. A later patch will move this to the text section. Make struct bpt::instr a pointer to the instructions in bpt_table[] associated with that breakpoint. This association is a simple mapping: bpts[n] -> bpt_table[n * words per breakpoint]. Currently we only need the copied instruction followed by a trap, so 2 words per breakpoint. Signed-off-by: Jordan Niethe <jniethe5@gmail.com> --- v4: New to series v5: - Do not use __section(), use a .space directive in .S file - Simplify in_breakpoint_table() calculation - Define BPT_SIZE v6: - Seperate moving to text section --- arch/powerpc/xmon/xmon.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)