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 <jniet...@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 ?
- 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))
+extern unsigned int bpt_table[NBPTS * BPT_WORDS];
Should go in xmon_bpts.h if we keep the definition in xmon_bpts.S
+
/* 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 ?
*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.
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.
Christophe
