* [PATCH v2 01/13] powerpc: Enable Prefixed Instructions
2020-02-11 5:33 [PATCH v2 00/13] Initial Prefixed Instruction support Jordan Niethe
@ 2020-02-11 5:33 ` Jordan Niethe
2020-02-11 5:33 ` [PATCH v2 02/13] powerpc: Define new SRR1 bits for a future ISA version Jordan Niethe
` (11 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, dja, bala24
From: Alistair Popple <alistair@popple.id.au>
Prefix instructions have their own FSCR bit which needs to enabled via
a CPU feature. The kernel will save the FSCR for problem state but it
needs to be enabled initially.
Signed-off-by: Alistair Popple <alistair@popple.id.au>
---
arch/powerpc/include/asm/reg.h | 3 +++
arch/powerpc/kernel/dt_cpu_ftrs.c | 23 +++++++++++++++++++++++
2 files changed, 26 insertions(+)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 1aa46dff0957..c7758c2ccc5f 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -397,6 +397,7 @@
#define SPRN_RWMR 0x375 /* Region-Weighting Mode Register */
/* HFSCR and FSCR bit numbers are the same */
+#define FSCR_PREFIX_LG 13 /* Enable Prefix Instructions */
#define FSCR_SCV_LG 12 /* Enable System Call Vectored */
#define FSCR_MSGP_LG 10 /* Enable MSGP */
#define FSCR_TAR_LG 8 /* Enable Target Address Register */
@@ -408,11 +409,13 @@
#define FSCR_VECVSX_LG 1 /* Enable VMX/VSX */
#define FSCR_FP_LG 0 /* Enable Floating Point */
#define SPRN_FSCR 0x099 /* Facility Status & Control Register */
+#define FSCR_PREFIX __MASK(FSCR_PREFIX_LG)
#define FSCR_SCV __MASK(FSCR_SCV_LG)
#define FSCR_TAR __MASK(FSCR_TAR_LG)
#define FSCR_EBB __MASK(FSCR_EBB_LG)
#define FSCR_DSCR __MASK(FSCR_DSCR_LG)
#define SPRN_HFSCR 0xbe /* HV=1 Facility Status & Control Register */
+#define HFSCR_PREFIX __MASK(FSCR_PREFIX_LG)
#define HFSCR_MSGP __MASK(FSCR_MSGP_LG)
#define HFSCR_TAR __MASK(FSCR_TAR_LG)
#define HFSCR_EBB __MASK(FSCR_EBB_LG)
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 182b4047c1ef..396f2c6c588e 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -553,6 +553,28 @@ static int __init feat_enable_large_ci(struct dt_cpu_feature *f)
return 1;
}
+static int __init feat_enable_prefix(struct dt_cpu_feature *f)
+{
+ u64 fscr, hfscr;
+
+ if (f->usable_privilege & USABLE_HV) {
+ hfscr = mfspr(SPRN_HFSCR);
+ hfscr |= HFSCR_PREFIX;
+ mtspr(SPRN_HFSCR, hfscr);
+ }
+
+ if (f->usable_privilege & USABLE_OS) {
+ fscr = mfspr(SPRN_FSCR);
+ fscr |= FSCR_PREFIX;
+ mtspr(SPRN_FSCR, fscr);
+
+ if (f->usable_privilege & USABLE_PR)
+ current->thread.fscr |= FSCR_PREFIX;
+ }
+
+ return 1;
+}
+
struct dt_cpu_feature_match {
const char *name;
int (*enable)(struct dt_cpu_feature *f);
@@ -626,6 +648,7 @@ static struct dt_cpu_feature_match __initdata
{"vector-binary128", feat_enable, 0},
{"vector-binary16", feat_enable, 0},
{"wait-v3", feat_enable, 0},
+ {"prefix-instructions", feat_enable_prefix, 0},
};
static bool __initdata using_dt_cpu_ftrs;
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 02/13] powerpc: Define new SRR1 bits for a future ISA version
2020-02-11 5:33 [PATCH v2 00/13] Initial Prefixed Instruction support Jordan Niethe
2020-02-11 5:33 ` [PATCH v2 01/13] powerpc: Enable Prefixed Instructions Jordan Niethe
@ 2020-02-11 5:33 ` Jordan Niethe
2020-02-11 5:33 ` [PATCH v2 03/13] powerpc sstep: Prepare to support prefixed instructions Jordan Niethe
` (10 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
Add the BOUNDARY SRR1 bit definition for when the cause of an alignment
exception is a prefixed instruction that crosses a 64-byte boundary.
Add the PREFIXED SRR1 bit definition for exceptions caused by prefixed
instructions.
Bit 35 of SRR1 is called SRR1_ISI_N_OR_G. This name comes from it being
used to indicate that an ISI was due to the access being no-exec or
guarded. A future ISA version adds another purpose. It is also set if
there is an access in a cache-inhibited location for prefixed
instruction. Rename from SRR1_ISI_N_OR_G to SRR1_ISI_N_G_OR_CIP.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: Combined all the commits concerning SRR1 bits.
---
arch/powerpc/include/asm/reg.h | 4 +++-
arch/powerpc/kvm/book3s_hv_nested.c | 2 +-
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 2 +-
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index c7758c2ccc5f..173f33df4fab 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -762,7 +762,7 @@
#endif
#define SRR1_ISI_NOPT 0x40000000 /* ISI: Not found in hash */
-#define SRR1_ISI_N_OR_G 0x10000000 /* ISI: Access is no-exec or G */
+#define SRR1_ISI_N_G_OR_CIP 0x10000000 /* ISI: Access is no-exec or G or CI for a prefixed instruction */
#define SRR1_ISI_PROT 0x08000000 /* ISI: Other protection fault */
#define SRR1_WAKEMASK 0x00380000 /* reason for wakeup */
#define SRR1_WAKEMASK_P8 0x003c0000 /* reason for wakeup on POWER8 and 9 */
@@ -789,6 +789,8 @@
#define SRR1_PROGADDR 0x00010000 /* SRR0 contains subsequent addr */
#define SRR1_MCE_MCP 0x00080000 /* Machine check signal caused interrupt */
+#define SRR1_BOUNDARY 0x10000000 /* Prefixed instruction crosses 64-byte boundary */
+#define SRR1_PREFIXED 0x20000000 /* Exception caused by prefixed instruction */
#define SPRN_HSRR0 0x13A /* Save/Restore Register 0 */
#define SPRN_HSRR1 0x13B /* Save/Restore Register 1 */
diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
index dc97e5be76f6..6ab685227574 100644
--- a/arch/powerpc/kvm/book3s_hv_nested.c
+++ b/arch/powerpc/kvm/book3s_hv_nested.c
@@ -1169,7 +1169,7 @@ static int kvmhv_translate_addr_nested(struct kvm_vcpu *vcpu,
} else if (vcpu->arch.trap == BOOK3S_INTERRUPT_H_INST_STORAGE) {
/* Can we execute? */
if (!gpte_p->may_execute) {
- flags |= SRR1_ISI_N_OR_G;
+ flags |= SRR1_ISI_N_G_OR_CIP;
goto forward_to_l1;
}
} else {
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 220305454c23..b53a9f1c1a46 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -1260,7 +1260,7 @@ long kvmppc_hpte_hv_fault(struct kvm_vcpu *vcpu, unsigned long addr,
status &= ~DSISR_NOHPTE; /* DSISR_NOHPTE == SRR1_ISI_NOPT */
if (!data) {
if (gr & (HPTE_R_N | HPTE_R_G))
- return status | SRR1_ISI_N_OR_G;
+ return status | SRR1_ISI_N_G_OR_CIP;
if (!hpte_read_permission(pp, slb_v & key))
return status | SRR1_ISI_PROT;
} else if (status & DSISR_ISSTORE) {
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 03/13] powerpc sstep: Prepare to support prefixed instructions
2020-02-11 5:33 [PATCH v2 00/13] Initial Prefixed Instruction support Jordan Niethe
2020-02-11 5:33 ` [PATCH v2 01/13] powerpc: Enable Prefixed Instructions Jordan Niethe
2020-02-11 5:33 ` [PATCH v2 02/13] powerpc: Define new SRR1 bits for a future ISA version Jordan Niethe
@ 2020-02-11 5:33 ` Jordan Niethe
2020-02-11 5:57 ` Christophe Leroy
2020-02-11 5:33 ` [PATCH v2 04/13] powerpc sstep: Add support for prefixed load/stores Jordan Niethe
` (9 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
Currently all instructions are a single word long. A future ISA version
will include prefixed instructions which have a double word length. The
functions used for analysing and emulating instructions need to be
modified so that they can handle these new instruction types.
A prefixed instruction is a word prefix followed by a word suffix. All
prefixes uniquely have the primary op-code 1. Suffixes may be valid word
instructions or instructions that only exist as suffixes.
In handling prefixed instructions it will be convenient to treat the
suffix and prefix as separate words. To facilitate this modify
analyse_instr() and emulate_step() to take a suffix as a
parameter. For word instructions it does not matter what is passed in
here - it will be ignored.
We also define a new flag, PREFIXED, to be used in instruction_op:type.
This flag will indicate when emulating an analysed instruction if the
NIP should be advanced by word length or double word length.
The callers of analyse_instr() and emulate_step() will need their own
changes to be able to support prefixed instructions. For now modify them
to pass in 0 as a suffix.
Note that at this point no prefixed instructions are emulated or
analysed - this is just making it possible to do so.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Move definition of __get_user_instr() and
__get_user_instr_inatomic() to "powerpc: Support prefixed instructions
in alignment handler."
- Use a macro for returning the length of an op
- Rename sufx -> suffix
- Define and use PPC_NO_SUFFIX instead of 0
---
arch/powerpc/include/asm/ppc-opcode.h | 5 +++++
arch/powerpc/include/asm/sstep.h | 9 ++++++--
arch/powerpc/kernel/align.c | 2 +-
arch/powerpc/kernel/hw_breakpoint.c | 4 ++--
arch/powerpc/kernel/kprobes.c | 2 +-
arch/powerpc/kernel/mce_power.c | 2 +-
arch/powerpc/kernel/optprobes.c | 3 ++-
arch/powerpc/kernel/uprobes.c | 2 +-
arch/powerpc/kvm/emulate_loadstore.c | 2 +-
arch/powerpc/lib/sstep.c | 12 ++++++-----
arch/powerpc/lib/test_emulate_step.c | 30 +++++++++++++--------------
arch/powerpc/xmon/xmon.c | 5 +++--
12 files changed, 46 insertions(+), 32 deletions(-)
diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index c1df75edde44..72783bc92e50 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -377,6 +377,11 @@
#define PPC_INST_VCMPEQUD 0x100000c7
#define PPC_INST_VCMPEQUB 0x10000006
+/* macro to check if a word is a prefix */
+#define IS_PREFIX(x) (((x) >> 26) == 1)
+#define PPC_NO_SUFFIX 0
+#define PPC_INST_LENGTH(x) (IS_PREFIX(x) ? 8 : 4)
+
/* macros to insert fields into opcodes */
#define ___PPC_RA(a) (((a) & 0x1f) << 16)
#define ___PPC_RB(b) (((b) & 0x1f) << 11)
diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
index 769f055509c9..9ea8904a1549 100644
--- a/arch/powerpc/include/asm/sstep.h
+++ b/arch/powerpc/include/asm/sstep.h
@@ -89,11 +89,15 @@ enum instruction_type {
#define VSX_LDLEFT 4 /* load VSX register from left */
#define VSX_CHECK_VEC 8 /* check MSR_VEC not MSR_VSX for reg >= 32 */
+/* Prefixed flag, ORed in with type */
+#define PREFIXED 0x800
+
/* Size field in type word */
#define SIZE(n) ((n) << 12)
#define GETSIZE(w) ((w) >> 12)
#define GETTYPE(t) ((t) & INSTR_TYPE_MASK)
+#define OP_LENGTH(t) (((t) & PREFIXED) ? 8 : 4)
#define MKOP(t, f, s) ((t) | (f) | SIZE(s))
@@ -132,7 +136,7 @@ union vsx_reg {
* otherwise.
*/
extern int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
- unsigned int instr);
+ unsigned int instr, unsigned int suffix);
/*
* Emulate an instruction that can be executed just by updating
@@ -149,7 +153,8 @@ void emulate_update_regs(struct pt_regs *reg, struct instruction_op *op);
* 0 if it could not be emulated, or -1 for an instruction that
* should not be emulated (rfid, mtmsrd clearing MSR_RI, etc.).
*/
-extern int emulate_step(struct pt_regs *regs, unsigned int instr);
+extern int emulate_step(struct pt_regs *regs, unsigned int instr,
+ unsigned int suffix);
/*
* Emulate a load or store instruction by reading/writing the
diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
index 92045ed64976..ba3bf5c3ab62 100644
--- a/arch/powerpc/kernel/align.c
+++ b/arch/powerpc/kernel/align.c
@@ -334,7 +334,7 @@ int fix_alignment(struct pt_regs *regs)
if ((instr & 0xfc0006fe) == (PPC_INST_COPY & 0xfc0006fe))
return -EIO;
- r = analyse_instr(&op, regs, instr);
+ r = analyse_instr(&op, regs, instr, PPC_NO_SUFFIX);
if (r < 0)
return -EINVAL;
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 2462cd7c565c..3a7ec6760dab 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -251,7 +251,7 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
goto fail;
- ret = analyse_instr(&op, regs, instr);
+ ret = analyse_instr(&op, regs, instr, PPC_NO_SUFFIX);
type = GETTYPE(op.type);
size = GETSIZE(op.type);
@@ -275,7 +275,7 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
return false;
}
- if (!emulate_step(regs, instr))
+ if (!emulate_step(regs, instr, PPC_NO_SUFFIX))
goto fail;
return true;
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 2d27ec4feee4..24a56f062d9e 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -219,7 +219,7 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
unsigned int insn = *p->ainsn.insn;
/* regs->nip is also adjusted if emulate_step returns 1 */
- ret = emulate_step(regs, insn);
+ ret = emulate_step(regs, insn, PPC_NO_SUFFIX);
if (ret > 0) {
/*
* Once this instruction has been boosted
diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
index 1cbf7f1a4e3d..824eda536f5d 100644
--- a/arch/powerpc/kernel/mce_power.c
+++ b/arch/powerpc/kernel/mce_power.c
@@ -374,7 +374,7 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr,
if (pfn != ULONG_MAX) {
instr_addr = (pfn << PAGE_SHIFT) + (regs->nip & ~PAGE_MASK);
instr = *(unsigned int *)(instr_addr);
- if (!analyse_instr(&op, &tmp, instr)) {
+ if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
pfn = addr_to_pfn(regs, op.ea);
*addr = op.ea;
*phys_addr = (pfn << PAGE_SHIFT);
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 024f7aad1952..f908d9422557 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -100,7 +100,8 @@ static unsigned long can_optimize(struct kprobe *p)
* and that can be emulated.
*/
if (!is_conditional_branch(*p->ainsn.insn) &&
- analyse_instr(&op, ®s, *p->ainsn.insn) == 1) {
+ analyse_instr(&op, ®s, *p->ainsn.insn,
+ PPC_NO_SUFFIX) == 1) {
emulate_update_regs(®s, &op);
nip = regs.nip;
}
diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index 1cfef0e5fec5..4ab40c4b576f 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -173,7 +173,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
* emulate_step() returns 1 if the insn was successfully emulated.
* For all other cases, we need to single-step in hardware.
*/
- ret = emulate_step(regs, auprobe->insn);
+ ret = emulate_step(regs, auprobe->insn, PPC_NO_SUFFIX);
if (ret > 0)
return true;
diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
index 1139bc56e004..2fc1951cdae5 100644
--- a/arch/powerpc/kvm/emulate_loadstore.c
+++ b/arch/powerpc/kvm/emulate_loadstore.c
@@ -95,7 +95,7 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
emulated = EMULATE_FAIL;
vcpu->arch.regs.msr = vcpu->arch.shared->msr;
- if (analyse_instr(&op, &vcpu->arch.regs, inst) == 0) {
+ if (analyse_instr(&op, &vcpu->arch.regs, inst, PPC_NO_SUFFIX) == 0) {
int type = op.type & INSTR_TYPE_MASK;
int size = GETSIZE(op.type);
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index c077acb983a1..65143ab1bf64 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1163,7 +1163,7 @@ static nokprobe_inline int trap_compare(long v1, long v2)
* otherwise.
*/
int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
- unsigned int instr)
+ unsigned int instr, unsigned int suffix)
{
unsigned int opcode, ra, rb, rc, rd, spr, u;
unsigned long int imm;
@@ -2756,7 +2756,8 @@ void emulate_update_regs(struct pt_regs *regs, struct instruction_op *op)
{
unsigned long next_pc;
- next_pc = truncate_if_32bit(regs->msr, regs->nip + 4);
+ next_pc = truncate_if_32bit(regs->msr,
+ regs->nip + OP_LENGTH(op->type));
switch (GETTYPE(op->type)) {
case COMPUTE:
if (op->type & SETREG)
@@ -3101,14 +3102,14 @@ NOKPROBE_SYMBOL(emulate_loadstore);
* or -1 if the instruction is one that should not be stepped,
* such as an rfid, or a mtmsrd that would clear MSR_RI.
*/
-int emulate_step(struct pt_regs *regs, unsigned int instr)
+int emulate_step(struct pt_regs *regs, unsigned int instr, unsigned int suffix)
{
struct instruction_op op;
int r, err, type;
unsigned long val;
unsigned long ea;
- r = analyse_instr(&op, regs, instr);
+ r = analyse_instr(&op, regs, instr, suffix);
if (r < 0)
return r;
if (r > 0) {
@@ -3200,7 +3201,8 @@ int emulate_step(struct pt_regs *regs, unsigned int instr)
return 0;
instr_done:
- regs->nip = truncate_if_32bit(regs->msr, regs->nip + 4);
+ regs->nip = truncate_if_32bit(regs->msr,
+ regs->nip + OP_LENGTH(op.type));
return 1;
}
NOKPROBE_SYMBOL(emulate_step);
diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
index 42347067739c..3bc042e15d00 100644
--- a/arch/powerpc/lib/test_emulate_step.c
+++ b/arch/powerpc/lib/test_emulate_step.c
@@ -103,7 +103,7 @@ static void __init test_ld(void)
regs.gpr[3] = (unsigned long) &a;
/* ld r5, 0(r3) */
- stepped = emulate_step(®s, TEST_LD(5, 3, 0));
+ stepped = emulate_step(®s, TEST_LD(5, 3, 0), PPC_NO_SUFFIX);
if (stepped == 1 && regs.gpr[5] == a)
show_result("ld", "PASS");
@@ -121,7 +121,7 @@ static void __init test_lwz(void)
regs.gpr[3] = (unsigned long) &a;
/* lwz r5, 0(r3) */
- stepped = emulate_step(®s, TEST_LWZ(5, 3, 0));
+ stepped = emulate_step(®s, TEST_LWZ(5, 3, 0), PPC_NO_SUFFIX);
if (stepped == 1 && regs.gpr[5] == a)
show_result("lwz", "PASS");
@@ -141,7 +141,7 @@ static void __init test_lwzx(void)
regs.gpr[5] = 0x8765;
/* lwzx r5, r3, r4 */
- stepped = emulate_step(®s, TEST_LWZX(5, 3, 4));
+ stepped = emulate_step(®s, TEST_LWZX(5, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1 && regs.gpr[5] == a[2])
show_result("lwzx", "PASS");
else
@@ -159,7 +159,7 @@ static void __init test_std(void)
regs.gpr[5] = 0x5678;
/* std r5, 0(r3) */
- stepped = emulate_step(®s, TEST_STD(5, 3, 0));
+ stepped = emulate_step(®s, TEST_STD(5, 3, 0), PPC_NO_SUFFIX);
if (stepped == 1 || regs.gpr[5] == a)
show_result("std", "PASS");
else
@@ -184,7 +184,7 @@ static void __init test_ldarx_stdcx(void)
regs.gpr[5] = 0x5678;
/* ldarx r5, r3, r4, 0 */
- stepped = emulate_step(®s, TEST_LDARX(5, 3, 4, 0));
+ stepped = emulate_step(®s, TEST_LDARX(5, 3, 4, 0), PPC_NO_SUFFIX);
/*
* Don't touch 'a' here. Touching 'a' can do Load/store
@@ -202,7 +202,7 @@ static void __init test_ldarx_stdcx(void)
regs.gpr[5] = 0x9ABC;
/* stdcx. r5, r3, r4 */
- stepped = emulate_step(®s, TEST_STDCX(5, 3, 4));
+ stepped = emulate_step(®s, TEST_STDCX(5, 3, 4), PPC_NO_SUFFIX);
/*
* Two possible scenarios that indicates successful emulation
@@ -242,7 +242,7 @@ static void __init test_lfsx_stfsx(void)
regs.gpr[4] = 0;
/* lfsx frt10, r3, r4 */
- stepped = emulate_step(®s, TEST_LFSX(10, 3, 4));
+ stepped = emulate_step(®s, TEST_LFSX(10, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1)
show_result("lfsx", "PASS");
@@ -255,7 +255,7 @@ static void __init test_lfsx_stfsx(void)
c.a = 678.91;
/* stfsx frs10, r3, r4 */
- stepped = emulate_step(®s, TEST_STFSX(10, 3, 4));
+ stepped = emulate_step(®s, TEST_STFSX(10, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1 && c.b == cached_b)
show_result("stfsx", "PASS");
@@ -285,7 +285,7 @@ static void __init test_lfdx_stfdx(void)
regs.gpr[4] = 0;
/* lfdx frt10, r3, r4 */
- stepped = emulate_step(®s, TEST_LFDX(10, 3, 4));
+ stepped = emulate_step(®s, TEST_LFDX(10, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1)
show_result("lfdx", "PASS");
@@ -298,7 +298,7 @@ static void __init test_lfdx_stfdx(void)
c.a = 987654.32;
/* stfdx frs10, r3, r4 */
- stepped = emulate_step(®s, TEST_STFDX(10, 3, 4));
+ stepped = emulate_step(®s, TEST_STFDX(10, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1 && c.b == cached_b)
show_result("stfdx", "PASS");
@@ -344,7 +344,7 @@ static void __init test_lvx_stvx(void)
regs.gpr[4] = 0;
/* lvx vrt10, r3, r4 */
- stepped = emulate_step(®s, TEST_LVX(10, 3, 4));
+ stepped = emulate_step(®s, TEST_LVX(10, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1)
show_result("lvx", "PASS");
@@ -360,7 +360,7 @@ static void __init test_lvx_stvx(void)
c.b[3] = 498532;
/* stvx vrs10, r3, r4 */
- stepped = emulate_step(®s, TEST_STVX(10, 3, 4));
+ stepped = emulate_step(®s, TEST_STVX(10, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1 && cached_b[0] == c.b[0] && cached_b[1] == c.b[1] &&
cached_b[2] == c.b[2] && cached_b[3] == c.b[3])
@@ -401,7 +401,7 @@ static void __init test_lxvd2x_stxvd2x(void)
regs.gpr[4] = 0;
/* lxvd2x vsr39, r3, r4 */
- stepped = emulate_step(®s, TEST_LXVD2X(39, 3, 4));
+ stepped = emulate_step(®s, TEST_LXVD2X(39, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1 && cpu_has_feature(CPU_FTR_VSX)) {
show_result("lxvd2x", "PASS");
@@ -421,7 +421,7 @@ static void __init test_lxvd2x_stxvd2x(void)
c.b[3] = 4;
/* stxvd2x vsr39, r3, r4 */
- stepped = emulate_step(®s, TEST_STXVD2X(39, 3, 4));
+ stepped = emulate_step(®s, TEST_STXVD2X(39, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1 && cached_b[0] == c.b[0] && cached_b[1] == c.b[1] &&
cached_b[2] == c.b[2] && cached_b[3] == c.b[3] &&
@@ -848,7 +848,7 @@ static int __init emulate_compute_instr(struct pt_regs *regs,
if (!regs || !instr)
return -EINVAL;
- if (analyse_instr(&op, regs, instr) != 1 ||
+ if (analyse_instr(&op, regs, instr, PPC_NO_SUFFIX) != 1 ||
GETTYPE(op.type) != COMPUTE) {
pr_info("emulation failed, instruction = 0x%08x\n", instr);
return -EFAULT;
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index e8c84d265602..897e512c6379 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -705,7 +705,8 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
if ((regs->msr & (MSR_IR|MSR_PR|MSR_64BIT)) == (MSR_IR|MSR_64BIT)) {
bp = at_breakpoint(regs->nip);
if (bp != NULL) {
- int stepped = emulate_step(regs, bp->instr[0]);
+ int stepped = emulate_step(regs, bp->instr[0],
+ PPC_NO_SUFFIX);
if (stepped == 0) {
regs->nip = (unsigned long) &bp->instr[0];
atomic_inc(&bp->ref_count);
@@ -1170,7 +1171,7 @@ static int do_step(struct pt_regs *regs)
/* 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);
+ stepped = emulate_step(regs, instr, PPC_NO_SUFFIX);
if (stepped < 0) {
printf("Couldn't single-step %s instruction\n",
(IS_RFID(instr)? "rfid": "mtmsrd"));
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 03/13] powerpc sstep: Prepare to support prefixed instructions
2020-02-11 5:33 ` [PATCH v2 03/13] powerpc sstep: Prepare to support prefixed instructions Jordan Niethe
@ 2020-02-11 5:57 ` Christophe Leroy
2020-02-11 23:31 ` Jordan Niethe
0 siblings, 1 reply; 26+ messages in thread
From: Christophe Leroy @ 2020-02-11 5:57 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: bala24, alistair, mpe, dja
Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> Currently all instructions are a single word long. A future ISA version
> will include prefixed instructions which have a double word length. The
> functions used for analysing and emulating instructions need to be
> modified so that they can handle these new instruction types.
>
> A prefixed instruction is a word prefix followed by a word suffix. All
> prefixes uniquely have the primary op-code 1. Suffixes may be valid word
> instructions or instructions that only exist as suffixes.
>
> In handling prefixed instructions it will be convenient to treat the
> suffix and prefix as separate words. To facilitate this modify
> analyse_instr() and emulate_step() to take a suffix as a
> parameter. For word instructions it does not matter what is passed in
> here - it will be ignored.
>
> We also define a new flag, PREFIXED, to be used in instruction_op:type.
> This flag will indicate when emulating an analysed instruction if the
> NIP should be advanced by word length or double word length.
>
> The callers of analyse_instr() and emulate_step() will need their own
> changes to be able to support prefixed instructions. For now modify them
> to pass in 0 as a suffix.
>
> Note that at this point no prefixed instructions are emulated or
> analysed - this is just making it possible to do so.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v2: - Move definition of __get_user_instr() and
> __get_user_instr_inatomic() to "powerpc: Support prefixed instructions
> in alignment handler."
> - Use a macro for returning the length of an op
> - Rename sufx -> suffix
> - Define and use PPC_NO_SUFFIX instead of 0
> ---
> arch/powerpc/include/asm/ppc-opcode.h | 5 +++++
> arch/powerpc/include/asm/sstep.h | 9 ++++++--
> arch/powerpc/kernel/align.c | 2 +-
> arch/powerpc/kernel/hw_breakpoint.c | 4 ++--
> arch/powerpc/kernel/kprobes.c | 2 +-
> arch/powerpc/kernel/mce_power.c | 2 +-
> arch/powerpc/kernel/optprobes.c | 3 ++-
> arch/powerpc/kernel/uprobes.c | 2 +-
> arch/powerpc/kvm/emulate_loadstore.c | 2 +-
> arch/powerpc/lib/sstep.c | 12 ++++++-----
> arch/powerpc/lib/test_emulate_step.c | 30 +++++++++++++--------------
> arch/powerpc/xmon/xmon.c | 5 +++--
> 12 files changed, 46 insertions(+), 32 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> index c1df75edde44..72783bc92e50 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -377,6 +377,11 @@
> #define PPC_INST_VCMPEQUD 0x100000c7
> #define PPC_INST_VCMPEQUB 0x10000006
>
> +/* macro to check if a word is a prefix */
> +#define IS_PREFIX(x) (((x) >> 26) == 1)
Can you add an OP_PREFIX in the OP list and use it instead of '1' ?
> +#define PPC_NO_SUFFIX 0
> +#define PPC_INST_LENGTH(x) (IS_PREFIX(x) ? 8 : 4)
> +
> /* macros to insert fields into opcodes */
> #define ___PPC_RA(a) (((a) & 0x1f) << 16)
> #define ___PPC_RB(b) (((b) & 0x1f) << 11)
> diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
> index 769f055509c9..9ea8904a1549 100644
> --- a/arch/powerpc/include/asm/sstep.h
> +++ b/arch/powerpc/include/asm/sstep.h
> @@ -89,11 +89,15 @@ enum instruction_type {
> #define VSX_LDLEFT 4 /* load VSX register from left */
> #define VSX_CHECK_VEC 8 /* check MSR_VEC not MSR_VSX for reg >= 32 */
>
> +/* Prefixed flag, ORed in with type */
> +#define PREFIXED 0x800
> +
> /* Size field in type word */
> #define SIZE(n) ((n) << 12)
> #define GETSIZE(w) ((w) >> 12)
>
> #define GETTYPE(t) ((t) & INSTR_TYPE_MASK)
> +#define OP_LENGTH(t) (((t) & PREFIXED) ? 8 : 4)
Is it worth naming it OP_LENGTH ? Can't it be mistaken as one of the
OP_xxx from the list in asm/opcode.h ?
What about GETLENGTH() instead to be consistant with the above lines ?
Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 03/13] powerpc sstep: Prepare to support prefixed instructions
2020-02-11 5:57 ` Christophe Leroy
@ 2020-02-11 23:31 ` Jordan Niethe
0 siblings, 0 replies; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 23:31 UTC (permalink / raw)
To: Christophe Leroy
Cc: Balamuruhan S, Alistair Popple, Daniel Axtens, linuxppc-dev
On Tue, Feb 11, 2020 at 4:57 PM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> > Currently all instructions are a single word long. A future ISA version
> > will include prefixed instructions which have a double word length. The
> > functions used for analysing and emulating instructions need to be
> > modified so that they can handle these new instruction types.
> >
> > A prefixed instruction is a word prefix followed by a word suffix. All
> > prefixes uniquely have the primary op-code 1. Suffixes may be valid word
> > instructions or instructions that only exist as suffixes.
> >
> > In handling prefixed instructions it will be convenient to treat the
> > suffix and prefix as separate words. To facilitate this modify
> > analyse_instr() and emulate_step() to take a suffix as a
> > parameter. For word instructions it does not matter what is passed in
> > here - it will be ignored.
> >
> > We also define a new flag, PREFIXED, to be used in instruction_op:type.
> > This flag will indicate when emulating an analysed instruction if the
> > NIP should be advanced by word length or double word length.
> >
> > The callers of analyse_instr() and emulate_step() will need their own
> > changes to be able to support prefixed instructions. For now modify them
> > to pass in 0 as a suffix.
> >
> > Note that at this point no prefixed instructions are emulated or
> > analysed - this is just making it possible to do so.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v2: - Move definition of __get_user_instr() and
> > __get_user_instr_inatomic() to "powerpc: Support prefixed instructions
> > in alignment handler."
> > - Use a macro for returning the length of an op
> > - Rename sufx -> suffix
> > - Define and use PPC_NO_SUFFIX instead of 0
> > ---
> > arch/powerpc/include/asm/ppc-opcode.h | 5 +++++
> > arch/powerpc/include/asm/sstep.h | 9 ++++++--
> > arch/powerpc/kernel/align.c | 2 +-
> > arch/powerpc/kernel/hw_breakpoint.c | 4 ++--
> > arch/powerpc/kernel/kprobes.c | 2 +-
> > arch/powerpc/kernel/mce_power.c | 2 +-
> > arch/powerpc/kernel/optprobes.c | 3 ++-
> > arch/powerpc/kernel/uprobes.c | 2 +-
> > arch/powerpc/kvm/emulate_loadstore.c | 2 +-
> > arch/powerpc/lib/sstep.c | 12 ++++++-----
> > arch/powerpc/lib/test_emulate_step.c | 30 +++++++++++++--------------
> > arch/powerpc/xmon/xmon.c | 5 +++--
> > 12 files changed, 46 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> > index c1df75edde44..72783bc92e50 100644
> > --- a/arch/powerpc/include/asm/ppc-opcode.h
> > +++ b/arch/powerpc/include/asm/ppc-opcode.h
> > @@ -377,6 +377,11 @@
> > #define PPC_INST_VCMPEQUD 0x100000c7
> > #define PPC_INST_VCMPEQUB 0x10000006
> >
> > +/* macro to check if a word is a prefix */
> > +#define IS_PREFIX(x) (((x) >> 26) == 1)
>
> Can you add an OP_PREFIX in the OP list and use it instead of '1' ?
Will do.
>
> > +#define PPC_NO_SUFFIX 0
> > +#define PPC_INST_LENGTH(x) (IS_PREFIX(x) ? 8 : 4)
> > +
> > /* macros to insert fields into opcodes */
> > #define ___PPC_RA(a) (((a) & 0x1f) << 16)
> > #define ___PPC_RB(b) (((b) & 0x1f) << 11)
> > diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
> > index 769f055509c9..9ea8904a1549 100644
> > --- a/arch/powerpc/include/asm/sstep.h
> > +++ b/arch/powerpc/include/asm/sstep.h
> > @@ -89,11 +89,15 @@ enum instruction_type {
> > #define VSX_LDLEFT 4 /* load VSX register from left */
> > #define VSX_CHECK_VEC 8 /* check MSR_VEC not MSR_VSX for reg >= 32 */
> >
> > +/* Prefixed flag, ORed in with type */
> > +#define PREFIXED 0x800
> > +
> > /* Size field in type word */
> > #define SIZE(n) ((n) << 12)
> > #define GETSIZE(w) ((w) >> 12)
> >
> > #define GETTYPE(t) ((t) & INSTR_TYPE_MASK)
> > +#define OP_LENGTH(t) (((t) & PREFIXED) ? 8 : 4)
>
> Is it worth naming it OP_LENGTH ? Can't it be mistaken as one of the
> OP_xxx from the list in asm/opcode.h ?
>
> What about GETLENGTH() instead to be consistant with the above lines ?
Good point, will do.
>
> Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 04/13] powerpc sstep: Add support for prefixed load/stores
2020-02-11 5:33 [PATCH v2 00/13] Initial Prefixed Instruction support Jordan Niethe
` (2 preceding siblings ...)
2020-02-11 5:33 ` [PATCH v2 03/13] powerpc sstep: Prepare to support prefixed instructions Jordan Niethe
@ 2020-02-11 5:33 ` Jordan Niethe
2020-02-11 6:05 ` Christophe Leroy
2020-02-11 5:33 ` [PATCH v2 05/13] powerpc sstep: Add support for prefixed fixed-point arithmetic Jordan Niethe
` (8 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
This adds emulation support for the following prefixed integer
load/stores:
* Prefixed Load Byte and Zero (plbz)
* Prefixed Load Halfword and Zero (plhz)
* Prefixed Load Halfword Algebraic (plha)
* Prefixed Load Word and Zero (plwz)
* Prefixed Load Word Algebraic (plwa)
* Prefixed Load Doubleword (pld)
* Prefixed Store Byte (pstb)
* Prefixed Store Halfword (psth)
* Prefixed Store Word (pstw)
* Prefixed Store Doubleword (pstd)
* Prefixed Load Quadword (plq)
* Prefixed Store Quadword (pstq)
the follow prefixed floating-point load/stores:
* Prefixed Load Floating-Point Single (plfs)
* Prefixed Load Floating-Point Double (plfd)
* Prefixed Store Floating-Point Single (pstfs)
* Prefixed Store Floating-Point Double (pstfd)
and for the following prefixed VSX load/stores:
* Prefixed Load VSX Scalar Doubleword (plxsd)
* Prefixed Load VSX Scalar Single-Precision (plxssp)
* Prefixed Load VSX Vector [0|1] (plxv, plxv0, plxv1)
* Prefixed Store VSX Scalar Doubleword (pstxsd)
* Prefixed Store VSX Scalar Single-Precision (pstxssp)
* Prefixed Store VSX Vector [0|1] (pstxv, pstxv0, pstxv1)
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Combine all load/store patches
- Fix the name of Type 01 instructions
- Remove sign extension flag from pstd/pld
- Rename sufx -> suffix
---
arch/powerpc/lib/sstep.c | 165 +++++++++++++++++++++++++++++++++++++++
1 file changed, 165 insertions(+)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 65143ab1bf64..0e21c21ff2be 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -187,6 +187,44 @@ static nokprobe_inline unsigned long xform_ea(unsigned int instr,
return ea;
}
+/*
+ * Calculate effective address for a MLS:D-form / 8LS:D-form
+ * prefixed instruction
+ */
+static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned int instr,
+ unsigned int suffix,
+ const struct pt_regs *regs)
+{
+ int ra, prefix_r;
+ unsigned int dd;
+ unsigned long ea, d0, d1, d;
+
+ prefix_r = instr & (1ul << 20);
+ ra = (suffix >> 16) & 0x1f;
+
+ d0 = instr & 0x3ffff;
+ d1 = suffix & 0xffff;
+ d = (d0 << 16) | d1;
+
+ /*
+ * sign extend a 34 bit number
+ */
+ dd = (unsigned int) (d >> 2);
+ ea = (signed int) dd;
+ ea = (ea << 2) | (d & 0x3);
+
+ if (!prefix_r && ra)
+ ea += regs->gpr[ra];
+ else if (!prefix_r && !ra)
+ ; /* Leave ea as is */
+ else if (prefix_r && !ra)
+ ea += regs->nip;
+ else if (prefix_r && ra)
+ ; /* Invalid form. Should already be checked for by caller! */
+
+ return ea;
+}
+
/*
* Return the largest power of 2, not greater than sizeof(unsigned long),
* such that x is a multiple of it.
@@ -1166,6 +1204,7 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
unsigned int instr, unsigned int suffix)
{
unsigned int opcode, ra, rb, rc, rd, spr, u;
+ unsigned int suffixopcode, prefixtype, prefix_r;
unsigned long int imm;
unsigned long int val, val2;
unsigned int mb, me, sh;
@@ -2652,6 +2691,132 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
}
+/*
+ * Prefixed instructions
+ */
+ switch (opcode) {
+ case 1:
+ prefix_r = instr & (1ul << 20);
+ ra = (suffix >> 16) & 0x1f;
+ op->update_reg = ra;
+ rd = (suffix >> 21) & 0x1f;
+ op->reg = rd;
+ op->val = regs->gpr[rd];
+
+ suffixopcode = suffix >> 26;
+ prefixtype = (instr >> 24) & 0x3;
+ switch (prefixtype) {
+ case 0: /* Type 00 Eight-Byte Load/Store */
+ if (prefix_r && ra)
+ break;
+ op->ea = mlsd_8lsd_ea(instr, suffix, regs);
+ switch (suffixopcode) {
+ case 41: /* plwa */
+ op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 4);
+ break;
+ case 42: /* plxsd */
+ op->reg = rd + 32;
+ op->type = MKOP(LOAD_VSX, PREFIXED, 8);
+ op->element_size = 8;
+ op->vsx_flags = VSX_CHECK_VEC;
+ break;
+ case 43: /* plxssp */
+ op->reg = rd + 32;
+ op->type = MKOP(LOAD_VSX, PREFIXED, 4);
+ op->element_size = 8;
+ op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
+ break;
+ case 46: /* pstxsd */
+ op->reg = rd + 32;
+ op->type = MKOP(STORE_VSX, PREFIXED, 8);
+ op->element_size = 8;
+ op->vsx_flags = VSX_CHECK_VEC;
+ break;
+ case 47: /* pstxssp */
+ op->reg = rd + 32;
+ op->type = MKOP(STORE_VSX, PREFIXED, 4);
+ op->element_size = 8;
+ op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
+ break;
+ case 51: /* plxv1 */
+ op->reg += 32;
+
+ /* fallthru */
+ case 50: /* plxv0 */
+ op->type = MKOP(LOAD_VSX, PREFIXED, 16);
+ op->element_size = 16;
+ op->vsx_flags = VSX_CHECK_VEC;
+ break;
+ case 55: /* pstxv1 */
+ op->reg = rd + 32;
+
+ /* fallthru */
+ case 54: /* pstxv0 */
+ op->type = MKOP(STORE_VSX, PREFIXED, 16);
+ op->element_size = 16;
+ op->vsx_flags = VSX_CHECK_VEC;
+ break;
+ case 56: /* plq */
+ op->type = MKOP(LOAD, PREFIXED, 16);
+ break;
+ case 57: /* pld */
+ op->type = MKOP(LOAD, PREFIXED, 8);
+ break;
+ case 60: /* stq */
+ op->type = MKOP(STORE, PREFIXED, 16);
+ break;
+ case 61: /* pstd */
+ op->type = MKOP(STORE, PREFIXED, 8);
+ break;
+ }
+ break;
+ case 1: /* Type 01 Eight-Byte Register-to-Register */
+ break;
+ case 2: /* Type 10 Modified Load/Store */
+ if (prefix_r && ra)
+ break;
+ op->ea = mlsd_8lsd_ea(instr, suffix, regs);
+ switch (suffixopcode) {
+ case 32: /* plwz */
+ op->type = MKOP(LOAD, PREFIXED, 4);
+ break;
+ case 34: /* plbz */
+ op->type = MKOP(LOAD, PREFIXED, 1);
+ break;
+ case 36: /* pstw */
+ op->type = MKOP(STORE, PREFIXED, 4);
+ break;
+ case 38: /* pstb */
+ op->type = MKOP(STORE, PREFIXED, 1);
+ break;
+ case 40: /* plhz */
+ op->type = MKOP(LOAD, PREFIXED, 2);
+ break;
+ case 42: /* plha */
+ op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 2);
+ break;
+ case 44: /* psth */
+ op->type = MKOP(STORE, PREFIXED, 2);
+ break;
+ case 48: /* plfs */
+ op->type = MKOP(LOAD_FP, PREFIXED | FPCONV, 4);
+ break;
+ case 50: /* plfd */
+ op->type = MKOP(LOAD_FP, PREFIXED, 8);
+ break;
+ case 52: /* pstfs */
+ op->type = MKOP(STORE_FP, PREFIXED | FPCONV, 4);
+ break;
+ case 54: /* pstfd */
+ op->type = MKOP(STORE_FP, PREFIXED, 8);
+ break;
+ }
+ break;
+ case 3: /* Type 11 Modified Register-to-Register */
+ break;
+ }
+ }
+
#ifdef CONFIG_VSX
if ((GETTYPE(op->type) == LOAD_VSX ||
GETTYPE(op->type) == STORE_VSX) &&
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 04/13] powerpc sstep: Add support for prefixed load/stores
2020-02-11 5:33 ` [PATCH v2 04/13] powerpc sstep: Add support for prefixed load/stores Jordan Niethe
@ 2020-02-11 6:05 ` Christophe Leroy
2020-02-12 2:59 ` Jordan Niethe
0 siblings, 1 reply; 26+ messages in thread
From: Christophe Leroy @ 2020-02-11 6:05 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: bala24, alistair, mpe, dja
Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> This adds emulation support for the following prefixed integer
> load/stores:
> * Prefixed Load Byte and Zero (plbz)
> * Prefixed Load Halfword and Zero (plhz)
> * Prefixed Load Halfword Algebraic (plha)
> * Prefixed Load Word and Zero (plwz)
> * Prefixed Load Word Algebraic (plwa)
> * Prefixed Load Doubleword (pld)
> * Prefixed Store Byte (pstb)
> * Prefixed Store Halfword (psth)
> * Prefixed Store Word (pstw)
> * Prefixed Store Doubleword (pstd)
> * Prefixed Load Quadword (plq)
> * Prefixed Store Quadword (pstq)
>
> the follow prefixed floating-point load/stores:
> * Prefixed Load Floating-Point Single (plfs)
> * Prefixed Load Floating-Point Double (plfd)
> * Prefixed Store Floating-Point Single (pstfs)
> * Prefixed Store Floating-Point Double (pstfd)
>
> and for the following prefixed VSX load/stores:
> * Prefixed Load VSX Scalar Doubleword (plxsd)
> * Prefixed Load VSX Scalar Single-Precision (plxssp)
> * Prefixed Load VSX Vector [0|1] (plxv, plxv0, plxv1)
> * Prefixed Store VSX Scalar Doubleword (pstxsd)
> * Prefixed Store VSX Scalar Single-Precision (pstxssp)
> * Prefixed Store VSX Vector [0|1] (pstxv, pstxv0, pstxv1)
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v2: - Combine all load/store patches
> - Fix the name of Type 01 instructions
> - Remove sign extension flag from pstd/pld
> - Rename sufx -> suffix
> ---
> arch/powerpc/lib/sstep.c | 165 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 165 insertions(+)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index 65143ab1bf64..0e21c21ff2be 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -187,6 +187,44 @@ static nokprobe_inline unsigned long xform_ea(unsigned int instr,
> return ea;
> }
>
> +/*
> + * Calculate effective address for a MLS:D-form / 8LS:D-form
> + * prefixed instruction
> + */
> +static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned int instr,
> + unsigned int suffix,
> + const struct pt_regs *regs)
> +{
> + int ra, prefix_r;
> + unsigned int dd;
> + unsigned long ea, d0, d1, d;
> +
> + prefix_r = instr & (1ul << 20);
> + ra = (suffix >> 16) & 0x1f;
> +
> + d0 = instr & 0x3ffff;
> + d1 = suffix & 0xffff;
> + d = (d0 << 16) | d1;
> +
> + /*
> + * sign extend a 34 bit number
> + */
> + dd = (unsigned int) (d >> 2);
> + ea = (signed int) dd;
> + ea = (ea << 2) | (d & 0x3);
> +
> + if (!prefix_r && ra)
> + ea += regs->gpr[ra];
> + else if (!prefix_r && !ra)
> + ; /* Leave ea as is */
> + else if (prefix_r && !ra)
> + ea += regs->nip;
> + else if (prefix_r && ra)
> + ; /* Invalid form. Should already be checked for by caller! */
> +
> + return ea;
> +}
> +
> /*
> * Return the largest power of 2, not greater than sizeof(unsigned long),
> * such that x is a multiple of it.
> @@ -1166,6 +1204,7 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> unsigned int instr, unsigned int suffix)
> {
> unsigned int opcode, ra, rb, rc, rd, spr, u;
> + unsigned int suffixopcode, prefixtype, prefix_r;
> unsigned long int imm;
> unsigned long int val, val2;
> unsigned int mb, me, sh;
> @@ -2652,6 +2691,132 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
>
> }
>
> +/*
> + * Prefixed instructions
> + */
> + switch (opcode) {
> + case 1:
Why not include it in the above switch () ?
Should it be enclosed by #ifdef __powerpc64__, or will this new ISA also
apply to 32 bits processors ?
> + prefix_r = instr & (1ul << 20);
> + ra = (suffix >> 16) & 0x1f;
> + op->update_reg = ra;
> + rd = (suffix >> 21) & 0x1f;
> + op->reg = rd;
> + op->val = regs->gpr[rd];
> +
> + suffixopcode = suffix >> 26;
> + prefixtype = (instr >> 24) & 0x3;
> + switch (prefixtype) {
> + case 0: /* Type 00 Eight-Byte Load/Store */
> + if (prefix_r && ra)
> + break;
> + op->ea = mlsd_8lsd_ea(instr, suffix, regs);
> + switch (suffixopcode) {
> + case 41: /* plwa */
> + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 4);
> + break;
> + case 42: /* plxsd */
> + op->reg = rd + 32;
> + op->type = MKOP(LOAD_VSX, PREFIXED, 8);
> + op->element_size = 8;
> + op->vsx_flags = VSX_CHECK_VEC;
> + break;
> + case 43: /* plxssp */
> + op->reg = rd + 32;
> + op->type = MKOP(LOAD_VSX, PREFIXED, 4);
> + op->element_size = 8;
> + op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
> + break;
> + case 46: /* pstxsd */
> + op->reg = rd + 32;
> + op->type = MKOP(STORE_VSX, PREFIXED, 8);
> + op->element_size = 8;
> + op->vsx_flags = VSX_CHECK_VEC;
> + break;
> + case 47: /* pstxssp */
> + op->reg = rd + 32;
> + op->type = MKOP(STORE_VSX, PREFIXED, 4);
> + op->element_size = 8;
> + op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
> + break;
> + case 51: /* plxv1 */
> + op->reg += 32;
> +
> + /* fallthru */
> + case 50: /* plxv0 */
> + op->type = MKOP(LOAD_VSX, PREFIXED, 16);
> + op->element_size = 16;
> + op->vsx_flags = VSX_CHECK_VEC;
> + break;
> + case 55: /* pstxv1 */
> + op->reg = rd + 32;
> +
> + /* fallthru */
> + case 54: /* pstxv0 */
> + op->type = MKOP(STORE_VSX, PREFIXED, 16);
> + op->element_size = 16;
> + op->vsx_flags = VSX_CHECK_VEC;
> + break;
> + case 56: /* plq */
> + op->type = MKOP(LOAD, PREFIXED, 16);
> + break;
> + case 57: /* pld */
> + op->type = MKOP(LOAD, PREFIXED, 8);
> + break;
> + case 60: /* stq */
> + op->type = MKOP(STORE, PREFIXED, 16);
> + break;
> + case 61: /* pstd */
> + op->type = MKOP(STORE, PREFIXED, 8);
> + break;
> + }
> + break;
> + case 1: /* Type 01 Eight-Byte Register-to-Register */
> + break;
> + case 2: /* Type 10 Modified Load/Store */
> + if (prefix_r && ra)
> + break;
> + op->ea = mlsd_8lsd_ea(instr, suffix, regs);
> + switch (suffixopcode) {
> + case 32: /* plwz */
> + op->type = MKOP(LOAD, PREFIXED, 4);
> + break;
> + case 34: /* plbz */
> + op->type = MKOP(LOAD, PREFIXED, 1);
> + break;
> + case 36: /* pstw */
> + op->type = MKOP(STORE, PREFIXED, 4);
> + break;
> + case 38: /* pstb */
> + op->type = MKOP(STORE, PREFIXED, 1);
> + break;
> + case 40: /* plhz */
> + op->type = MKOP(LOAD, PREFIXED, 2);
> + break;
> + case 42: /* plha */
> + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 2);
> + break;
> + case 44: /* psth */
> + op->type = MKOP(STORE, PREFIXED, 2);
> + break;
> + case 48: /* plfs */
> + op->type = MKOP(LOAD_FP, PREFIXED | FPCONV, 4);
> + break;
> + case 50: /* plfd */
> + op->type = MKOP(LOAD_FP, PREFIXED, 8);
> + break;
> + case 52: /* pstfs */
> + op->type = MKOP(STORE_FP, PREFIXED | FPCONV, 4);
> + break;
> + case 54: /* pstfd */
> + op->type = MKOP(STORE_FP, PREFIXED, 8);
> + break;
> + }
> + break;
> + case 3: /* Type 11 Modified Register-to-Register */
> + break;
> + }
> + }
> +
> #ifdef CONFIG_VSX
> if ((GETTYPE(op->type) == LOAD_VSX ||
> GETTYPE(op->type) == STORE_VSX) &&
>
Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 04/13] powerpc sstep: Add support for prefixed load/stores
2020-02-11 6:05 ` Christophe Leroy
@ 2020-02-12 2:59 ` Jordan Niethe
0 siblings, 0 replies; 26+ messages in thread
From: Jordan Niethe @ 2020-02-12 2:59 UTC (permalink / raw)
To: Christophe Leroy
Cc: Balamuruhan S, Alistair Popple, Daniel Axtens, linuxppc-dev
On Tue, Feb 11, 2020 at 5:05 PM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> > This adds emulation support for the following prefixed integer
> > load/stores:
> > * Prefixed Load Byte and Zero (plbz)
> > * Prefixed Load Halfword and Zero (plhz)
> > * Prefixed Load Halfword Algebraic (plha)
> > * Prefixed Load Word and Zero (plwz)
> > * Prefixed Load Word Algebraic (plwa)
> > * Prefixed Load Doubleword (pld)
> > * Prefixed Store Byte (pstb)
> > * Prefixed Store Halfword (psth)
> > * Prefixed Store Word (pstw)
> > * Prefixed Store Doubleword (pstd)
> > * Prefixed Load Quadword (plq)
> > * Prefixed Store Quadword (pstq)
> >
> > the follow prefixed floating-point load/stores:
> > * Prefixed Load Floating-Point Single (plfs)
> > * Prefixed Load Floating-Point Double (plfd)
> > * Prefixed Store Floating-Point Single (pstfs)
> > * Prefixed Store Floating-Point Double (pstfd)
> >
> > and for the following prefixed VSX load/stores:
> > * Prefixed Load VSX Scalar Doubleword (plxsd)
> > * Prefixed Load VSX Scalar Single-Precision (plxssp)
> > * Prefixed Load VSX Vector [0|1] (plxv, plxv0, plxv1)
> > * Prefixed Store VSX Scalar Doubleword (pstxsd)
> > * Prefixed Store VSX Scalar Single-Precision (pstxssp)
> > * Prefixed Store VSX Vector [0|1] (pstxv, pstxv0, pstxv1)
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v2: - Combine all load/store patches
> > - Fix the name of Type 01 instructions
> > - Remove sign extension flag from pstd/pld
> > - Rename sufx -> suffix
> > ---
> > arch/powerpc/lib/sstep.c | 165 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 165 insertions(+)
> >
> > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> > index 65143ab1bf64..0e21c21ff2be 100644
> > --- a/arch/powerpc/lib/sstep.c
> > +++ b/arch/powerpc/lib/sstep.c
> > @@ -187,6 +187,44 @@ static nokprobe_inline unsigned long xform_ea(unsigned int instr,
> > return ea;
> > }
> >
> > +/*
> > + * Calculate effective address for a MLS:D-form / 8LS:D-form
> > + * prefixed instruction
> > + */
> > +static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned int instr,
> > + unsigned int suffix,
> > + const struct pt_regs *regs)
> > +{
> > + int ra, prefix_r;
> > + unsigned int dd;
> > + unsigned long ea, d0, d1, d;
> > +
> > + prefix_r = instr & (1ul << 20);
> > + ra = (suffix >> 16) & 0x1f;
> > +
> > + d0 = instr & 0x3ffff;
> > + d1 = suffix & 0xffff;
> > + d = (d0 << 16) | d1;
> > +
> > + /*
> > + * sign extend a 34 bit number
> > + */
> > + dd = (unsigned int) (d >> 2);
> > + ea = (signed int) dd;
> > + ea = (ea << 2) | (d & 0x3);
> > +
> > + if (!prefix_r && ra)
> > + ea += regs->gpr[ra];
> > + else if (!prefix_r && !ra)
> > + ; /* Leave ea as is */
> > + else if (prefix_r && !ra)
> > + ea += regs->nip;
> > + else if (prefix_r && ra)
> > + ; /* Invalid form. Should already be checked for by caller! */
> > +
> > + return ea;
> > +}
> > +
> > /*
> > * Return the largest power of 2, not greater than sizeof(unsigned long),
> > * such that x is a multiple of it.
> > @@ -1166,6 +1204,7 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> > unsigned int instr, unsigned int suffix)
> > {
> > unsigned int opcode, ra, rb, rc, rd, spr, u;
> > + unsigned int suffixopcode, prefixtype, prefix_r;
> > unsigned long int imm;
> > unsigned long int val, val2;
> > unsigned int mb, me, sh;
> > @@ -2652,6 +2691,132 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> >
> > }
> >
> > +/*
> > + * Prefixed instructions
> > + */
> > + switch (opcode) {
> > + case 1:
>
> Why not include it in the above switch () ?
I was wanting to keep all the prefixed instructions together, but you
are right, these are all load/stores so it would be clearer for them
to go in the Load and Stores switch.
>
> Should it be enclosed by #ifdef __powerpc64__, or will this new ISA also
> apply to 32 bits processors ?
No at this time it will not affect 32bit processors. I will #ifdef it.
>
> > + prefix_r = instr & (1ul << 20);
> > + ra = (suffix >> 16) & 0x1f;
> > + op->update_reg = ra;
> > + rd = (suffix >> 21) & 0x1f;
> > + op->reg = rd;
> > + op->val = regs->gpr[rd];
> > +
> > + suffixopcode = suffix >> 26;
> > + prefixtype = (instr >> 24) & 0x3;
> > + switch (prefixtype) {
> > + case 0: /* Type 00 Eight-Byte Load/Store */
> > + if (prefix_r && ra)
> > + break;
> > + op->ea = mlsd_8lsd_ea(instr, suffix, regs);
> > + switch (suffixopcode) {
> > + case 41: /* plwa */
> > + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 4);
> > + break;
> > + case 42: /* plxsd */
> > + op->reg = rd + 32;
> > + op->type = MKOP(LOAD_VSX, PREFIXED, 8);
> > + op->element_size = 8;
> > + op->vsx_flags = VSX_CHECK_VEC;
> > + break;
> > + case 43: /* plxssp */
> > + op->reg = rd + 32;
> > + op->type = MKOP(LOAD_VSX, PREFIXED, 4);
> > + op->element_size = 8;
> > + op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
> > + break;
> > + case 46: /* pstxsd */
> > + op->reg = rd + 32;
> > + op->type = MKOP(STORE_VSX, PREFIXED, 8);
> > + op->element_size = 8;
> > + op->vsx_flags = VSX_CHECK_VEC;
> > + break;
> > + case 47: /* pstxssp */
> > + op->reg = rd + 32;
> > + op->type = MKOP(STORE_VSX, PREFIXED, 4);
> > + op->element_size = 8;
> > + op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
> > + break;
> > + case 51: /* plxv1 */
> > + op->reg += 32;
> > +
> > + /* fallthru */
> > + case 50: /* plxv0 */
> > + op->type = MKOP(LOAD_VSX, PREFIXED, 16);
> > + op->element_size = 16;
> > + op->vsx_flags = VSX_CHECK_VEC;
> > + break;
> > + case 55: /* pstxv1 */
> > + op->reg = rd + 32;
> > +
> > + /* fallthru */
> > + case 54: /* pstxv0 */
> > + op->type = MKOP(STORE_VSX, PREFIXED, 16);
> > + op->element_size = 16;
> > + op->vsx_flags = VSX_CHECK_VEC;
> > + break;
> > + case 56: /* plq */
> > + op->type = MKOP(LOAD, PREFIXED, 16);
> > + break;
> > + case 57: /* pld */
> > + op->type = MKOP(LOAD, PREFIXED, 8);
> > + break;
> > + case 60: /* stq */
> > + op->type = MKOP(STORE, PREFIXED, 16);
> > + break;
> > + case 61: /* pstd */
> > + op->type = MKOP(STORE, PREFIXED, 8);
> > + break;
> > + }
> > + break;
> > + case 1: /* Type 01 Eight-Byte Register-to-Register */
> > + break;
> > + case 2: /* Type 10 Modified Load/Store */
> > + if (prefix_r && ra)
> > + break;
> > + op->ea = mlsd_8lsd_ea(instr, suffix, regs);
> > + switch (suffixopcode) {
> > + case 32: /* plwz */
> > + op->type = MKOP(LOAD, PREFIXED, 4);
> > + break;
> > + case 34: /* plbz */
> > + op->type = MKOP(LOAD, PREFIXED, 1);
> > + break;
> > + case 36: /* pstw */
> > + op->type = MKOP(STORE, PREFIXED, 4);
> > + break;
> > + case 38: /* pstb */
> > + op->type = MKOP(STORE, PREFIXED, 1);
> > + break;
> > + case 40: /* plhz */
> > + op->type = MKOP(LOAD, PREFIXED, 2);
> > + break;
> > + case 42: /* plha */
> > + op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 2);
> > + break;
> > + case 44: /* psth */
> > + op->type = MKOP(STORE, PREFIXED, 2);
> > + break;
> > + case 48: /* plfs */
> > + op->type = MKOP(LOAD_FP, PREFIXED | FPCONV, 4);
> > + break;
> > + case 50: /* plfd */
> > + op->type = MKOP(LOAD_FP, PREFIXED, 8);
> > + break;
> > + case 52: /* pstfs */
> > + op->type = MKOP(STORE_FP, PREFIXED | FPCONV, 4);
> > + break;
> > + case 54: /* pstfd */
> > + op->type = MKOP(STORE_FP, PREFIXED, 8);
> > + break;
> > + }
> > + break;
> > + case 3: /* Type 11 Modified Register-to-Register */
> > + break;
> > + }
> > + }
> > +
> > #ifdef CONFIG_VSX
> > if ((GETTYPE(op->type) == LOAD_VSX ||
> > GETTYPE(op->type) == STORE_VSX) &&
> >
>
> Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 05/13] powerpc sstep: Add support for prefixed fixed-point arithmetic
2020-02-11 5:33 [PATCH v2 00/13] Initial Prefixed Instruction support Jordan Niethe
` (3 preceding siblings ...)
2020-02-11 5:33 ` [PATCH v2 04/13] powerpc sstep: Add support for prefixed load/stores Jordan Niethe
@ 2020-02-11 5:33 ` Jordan Niethe
2020-02-11 5:33 ` [PATCH v2 06/13] powerpc: Support prefixed instructions in alignment handler Jordan Niethe
` (7 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
This adds emulation support for the following prefixed Fixed-Point
Arithmetic instructions:
* Prefixed Add Immediate (paddi)
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
arch/powerpc/lib/sstep.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 0e21c21ff2be..8ba74c10bc03 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -2777,6 +2777,10 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
break;
op->ea = mlsd_8lsd_ea(instr, suffix, regs);
switch (suffixopcode) {
+ case 14: /* paddi */
+ op->type = COMPUTE | PREFIXED;
+ op->val = op->ea;
+ goto compute_done;
case 32: /* plwz */
op->type = MKOP(LOAD, PREFIXED, 4);
break;
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 06/13] powerpc: Support prefixed instructions in alignment handler
2020-02-11 5:33 [PATCH v2 00/13] Initial Prefixed Instruction support Jordan Niethe
` (4 preceding siblings ...)
2020-02-11 5:33 ` [PATCH v2 05/13] powerpc sstep: Add support for prefixed fixed-point arithmetic Jordan Niethe
@ 2020-02-11 5:33 ` Jordan Niethe
2020-02-11 6:14 ` Christophe Leroy
2020-02-11 5:33 ` [PATCH v2 07/13] powerpc/traps: Check for prefixed instructions in facility_unavailable_exception() Jordan Niethe
` (6 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
Alignment interrupts can be caused by prefixed instructions accessing
memory. In the alignment handler the instruction that caused the
exception is loaded and attempted emulate. If the instruction is a
prefixed instruction load the prefix and suffix to emulate. After
emulating increment the NIP by 8.
Prefixed instructions are not permitted to cross 64-byte boundaries. If
they do the alignment interrupt is invoked with SRR1 BOUNDARY bit set.
If this occurs send a SIGBUS to the offending process if in user mode.
If in kernel mode call bad_page_fault().
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Move __get_user_instr() and __get_user_instr_inatomic() to this
commit (previously in "powerpc sstep: Prepare to support prefixed
instructions").
- Rename sufx to suffix
- Use a macro for calculating instruction length
---
arch/powerpc/include/asm/uaccess.h | 30 ++++++++++++++++++++++++++++++
arch/powerpc/kernel/align.c | 8 +++++---
arch/powerpc/kernel/traps.c | 21 ++++++++++++++++++++-
3 files changed, 55 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 2f500debae21..30f63a81c8d8 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -474,4 +474,34 @@ static __must_check inline bool user_access_begin(const void __user *ptr, size_t
#define unsafe_copy_to_user(d, s, l, e) \
unsafe_op_wrap(raw_copy_to_user_allowed(d, s, l), e)
+/*
+ * When reading an instruction iff it is a prefix, the suffix needs to be also
+ * loaded.
+ */
+#define __get_user_instr(x, y, ptr) \
+({ \
+ long __gui_ret = 0; \
+ y = 0; \
+ __gui_ret = __get_user(x, ptr); \
+ if (!__gui_ret) { \
+ if (IS_PREFIX(x)) \
+ __gui_ret = __get_user(y, ptr + 1); \
+ } \
+ \
+ __gui_ret; \
+})
+
+#define __get_user_instr_inatomic(x, y, ptr) \
+({ \
+ long __gui_ret = 0; \
+ y = 0; \
+ __gui_ret = __get_user_inatomic(x, ptr); \
+ if (!__gui_ret) { \
+ if (IS_PREFIX(x)) \
+ __gui_ret = __get_user_inatomic(y, ptr + 1); \
+ } \
+ \
+ __gui_ret; \
+})
+
#endif /* _ARCH_POWERPC_UACCESS_H */
diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
index ba3bf5c3ab62..e42cfaa616d3 100644
--- a/arch/powerpc/kernel/align.c
+++ b/arch/powerpc/kernel/align.c
@@ -293,7 +293,7 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
int fix_alignment(struct pt_regs *regs)
{
- unsigned int instr;
+ unsigned int instr, suffix;
struct instruction_op op;
int r, type;
@@ -303,13 +303,15 @@ int fix_alignment(struct pt_regs *regs)
*/
CHECK_FULL_REGS(regs);
- if (unlikely(__get_user(instr, (unsigned int __user *)regs->nip)))
+ if (unlikely(__get_user_instr(instr, suffix,
+ (unsigned int __user *)regs->nip)))
return -EFAULT;
if ((regs->msr & MSR_LE) != (MSR_KERNEL & MSR_LE)) {
/* We don't handle PPC little-endian any more... */
if (cpu_has_feature(CPU_FTR_PPC_LE))
return -EIO;
instr = swab32(instr);
+ suffix = swab32(suffix);
}
#ifdef CONFIG_SPE
@@ -334,7 +336,7 @@ int fix_alignment(struct pt_regs *regs)
if ((instr & 0xfc0006fe) == (PPC_INST_COPY & 0xfc0006fe))
return -EIO;
- r = analyse_instr(&op, regs, instr, PPC_NO_SUFFIX);
+ r = analyse_instr(&op, regs, instr, suffix);
if (r < 0)
return -EINVAL;
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 82a3438300fd..d80b82fc1ae3 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -583,6 +583,10 @@ static inline int check_io_access(struct pt_regs *regs)
#define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
#define REASON_PRIVILEGED ESR_PPR
#define REASON_TRAP ESR_PTR
+#define REASON_PREFIXED 0
+#define REASON_BOUNDARY 0
+
+#define inst_length(reason) 4
/* single-step stuff */
#define single_stepping(regs) (current->thread.debug.dbcr0 & DBCR0_IC)
@@ -597,6 +601,10 @@ static inline int check_io_access(struct pt_regs *regs)
#define REASON_ILLEGAL SRR1_PROGILL
#define REASON_PRIVILEGED SRR1_PROGPRIV
#define REASON_TRAP SRR1_PROGTRAP
+#define REASON_PREFIXED SRR1_PREFIXED
+#define REASON_BOUNDARY SRR1_BOUNDARY
+
+#define inst_length(reason) (((reason) & REASON_PREFIXED) ? 8 : 4)
#define single_stepping(regs) ((regs)->msr & MSR_SE)
#define clear_single_step(regs) ((regs)->msr &= ~MSR_SE)
@@ -1593,11 +1601,20 @@ void alignment_exception(struct pt_regs *regs)
{
enum ctx_state prev_state = exception_enter();
int sig, code, fixed = 0;
+ unsigned long reason;
/* We restore the interrupt state now */
if (!arch_irq_disabled_regs(regs))
local_irq_enable();
+ reason = get_reason(regs);
+
+ if (reason & REASON_BOUNDARY) {
+ sig = SIGBUS;
+ code = BUS_ADRALN;
+ goto bad;
+ }
+
if (tm_abort_check(regs, TM_CAUSE_ALIGNMENT | TM_CAUSE_PERSISTENT))
goto bail;
@@ -1606,7 +1623,8 @@ void alignment_exception(struct pt_regs *regs)
fixed = fix_alignment(regs);
if (fixed == 1) {
- regs->nip += 4; /* skip over emulated instruction */
+ /* skip over emulated instruction */
+ regs->nip += inst_length(reason);
emulate_single_step(regs);
goto bail;
}
@@ -1619,6 +1637,7 @@ void alignment_exception(struct pt_regs *regs)
sig = SIGBUS;
code = BUS_ADRALN;
}
+bad:
if (user_mode(regs))
_exception(sig, regs, code, regs->dar);
else
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 06/13] powerpc: Support prefixed instructions in alignment handler
2020-02-11 5:33 ` [PATCH v2 06/13] powerpc: Support prefixed instructions in alignment handler Jordan Niethe
@ 2020-02-11 6:14 ` Christophe Leroy
2020-02-12 2:55 ` Jordan Niethe
0 siblings, 1 reply; 26+ messages in thread
From: Christophe Leroy @ 2020-02-11 6:14 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: bala24, alistair, mpe, dja
Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> Alignment interrupts can be caused by prefixed instructions accessing
> memory. In the alignment handler the instruction that caused the
> exception is loaded and attempted emulate. If the instruction is a
> prefixed instruction load the prefix and suffix to emulate. After
> emulating increment the NIP by 8.
>
> Prefixed instructions are not permitted to cross 64-byte boundaries. If
> they do the alignment interrupt is invoked with SRR1 BOUNDARY bit set.
> If this occurs send a SIGBUS to the offending process if in user mode.
> If in kernel mode call bad_page_fault().
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v2: - Move __get_user_instr() and __get_user_instr_inatomic() to this
> commit (previously in "powerpc sstep: Prepare to support prefixed
> instructions").
> - Rename sufx to suffix
> - Use a macro for calculating instruction length
> ---
> arch/powerpc/include/asm/uaccess.h | 30 ++++++++++++++++++++++++++++++
> arch/powerpc/kernel/align.c | 8 +++++---
> arch/powerpc/kernel/traps.c | 21 ++++++++++++++++++++-
> 3 files changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 2f500debae21..30f63a81c8d8 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -474,4 +474,34 @@ static __must_check inline bool user_access_begin(const void __user *ptr, size_t
> #define unsafe_copy_to_user(d, s, l, e) \
> unsafe_op_wrap(raw_copy_to_user_allowed(d, s, l), e)
>
Could it go close to other __get_user() and friends instead of being at
the end of the file ?
> +/*
> + * When reading an instruction iff it is a prefix, the suffix needs to be also
> + * loaded.
> + */
> +#define __get_user_instr(x, y, ptr) \
> +({ \
> + long __gui_ret = 0; \
> + y = 0; \
> + __gui_ret = __get_user(x, ptr); \
> + if (!__gui_ret) { \
> + if (IS_PREFIX(x)) \
Does this apply to PPC32 ?
If not, can we make sure IS_PREFIX is constant 0 on PPC32 so that the
second read gets dropped at compile time ?
Can we instead do :
if (!__gui_ret && IS_PREFIX(x))
> + __gui_ret = __get_user(y, ptr + 1); \
> + } \
> + \
> + __gui_ret; \
> +})
> +
> +#define __get_user_instr_inatomic(x, y, ptr) \
> +({ \
> + long __gui_ret = 0; \
> + y = 0; \
> + __gui_ret = __get_user_inatomic(x, ptr); \
> + if (!__gui_ret) { \
> + if (IS_PREFIX(x)) \
Same commments as above
> + __gui_ret = __get_user_inatomic(y, ptr + 1); \
> + } \
> + \
> + __gui_ret; \
> +})
> +
> #endif /* _ARCH_POWERPC_UACCESS_H */
> diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
> index ba3bf5c3ab62..e42cfaa616d3 100644
> --- a/arch/powerpc/kernel/align.c
> +++ b/arch/powerpc/kernel/align.c
> @@ -293,7 +293,7 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
>
> int fix_alignment(struct pt_regs *regs)
> {
> - unsigned int instr;
> + unsigned int instr, suffix;
> struct instruction_op op;
> int r, type;
>
> @@ -303,13 +303,15 @@ int fix_alignment(struct pt_regs *regs)
> */
> CHECK_FULL_REGS(regs);
>
> - if (unlikely(__get_user(instr, (unsigned int __user *)regs->nip)))
> + if (unlikely(__get_user_instr(instr, suffix,
> + (unsigned int __user *)regs->nip)))
> return -EFAULT;
> if ((regs->msr & MSR_LE) != (MSR_KERNEL & MSR_LE)) {
> /* We don't handle PPC little-endian any more... */
> if (cpu_has_feature(CPU_FTR_PPC_LE))
> return -EIO;
> instr = swab32(instr);
> + suffix = swab32(suffix);
> }
>
> #ifdef CONFIG_SPE
> @@ -334,7 +336,7 @@ int fix_alignment(struct pt_regs *regs)
> if ((instr & 0xfc0006fe) == (PPC_INST_COPY & 0xfc0006fe))
> return -EIO;
>
> - r = analyse_instr(&op, regs, instr, PPC_NO_SUFFIX);
> + r = analyse_instr(&op, regs, instr, suffix);
> if (r < 0)
> return -EINVAL;
>
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 82a3438300fd..d80b82fc1ae3 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -583,6 +583,10 @@ static inline int check_io_access(struct pt_regs *regs)
> #define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
> #define REASON_PRIVILEGED ESR_PPR
> #define REASON_TRAP ESR_PTR
> +#define REASON_PREFIXED 0
> +#define REASON_BOUNDARY 0
> +
> +#define inst_length(reason) 4
>
> /* single-step stuff */
> #define single_stepping(regs) (current->thread.debug.dbcr0 & DBCR0_IC)
> @@ -597,6 +601,10 @@ static inline int check_io_access(struct pt_regs *regs)
> #define REASON_ILLEGAL SRR1_PROGILL
> #define REASON_PRIVILEGED SRR1_PROGPRIV
> #define REASON_TRAP SRR1_PROGTRAP
> +#define REASON_PREFIXED SRR1_PREFIXED
> +#define REASON_BOUNDARY SRR1_BOUNDARY
> +
> +#define inst_length(reason) (((reason) & REASON_PREFIXED) ? 8 : 4)
>
> #define single_stepping(regs) ((regs)->msr & MSR_SE)
> #define clear_single_step(regs) ((regs)->msr &= ~MSR_SE)
> @@ -1593,11 +1601,20 @@ void alignment_exception(struct pt_regs *regs)
> {
> enum ctx_state prev_state = exception_enter();
> int sig, code, fixed = 0;
> + unsigned long reason;
>
> /* We restore the interrupt state now */
> if (!arch_irq_disabled_regs(regs))
> local_irq_enable();
>
> + reason = get_reason(regs);
> +
> + if (reason & REASON_BOUNDARY) {
> + sig = SIGBUS;
> + code = BUS_ADRALN;
> + goto bad;
> + }
> +
> if (tm_abort_check(regs, TM_CAUSE_ALIGNMENT | TM_CAUSE_PERSISTENT))
> goto bail;
>
> @@ -1606,7 +1623,8 @@ void alignment_exception(struct pt_regs *regs)
> fixed = fix_alignment(regs);
>
> if (fixed == 1) {
> - regs->nip += 4; /* skip over emulated instruction */
> + /* skip over emulated instruction */
> + regs->nip += inst_length(reason);
> emulate_single_step(regs);
> goto bail;
> }
> @@ -1619,6 +1637,7 @@ void alignment_exception(struct pt_regs *regs)
> sig = SIGBUS;
> code = BUS_ADRALN;
> }
> +bad:
> if (user_mode(regs))
> _exception(sig, regs, code, regs->dar);
> else
>
Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 06/13] powerpc: Support prefixed instructions in alignment handler
2020-02-11 6:14 ` Christophe Leroy
@ 2020-02-12 2:55 ` Jordan Niethe
0 siblings, 0 replies; 26+ messages in thread
From: Jordan Niethe @ 2020-02-12 2:55 UTC (permalink / raw)
To: Christophe Leroy
Cc: Balamuruhan S, Alistair Popple, Daniel Axtens, linuxppc-dev
On Tue, Feb 11, 2020 at 5:14 PM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> > Alignment interrupts can be caused by prefixed instructions accessing
> > memory. In the alignment handler the instruction that caused the
> > exception is loaded and attempted emulate. If the instruction is a
> > prefixed instruction load the prefix and suffix to emulate. After
> > emulating increment the NIP by 8.
> >
> > Prefixed instructions are not permitted to cross 64-byte boundaries. If
> > they do the alignment interrupt is invoked with SRR1 BOUNDARY bit set.
> > If this occurs send a SIGBUS to the offending process if in user mode.
> > If in kernel mode call bad_page_fault().
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v2: - Move __get_user_instr() and __get_user_instr_inatomic() to this
> > commit (previously in "powerpc sstep: Prepare to support prefixed
> > instructions").
> > - Rename sufx to suffix
> > - Use a macro for calculating instruction length
> > ---
> > arch/powerpc/include/asm/uaccess.h | 30 ++++++++++++++++++++++++++++++
> > arch/powerpc/kernel/align.c | 8 +++++---
> > arch/powerpc/kernel/traps.c | 21 ++++++++++++++++++++-
> > 3 files changed, 55 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> > index 2f500debae21..30f63a81c8d8 100644
> > --- a/arch/powerpc/include/asm/uaccess.h
> > +++ b/arch/powerpc/include/asm/uaccess.h
> > @@ -474,4 +474,34 @@ static __must_check inline bool user_access_begin(const void __user *ptr, size_t
> > #define unsafe_copy_to_user(d, s, l, e) \
> > unsafe_op_wrap(raw_copy_to_user_allowed(d, s, l), e)
> >
>
> Could it go close to other __get_user() and friends instead of being at
> the end of the file ?
Will do.
>
> > +/*
> > + * When reading an instruction iff it is a prefix, the suffix needs to be also
> > + * loaded.
> > + */
> > +#define __get_user_instr(x, y, ptr) \
> > +({ \
> > + long __gui_ret = 0; \
> > + y = 0; \
> > + __gui_ret = __get_user(x, ptr); \
> > + if (!__gui_ret) { \
> > + if (IS_PREFIX(x)) \
>
> Does this apply to PPC32 ?
No, for now (and the foreseeable future) it will just affect 64s.
> If not, can we make sure IS_PREFIX is constant 0 on PPC32 so that the
> second read gets dropped at compile time ?
>
> Can we instead do :
>
> if (!__gui_ret && IS_PREFIX(x))
Will do.
>
> > + __gui_ret = __get_user(y, ptr + 1); \
> > + } \
> > + \
> > + __gui_ret; \
> > +})
> > +
> > +#define __get_user_instr_inatomic(x, y, ptr) \
> > +({ \
> > + long __gui_ret = 0; \
> > + y = 0; \
> > + __gui_ret = __get_user_inatomic(x, ptr); \
> > + if (!__gui_ret) { \
> > + if (IS_PREFIX(x)) \
>
> Same commments as above
>
> > + __gui_ret = __get_user_inatomic(y, ptr + 1); \
> > + } \
> > + \
> > + __gui_ret; \
> > +})
> > +
> > #endif /* _ARCH_POWERPC_UACCESS_H */
> > diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
> > index ba3bf5c3ab62..e42cfaa616d3 100644
> > --- a/arch/powerpc/kernel/align.c
> > +++ b/arch/powerpc/kernel/align.c
> > @@ -293,7 +293,7 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
> >
> > int fix_alignment(struct pt_regs *regs)
> > {
> > - unsigned int instr;
> > + unsigned int instr, suffix;
> > struct instruction_op op;
> > int r, type;
> >
> > @@ -303,13 +303,15 @@ int fix_alignment(struct pt_regs *regs)
> > */
> > CHECK_FULL_REGS(regs);
> >
> > - if (unlikely(__get_user(instr, (unsigned int __user *)regs->nip)))
> > + if (unlikely(__get_user_instr(instr, suffix,
> > + (unsigned int __user *)regs->nip)))
> > return -EFAULT;
> > if ((regs->msr & MSR_LE) != (MSR_KERNEL & MSR_LE)) {
> > /* We don't handle PPC little-endian any more... */
> > if (cpu_has_feature(CPU_FTR_PPC_LE))
> > return -EIO;
> > instr = swab32(instr);
> > + suffix = swab32(suffix);
> > }
> >
> > #ifdef CONFIG_SPE
> > @@ -334,7 +336,7 @@ int fix_alignment(struct pt_regs *regs)
> > if ((instr & 0xfc0006fe) == (PPC_INST_COPY & 0xfc0006fe))
> > return -EIO;
> >
> > - r = analyse_instr(&op, regs, instr, PPC_NO_SUFFIX);
> > + r = analyse_instr(&op, regs, instr, suffix);
> > if (r < 0)
> > return -EINVAL;
> >
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index 82a3438300fd..d80b82fc1ae3 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -583,6 +583,10 @@ static inline int check_io_access(struct pt_regs *regs)
> > #define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
> > #define REASON_PRIVILEGED ESR_PPR
> > #define REASON_TRAP ESR_PTR
> > +#define REASON_PREFIXED 0
> > +#define REASON_BOUNDARY 0
> > +
> > +#define inst_length(reason) 4
> >
> > /* single-step stuff */
> > #define single_stepping(regs) (current->thread.debug.dbcr0 & DBCR0_IC)
> > @@ -597,6 +601,10 @@ static inline int check_io_access(struct pt_regs *regs)
> > #define REASON_ILLEGAL SRR1_PROGILL
> > #define REASON_PRIVILEGED SRR1_PROGPRIV
> > #define REASON_TRAP SRR1_PROGTRAP
> > +#define REASON_PREFIXED SRR1_PREFIXED
> > +#define REASON_BOUNDARY SRR1_BOUNDARY
> > +
> > +#define inst_length(reason) (((reason) & REASON_PREFIXED) ? 8 : 4)
> >
> > #define single_stepping(regs) ((regs)->msr & MSR_SE)
> > #define clear_single_step(regs) ((regs)->msr &= ~MSR_SE)
> > @@ -1593,11 +1601,20 @@ void alignment_exception(struct pt_regs *regs)
> > {
> > enum ctx_state prev_state = exception_enter();
> > int sig, code, fixed = 0;
> > + unsigned long reason;
> >
> > /* We restore the interrupt state now */
> > if (!arch_irq_disabled_regs(regs))
> > local_irq_enable();
> >
> > + reason = get_reason(regs);
> > +
> > + if (reason & REASON_BOUNDARY) {
> > + sig = SIGBUS;
> > + code = BUS_ADRALN;
> > + goto bad;
> > + }
> > +
> > if (tm_abort_check(regs, TM_CAUSE_ALIGNMENT | TM_CAUSE_PERSISTENT))
> > goto bail;
> >
> > @@ -1606,7 +1623,8 @@ void alignment_exception(struct pt_regs *regs)
> > fixed = fix_alignment(regs);
> >
> > if (fixed == 1) {
> > - regs->nip += 4; /* skip over emulated instruction */
> > + /* skip over emulated instruction */
> > + regs->nip += inst_length(reason);
> > emulate_single_step(regs);
> > goto bail;
> > }
> > @@ -1619,6 +1637,7 @@ void alignment_exception(struct pt_regs *regs)
> > sig = SIGBUS;
> > code = BUS_ADRALN;
> > }
> > +bad:
> > if (user_mode(regs))
> > _exception(sig, regs, code, regs->dar);
> > else
> >
>
> Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 07/13] powerpc/traps: Check for prefixed instructions in facility_unavailable_exception()
2020-02-11 5:33 [PATCH v2 00/13] Initial Prefixed Instruction support Jordan Niethe
` (5 preceding siblings ...)
2020-02-11 5:33 ` [PATCH v2 06/13] powerpc: Support prefixed instructions in alignment handler Jordan Niethe
@ 2020-02-11 5:33 ` Jordan Niethe
2020-02-11 5:33 ` [PATCH v2 08/13] powerpc/xmon: Add initial support for prefixed instructions Jordan Niethe
` (5 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
If prefixed instructions are made unavailable by the [H]FSCR, attempting
to use them will cause a facility unavailable exception. Add "PREFIX" to
the facility_strings[].
Currently there are no prefixed instructions that are actually emulated
by emulate_instruction() within facility_unavailable_exception().
However, when caused by a prefixed instructions the SRR1 PREFIXED bit is
set. Prepare for dealing with emulated prefixed instructions by checking
for this bit.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
arch/powerpc/kernel/traps.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index d80b82fc1ae3..cd8b3043c268 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1739,6 +1739,7 @@ void facility_unavailable_exception(struct pt_regs *regs)
[FSCR_TAR_LG] = "TAR",
[FSCR_MSGP_LG] = "MSGP",
[FSCR_SCV_LG] = "SCV",
+ [FSCR_PREFIX_LG] = "PREFIX",
};
char *facility = "unknown";
u64 value;
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 08/13] powerpc/xmon: Add initial support for prefixed instructions
2020-02-11 5:33 [PATCH v2 00/13] Initial Prefixed Instruction support Jordan Niethe
` (6 preceding siblings ...)
2020-02-11 5:33 ` [PATCH v2 07/13] powerpc/traps: Check for prefixed instructions in facility_unavailable_exception() Jordan Niethe
@ 2020-02-11 5:33 ` Jordan Niethe
2020-02-11 6:32 ` Christophe Leroy
2020-02-11 5:33 ` [PATCH v2 09/13] powerpc/xmon: Dump " Jordan Niethe
` (4 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
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 */
#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[] = { "--", "**", "##" };
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 08/13] powerpc/xmon: Add initial support for prefixed instructions
2020-02-11 5:33 ` [PATCH v2 08/13] powerpc/xmon: Add initial support for prefixed instructions Jordan Niethe
@ 2020-02-11 6:32 ` Christophe Leroy
2020-02-12 0:28 ` Jordan Niethe
0 siblings, 1 reply; 26+ messages in thread
From: Christophe Leroy @ 2020-02-11 6:32 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: bala24, alistair, mpe, dja
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 08/13] powerpc/xmon: Add initial support for prefixed instructions
2020-02-11 6:32 ` Christophe Leroy
@ 2020-02-12 0:28 ` Jordan Niethe
0 siblings, 0 replies; 26+ messages in thread
From: Jordan Niethe @ 2020-02-12 0:28 UTC (permalink / raw)
To: Christophe Leroy
Cc: Balamuruhan S, Alistair Popple, Daniel Axtens, linuxppc-dev
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 09/13] powerpc/xmon: Dump prefixed instructions
2020-02-11 5:33 [PATCH v2 00/13] Initial Prefixed Instruction support Jordan Niethe
` (7 preceding siblings ...)
2020-02-11 5:33 ` [PATCH v2 08/13] powerpc/xmon: Add initial support for prefixed instructions Jordan Niethe
@ 2020-02-11 5:33 ` Jordan Niethe
2020-02-11 6:39 ` Christophe Leroy
2020-02-11 5:33 ` [PATCH v2 10/13] powerpc/kprobes: Support kprobes on " Jordan Niethe
` (3 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
Currently when xmon is dumping instructions it reads a word at a time
and then prints that instruction (either as a hex number or by
disassembling it). For prefixed instructions it would be nice to show
its prefix and suffix as together. Use read_instr() so that if a prefix
is encountered its suffix is loaded too. Then print these in the form:
prefix:suffix
Xmon uses the disassembly routines from GNU binutils. These currently do
not support prefixed instructions so we will not disassemble the
prefixed instructions yet.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: Rename sufx to suffix
---
arch/powerpc/xmon/xmon.c | 50 +++++++++++++++++++++++++++++++---------
1 file changed, 39 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 0b085642bbe7..513901ee18b0 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -2903,6 +2903,21 @@ prdump(unsigned long adrs, long ndump)
}
}
+static bool instrs_are_equal(unsigned long insta, unsigned long suffixa,
+ unsigned long instb, unsigned long suffixb)
+{
+ if (insta != instb)
+ return false;
+
+ if (!IS_PREFIX(insta) && !IS_PREFIX(instb))
+ return true;
+
+ if (IS_PREFIX(insta) && IS_PREFIX(instb))
+ return suffixa == suffixb;
+
+ return false;
+}
+
typedef int (*instruction_dump_func)(unsigned long inst, unsigned long addr);
static int
@@ -2911,12 +2926,11 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
{
int nr, dotted;
unsigned long first_adr;
- unsigned int inst, last_inst = 0;
- unsigned char val[4];
+ unsigned int inst, suffix, last_inst = 0, last_suffix = 0;
dotted = 0;
- for (first_adr = adr; count > 0; --count, adr += 4) {
- nr = mread(adr, val, 4);
+ for (first_adr = adr; count > 0; --count, adr += nr) {
+ nr = read_instr(adr, &inst, &suffix);
if (nr == 0) {
if (praddr) {
const char *x = fault_chars[fault_type];
@@ -2924,8 +2938,9 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
}
break;
}
- inst = GETWORD(val);
- if (adr > first_adr && inst == last_inst) {
+ if (adr > first_adr && instrs_are_equal(inst, suffix,
+ last_inst,
+ last_suffix)) {
if (!dotted) {
printf(" ...\n");
dotted = 1;
@@ -2934,11 +2949,24 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
}
dotted = 0;
last_inst = inst;
- if (praddr)
- printf(REG" %.8x", adr, inst);
- printf("\t");
- dump_func(inst, adr);
- printf("\n");
+ last_suffix = suffix;
+ if (IS_PREFIX(inst)) {
+ if (praddr)
+ printf(REG" %.8x:%.8x", adr, inst, suffix);
+ printf("\t");
+ /*
+ * Just use this until binutils ppc disassembly
+ * prints prefixed instructions.
+ */
+ printf("%.8x:%.8x", inst, suffix);
+ printf("\n");
+ } else {
+ if (praddr)
+ printf(REG" %.8x", adr, inst);
+ printf("\t");
+ dump_func(inst, adr);
+ printf("\n");
+ }
}
return adr - first_adr;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 09/13] powerpc/xmon: Dump prefixed instructions
2020-02-11 5:33 ` [PATCH v2 09/13] powerpc/xmon: Dump " Jordan Niethe
@ 2020-02-11 6:39 ` Christophe Leroy
2020-02-12 0:31 ` Jordan Niethe
0 siblings, 1 reply; 26+ messages in thread
From: Christophe Leroy @ 2020-02-11 6:39 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: bala24, alistair, mpe, dja
Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> Currently when xmon is dumping instructions it reads a word at a time
> and then prints that instruction (either as a hex number or by
> disassembling it). For prefixed instructions it would be nice to show
> its prefix and suffix as together. Use read_instr() so that if a prefix
> is encountered its suffix is loaded too. Then print these in the form:
> prefix:suffix
> Xmon uses the disassembly routines from GNU binutils. These currently do
> not support prefixed instructions so we will not disassemble the
> prefixed instructions yet.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v2: Rename sufx to suffix
> ---
> arch/powerpc/xmon/xmon.c | 50 +++++++++++++++++++++++++++++++---------
> 1 file changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index 0b085642bbe7..513901ee18b0 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -2903,6 +2903,21 @@ prdump(unsigned long adrs, long ndump)
> }
> }
>
> +static bool instrs_are_equal(unsigned long insta, unsigned long suffixa,
> + unsigned long instb, unsigned long suffixb)
> +{
> + if (insta != instb)
> + return false;
> +
> + if (!IS_PREFIX(insta) && !IS_PREFIX(instb))
> + return true;
> +
> + if (IS_PREFIX(insta) && IS_PREFIX(instb))
> + return suffixa == suffixb;
> +
> + return false;
> +}
> +
> typedef int (*instruction_dump_func)(unsigned long inst, unsigned long addr);
>
> static int
> @@ -2911,12 +2926,11 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
> {
> int nr, dotted;
> unsigned long first_adr;
> - unsigned int inst, last_inst = 0;
> - unsigned char val[4];
> + unsigned int inst, suffix, last_inst = 0, last_suffix = 0;
>
> dotted = 0;
> - for (first_adr = adr; count > 0; --count, adr += 4) {
> - nr = mread(adr, val, 4);
> + for (first_adr = adr; count > 0; --count, adr += nr) {
> + nr = read_instr(adr, &inst, &suffix);
> if (nr == 0) {
> if (praddr) {
> const char *x = fault_chars[fault_type];
> @@ -2924,8 +2938,9 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
> }
> break;
> }
> - inst = GETWORD(val);
> - if (adr > first_adr && inst == last_inst) {
> + if (adr > first_adr && instrs_are_equal(inst, suffix,
> + last_inst,
> + last_suffix)) {
> if (!dotted) {
> printf(" ...\n");
> dotted = 1;
> @@ -2934,11 +2949,24 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
> }
> dotted = 0;
> last_inst = inst;
> - if (praddr)
> - printf(REG" %.8x", adr, inst);
> - printf("\t");
> - dump_func(inst, adr);
> - printf("\n");
> + last_suffix = suffix;
> + if (IS_PREFIX(inst)) {
> + if (praddr)
> + printf(REG" %.8x:%.8x", adr, inst, suffix);
> + printf("\t");
> + /*
> + * Just use this until binutils ppc disassembly
> + * prints prefixed instructions.
> + */
> + printf("%.8x:%.8x", inst, suffix);
> + printf("\n");
> + } else {
> + if (praddr)
> + printf(REG" %.8x", adr, inst);
> + printf("\t");
> + dump_func(inst, adr);
> + printf("\n");
> + }
What about:
if (pr_addr) {
printf(REG" %.8x", adr, inst);
if (IS_PREFIX(inst))
printf(":%.8x", suffix);
}
printf("\t");
if (IS_PREFIX(inst))
printf("%.8x:%.8x", inst, suffix);
else
dump_func(inst, adr);
printf("\n");
> }
> return adr - first_adr;
> }
>
Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 09/13] powerpc/xmon: Dump prefixed instructions
2020-02-11 6:39 ` Christophe Leroy
@ 2020-02-12 0:31 ` Jordan Niethe
0 siblings, 0 replies; 26+ messages in thread
From: Jordan Niethe @ 2020-02-12 0:31 UTC (permalink / raw)
To: Christophe Leroy
Cc: Balamuruhan S, Alistair Popple, Daniel Axtens, linuxppc-dev
On Tue, Feb 11, 2020 at 5:39 PM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> > Currently when xmon is dumping instructions it reads a word at a time
> > and then prints that instruction (either as a hex number or by
> > disassembling it). For prefixed instructions it would be nice to show
> > its prefix and suffix as together. Use read_instr() so that if a prefix
> > is encountered its suffix is loaded too. Then print these in the form:
> > prefix:suffix
> > Xmon uses the disassembly routines from GNU binutils. These currently do
> > not support prefixed instructions so we will not disassemble the
> > prefixed instructions yet.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v2: Rename sufx to suffix
> > ---
> > arch/powerpc/xmon/xmon.c | 50 +++++++++++++++++++++++++++++++---------
> > 1 file changed, 39 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index 0b085642bbe7..513901ee18b0 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -2903,6 +2903,21 @@ prdump(unsigned long adrs, long ndump)
> > }
> > }
> >
> > +static bool instrs_are_equal(unsigned long insta, unsigned long suffixa,
> > + unsigned long instb, unsigned long suffixb)
> > +{
> > + if (insta != instb)
> > + return false;
> > +
> > + if (!IS_PREFIX(insta) && !IS_PREFIX(instb))
> > + return true;
> > +
> > + if (IS_PREFIX(insta) && IS_PREFIX(instb))
> > + return suffixa == suffixb;
> > +
> > + return false;
> > +}
> > +
> > typedef int (*instruction_dump_func)(unsigned long inst, unsigned long addr);
> >
> > static int
> > @@ -2911,12 +2926,11 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
> > {
> > int nr, dotted;
> > unsigned long first_adr;
> > - unsigned int inst, last_inst = 0;
> > - unsigned char val[4];
> > + unsigned int inst, suffix, last_inst = 0, last_suffix = 0;
> >
> > dotted = 0;
> > - for (first_adr = adr; count > 0; --count, adr += 4) {
> > - nr = mread(adr, val, 4);
> > + for (first_adr = adr; count > 0; --count, adr += nr) {
> > + nr = read_instr(adr, &inst, &suffix);
> > if (nr == 0) {
> > if (praddr) {
> > const char *x = fault_chars[fault_type];
> > @@ -2924,8 +2938,9 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
> > }
> > break;
> > }
> > - inst = GETWORD(val);
> > - if (adr > first_adr && inst == last_inst) {
> > + if (adr > first_adr && instrs_are_equal(inst, suffix,
> > + last_inst,
> > + last_suffix)) {
> > if (!dotted) {
> > printf(" ...\n");
> > dotted = 1;
> > @@ -2934,11 +2949,24 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
> > }
> > dotted = 0;
> > last_inst = inst;
> > - if (praddr)
> > - printf(REG" %.8x", adr, inst);
> > - printf("\t");
> > - dump_func(inst, adr);
> > - printf("\n");
> > + last_suffix = suffix;
> > + if (IS_PREFIX(inst)) {
> > + if (praddr)
> > + printf(REG" %.8x:%.8x", adr, inst, suffix);
> > + printf("\t");
> > + /*
> > + * Just use this until binutils ppc disassembly
> > + * prints prefixed instructions.
> > + */
> > + printf("%.8x:%.8x", inst, suffix);
> > + printf("\n");
> > + } else {
> > + if (praddr)
> > + printf(REG" %.8x", adr, inst);
> > + printf("\t");
> > + dump_func(inst, adr);
> > + printf("\n");
> > + }
>
> What about:
>
>
> if (pr_addr) {
> printf(REG" %.8x", adr, inst);
> if (IS_PREFIX(inst))
> printf(":%.8x", suffix);
> }
> printf("\t");
> if (IS_PREFIX(inst))
> printf("%.8x:%.8x", inst, suffix);
> else
> dump_func(inst, adr);
> printf("\n");
>
Yeah that looks better.
> > }
> > return adr - first_adr;
> > }
> >
>
> Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 10/13] powerpc/kprobes: Support kprobes on prefixed instructions
2020-02-11 5:33 [PATCH v2 00/13] Initial Prefixed Instruction support Jordan Niethe
` (8 preceding siblings ...)
2020-02-11 5:33 ` [PATCH v2 09/13] powerpc/xmon: Dump " Jordan Niethe
@ 2020-02-11 5:33 ` Jordan Niethe
2020-02-11 6:46 ` Christophe Leroy
2020-02-11 5:33 ` [PATCH v2 11/13] powerpc/uprobes: Add support for " Jordan Niethe
` (2 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
A prefixed instruction is composed of a word prefix followed by a word
suffix. It does not make sense to be able to have a kprobe on the suffix
of a prefixed instruction, so make this impossible.
Kprobes work by replacing an instruction with a trap and saving that
instruction to be single stepped out of place later. Currently there is
not enough space allocated to keep a prefixed instruction for single
stepping. Increase the amount of space allocated for holding the
instruction copy.
kprobe_post_handler() expects all instructions to be 4 bytes long which
means that it does not function correctly for prefixed instructions.
Add checks for prefixed instructions which will use a length of 8 bytes
instead.
For optprobes we normally patch in loading the instruction we put a
probe on into r4 before calling emulate_step(). We now make space and
patch in loading the suffix into r5 as well.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
arch/powerpc/include/asm/kprobes.h | 5 +--
arch/powerpc/kernel/kprobes.c | 47 +++++++++++++++++++++-------
arch/powerpc/kernel/optprobes.c | 32 ++++++++++---------
arch/powerpc/kernel/optprobes_head.S | 6 ++++
4 files changed, 63 insertions(+), 27 deletions(-)
diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
index 66b3f2983b22..0d44ce8a3163 100644
--- a/arch/powerpc/include/asm/kprobes.h
+++ b/arch/powerpc/include/asm/kprobes.h
@@ -38,12 +38,13 @@ extern kprobe_opcode_t optprobe_template_entry[];
extern kprobe_opcode_t optprobe_template_op_address[];
extern kprobe_opcode_t optprobe_template_call_handler[];
extern kprobe_opcode_t optprobe_template_insn[];
+extern kprobe_opcode_t optprobe_template_suffix[];
extern kprobe_opcode_t optprobe_template_call_emulate[];
extern kprobe_opcode_t optprobe_template_ret[];
extern kprobe_opcode_t optprobe_template_end[];
-/* Fixed instruction size for powerpc */
-#define MAX_INSN_SIZE 1
+/* Prefixed instructions are two words */
+#define MAX_INSN_SIZE 2
#define MAX_OPTIMIZED_LENGTH sizeof(kprobe_opcode_t) /* 4 bytes */
#define MAX_OPTINSN_SIZE (optprobe_template_end - optprobe_template_entry)
#define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 24a56f062d9e..b061deba4fe7 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -104,17 +104,30 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
int arch_prepare_kprobe(struct kprobe *p)
{
+ int len;
int ret = 0;
+ struct kprobe *prev;
kprobe_opcode_t insn = *p->addr;
+ kprobe_opcode_t prefix = *(p->addr - 1);
+ preempt_disable();
if ((unsigned long)p->addr & 0x03) {
printk("Attempt to register kprobe at an unaligned address\n");
ret = -EINVAL;
} else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) {
printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n");
ret = -EINVAL;
+ } else if (IS_PREFIX(prefix)) {
+ printk("Cannot register a kprobe on the second word of prefixed instruction\n");
+ ret = -EINVAL;
+ }
+ prev = get_kprobe(p->addr - 1);
+ if (prev && IS_PREFIX(*prev->ainsn.insn)) {
+ printk("Cannot register a kprobe on the second word of prefixed instruction\n");
+ ret = -EINVAL;
}
+
/* insn must be on a special executable page on ppc64. This is
* not explicitly required on ppc32 (right now), but it doesn't hurt */
if (!ret) {
@@ -124,14 +137,18 @@ int arch_prepare_kprobe(struct kprobe *p)
}
if (!ret) {
- memcpy(p->ainsn.insn, p->addr,
- MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
+ if (IS_PREFIX(insn))
+ len = MAX_INSN_SIZE * sizeof(kprobe_opcode_t);
+ else
+ len = sizeof(kprobe_opcode_t);
+ memcpy(p->ainsn.insn, p->addr, len);
p->opcode = *p->addr;
flush_icache_range((unsigned long)p->ainsn.insn,
(unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t));
}
p->ainsn.boostable = 0;
+ preempt_enable_no_resched();
return ret;
}
NOKPROBE_SYMBOL(arch_prepare_kprobe);
@@ -216,10 +233,11 @@ NOKPROBE_SYMBOL(arch_prepare_kretprobe);
static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
{
int ret;
- unsigned int insn = *p->ainsn.insn;
+ unsigned int insn = p->ainsn.insn[0];
+ unsigned int suffix = p->ainsn.insn[1];
/* regs->nip is also adjusted if emulate_step returns 1 */
- ret = emulate_step(regs, insn, PPC_NO_SUFFIX);
+ ret = emulate_step(regs, insn, suffix);
if (ret > 0) {
/*
* Once this instruction has been boosted
@@ -233,7 +251,11 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
* So, we should never get here... but, its still
* good to catch them, just in case...
*/
- printk("Can't step on instruction %x\n", insn);
+ if (!IS_PREFIX(insn))
+ printk("Can't step on instruction %x\n", insn);
+ else
+ printk("Can't step on instruction %x %x\n", insn,
+ suffix);
BUG();
} else {
/*
@@ -275,7 +297,7 @@ int kprobe_handler(struct pt_regs *regs)
if (kprobe_running()) {
p = get_kprobe(addr);
if (p) {
- kprobe_opcode_t insn = *p->ainsn.insn;
+ kprobe_opcode_t insn = p->ainsn.insn[0];
if (kcb->kprobe_status == KPROBE_HIT_SS &&
is_trap(insn)) {
/* Turn off 'trace' bits */
@@ -448,9 +470,10 @@ static int trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
* the link register properly so that the subsequent 'blr' in
* kretprobe_trampoline jumps back to the right instruction.
*
- * For nip, we should set the address to the previous instruction since
- * we end up emulating it in kprobe_handler(), which increments the nip
- * again.
+ * To keep the nip at the correct address we need to counter the
+ * increment that happens when we emulate the kretprobe_trampoline noop
+ * in kprobe_handler(). We do this by decrementing the address by the
+ * length of the noop which is always 4 bytes.
*/
regs->nip = orig_ret_address - 4;
regs->link = orig_ret_address;
@@ -478,12 +501,14 @@ int kprobe_post_handler(struct pt_regs *regs)
{
struct kprobe *cur = kprobe_running();
struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+ kprobe_opcode_t insn;
if (!cur || user_mode(regs))
return 0;
+ insn = *cur->ainsn.insn;
/* make sure we got here for instruction we have a kprobe on */
- if (((unsigned long)cur->ainsn.insn + 4) != regs->nip)
+ if ((unsigned long)cur->ainsn.insn + PPC_INST_LENGTH(insn) != regs->nip)
return 0;
if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) {
@@ -492,7 +517,7 @@ int kprobe_post_handler(struct pt_regs *regs)
}
/* Adjust nip to after the single-stepped instruction */
- regs->nip = (unsigned long)cur->addr + 4;
+ regs->nip = (unsigned long)cur->addr + PPC_INST_LENGTH(insn);
regs->msr |= kcb->kprobe_saved_msr;
/*Restore back the original saved kprobes variables and continue. */
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index f908d9422557..60cf8e8485ab 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -27,6 +27,8 @@
(optprobe_template_op_address - optprobe_template_entry)
#define TMPL_INSN_IDX \
(optprobe_template_insn - optprobe_template_entry)
+#define TMPL_SUFX_IDX \
+ (optprobe_template_suffix - optprobe_template_entry)
#define TMPL_END_IDX \
(optprobe_template_end - optprobe_template_entry)
@@ -100,8 +102,8 @@ static unsigned long can_optimize(struct kprobe *p)
* and that can be emulated.
*/
if (!is_conditional_branch(*p->ainsn.insn) &&
- analyse_instr(&op, ®s, *p->ainsn.insn,
- PPC_NO_SUFFIX) == 1) {
+ analyse_instr(&op, ®s, p->ainsn.insn[0],
+ p->ainsn.insn[1]) == 1) {
emulate_update_regs(®s, &op);
nip = regs.nip;
}
@@ -141,27 +143,27 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op)
}
/*
- * emulate_step() requires insn to be emulated as
- * second parameter. Load register 'r4' with the
- * instruction.
+ * emulate_step() requires insn to be emulated as second parameter, and the
+ * suffix as the third parameter. Load these into registers.
*/
-void patch_imm32_load_insns(unsigned int val, kprobe_opcode_t *addr)
+static void patch_imm32_load_insns(int reg, unsigned int val,
+ kprobe_opcode_t *addr)
{
- /* addis r4,0,(insn)@h */
- patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(4) |
+ /* addis reg,0,(insn)@h */
+ patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(reg) |
((val >> 16) & 0xffff));
addr++;
- /* ori r4,r4,(insn)@l */
- patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(4) |
- ___PPC_RS(4) | (val & 0xffff));
+ /* ori reg,reg,(insn)@l */
+ patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(reg) |
+ ___PPC_RS(reg) | (val & 0xffff));
}
/*
* Generate instructions to load provided immediate 64-bit value
* to register 'r3' and patch these instructions at 'addr'.
*/
-void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
+static void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
{
/* lis r3,(op)@highest */
patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(3) |
@@ -267,9 +269,11 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
patch_instruction(buff + TMPL_EMULATE_IDX, branch_emulate_step);
/*
- * 3. load instruction to be emulated into relevant register, and
+ * 3. load instruction and suffix to be emulated into the relevant
+ * registers, and
*/
- patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX);
+ patch_imm32_load_insns(4, p->ainsn.insn[0], buff + TMPL_INSN_IDX);
+ patch_imm32_load_insns(5, p->ainsn.insn[1], buff + TMPL_SUFX_IDX);
/*
* 4. branch back from trampoline
diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S
index cf383520843f..395d1643f59d 100644
--- a/arch/powerpc/kernel/optprobes_head.S
+++ b/arch/powerpc/kernel/optprobes_head.S
@@ -95,6 +95,12 @@ optprobe_template_insn:
nop
nop
+ .global optprobe_template_suffix
+optprobe_template_suffix:
+ /* Pass suffix to be emulated in r5 */
+ nop
+ nop
+
.global optprobe_template_call_emulate
optprobe_template_call_emulate:
/* Branch to emulate_step() */
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 10/13] powerpc/kprobes: Support kprobes on prefixed instructions
2020-02-11 5:33 ` [PATCH v2 10/13] powerpc/kprobes: Support kprobes on " Jordan Niethe
@ 2020-02-11 6:46 ` Christophe Leroy
2020-02-12 0:32 ` Jordan Niethe
0 siblings, 1 reply; 26+ messages in thread
From: Christophe Leroy @ 2020-02-11 6:46 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: bala24, alistair, mpe, dja
Le 11/02/2020 à 06:33, Jordan Niethe a écrit :
> A prefixed instruction is composed of a word prefix followed by a word
> suffix. It does not make sense to be able to have a kprobe on the suffix
> of a prefixed instruction, so make this impossible.
>
> Kprobes work by replacing an instruction with a trap and saving that
> instruction to be single stepped out of place later. Currently there is
> not enough space allocated to keep a prefixed instruction for single
> stepping. Increase the amount of space allocated for holding the
> instruction copy.
>
> kprobe_post_handler() expects all instructions to be 4 bytes long which
> means that it does not function correctly for prefixed instructions.
> Add checks for prefixed instructions which will use a length of 8 bytes
> instead.
>
> For optprobes we normally patch in loading the instruction we put a
> probe on into r4 before calling emulate_step(). We now make space and
> patch in loading the suffix into r5 as well.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> arch/powerpc/include/asm/kprobes.h | 5 +--
> arch/powerpc/kernel/kprobes.c | 47 +++++++++++++++++++++-------
> arch/powerpc/kernel/optprobes.c | 32 ++++++++++---------
> arch/powerpc/kernel/optprobes_head.S | 6 ++++
> 4 files changed, 63 insertions(+), 27 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
> index 66b3f2983b22..0d44ce8a3163 100644
> --- a/arch/powerpc/include/asm/kprobes.h
> +++ b/arch/powerpc/include/asm/kprobes.h
> @@ -38,12 +38,13 @@ extern kprobe_opcode_t optprobe_template_entry[];
> extern kprobe_opcode_t optprobe_template_op_address[];
> extern kprobe_opcode_t optprobe_template_call_handler[];
> extern kprobe_opcode_t optprobe_template_insn[];
> +extern kprobe_opcode_t optprobe_template_suffix[];
> extern kprobe_opcode_t optprobe_template_call_emulate[];
> extern kprobe_opcode_t optprobe_template_ret[];
> extern kprobe_opcode_t optprobe_template_end[];
>
> -/* Fixed instruction size for powerpc */
> -#define MAX_INSN_SIZE 1
> +/* Prefixed instructions are two words */
> +#define MAX_INSN_SIZE 2
> #define MAX_OPTIMIZED_LENGTH sizeof(kprobe_opcode_t) /* 4 bytes */
> #define MAX_OPTINSN_SIZE (optprobe_template_end - optprobe_template_entry)
> #define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 24a56f062d9e..b061deba4fe7 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -104,17 +104,30 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>
> int arch_prepare_kprobe(struct kprobe *p)
> {
> + int len;
> int ret = 0;
> + struct kprobe *prev;
> kprobe_opcode_t insn = *p->addr;
> + kprobe_opcode_t prefix = *(p->addr - 1);
>
> + preempt_disable();
> if ((unsigned long)p->addr & 0x03) {
> printk("Attempt to register kprobe at an unaligned address\n");
> ret = -EINVAL;
> } else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) {
> printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n");
> ret = -EINVAL;
> + } else if (IS_PREFIX(prefix)) {
> + printk("Cannot register a kprobe on the second word of prefixed instruction\n");
> + ret = -EINVAL;
> + }
> + prev = get_kprobe(p->addr - 1);
> + if (prev && IS_PREFIX(*prev->ainsn.insn)) {
> + printk("Cannot register a kprobe on the second word of prefixed instruction\n");
> + ret = -EINVAL;
> }
>
> +
> /* insn must be on a special executable page on ppc64. This is
> * not explicitly required on ppc32 (right now), but it doesn't hurt */
> if (!ret) {
> @@ -124,14 +137,18 @@ int arch_prepare_kprobe(struct kprobe *p)
> }
>
> if (!ret) {
> - memcpy(p->ainsn.insn, p->addr,
> - MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
> + if (IS_PREFIX(insn))
> + len = MAX_INSN_SIZE * sizeof(kprobe_opcode_t);
> + else
> + len = sizeof(kprobe_opcode_t);
> + memcpy(p->ainsn.insn, p->addr, len);
This code is about to get changed, see
https://patchwork.ozlabs.org/patch/1232619/
> p->opcode = *p->addr;
> flush_icache_range((unsigned long)p->ainsn.insn,
> (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t));
> }
>
> p->ainsn.boostable = 0;
> + preempt_enable_no_resched();
> return ret;
> }
> NOKPROBE_SYMBOL(arch_prepare_kprobe);
> @@ -216,10 +233,11 @@ NOKPROBE_SYMBOL(arch_prepare_kretprobe);
> static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
> {
> int ret;
> - unsigned int insn = *p->ainsn.insn;
> + unsigned int insn = p->ainsn.insn[0];
> + unsigned int suffix = p->ainsn.insn[1];
>
> /* regs->nip is also adjusted if emulate_step returns 1 */
> - ret = emulate_step(regs, insn, PPC_NO_SUFFIX);
> + ret = emulate_step(regs, insn, suffix);
> if (ret > 0) {
> /*
> * Once this instruction has been boosted
> @@ -233,7 +251,11 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
> * So, we should never get here... but, its still
> * good to catch them, just in case...
> */
> - printk("Can't step on instruction %x\n", insn);
> + if (!IS_PREFIX(insn))
> + printk("Can't step on instruction %x\n", insn);
> + else
> + printk("Can't step on instruction %x %x\n", insn,
> + suffix);
Maybe %x:%x as in xmon ?
Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v2 10/13] powerpc/kprobes: Support kprobes on prefixed instructions
2020-02-11 6:46 ` Christophe Leroy
@ 2020-02-12 0:32 ` Jordan Niethe
0 siblings, 0 replies; 26+ messages in thread
From: Jordan Niethe @ 2020-02-12 0:32 UTC (permalink / raw)
To: Christophe Leroy
Cc: Balamuruhan S, Alistair Popple, Daniel Axtens, linuxppc-dev
On Tue, Feb 11, 2020 at 5:46 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 followed by a word
> > suffix. It does not make sense to be able to have a kprobe on the suffix
> > of a prefixed instruction, so make this impossible.
> >
> > Kprobes work by replacing an instruction with a trap and saving that
> > instruction to be single stepped out of place later. Currently there is
> > not enough space allocated to keep a prefixed instruction for single
> > stepping. Increase the amount of space allocated for holding the
> > instruction copy.
> >
> > kprobe_post_handler() expects all instructions to be 4 bytes long which
> > means that it does not function correctly for prefixed instructions.
> > Add checks for prefixed instructions which will use a length of 8 bytes
> > instead.
> >
> > For optprobes we normally patch in loading the instruction we put a
> > probe on into r4 before calling emulate_step(). We now make space and
> > patch in loading the suffix into r5 as well.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > arch/powerpc/include/asm/kprobes.h | 5 +--
> > arch/powerpc/kernel/kprobes.c | 47 +++++++++++++++++++++-------
> > arch/powerpc/kernel/optprobes.c | 32 ++++++++++---------
> > arch/powerpc/kernel/optprobes_head.S | 6 ++++
> > 4 files changed, 63 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
> > index 66b3f2983b22..0d44ce8a3163 100644
> > --- a/arch/powerpc/include/asm/kprobes.h
> > +++ b/arch/powerpc/include/asm/kprobes.h
> > @@ -38,12 +38,13 @@ extern kprobe_opcode_t optprobe_template_entry[];
> > extern kprobe_opcode_t optprobe_template_op_address[];
> > extern kprobe_opcode_t optprobe_template_call_handler[];
> > extern kprobe_opcode_t optprobe_template_insn[];
> > +extern kprobe_opcode_t optprobe_template_suffix[];
> > extern kprobe_opcode_t optprobe_template_call_emulate[];
> > extern kprobe_opcode_t optprobe_template_ret[];
> > extern kprobe_opcode_t optprobe_template_end[];
> >
> > -/* Fixed instruction size for powerpc */
> > -#define MAX_INSN_SIZE 1
> > +/* Prefixed instructions are two words */
> > +#define MAX_INSN_SIZE 2
> > #define MAX_OPTIMIZED_LENGTH sizeof(kprobe_opcode_t) /* 4 bytes */
> > #define MAX_OPTINSN_SIZE (optprobe_template_end - optprobe_template_entry)
> > #define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */
> > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> > index 24a56f062d9e..b061deba4fe7 100644
> > --- a/arch/powerpc/kernel/kprobes.c
> > +++ b/arch/powerpc/kernel/kprobes.c
> > @@ -104,17 +104,30 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
> >
> > int arch_prepare_kprobe(struct kprobe *p)
> > {
> > + int len;
> > int ret = 0;
> > + struct kprobe *prev;
> > kprobe_opcode_t insn = *p->addr;
> > + kprobe_opcode_t prefix = *(p->addr - 1);
> >
> > + preempt_disable();
> > if ((unsigned long)p->addr & 0x03) {
> > printk("Attempt to register kprobe at an unaligned address\n");
> > ret = -EINVAL;
> > } else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) {
> > printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n");
> > ret = -EINVAL;
> > + } else if (IS_PREFIX(prefix)) {
> > + printk("Cannot register a kprobe on the second word of prefixed instruction\n");
> > + ret = -EINVAL;
> > + }
> > + prev = get_kprobe(p->addr - 1);
> > + if (prev && IS_PREFIX(*prev->ainsn.insn)) {
> > + printk("Cannot register a kprobe on the second word of prefixed instruction\n");
> > + ret = -EINVAL;
> > }
> >
> > +
> > /* insn must be on a special executable page on ppc64. This is
> > * not explicitly required on ppc32 (right now), but it doesn't hurt */
> > if (!ret) {
> > @@ -124,14 +137,18 @@ int arch_prepare_kprobe(struct kprobe *p)
> > }
> >
> > if (!ret) {
> > - memcpy(p->ainsn.insn, p->addr,
> > - MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
> > + if (IS_PREFIX(insn))
> > + len = MAX_INSN_SIZE * sizeof(kprobe_opcode_t);
> > + else
> > + len = sizeof(kprobe_opcode_t);
> > + memcpy(p->ainsn.insn, p->addr, len);
>
> This code is about to get changed, see
> https://patchwork.ozlabs.org/patch/1232619/
Ah thank you for the heads up.
>
> > p->opcode = *p->addr;
> > flush_icache_range((unsigned long)p->ainsn.insn,
> > (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t));
> > }
> >
> > p->ainsn.boostable = 0;
> > + preempt_enable_no_resched();
> > return ret;
> > }
> > NOKPROBE_SYMBOL(arch_prepare_kprobe);
> > @@ -216,10 +233,11 @@ NOKPROBE_SYMBOL(arch_prepare_kretprobe);
> > static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
> > {
> > int ret;
> > - unsigned int insn = *p->ainsn.insn;
> > + unsigned int insn = p->ainsn.insn[0];
> > + unsigned int suffix = p->ainsn.insn[1];
> >
> > /* regs->nip is also adjusted if emulate_step returns 1 */
> > - ret = emulate_step(regs, insn, PPC_NO_SUFFIX);
> > + ret = emulate_step(regs, insn, suffix);
> > if (ret > 0) {
> > /*
> > * Once this instruction has been boosted
> > @@ -233,7 +251,11 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
> > * So, we should never get here... but, its still
> > * good to catch them, just in case...
> > */
> > - printk("Can't step on instruction %x\n", insn);
> > + if (!IS_PREFIX(insn))
> > + printk("Can't step on instruction %x\n", insn);
> > + else
> > + printk("Can't step on instruction %x %x\n", insn,
> > + suffix);
>
> Maybe %x:%x as in xmon ?
Good point.
>
> Christophe
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 11/13] powerpc/uprobes: Add support for prefixed instructions
2020-02-11 5:33 [PATCH v2 00/13] Initial Prefixed Instruction support Jordan Niethe
` (9 preceding siblings ...)
2020-02-11 5:33 ` [PATCH v2 10/13] powerpc/kprobes: Support kprobes on " Jordan Niethe
@ 2020-02-11 5:33 ` Jordan Niethe
2020-02-11 5:33 ` [PATCH v2 12/13] powerpc/hw_breakpoints: Initial " Jordan Niethe
2020-02-11 5:33 ` [PATCH v2 13/13] powerpc: Add prefix support to mce_find_instr_ea_and_pfn() Jordan Niethe
12 siblings, 0 replies; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
Uprobes can execute instructions out of line. Increase the size of the
buffer used for this so that this works for prefixed instructions. Take
into account the length of prefixed instructions when fixing up the nip.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Fix typo
- Use macro for instruction length
---
arch/powerpc/include/asm/uprobes.h | 16 ++++++++++++----
arch/powerpc/kernel/uprobes.c | 4 ++--
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
index 2bbdf27d09b5..5516ab27db47 100644
--- a/arch/powerpc/include/asm/uprobes.h
+++ b/arch/powerpc/include/asm/uprobes.h
@@ -14,18 +14,26 @@
typedef ppc_opcode_t uprobe_opcode_t;
+/*
+ * Ensure we have enough space for prefixed instructions, which
+ * are double the size of a word instruction, i.e. 8 bytes.
+ */
#define MAX_UINSN_BYTES 4
-#define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
+#define UPROBE_XOL_SLOT_BYTES (2 * MAX_UINSN_BYTES)
/* The following alias is needed for reference from arch-agnostic code */
#define UPROBE_SWBP_INSN BREAKPOINT_INSTRUCTION
#define UPROBE_SWBP_INSN_SIZE 4 /* swbp insn size in bytes */
struct arch_uprobe {
+ /*
+ * Ensure there is enough space for prefixed instructions. Prefixed
+ * instructions must not cross 64-byte boundaries.
+ */
union {
- u32 insn;
- u32 ixol;
- };
+ uprobe_opcode_t insn[2];
+ uprobe_opcode_t ixol[2];
+ } __aligned(64);
};
struct arch_uprobe_task {
diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index 4ab40c4b576f..7e0334ad5cfe 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -111,7 +111,7 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
* support doesn't exist and have to fix-up the next instruction
* to be executed.
*/
- regs->nip = utask->vaddr + MAX_UINSN_BYTES;
+ regs->nip = utask->vaddr + PPC_INST_LENGTH(auprobe->insn[0]);
user_disable_single_step(current);
return 0;
@@ -173,7 +173,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
* emulate_step() returns 1 if the insn was successfully emulated.
* For all other cases, we need to single-step in hardware.
*/
- ret = emulate_step(regs, auprobe->insn, PPC_NO_SUFFIX);
+ ret = emulate_step(regs, auprobe->insn[0], auprobe->insn[1]);
if (ret > 0)
return true;
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 12/13] powerpc/hw_breakpoints: Initial support for prefixed instructions
2020-02-11 5:33 [PATCH v2 00/13] Initial Prefixed Instruction support Jordan Niethe
` (10 preceding siblings ...)
2020-02-11 5:33 ` [PATCH v2 11/13] powerpc/uprobes: Add support for " Jordan Niethe
@ 2020-02-11 5:33 ` Jordan Niethe
2020-02-11 5:33 ` [PATCH v2 13/13] powerpc: Add prefix support to mce_find_instr_ea_and_pfn() Jordan Niethe
12 siblings, 0 replies; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
Currently when getting an instruction to emulate in
hw_breakpoint_handler() we do not load the suffix of a prefixed
instruction. Ensure we load the suffix if the instruction we need to
emulate is a prefixed instruction.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: Rename sufx to suffix
---
arch/powerpc/kernel/hw_breakpoint.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 3a7ec6760dab..c69189641b05 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -243,15 +243,15 @@ dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
struct arch_hw_breakpoint *info)
{
- unsigned int instr = 0;
+ unsigned int instr = 0, suffix = 0;
int ret, type, size;
struct instruction_op op;
unsigned long addr = info->address;
- if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
+ if (__get_user_instr_inatomic(instr, suffix, (unsigned int *)regs->nip))
goto fail;
- ret = analyse_instr(&op, regs, instr, PPC_NO_SUFFIX);
+ ret = analyse_instr(&op, regs, instr, suffix);
type = GETTYPE(op.type);
size = GETSIZE(op.type);
@@ -275,7 +275,7 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
return false;
}
- if (!emulate_step(regs, instr, PPC_NO_SUFFIX))
+ if (!emulate_step(regs, instr, suffix))
goto fail;
return true;
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 13/13] powerpc: Add prefix support to mce_find_instr_ea_and_pfn()
2020-02-11 5:33 [PATCH v2 00/13] Initial Prefixed Instruction support Jordan Niethe
` (11 preceding siblings ...)
2020-02-11 5:33 ` [PATCH v2 12/13] powerpc/hw_breakpoints: Initial " Jordan Niethe
@ 2020-02-11 5:33 ` Jordan Niethe
12 siblings, 0 replies; 26+ messages in thread
From: Jordan Niethe @ 2020-02-11 5:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, mpe, Jordan Niethe, dja, bala24
mce_find_instr_ea_and_pfn analyses an instruction to determine the
effective address that caused the machine check. Update this to load and
pass the suffix to analyse_instr for prefixed instructions.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Rename sufx to suffix
---
arch/powerpc/kernel/mce_power.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
index 824eda536f5d..091bab4a5464 100644
--- a/arch/powerpc/kernel/mce_power.c
+++ b/arch/powerpc/kernel/mce_power.c
@@ -365,7 +365,7 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr,
* in real-mode is tricky and can lead to recursive
* faults
*/
- int instr;
+ int instr, suffix = 0;
unsigned long pfn, instr_addr;
struct instruction_op op;
struct pt_regs tmp = *regs;
@@ -374,7 +374,9 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr,
if (pfn != ULONG_MAX) {
instr_addr = (pfn << PAGE_SHIFT) + (regs->nip & ~PAGE_MASK);
instr = *(unsigned int *)(instr_addr);
- if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
+ if (IS_PREFIX(instr))
+ suffix = *(unsigned int *)(instr_addr + 4);
+ if (!analyse_instr(&op, &tmp, instr, suffix)) {
pfn = addr_to_pfn(regs, op.ea);
*addr = op.ea;
*phys_addr = (pfn << PAGE_SHIFT);
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread