[PATCH v5 02/21] powerpc/xmon: Move out-of-line instructions to text section
Jordan Niethe
jniethe5 at gmail.com
Thu Apr 9 17:26:13 AEST 2020
On Thu, Apr 9, 2020 at 4:11 PM Christophe Leroy <christophe.leroy at c-s.fr> wrote:
>
>
>
> Le 06/04/2020 à 10:09, Jordan Niethe a écrit :
> > 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. Place this array in 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 at gmail.com>
> > ---
> > v4: New to series
> > v5: - Do not use __section(), use a .space directive in .S file
>
> I was going to comment to use __section() instead of creating a
> dedicated .S file.
>
> Why did you change that in v5 ?
I noticed with some toolchains I was getting this message:
Warning: setting incorrect section attributes for .text.xmon_bpts
I was talking with mpe about it and he said that the usual way for
doing things like this was with .space in a .S file so I changed to
that way.
>
> > - Simplify in_breakpoint_table() calculation
> > - Define BPT_SIZE
> > ---
> > arch/powerpc/xmon/Makefile | 2 +-
> > arch/powerpc/xmon/xmon.c | 23 +++++++++++++----------
> > arch/powerpc/xmon/xmon_bpts.S | 8 ++++++++
> > arch/powerpc/xmon/xmon_bpts.h | 8 ++++++++
> > 4 files changed, 30 insertions(+), 11 deletions(-)
> > create mode 100644 arch/powerpc/xmon/xmon_bpts.S
> > create mode 100644 arch/powerpc/xmon/xmon_bpts.h
> >
> > diff --git a/arch/powerpc/xmon/Makefile b/arch/powerpc/xmon/Makefile
> > index c3842dbeb1b7..515a13ea6f28 100644
> > --- a/arch/powerpc/xmon/Makefile
> > +++ b/arch/powerpc/xmon/Makefile
> > @@ -21,7 +21,7 @@ endif
> >
> > ccflags-$(CONFIG_PPC64) := $(NO_MINIMAL_TOC)
> >
> > -obj-y += xmon.o nonstdio.o spr_access.o
> > +obj-y += xmon.o nonstdio.o spr_access.o xmon_bpts.o
> >
> > ifdef CONFIG_XMON_DISASSEMBLY
> > obj-y += ppc-dis.o ppc-opc.o
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index 02e3bd62cab4..049375206510 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -62,6 +62,7 @@
> >
> > #include "nonstdio.h"
> > #include "dis-asm.h"
> > +#include "xmon_bpts.h"
> >
> > #ifdef CONFIG_SMP
> > static cpumask_t cpus_in_xmon = CPU_MASK_NONE;
> > @@ -97,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;
> > @@ -108,7 +109,6 @@ struct bpt {
> > #define BP_TRAP 2
> > #define BP_DABR 4
> >
> > -#define NBPTS 256
> > static struct bpt bpts[NBPTS];
> > static struct bpt dabr;
> > static struct bpt *iabr;
> > @@ -116,6 +116,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))
>
> Wouldn't it make more sense to do the following ? :
>
> #define BPT_WORDS 2
> #define BPT_SIZE (BPT_WORDS * sizeof(unsigned int))
I defined it like that so when we changed unsigned int -> struct
ppc_inst it would be the correct size whether or not the struct
included a suffix.
Otherwise it would be more straightforward to do it like that.
>
> > +extern unsigned int bpt_table[NBPTS * BPT_WORDS];
>
> Should go in xmon_bpts.h if we keep the definition in xmon_bpts.S
Right.
>
> > +
> > /* Prototypes */
> > static int cmds(struct pt_regs *);
> > static int mread(unsigned long, void *, int);
> > @@ -853,15 +857,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;
>
> Can we use logical operation instead ?
Sure, I was just adapting how it was already but that would probably be clearer.
>
> *offp = off & (BPT_SIZE - 1);
>
> > + if (*offp != 0 && *offp != 4)
>
> Could be:
> if (off & 3)
>
> > 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)
> > @@ -876,7 +878,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_bpts.S b/arch/powerpc/xmon/xmon_bpts.S
> > new file mode 100644
> > index 000000000000..ebb2dbc70ca8
> > --- /dev/null
> > +++ b/arch/powerpc/xmon/xmon_bpts.S
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#include <asm/ppc_asm.h>
> > +#include <asm/asm-compat.h>
> > +#include "xmon_bpts.h"
> > +
> > +.global bpt_table
> > +bpt_table:
> > + .space NBPTS * 8
>
> Should use BPT_SIZE instead of raw coding 8.
Right.
>
> > diff --git a/arch/powerpc/xmon/xmon_bpts.h b/arch/powerpc/xmon/xmon_bpts.h
> > new file mode 100644
> > index 000000000000..840e70be7945
> > --- /dev/null
> > +++ b/arch/powerpc/xmon/xmon_bpts.h
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef XMON_BPTS_H
> > +#define XMON_BPTS_H
> > +
> > +#define NBPTS 256
> > +
> > +#endif /* XMON_BPTS_H */
> > +
> >
>
> I think it would be better to split this patch in two patches:
> 1/ First patch to move breakpoints out of struct bpt into a bpt_table.
> 2/ Second patch to move bpt_table into .text section.
Bala suggested so too, I will separate them next time.
>
> Christophe
More information about the Linuxppc-dev
mailing list