* [PATCH v3 05/14] powerpc sstep: Add support for prefixed fixed-point arithmetic
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
In-Reply-To: <20200226040716.32395-1-jniethe5@gmail.com>
This adds emulation support for the following prefixed Fixed-Point
Arithmetic instructions:
* Prefixed Add Immediate (paddi)
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v3: Since we moved the prefixed loads/stores into the load/store switch
statement it no longer makes sense to have paddi in there, so move it
out.
---
arch/powerpc/lib/sstep.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 8e4ec953e279..f2010a3e1e06 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1331,6 +1331,26 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
switch (opcode) {
#ifdef __powerpc64__
+ case 1:
+ prefix_r = instr & (1ul << 20);
+ ra = (suffix >> 16) & 0x1f;
+ rd = (suffix >> 21) & 0x1f;
+ op->reg = rd;
+ op->val = regs->gpr[rd];
+ suffixopcode = suffix >> 26;
+ prefixtype = (instr >> 24) & 0x3;
+ switch (prefixtype) {
+ case 2:
+ if (prefix_r && ra)
+ return 0;
+ switch (suffixopcode) {
+ case 14: /* paddi */
+ op->type = COMPUTE | PREFIXED;
+ op->val = mlsd_8lsd_ea(instr, suffix, regs);
+ goto compute_done;
+ }
+ }
+ break;
case 2: /* tdi */
if (rd & trap_compare(regs->gpr[ra], (short) instr))
goto trap;
--
2.17.1
^ permalink raw reply related
* [PATCH v3 04/14] powerpc sstep: Add support for prefixed load/stores
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
In-Reply-To: <20200226040716.32395-1-jniethe5@gmail.com>
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
v3: - Move prefixed loads and stores into the switch statement
---
arch/powerpc/lib/sstep.c | 159 +++++++++++++++++++++++++++++++++++++++
1 file changed, 159 insertions(+)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index efbe72370670..8e4ec953e279 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;
@@ -2648,6 +2687,126 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
break;
}
break;
+ case 1: /* Prefixed instructions */
+ 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;
+ }
#endif /* __powerpc64__ */
}
--
2.17.1
^ permalink raw reply related
* [PATCH v3 03/14] powerpc sstep: Prepare to support prefixed instructions
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
In-Reply-To: <20200226040716.32395-1-jniethe5@gmail.com>
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
v3: - Define and use OP_PREFIX
- Rename OP_LENGTH() to GETLENGTH()
- Define IS_PREFIX() as 0 for non 64 bit ppc
---
arch/powerpc/include/asm/ppc-opcode.h | 13 ++++++++++++
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, 54 insertions(+), 32 deletions(-)
diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index c1df75edde44..24dc193cd3ef 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -158,6 +158,9 @@
/* VMX Vector Store Instructions */
#define OP_31_XOP_STVX 231
+/* Prefixed Instructions */
+#define OP_PREFIX 1
+
#define OP_31 31
#define OP_LWZ 32
#define OP_STFS 52
@@ -377,6 +380,16 @@
#define PPC_INST_VCMPEQUD 0x100000c7
#define PPC_INST_VCMPEQUB 0x10000006
+/* macros for prefixed instructions */
+#ifdef __powerpc64__
+#define IS_PREFIX(x) (((x) >> 26) == OP_PREFIX)
+#else
+#define IS_PREFIX(x) (0)
+#endif
+
+#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..5539df5c50a4 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 GETLENGTH(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 337516df17d4..6b2e9e37f12b 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -228,7 +228,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..efbe72370670 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 + GETLENGTH(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 + GETLENGTH(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
* [PATCH v3 02/14] powerpc: Define new SRR1 bits for a future ISA version
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
In-Reply-To: <20200226040716.32395-1-jniethe5@gmail.com>
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
* [PATCH v3 01/14] powerpc: Enable Prefixed Instructions
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, dja
In-Reply-To: <20200226040716.32395-1-jniethe5@gmail.com>
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
* [PATCH v3 00/14] Initial Prefixed Instruction support
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
A future revision of the ISA will introduce prefixed instructions. A
prefixed instruction is composed of a 4-byte prefix followed by a
4-byte suffix.
All prefixes have the major opcode 1. A prefix will never be a valid
word instruction. A suffix may be an existing word instruction or a
new instruction.
This series enables prefixed instructions and extends the instruction
emulation to support them. Then the places where prefixed instructions
might need to be emulated are updated.
v3 is based on feedback from Christophe Leroy. The major changes:
- Completely replacing store_inst() with patch_instruction() in
xmon
- Improve implementation of mread_instr() to not use mread().
- Base the series on top of
https://patchwork.ozlabs.org/patch/1232619/ as this will effect
kprobes.
- Some renaming and simplification of conditionals.
v2 incorporates feedback from Daniel Axtens and and Balamuruhan
S. The major changes are:
- Squashing together all commits about SRR1 bits
- Squashing all commits for supporting prefixed load stores
- Changing abbreviated references to sufx/prfx -> suffix/prefix
- Introducing macros for returning the length of an instruction
- Removing sign extension flag from pstd/pld in sstep.c
- Dropping patch "powerpc/fault: Use analyse_instr() to check for
store with updates to sp" from the series, it did not really fit
with prefixed enablement in the first place and as reported by Greg
Kurz did not work correctly.
Alistair Popple (1):
powerpc: Enable Prefixed Instructions
Jordan Niethe (13):
powerpc: Define new SRR1 bits for a future ISA version
powerpc sstep: Prepare to support prefixed instructions
powerpc sstep: Add support for prefixed load/stores
powerpc sstep: Add support for prefixed fixed-point arithmetic
powerpc: Support prefixed instructions in alignment handler
powerpc/traps: Check for prefixed instructions in
facility_unavailable_exception()
powerpc/xmon: Remove store_inst() for patch_instruction()
powerpc/xmon: Add initial support for prefixed instructions
powerpc/xmon: Dump prefixed instructions
powerpc/kprobes: Support kprobes on prefixed instructions
powerpc/uprobes: Add support for prefixed instructions
powerpc/hw_breakpoints: Initial support for prefixed instructions
powerpc: Add prefix support to mce_find_instr_ea_and_pfn()
arch/powerpc/include/asm/kprobes.h | 5 +-
arch/powerpc/include/asm/ppc-opcode.h | 13 ++
arch/powerpc/include/asm/reg.h | 7 +-
arch/powerpc/include/asm/sstep.h | 9 +-
arch/powerpc/include/asm/uaccess.h | 25 ++++
arch/powerpc/include/asm/uprobes.h | 16 ++-
arch/powerpc/kernel/align.c | 8 +-
arch/powerpc/kernel/dt_cpu_ftrs.c | 23 ++++
arch/powerpc/kernel/hw_breakpoint.c | 9 +-
arch/powerpc/kernel/kprobes.c | 43 ++++--
arch/powerpc/kernel/mce_power.c | 6 +-
arch/powerpc/kernel/optprobes.c | 31 +++--
arch/powerpc/kernel/optprobes_head.S | 6 +
arch/powerpc/kernel/traps.c | 22 ++-
arch/powerpc/kernel/uprobes.c | 4 +-
arch/powerpc/kvm/book3s_hv_nested.c | 2 +-
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 2 +-
arch/powerpc/kvm/emulate_loadstore.c | 2 +-
arch/powerpc/lib/sstep.c | 191 +++++++++++++++++++++++++-
arch/powerpc/lib/test_emulate_step.c | 30 ++--
arch/powerpc/xmon/xmon.c | 140 +++++++++++++++----
21 files changed, 497 insertions(+), 97 deletions(-)
--
2.17.1
^ permalink raw reply
* [Bug 206669] Little-endian kernel crashing on POWER8 on heavy big-endian PowerKVM load
From: bugzilla-daemon @ 2020-02-26 4:06 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-206669-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=206669
--- Comment #1 from npiggin@gmail.com ---
bugzilla-daemon@bugzilla.kernel.org's on February 26, 2020 1:26 am:
> https://bugzilla.kernel.org/show_bug.cgi?id=206669
>
> Bug ID: 206669
> Summary: Little-endian kernel crashing on POWER8 on heavy
> big-endian PowerKVM load
> Product: Platform Specific/Hardware
> Version: 2.5
> Kernel Version: 5.4.x
> Hardware: All
> OS: Linux
> Tree: Mainline
> Status: NEW
> Severity: normal
> Priority: P1
> Component: PPC-64
> Assignee: platform_ppc-64@kernel-bugs.osdl.org
> Reporter: glaubitz@physik.fu-berlin.de
> CC: matorola@gmail.com
> Regression: No
>
> Created attachment 287605
> --> https://bugzilla.kernel.org/attachment.cgi?id=287605&action=edit
> Backtrace of host system crashing with little-endian kernel
>
> We have an IBM POWER server (8247-42L) running Linux kernel 5.4.13 on Debian
> unstable hosting a big-endian ppc64 virtual machine running the same kernel
> in
> big-endian mode.
>
> When building OpenJDK-11 on the big-endian VM, the testsuite crashes the
> *host*
> system which is little-endian with the following kernel backtrace. The
> problem
> reproduces both with kernel 4.19.98 as well as 5.4.13, both guest and host
> running 5.4.x.
>
> Backtrace attached.
Thanks for the report, we need to get more data about the first BUG if
we can. What function in your vmlinux contains address
0xc00000000017a778? (use nm or objdump etc) Is that the first message you get,
No warnings or anything else earlier in the dmesg?
Also 0xc0000000002659a0 would be interesting.
When reproducing, do you ever get a clean trace of the first bug? Could
you try setting /proc/sys/kernel/panic_on_oops and reproducing?
Thanks,
Nick
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [Bug 206669] New: Little-endian kernel crashing on POWER8 on heavy big-endian PowerKVM load
From: Nicholas Piggin @ 2020-02-26 4:02 UTC (permalink / raw)
To: bugzilla-daemon, linuxppc-dev
In-Reply-To: <bug-206669-206035@https.bugzilla.kernel.org/>
bugzilla-daemon@bugzilla.kernel.org's on February 26, 2020 1:26 am:
> https://bugzilla.kernel.org/show_bug.cgi?id=206669
>
> Bug ID: 206669
> Summary: Little-endian kernel crashing on POWER8 on heavy
> big-endian PowerKVM load
> Product: Platform Specific/Hardware
> Version: 2.5
> Kernel Version: 5.4.x
> Hardware: All
> OS: Linux
> Tree: Mainline
> Status: NEW
> Severity: normal
> Priority: P1
> Component: PPC-64
> Assignee: platform_ppc-64@kernel-bugs.osdl.org
> Reporter: glaubitz@physik.fu-berlin.de
> CC: matorola@gmail.com
> Regression: No
>
> Created attachment 287605
> --> https://bugzilla.kernel.org/attachment.cgi?id=287605&action=edit
> Backtrace of host system crashing with little-endian kernel
>
> We have an IBM POWER server (8247-42L) running Linux kernel 5.4.13 on Debian
> unstable hosting a big-endian ppc64 virtual machine running the same kernel in
> big-endian mode.
>
> When building OpenJDK-11 on the big-endian VM, the testsuite crashes the *host*
> system which is little-endian with the following kernel backtrace. The problem
> reproduces both with kernel 4.19.98 as well as 5.4.13, both guest and host
> running 5.4.x.
>
> Backtrace attached.
Thanks for the report, we need to get more data about the first BUG if
we can. What function in your vmlinux contains address
0xc00000000017a778? (use nm or objdump etc) Is that the first message you get,
No warnings or anything else earlier in the dmesg?
Also 0xc0000000002659a0 would be interesting.
When reproducing, do you ever get a clean trace of the first bug? Could
you try setting /proc/sys/kernel/panic_on_oops and reproducing?
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v3 26/32] powerpc/64: system call zero volatile registers when returning
From: Nicholas Piggin @ 2020-02-26 3:39 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: Michal Suchanek, linuxppc-dev
In-Reply-To: <20200225212032.GI22482@gate.crashing.org>
Segher Boessenkool's on February 26, 2020 7:20 am:
> Hi!
>
> On Wed, Feb 26, 2020 at 03:35:35AM +1000, Nicholas Piggin wrote:
>> Kernel addresses and potentially other sensitive data could be leaked
>> in volatile registers after a syscall.
>
>> cmpdi r3,0
>> bne .Lsyscall_restore_regs
>> + li r0,0
>> + li r4,0
>> + li r5,0
>> + li r6,0
>> + li r7,0
>> + li r8,0
>> + li r9,0
>> + li r10,0
>> + li r11,0
>> + li r12,0
>> + mtctr r0
>> + mtspr SPRN_XER,r0
>> .Lsyscall_restore_regs_cont:
>
> What about LR? Is that taken care of later?
LR is preserved by sc as per ABI.
> This also deserves a big fat comment imo, it is very important after
> all, and not so obvious.
Sure I can add something.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v3 3/6] powerpc/fsl_booke/64: implement KASLR for fsl_booke64
From: Jason Yan @ 2020-02-26 3:33 UTC (permalink / raw)
To: Christophe Leroy, mpe, linuxppc-dev, diana.craciun, benh, paulus,
npiggin, keescook, kernel-hardening, oss
Cc: linux-kernel, zhaohongjiang
In-Reply-To: <dbe0b316-40a2-7da4-c26b-e59efa555400@huawei.com>
在 2020/2/26 10:40, Jason Yan 写道:
>
>
> 在 2020/2/20 21:48, Christophe Leroy 写道:
>>
>>
>> Le 06/02/2020 à 03:58, Jason Yan a écrit :
>>> The implementation for Freescale BookE64 is similar as BookE32. One
>>> difference is that Freescale BookE64 set up a TLB mapping of 1G during
>>> booting. Another difference is that ppc64 needs the kernel to be
>>> 64K-aligned. So we can randomize the kernel in this 1G mapping and make
>>> it 64K-aligned. This can save some code to creat another TLB map at
>>> early boot. The disadvantage is that we only have about 1G/64K = 16384
>>> slots to put the kernel in.
>>>
>>> To support secondary cpu boot up, a variable __kaslr_offset was added in
>>> first_256B section. This can help secondary cpu get the kaslr offset
>>> before the 1:1 mapping has been setup.
>>>
>>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>>> Cc: Scott Wood <oss@buserror.net>
>>> Cc: Diana Craciun <diana.craciun@nxp.com>
>>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Cc: Paul Mackerras <paulus@samba.org>
>>> Cc: Nicholas Piggin <npiggin@gmail.com>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> ---
>>> arch/powerpc/Kconfig | 2 +-
>>> arch/powerpc/kernel/exceptions-64e.S | 10 +++++++++
>>> arch/powerpc/kernel/head_64.S | 7 ++++++
>>> arch/powerpc/kernel/setup_64.c | 4 +++-
>>> arch/powerpc/mm/mmu_decl.h | 16 +++++++-------
>>> arch/powerpc/mm/nohash/kaslr_booke.c | 33 +++++++++++++++++++++++++---
>>> 6 files changed, 59 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index c150a9d49343..754aeb96bb1c 100644
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -568,7 +568,7 @@ config RELOCATABLE
>>> config RANDOMIZE_BASE
>>> bool "Randomize the address of the kernel image"
>>> - depends on (FSL_BOOKE && FLATMEM && PPC32)
>>> + depends on (PPC_FSL_BOOK3E && FLATMEM)
>>> depends on RELOCATABLE
>>> help
>>> Randomizes the virtual address at which the kernel image is
>>> diff --git a/arch/powerpc/kernel/exceptions-64e.S
>>> b/arch/powerpc/kernel/exceptions-64e.S
>>> index 1b9b174bee86..c1c05b8684ca 100644
>>> --- a/arch/powerpc/kernel/exceptions-64e.S
>>> +++ b/arch/powerpc/kernel/exceptions-64e.S
>>> @@ -1378,6 +1378,7 @@ skpinv: addi r6,r6,1 /*
>>> Increment */
>>> 1: mflr r6
>>> addi r6,r6,(2f - 1b)
>>> tovirt(r6,r6)
>>> + add r6,r6,r19
>>> lis r7,MSR_KERNEL@h
>>> ori r7,r7,MSR_KERNEL@l
>>> mtspr SPRN_SRR0,r6
>>> @@ -1400,6 +1401,7 @@ skpinv: addi r6,r6,1 /*
>>> Increment */
>>> /* We translate LR and return */
>>> tovirt(r8,r8)
>>> + add r8,r8,r19
>>> mtlr r8
>>> blr
>>> @@ -1528,6 +1530,7 @@ a2_tlbinit_code_end:
>>> */
>>> _GLOBAL(start_initialization_book3e)
>>> mflr r28
>>> + li r19, 0
>>> /* First, we need to setup some initial TLBs to map the kernel
>>> * text, data and bss at PAGE_OFFSET. We don't have a real mode
>>> @@ -1570,6 +1573,12 @@ _GLOBAL(book3e_secondary_core_init)
>>> cmplwi r4,0
>>> bne 2f
>>> + li r19, 0
>>> +#ifdef CONFIG_RANDOMIZE_BASE
>>> + LOAD_REG_ADDR_PIC(r19, __kaslr_offset)
>>> + lwz r19,0(r19)
>>> + rlwinm r19,r19,0,0,5
>>> +#endif
>>> /* Setup TLB for this core */
>>> bl initial_tlb_book3e
>>> @@ -1602,6 +1611,7 @@ _GLOBAL(book3e_secondary_core_init)
>>> lis r3,PAGE_OFFSET@highest
>>> sldi r3,r3,32
>>> or r28,r28,r3
>>> + add r28,r28,r19
>>> 1: mtlr r28
>>> blr
>>> diff --git a/arch/powerpc/kernel/head_64.S
>>> b/arch/powerpc/kernel/head_64.S
>>> index ad79fddb974d..744624140fb8 100644
>>> --- a/arch/powerpc/kernel/head_64.S
>>> +++ b/arch/powerpc/kernel/head_64.S
>>> @@ -104,6 +104,13 @@ __secondary_hold_acknowledge:
>>> .8byte 0x0
>>> #ifdef CONFIG_RELOCATABLE
>>> +#ifdef CONFIG_RANDOMIZE_BASE
>>> + . = 0x58
>>> + .globl __kaslr_offset
>>> +__kaslr_offset:
>>> +DEFINE_FIXED_SYMBOL(__kaslr_offset)
>>> + .long 0
>>> +#endif
>>> /* This flag is set to 1 by a loader if the kernel should run
>>> * at the loaded address instead of the linked address. This
>>> * is used by kexec-tools to keep the the kdump kernel in the
>>> diff --git a/arch/powerpc/kernel/setup_64.c
>>> b/arch/powerpc/kernel/setup_64.c
>>> index 6104917a282d..a16b970a8d1a 100644
>>> --- a/arch/powerpc/kernel/setup_64.c
>>> +++ b/arch/powerpc/kernel/setup_64.c
>>> @@ -66,7 +66,7 @@
>>> #include <asm/feature-fixups.h>
>>> #include <asm/kup.h>
>>> #include <asm/early_ioremap.h>
>>> -
>>
>> Why remove this new line which clearly separates things in asm/ and
>> things in local dir ?
>
> Sorry to break this. I will add the new line back.
>
>>
>>> +#include <mm/mmu_decl.h>
>>> #include "setup.h"
>>> int spinning_secondaries;
>>> @@ -300,6 +300,8 @@ void __init early_setup(unsigned long dt_ptr)
>>> /* Enable early debugging if any specified (see udbg.h) */
>>> udbg_early_init();
>>> + kaslr_early_init(__va(dt_ptr), 0);
>>> +
>>> udbg_printf(" -> %s(), dt_ptr: 0x%lx\n", __func__, dt_ptr);
>>> /*
>>> diff --git a/arch/powerpc/mm/mmu_decl.h b/arch/powerpc/mm/mmu_decl.h
>>> index 3e1c85c7d10b..bbd721d1e3d7 100644
>>> --- a/arch/powerpc/mm/mmu_decl.h
>>> +++ b/arch/powerpc/mm/mmu_decl.h
>>> @@ -147,14 +147,6 @@ void reloc_kernel_entry(void *fdt, long addr);
>>> extern void loadcam_entry(unsigned int index);
>>> extern void loadcam_multi(int first_idx, int num, int tmp_idx);
>>> -#ifdef CONFIG_RANDOMIZE_BASE
>>> -void kaslr_early_init(void *dt_ptr, phys_addr_t size);
>>> -void kaslr_late_init(void);
>>> -#else
>>> -static inline void kaslr_early_init(void *dt_ptr, phys_addr_t size) {}
>>> -static inline void kaslr_late_init(void) {}
>>> -#endif
>>> -
>>> struct tlbcam {
>>> u32 MAS0;
>>> u32 MAS1;
>>> @@ -164,6 +156,14 @@ struct tlbcam {
>>> };
>>> #endif
>>> +#ifdef CONFIG_RANDOMIZE_BASE
>>> +void kaslr_early_init(void *dt_ptr, phys_addr_t size);
>>> +void kaslr_late_init(void);
>>> +#else
>>> +static inline void kaslr_early_init(void *dt_ptr, phys_addr_t size) {}
>>> +static inline void kaslr_late_init(void) {}
>>> +#endif
>>> +
>>> #if defined(CONFIG_PPC_BOOK3S_32) || defined(CONFIG_FSL_BOOKE) ||
>>> defined(CONFIG_PPC_8xx)
>>> /* 6xx have BATS */
>>> /* FSL_BOOKE have TLBCAM */
>>> diff --git a/arch/powerpc/mm/nohash/kaslr_booke.c
>>> b/arch/powerpc/mm/nohash/kaslr_booke.c
>>> index 07b036e98353..c6f5c1db1394 100644
>>> --- a/arch/powerpc/mm/nohash/kaslr_booke.c
>>> +++ b/arch/powerpc/mm/nohash/kaslr_booke.c
>>> @@ -231,7 +231,7 @@ static __init unsigned long
>>> get_usable_address(const void *fdt,
>>> unsigned long pa;
>>> unsigned long pa_end;
>>> - for (pa = offset; (long)pa > (long)start; pa -= SZ_16K) {
>>> + for (pa = offset; (long)pa > (long)start; pa -= SZ_64K) {
>>
>> Doesn't this modify the behaviour for PPC32 too ?
>
> Oh, yes. I will fix this.
>
>>
>>> pa_end = pa + regions.kernel_size;
>>> if (overlaps_region(fdt, pa, pa_end))
>>> continue;
>>> @@ -265,14 +265,14 @@ static unsigned long __init
>>> kaslr_legal_offset(void *dt_ptr, unsigned long rando
>>> {
>>> unsigned long koffset = 0;
>>> unsigned long start;
>>> - unsigned long index;
>>> unsigned long offset;
>>> +#ifdef CONFIG_PPC32
>>
>> Can we use
>>
>> if (IS_ENABLED(CONFIG_PPC32)) {
>> /* 32 bits stuff */
>> } else {
>> /* 64 bits stuff */
>> }
>
> Thansk for the suggestion. I will consider to use IS_ENABLED() instead.
>
>>
>>> /*
>>> * Decide which 64M we want to start
>>> * Only use the low 8 bits of the random seed
>>> */
>>> - index = random & 0xFF;
>>> + unsigned long index = random & 0xFF;
>>
>> That's not good in terms of readability, index declaration should
>> remain at the top of the function, should be possible if using
>> IS_ENABLED() instead
>
> I'm wondering how to declare a variable inside a code block such as if
> (IS_ENABLED(CONFIG_PPC32)) at the top of the function and use the
> variable in another if (IS_ENABLED(CONFIG_PPC32)). Is there any good idea?
>
Hi Christophe,
When using a standard C if/else, all code compiled for PPC32 and PPC64,
but this will bring some build error because not all variables both
defined for PPC32 and PPC64.
[yanaijie@138 linux]$ sh ppc64build.sh
CALL scripts/atomic/check-atomics.sh
CALL scripts/checksyscalls.sh
CHK include/generated/compile.h
CC arch/powerpc/mm/nohash/kaslr_booke.o
arch/powerpc/mm/nohash/kaslr_booke.c: In function 'kaslr_choose_location':
arch/powerpc/mm/nohash/kaslr_booke.c:341:30: error:
'CONFIG_LOWMEM_CAM_NUM' undeclared (first use in this function); did you
mean 'CONFIG_FLATMEM_MANUAL'?
ram = map_mem_in_cams(ram, CONFIG_LOWMEM_CAM_NUM, true);
^~~~~~~~~~~~~~~~~~~~~
CONFIG_FLATMEM_MANUAL
arch/powerpc/mm/nohash/kaslr_booke.c:341:30: note: each undeclared
identifier is reported only once for each function it appears in
arch/powerpc/mm/nohash/kaslr_booke.c: In function 'kaslr_early_init':
arch/powerpc/mm/nohash/kaslr_booke.c:404:3: error: 'is_second_reloc'
undeclared (first use in this function); did you mean '__cond_lock'?
is_second_reloc = 1;
^~~~~~~~~~~~~~~
__cond_lock
arch/powerpc/mm/nohash/kaslr_booke.c:411:4: error: implicit declaration
of function 'create_kaslr_tlb_entry'; did you mean 'reloc_kernel_entry'?
[-Werror=implicit-function-declaration]
create_kaslr_tlb_entry(1, tlb_virt, tlb_phys);
^~~~~~~~~~~~~~~~~~~~~~
reloc_kernel_entry
cc1: all warnings being treated as errors
make[3]: *** [scripts/Makefile.build:268:
arch/powerpc/mm/nohash/kaslr_booke.o] Error 1
make[2]: *** [scripts/Makefile.build:505: arch/powerpc/mm/nohash] Error 2
make[1]: *** [scripts/Makefile.build:505: arch/powerpc/mm] Error 2
make: *** [Makefile:1681: arch/powerpc] Error 2
Thanks,
Jason
>>
>>> index %= regions.linear_sz / SZ_64M;
>>> /* Decide offset inside 64M */
>>> @@ -287,6 +287,15 @@ static unsigned long __init
>>> kaslr_legal_offset(void *dt_ptr, unsigned long rando
>>> break;
>>> index--;
>>> }
>>> +#else
>>> + /* Decide kernel offset inside 1G */
>>> + offset = random % (SZ_1G - regions.kernel_size);
>>> + offset = round_down(offset, SZ_64K);
>>> +
>>> + start = memstart_addr;
>>> + offset = memstart_addr + offset;
>>> + koffset = get_usable_address(dt_ptr, start, offset);
>>> +#endif
>>> if (koffset != 0)
>>> koffset -= memstart_addr;
>>> @@ -325,6 +334,7 @@ static unsigned long __init
>>> kaslr_choose_location(void *dt_ptr, phys_addr_t size
>>> else
>>> pr_warn("KASLR: No safe seed for randomizing the kernel
>>> base.\n");
>>> +#ifdef CONFIG_PPC32
>>> ram = min_t(phys_addr_t, __max_low_memory, size);
>>> ram = map_mem_in_cams(ram, CONFIG_LOWMEM_CAM_NUM, true);
>>> linear_sz = min_t(unsigned long, ram, SZ_512M);
>>> @@ -332,6 +342,7 @@ static unsigned long __init
>>> kaslr_choose_location(void *dt_ptr, phys_addr_t size
>>> /* If the linear size is smaller than 64M, do not randmize */
>>> if (linear_sz < SZ_64M)
>>> return 0;
>>> +#endif
>>> /* check for a reserved-memory node and record its cell sizes */
>>> regions.reserved_mem = fdt_path_offset(dt_ptr,
>>> "/reserved-memory");
>>> @@ -363,6 +374,17 @@ notrace void __init kaslr_early_init(void
>>> *dt_ptr, phys_addr_t size)
>>> unsigned long offset;
>>> unsigned long kernel_sz;
>>> +#ifdef CONFIG_PPC64
>>
>> Same, can we use a standard C if/else sequence with
>> IS_ENABLED(CONFIG_PPC64) ?
>
> OK, I will try to do this if I can deal with the declaration of
> variables in different if/else sequence.
>
> Thanks,
> Jason
>
>
>>
>>> + unsigned int *__kaslr_offset = (unsigned int *)(KERNELBASE + 0x58);
>>> + unsigned int *__run_at_load = (unsigned int *)(KERNELBASE + 0x5c);
>>> +
>>> + if (*__run_at_load == 1)
>>> + return;
>>> +
>>> + /* Setup flat device-tree pointer */
>>> + initial_boot_params = dt_ptr;
>>> +#endif
>>> +
>>> kernel_sz = (unsigned long)_end - (unsigned long)_stext;
>>> offset = kaslr_choose_location(dt_ptr, size, kernel_sz);
>>> @@ -372,6 +394,7 @@ notrace void __init kaslr_early_init(void
>>> *dt_ptr, phys_addr_t size)
>>> kernstart_virt_addr += offset;
>>> kernstart_addr += offset;
>>> +#ifdef CONFIG_PPC32
>>> is_second_reloc = 1;
>>> if (offset >= SZ_64M) {
>>> @@ -381,6 +404,10 @@ notrace void __init kaslr_early_init(void
>>> *dt_ptr, phys_addr_t size)
>>> /* Create kernel map to relocate in */
>>> create_kaslr_tlb_entry(1, tlb_virt, tlb_phys);
>>> }
>>> +#else
>>> + *__kaslr_offset = kernstart_virt_addr - KERNELBASE;
>>> + *__run_at_load = 1;
>>> +#endif
>>> /* Copy the kernel to it's new location and run */
>>> memcpy((void *)kernstart_virt_addr, (void *)_stext, kernel_sz);
>>>
>>
>> Christophe
>>
>> .
>
>
> .
^ permalink raw reply
* Re: [PATCH v2 3/3] ASoC: fsl_easrc: Add EASRC ASoC CPU DAI and platform drivers
From: Nicolin Chen @ 2020-02-26 2:55 UTC (permalink / raw)
To: Shengjiu Wang
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com, S.j. Wang,
tiwai@suse.com, lgirdwood@gmail.com, robh+dt@kernel.org,
broonie@kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAA+D8AMFzDs8uXiR-N8harRVmhC+3i8p9HdO2CgxOCX8WVfXAw@mail.gmail.com>
On Wed, Feb 26, 2020 at 09:51:39AM +0800, Shengjiu Wang wrote:
> > > > > +static const struct regmap_config fsl_easrc_regmap_config = {
> > > > > + .readable_reg = fsl_easrc_readable_reg,
> > > > > + .volatile_reg = fsl_easrc_volatile_reg,
> > > > > + .writeable_reg = fsl_easrc_writeable_reg,
> > > >
> > > > Can we use regmap_range and regmap_access_table?
> > > >
> > >
> > > Can the regmap_range support discontinuous registers? The
> > > reg_stride = 4.
> >
> > I think it does. Giving an example here:
> > https://github.com/torvalds/linux/blob/master/drivers/mfd/da9063-i2c.c
>
> The register in this i2c driver are continuous, from 0x00, 0x01, 0x02...
>
> But our case is 0x00, 0x04, 0x08, does it work?
Ah...I see your point now. I am not very sure -- have only used
in I2C drivers. You can ignore if it doesn't likely work for us.
^ permalink raw reply
* Re: [PATCH v3 6/6] powerpc/fsl_booke/kaslr: rename kaslr-booke32.rst to kaslr-booke.rst and add 64bit part
From: Jason Yan @ 2020-02-26 2:46 UTC (permalink / raw)
To: Christophe Leroy, mpe, linuxppc-dev, diana.craciun, benh, paulus,
npiggin, keescook, kernel-hardening, oss
Cc: linux-kernel, zhaohongjiang
In-Reply-To: <77c4a404-3ce5-5090-bbff-aaca71507146@c-s.fr>
在 2020/2/20 21:50, Christophe Leroy 写道:
>
>
> Le 06/02/2020 à 03:58, Jason Yan a écrit :
>> Now we support both 32 and 64 bit KASLR for fsl booke. Add document for
>> 64 bit part and rename kaslr-booke32.rst to kaslr-booke.rst.
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> Cc: Scott Wood <oss@buserror.net>
>> Cc: Diana Craciun <diana.craciun@nxp.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Nicholas Piggin <npiggin@gmail.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> ---
>> .../{kaslr-booke32.rst => kaslr-booke.rst} | 35 ++++++++++++++++---
>> 1 file changed, 31 insertions(+), 4 deletions(-)
>> rename Documentation/powerpc/{kaslr-booke32.rst => kaslr-booke.rst}
>> (59%)
>
> Also update Documentation/powerpc/index.rst ?
>
Oh yes, thanks for reminding me of this.
Thanks,
Jason
> Christophe
>
> .
^ permalink raw reply
* Re: [PATCH v3 5/6] powerpc/fsl_booke/64: clear the original kernel if randomized
From: Jason Yan @ 2020-02-26 2:44 UTC (permalink / raw)
To: Christophe Leroy, mpe, linuxppc-dev, diana.craciun, benh, paulus,
npiggin, keescook, kernel-hardening, oss
Cc: linux-kernel, zhaohongjiang
In-Reply-To: <0f778e1c-5e29-e600-1cf0-aeb3e1a6fe08@c-s.fr>
在 2020/2/20 21:49, Christophe Leroy 写道:
>
>
> Le 06/02/2020 à 03:58, Jason Yan a écrit :
>> The original kernel still exists in the memory, clear it now.
>
> No such problem with PPC32 ? Or is that common ?
>
PPC32 did this in relocate_init() in fsl_booke.c because PPC32 will not
reach kaslr_early_init for the second pass after relocation.
Thanks,
Jason
> Christophe
>
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> Cc: Scott Wood <oss@buserror.net>
>> Cc: Diana Craciun <diana.craciun@nxp.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Nicholas Piggin <npiggin@gmail.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> ---
>> arch/powerpc/mm/nohash/kaslr_booke.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/mm/nohash/kaslr_booke.c
>> b/arch/powerpc/mm/nohash/kaslr_booke.c
>> index c6f5c1db1394..ed1277059368 100644
>> --- a/arch/powerpc/mm/nohash/kaslr_booke.c
>> +++ b/arch/powerpc/mm/nohash/kaslr_booke.c
>> @@ -378,8 +378,10 @@ notrace void __init kaslr_early_init(void
>> *dt_ptr, phys_addr_t size)
>> unsigned int *__kaslr_offset = (unsigned int *)(KERNELBASE + 0x58);
>> unsigned int *__run_at_load = (unsigned int *)(KERNELBASE + 0x5c);
>> - if (*__run_at_load == 1)
>> + if (*__run_at_load == 1) {
>> + kaslr_late_init();
>> return;
>> + }
>> /* Setup flat device-tree pointer */
>> initial_boot_params = dt_ptr;
>>
>
> .
^ permalink raw reply
* Re: [PATCH v3 3/6] powerpc/fsl_booke/64: implement KASLR for fsl_booke64
From: Jason Yan @ 2020-02-26 2:40 UTC (permalink / raw)
To: Christophe Leroy, mpe, linuxppc-dev, diana.craciun, benh, paulus,
npiggin, keescook, kernel-hardening, oss
Cc: linux-kernel, zhaohongjiang
In-Reply-To: <41b9f1ca-c6fd-291a-2c96-2a0e8a754ec4@c-s.fr>
在 2020/2/20 21:48, Christophe Leroy 写道:
>
>
> Le 06/02/2020 à 03:58, Jason Yan a écrit :
>> The implementation for Freescale BookE64 is similar as BookE32. One
>> difference is that Freescale BookE64 set up a TLB mapping of 1G during
>> booting. Another difference is that ppc64 needs the kernel to be
>> 64K-aligned. So we can randomize the kernel in this 1G mapping and make
>> it 64K-aligned. This can save some code to creat another TLB map at
>> early boot. The disadvantage is that we only have about 1G/64K = 16384
>> slots to put the kernel in.
>>
>> To support secondary cpu boot up, a variable __kaslr_offset was added in
>> first_256B section. This can help secondary cpu get the kaslr offset
>> before the 1:1 mapping has been setup.
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> Cc: Scott Wood <oss@buserror.net>
>> Cc: Diana Craciun <diana.craciun@nxp.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Nicholas Piggin <npiggin@gmail.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> ---
>> arch/powerpc/Kconfig | 2 +-
>> arch/powerpc/kernel/exceptions-64e.S | 10 +++++++++
>> arch/powerpc/kernel/head_64.S | 7 ++++++
>> arch/powerpc/kernel/setup_64.c | 4 +++-
>> arch/powerpc/mm/mmu_decl.h | 16 +++++++-------
>> arch/powerpc/mm/nohash/kaslr_booke.c | 33 +++++++++++++++++++++++++---
>> 6 files changed, 59 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index c150a9d49343..754aeb96bb1c 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -568,7 +568,7 @@ config RELOCATABLE
>> config RANDOMIZE_BASE
>> bool "Randomize the address of the kernel image"
>> - depends on (FSL_BOOKE && FLATMEM && PPC32)
>> + depends on (PPC_FSL_BOOK3E && FLATMEM)
>> depends on RELOCATABLE
>> help
>> Randomizes the virtual address at which the kernel image is
>> diff --git a/arch/powerpc/kernel/exceptions-64e.S
>> b/arch/powerpc/kernel/exceptions-64e.S
>> index 1b9b174bee86..c1c05b8684ca 100644
>> --- a/arch/powerpc/kernel/exceptions-64e.S
>> +++ b/arch/powerpc/kernel/exceptions-64e.S
>> @@ -1378,6 +1378,7 @@ skpinv: addi r6,r6,1 /*
>> Increment */
>> 1: mflr r6
>> addi r6,r6,(2f - 1b)
>> tovirt(r6,r6)
>> + add r6,r6,r19
>> lis r7,MSR_KERNEL@h
>> ori r7,r7,MSR_KERNEL@l
>> mtspr SPRN_SRR0,r6
>> @@ -1400,6 +1401,7 @@ skpinv: addi r6,r6,1 /*
>> Increment */
>> /* We translate LR and return */
>> tovirt(r8,r8)
>> + add r8,r8,r19
>> mtlr r8
>> blr
>> @@ -1528,6 +1530,7 @@ a2_tlbinit_code_end:
>> */
>> _GLOBAL(start_initialization_book3e)
>> mflr r28
>> + li r19, 0
>> /* First, we need to setup some initial TLBs to map the kernel
>> * text, data and bss at PAGE_OFFSET. We don't have a real mode
>> @@ -1570,6 +1573,12 @@ _GLOBAL(book3e_secondary_core_init)
>> cmplwi r4,0
>> bne 2f
>> + li r19, 0
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> + LOAD_REG_ADDR_PIC(r19, __kaslr_offset)
>> + lwz r19,0(r19)
>> + rlwinm r19,r19,0,0,5
>> +#endif
>> /* Setup TLB for this core */
>> bl initial_tlb_book3e
>> @@ -1602,6 +1611,7 @@ _GLOBAL(book3e_secondary_core_init)
>> lis r3,PAGE_OFFSET@highest
>> sldi r3,r3,32
>> or r28,r28,r3
>> + add r28,r28,r19
>> 1: mtlr r28
>> blr
>> diff --git a/arch/powerpc/kernel/head_64.S
>> b/arch/powerpc/kernel/head_64.S
>> index ad79fddb974d..744624140fb8 100644
>> --- a/arch/powerpc/kernel/head_64.S
>> +++ b/arch/powerpc/kernel/head_64.S
>> @@ -104,6 +104,13 @@ __secondary_hold_acknowledge:
>> .8byte 0x0
>> #ifdef CONFIG_RELOCATABLE
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> + . = 0x58
>> + .globl __kaslr_offset
>> +__kaslr_offset:
>> +DEFINE_FIXED_SYMBOL(__kaslr_offset)
>> + .long 0
>> +#endif
>> /* This flag is set to 1 by a loader if the kernel should run
>> * at the loaded address instead of the linked address. This
>> * is used by kexec-tools to keep the the kdump kernel in the
>> diff --git a/arch/powerpc/kernel/setup_64.c
>> b/arch/powerpc/kernel/setup_64.c
>> index 6104917a282d..a16b970a8d1a 100644
>> --- a/arch/powerpc/kernel/setup_64.c
>> +++ b/arch/powerpc/kernel/setup_64.c
>> @@ -66,7 +66,7 @@
>> #include <asm/feature-fixups.h>
>> #include <asm/kup.h>
>> #include <asm/early_ioremap.h>
>> -
>
> Why remove this new line which clearly separates things in asm/ and
> things in local dir ?
Sorry to break this. I will add the new line back.
>
>> +#include <mm/mmu_decl.h>
>> #include "setup.h"
>> int spinning_secondaries;
>> @@ -300,6 +300,8 @@ void __init early_setup(unsigned long dt_ptr)
>> /* Enable early debugging if any specified (see udbg.h) */
>> udbg_early_init();
>> + kaslr_early_init(__va(dt_ptr), 0);
>> +
>> udbg_printf(" -> %s(), dt_ptr: 0x%lx\n", __func__, dt_ptr);
>> /*
>> diff --git a/arch/powerpc/mm/mmu_decl.h b/arch/powerpc/mm/mmu_decl.h
>> index 3e1c85c7d10b..bbd721d1e3d7 100644
>> --- a/arch/powerpc/mm/mmu_decl.h
>> +++ b/arch/powerpc/mm/mmu_decl.h
>> @@ -147,14 +147,6 @@ void reloc_kernel_entry(void *fdt, long addr);
>> extern void loadcam_entry(unsigned int index);
>> extern void loadcam_multi(int first_idx, int num, int tmp_idx);
>> -#ifdef CONFIG_RANDOMIZE_BASE
>> -void kaslr_early_init(void *dt_ptr, phys_addr_t size);
>> -void kaslr_late_init(void);
>> -#else
>> -static inline void kaslr_early_init(void *dt_ptr, phys_addr_t size) {}
>> -static inline void kaslr_late_init(void) {}
>> -#endif
>> -
>> struct tlbcam {
>> u32 MAS0;
>> u32 MAS1;
>> @@ -164,6 +156,14 @@ struct tlbcam {
>> };
>> #endif
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> +void kaslr_early_init(void *dt_ptr, phys_addr_t size);
>> +void kaslr_late_init(void);
>> +#else
>> +static inline void kaslr_early_init(void *dt_ptr, phys_addr_t size) {}
>> +static inline void kaslr_late_init(void) {}
>> +#endif
>> +
>> #if defined(CONFIG_PPC_BOOK3S_32) || defined(CONFIG_FSL_BOOKE) ||
>> defined(CONFIG_PPC_8xx)
>> /* 6xx have BATS */
>> /* FSL_BOOKE have TLBCAM */
>> diff --git a/arch/powerpc/mm/nohash/kaslr_booke.c
>> b/arch/powerpc/mm/nohash/kaslr_booke.c
>> index 07b036e98353..c6f5c1db1394 100644
>> --- a/arch/powerpc/mm/nohash/kaslr_booke.c
>> +++ b/arch/powerpc/mm/nohash/kaslr_booke.c
>> @@ -231,7 +231,7 @@ static __init unsigned long
>> get_usable_address(const void *fdt,
>> unsigned long pa;
>> unsigned long pa_end;
>> - for (pa = offset; (long)pa > (long)start; pa -= SZ_16K) {
>> + for (pa = offset; (long)pa > (long)start; pa -= SZ_64K) {
>
> Doesn't this modify the behaviour for PPC32 too ?
Oh, yes. I will fix this.
>
>> pa_end = pa + regions.kernel_size;
>> if (overlaps_region(fdt, pa, pa_end))
>> continue;
>> @@ -265,14 +265,14 @@ static unsigned long __init
>> kaslr_legal_offset(void *dt_ptr, unsigned long rando
>> {
>> unsigned long koffset = 0;
>> unsigned long start;
>> - unsigned long index;
>> unsigned long offset;
>> +#ifdef CONFIG_PPC32
>
> Can we use
>
> if (IS_ENABLED(CONFIG_PPC32)) {
> /* 32 bits stuff */
> } else {
> /* 64 bits stuff */
> }
Thansk for the suggestion. I will consider to use IS_ENABLED() instead.
>
>> /*
>> * Decide which 64M we want to start
>> * Only use the low 8 bits of the random seed
>> */
>> - index = random & 0xFF;
>> + unsigned long index = random & 0xFF;
>
> That's not good in terms of readability, index declaration should remain
> at the top of the function, should be possible if using IS_ENABLED()
> instead
I'm wondering how to declare a variable inside a code block such as if
(IS_ENABLED(CONFIG_PPC32)) at the top of the function and use the
variable in another if (IS_ENABLED(CONFIG_PPC32)). Is there any good idea?
>
>> index %= regions.linear_sz / SZ_64M;
>> /* Decide offset inside 64M */
>> @@ -287,6 +287,15 @@ static unsigned long __init
>> kaslr_legal_offset(void *dt_ptr, unsigned long rando
>> break;
>> index--;
>> }
>> +#else
>> + /* Decide kernel offset inside 1G */
>> + offset = random % (SZ_1G - regions.kernel_size);
>> + offset = round_down(offset, SZ_64K);
>> +
>> + start = memstart_addr;
>> + offset = memstart_addr + offset;
>> + koffset = get_usable_address(dt_ptr, start, offset);
>> +#endif
>> if (koffset != 0)
>> koffset -= memstart_addr;
>> @@ -325,6 +334,7 @@ static unsigned long __init
>> kaslr_choose_location(void *dt_ptr, phys_addr_t size
>> else
>> pr_warn("KASLR: No safe seed for randomizing the kernel
>> base.\n");
>> +#ifdef CONFIG_PPC32
>> ram = min_t(phys_addr_t, __max_low_memory, size);
>> ram = map_mem_in_cams(ram, CONFIG_LOWMEM_CAM_NUM, true);
>> linear_sz = min_t(unsigned long, ram, SZ_512M);
>> @@ -332,6 +342,7 @@ static unsigned long __init
>> kaslr_choose_location(void *dt_ptr, phys_addr_t size
>> /* If the linear size is smaller than 64M, do not randmize */
>> if (linear_sz < SZ_64M)
>> return 0;
>> +#endif
>> /* check for a reserved-memory node and record its cell sizes */
>> regions.reserved_mem = fdt_path_offset(dt_ptr, "/reserved-memory");
>> @@ -363,6 +374,17 @@ notrace void __init kaslr_early_init(void
>> *dt_ptr, phys_addr_t size)
>> unsigned long offset;
>> unsigned long kernel_sz;
>> +#ifdef CONFIG_PPC64
>
> Same, can we use a standard C if/else sequence with
> IS_ENABLED(CONFIG_PPC64) ?
OK, I will try to do this if I can deal with the declaration of
variables in different if/else sequence.
Thanks,
Jason
>
>> + unsigned int *__kaslr_offset = (unsigned int *)(KERNELBASE + 0x58);
>> + unsigned int *__run_at_load = (unsigned int *)(KERNELBASE + 0x5c);
>> +
>> + if (*__run_at_load == 1)
>> + return;
>> +
>> + /* Setup flat device-tree pointer */
>> + initial_boot_params = dt_ptr;
>> +#endif
>> +
>> kernel_sz = (unsigned long)_end - (unsigned long)_stext;
>> offset = kaslr_choose_location(dt_ptr, size, kernel_sz);
>> @@ -372,6 +394,7 @@ notrace void __init kaslr_early_init(void *dt_ptr,
>> phys_addr_t size)
>> kernstart_virt_addr += offset;
>> kernstart_addr += offset;
>> +#ifdef CONFIG_PPC32
>> is_second_reloc = 1;
>> if (offset >= SZ_64M) {
>> @@ -381,6 +404,10 @@ notrace void __init kaslr_early_init(void
>> *dt_ptr, phys_addr_t size)
>> /* Create kernel map to relocate in */
>> create_kaslr_tlb_entry(1, tlb_virt, tlb_phys);
>> }
>> +#else
>> + *__kaslr_offset = kernstart_virt_addr - KERNELBASE;
>> + *__run_at_load = 1;
>> +#endif
>> /* Copy the kernel to it's new location and run */
>> memcpy((void *)kernstart_virt_addr, (void *)_stext, kernel_sz);
>>
>
> Christophe
>
> .
^ permalink raw reply
* Re: [PATCH v3 1/6] powerpc/fsl_booke/kaslr: refactor kaslr_legal_offset() and kaslr_early_init()
From: Jason Yan @ 2020-02-26 2:11 UTC (permalink / raw)
To: Christophe Leroy, mpe, linuxppc-dev, diana.craciun, benh, paulus,
npiggin, keescook, kernel-hardening, oss
Cc: linux-kernel, zhaohongjiang
In-Reply-To: <6c0b0720-6998-f43a-a2b6-0632d4df1126@c-s.fr>
在 2020/2/20 21:40, Christophe Leroy 写道:
>
>
> Le 06/02/2020 à 03:58, Jason Yan a écrit :
>> Some code refactor in kaslr_legal_offset() and kaslr_early_init(). No
>> functional change. This is a preparation for KASLR fsl_booke64.
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> Cc: Scott Wood <oss@buserror.net>
>> Cc: Diana Craciun <diana.craciun@nxp.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Nicholas Piggin <npiggin@gmail.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> ---
>> arch/powerpc/mm/nohash/kaslr_booke.c | 40 ++++++++++++++--------------
>> 1 file changed, 20 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/nohash/kaslr_booke.c
>> b/arch/powerpc/mm/nohash/kaslr_booke.c
>> index 4a75f2d9bf0e..07b036e98353 100644
>> --- a/arch/powerpc/mm/nohash/kaslr_booke.c
>> +++ b/arch/powerpc/mm/nohash/kaslr_booke.c
>> @@ -25,6 +25,7 @@ struct regions {
>> unsigned long pa_start;
>> unsigned long pa_end;
>> unsigned long kernel_size;
>> + unsigned long linear_sz;
>> unsigned long dtb_start;
>> unsigned long dtb_end;
>> unsigned long initrd_start;
>> @@ -260,11 +261,23 @@ static __init void get_cell_sizes(const void
>> *fdt, int node, int *addr_cells,
>> *size_cells = fdt32_to_cpu(*prop);
>> }
>> -static unsigned long __init kaslr_legal_offset(void *dt_ptr, unsigned
>> long index,
>> - unsigned long offset)
>> +static unsigned long __init kaslr_legal_offset(void *dt_ptr, unsigned
>> long random)
>> {
>> unsigned long koffset = 0;
>> unsigned long start;
>> + unsigned long index;
>> + unsigned long offset;
>> +
>> + /*
>> + * Decide which 64M we want to start
>> + * Only use the low 8 bits of the random seed
>> + */
>> + index = random & 0xFF;
>> + index %= regions.linear_sz / SZ_64M;
>> +
>> + /* Decide offset inside 64M */
>> + offset = random % (SZ_64M - regions.kernel_size);
>> + offset = round_down(offset, SZ_16K);
>> while ((long)index >= 0) {
>> offset = memstart_addr + index * SZ_64M + offset;
>> @@ -289,10 +302,9 @@ static inline __init bool kaslr_disabled(void)
>> static unsigned long __init kaslr_choose_location(void *dt_ptr,
>> phys_addr_t size,
>> unsigned long kernel_sz)
>> {
>> - unsigned long offset, random;
>> + unsigned long random;
>> unsigned long ram, linear_sz;
>> u64 seed;
>> - unsigned long index;
>> kaslr_get_cmdline(dt_ptr);
>> if (kaslr_disabled())
>> @@ -333,22 +345,12 @@ static unsigned long __init
>> kaslr_choose_location(void *dt_ptr, phys_addr_t size
>> regions.dtb_start = __pa(dt_ptr);
>> regions.dtb_end = __pa(dt_ptr) + fdt_totalsize(dt_ptr);
>> regions.kernel_size = kernel_sz;
>> + regions.linear_sz = linear_sz;
>> get_initrd_range(dt_ptr);
>> get_crash_kernel(dt_ptr, ram);
>> - /*
>> - * Decide which 64M we want to start
>> - * Only use the low 8 bits of the random seed
>> - */
>> - index = random & 0xFF;
>> - index %= linear_sz / SZ_64M;
>> -
>> - /* Decide offset inside 64M */
>> - offset = random % (SZ_64M - kernel_sz);
>> - offset = round_down(offset, SZ_16K);
>> -
>> - return kaslr_legal_offset(dt_ptr, index, offset);
>> + return kaslr_legal_offset(dt_ptr, random);
>> }
>> /*
>> @@ -358,8 +360,6 @@ static unsigned long __init
>> kaslr_choose_location(void *dt_ptr, phys_addr_t size
>> */
>> notrace void __init kaslr_early_init(void *dt_ptr, phys_addr_t size)
>> {
>> - unsigned long tlb_virt;
>> - phys_addr_t tlb_phys;
>> unsigned long offset;
>> unsigned long kernel_sz;
>> @@ -375,8 +375,8 @@ notrace void __init kaslr_early_init(void *dt_ptr,
>> phys_addr_t size)
>> is_second_reloc = 1;
>> if (offset >= SZ_64M) {
>> - tlb_virt = round_down(kernstart_virt_addr, SZ_64M);
>> - tlb_phys = round_down(kernstart_addr, SZ_64M);
>> + unsigned long tlb_virt = round_down(kernstart_virt_addr,
>> SZ_64M);
>> + phys_addr_t tlb_phys = round_down(kernstart_addr, SZ_64M);
>
> That looks like cleanup unrelated to the patch itself.
Hi, Christophe
These two variables is only for the booke32 code, so I moved the
definition here so that I can save a "#ifdef CONFIG_PPC32" for them.
Thanks,
Jason
>
>> /* Create kernel map to relocate in */
>> create_kaslr_tlb_entry(1, tlb_virt, tlb_phys);
>>
>
> Christophe
>
> .
^ permalink raw reply
* Re: [PATCH v2 3/3] ASoC: fsl_easrc: Add EASRC ASoC CPU DAI and platform drivers
From: Shengjiu Wang @ 2020-02-26 1:51 UTC (permalink / raw)
To: Nicolin Chen
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, festevam@gmail.com, S.j. Wang,
tiwai@suse.com, lgirdwood@gmail.com, robh+dt@kernel.org,
broonie@kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20200225080350.GA11332@Asurada>
On Tue, Feb 25, 2020 at 4:05 PM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Mon, Feb 24, 2020 at 08:53:25AM +0000, S.j. Wang wrote:
> > Hi
> >
> > > >
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > ---
> > > > sound/soc/fsl/Kconfig | 10 +
> > > > sound/soc/fsl/Makefile | 2 +
> > > > sound/soc/fsl/fsl_asrc_common.h | 1 +
> > > > sound/soc/fsl/fsl_easrc.c | 2265 +++++++++++++++++++++++++++++++
> > > > sound/soc/fsl/fsl_easrc.h | 668 +++++++++
> > > > sound/soc/fsl/fsl_easrc_dma.c | 440 ++++++
> > >
> > > I see a 90% similarity between fsl_asrc_dma and fsl_easrc_dma files.
> > > Would it be possible reuse the existing code? Could share structures from
> > > my point of view, just like it reuses "enum asrc_pair_index", I know
> > > differentiating "pair" and "context" is a big point here though.
> > >
> > > A possible quick solution for that, off the top of my head, could be:
> > >
> > > 1) in fsl_asrc_common.h
> > >
> > > struct fsl_asrc {
> > > ....
> > > };
> > >
> > > struct fsl_asrc_pair {
> > > ....
> > > };
> > >
> > > 2) in fsl_easrc.h
> > >
> > > /* Renaming shared structures */
> > > #define fsl_easrc fsl_asrc
> > > #define fsl_easrc_context fsl_asrc_pair
> > >
> > > May be a good idea to see if others have some opinion too.
> > >
> >
> > We need to modify the fsl_asrc and fsl_asrc_pair, let them
> > To be used by both driver, also we need to put the specific
> > Definition for each module to same struct, right?
>
> Yea. A merged structure if that doesn't look that bad. I see most
> of the fields in struct fsl_asrc are being reused by in fsl_easrc.
>
> > >
> > > > +static const struct regmap_config fsl_easrc_regmap_config = {
> > > > + .readable_reg = fsl_easrc_readable_reg,
> > > > + .volatile_reg = fsl_easrc_volatile_reg,
> > > > + .writeable_reg = fsl_easrc_writeable_reg,
> > >
> > > Can we use regmap_range and regmap_access_table?
> > >
> >
> > Can the regmap_range support discontinuous registers? The
> > reg_stride = 4.
>
> I think it does. Giving an example here:
> https://github.com/torvalds/linux/blob/master/drivers/mfd/da9063-i2c.c
The register in this i2c driver are continuous, from 0x00, 0x01, 0x02...
But our case is 0x00, 0x04, 0x08, does it work?
best regards
wang shengjiu
^ permalink raw reply
* RE: [PATCH v3 00/27] Add support for OpenCAPI Persistent Memory devices
From: Alastair D'Silva @ 2020-02-26 0:35 UTC (permalink / raw)
To: Dan Williams
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Paul Mackerras, Krzysztof Kozlowski, Mauro Carvalho Chehab,
Ira Weiny, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Matthew Wilcox, Anju T Sudhakar,
Mahesh Salgaonkar, Andrew Donnellan, Arnd Bergmann, Greg Kurz,
Nicholas Piggin, Cédric Le Goater, Thomas Gleixner,
Hari Bathini, Linux MM, Greg Kroah-Hartman,
Linux Kernel Mailing List, Vishal Verma, Frederic Barrat,
Oliver O'Halloran, Andrew Morton, linuxppc-dev,
David S. Miller
In-Reply-To: <CAPcyv4g_762vho=L21BuO=97zr9Cq14np88bnFieiYN25BvJtA@mail.gmail.com>
On Tue, 2020-02-25 at 16:32 -0800, Dan Williams wrote:
> On Tue, Feb 25, 2020 at 4:14 PM Alastair D'Silva <
> alastair@au1.ibm.com> wrote:
> > On Mon, 2020-02-24 at 17:51 +1100, Oliver O'Halloran wrote:
> > > On Mon, Feb 24, 2020 at 3:43 PM Alastair D'Silva <
> > > alastair@au1.ibm.com> wrote:
> > > > On Sun, 2020-02-23 at 20:37 -0800, Matthew Wilcox wrote:
> > > > > On Mon, Feb 24, 2020 at 03:34:07PM +1100, Alastair D'Silva
> > > > > wrote:
> > > > > > V3:
> > > > > > - Rebase against next/next-20200220
> > > > > > - Move driver to arch/powerpc/platforms/powernv, we now
> > > > > > expect
> > > > > > this
> > > > > > driver to go upstream via the powerpc tree
> > > > >
> > > > > That's rather the opposite direction of normal; mostly
> > > > > drivers
> > > > > live
> > > > > under
> > > > > drivers/ and not in arch/. It's easier for drivers to get
> > > > > overlooked
> > > > > when doing tree-wide changes if they're hiding.
> > > >
> > > > This is true, however, given that it was not all that desirable
> > > > to
> > > > have
> > > > it under drivers/nvdimm, it's sister driver (for the same
> > > > hardware)
> > > > is
> > > > also under arch, and that we don't expect this driver to be
> > > > used on
> > > > any
> > > > platform other than powernv, we think this was the most
> > > > reasonable
> > > > place to put it.
> > >
> > > Historically powernv specific platform drivers go in their
> > > respective
> > > subsystem trees rather than in arch/ and I'd prefer we kept it
> > > that
> > > way. When I added the papr_scm driver I put it in the pseries
> > > platform
> > > directory because most of the pseries paravirt code lives there
> > > for
> > > some reason; I don't know why. Luckily for me that followed the
> > > same
> > > model that Dan used when he put the NFIT driver in drivers/acpi/
> > > and
> > > the libnvdimm core in drivers/nvdimm/ so we didn't have anything
> > > to
> > > argue about. However, as Matthew pointed out, it is at odds with
> > > how
> > > most subsystems operate. Is there any particular reason we're
> > > doing
> > > things this way or should we think about moving libnvdimm users
> > > to
> > > drivers/nvdimm/?
> > >
> > > Oliver
> >
> > I'm not too fussed where it ends up, as long as it ends up
> > somewhere :)
> >
> > From what I can tell, the issue is that we have both
> > "infrastructure"
> > drivers, and end-device drivers. To me, it feels like
> > drivers/nvdimm
> > should contain both, and I think this feels like the right
> > approach.
> >
> > I could move it back to drivers/nvdimm/ocxl, but I felt that it was
> > only tolerated there, not desired. This could be cleared up with a
> > response from Dan Williams, and if it is indeed dersired, this is
> > my
> > preferred location.
>
> Apologies if I gave the impression it was only tolerated. I'm ok with
> drivers/nvdimm/ocxl/, and to the larger point I'd also be ok with a
> drivers/{acpi => nvdimm}/nfit and {arch/powerpc/platforms/pseries =>
> drivers/nvdimm}/papr_scm.c move as well to keep all the consumers of
> the nvdimm related code together with the core.
Great, thanks for clarifying, text is so imprecise when it comes to
nuance :)
I'll move ti back to drivers/nvdimm/ocxl then.
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply
* Re: [PATCH v3 00/27] Add support for OpenCAPI Persistent Memory devices
From: Dan Williams @ 2020-02-26 0:32 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Paul Mackerras, Krzysztof Kozlowski, Mauro Carvalho Chehab,
Ira Weiny, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Matthew Wilcox, Anju T Sudhakar,
Mahesh Salgaonkar, Andrew Donnellan, Arnd Bergmann, Greg Kurz,
Nicholas Piggin, Cédric Le Goater, Thomas Gleixner,
Hari Bathini, Linux MM, Greg Kroah-Hartman,
Linux Kernel Mailing List, Vishal Verma, Frederic Barrat,
Oliver O'Halloran, Andrew Morton, linuxppc-dev,
David S. Miller
In-Reply-To: <b981f4e6cc308a617e7944e3ce23009e804cfdbf.camel@au1.ibm.com>
On Tue, Feb 25, 2020 at 4:14 PM Alastair D'Silva <alastair@au1.ibm.com> wrote:
>
> On Mon, 2020-02-24 at 17:51 +1100, Oliver O'Halloran wrote:
> > On Mon, Feb 24, 2020 at 3:43 PM Alastair D'Silva <
> > alastair@au1.ibm.com> wrote:
> > > On Sun, 2020-02-23 at 20:37 -0800, Matthew Wilcox wrote:
> > > > On Mon, Feb 24, 2020 at 03:34:07PM +1100, Alastair D'Silva wrote:
> > > > > V3:
> > > > > - Rebase against next/next-20200220
> > > > > - Move driver to arch/powerpc/platforms/powernv, we now
> > > > > expect
> > > > > this
> > > > > driver to go upstream via the powerpc tree
> > > >
> > > > That's rather the opposite direction of normal; mostly drivers
> > > > live
> > > > under
> > > > drivers/ and not in arch/. It's easier for drivers to get
> > > > overlooked
> > > > when doing tree-wide changes if they're hiding.
> > >
> > > This is true, however, given that it was not all that desirable to
> > > have
> > > it under drivers/nvdimm, it's sister driver (for the same hardware)
> > > is
> > > also under arch, and that we don't expect this driver to be used on
> > > any
> > > platform other than powernv, we think this was the most reasonable
> > > place to put it.
> >
> > Historically powernv specific platform drivers go in their respective
> > subsystem trees rather than in arch/ and I'd prefer we kept it that
> > way. When I added the papr_scm driver I put it in the pseries
> > platform
> > directory because most of the pseries paravirt code lives there for
> > some reason; I don't know why. Luckily for me that followed the same
> > model that Dan used when he put the NFIT driver in drivers/acpi/ and
> > the libnvdimm core in drivers/nvdimm/ so we didn't have anything to
> > argue about. However, as Matthew pointed out, it is at odds with how
> > most subsystems operate. Is there any particular reason we're doing
> > things this way or should we think about moving libnvdimm users to
> > drivers/nvdimm/?
> >
> > Oliver
>
>
> I'm not too fussed where it ends up, as long as it ends up somewhere :)
>
> From what I can tell, the issue is that we have both "infrastructure"
> drivers, and end-device drivers. To me, it feels like drivers/nvdimm
> should contain both, and I think this feels like the right approach.
>
> I could move it back to drivers/nvdimm/ocxl, but I felt that it was
> only tolerated there, not desired. This could be cleared up with a
> response from Dan Williams, and if it is indeed dersired, this is my
> preferred location.
Apologies if I gave the impression it was only tolerated. I'm ok with
drivers/nvdimm/ocxl/, and to the larger point I'd also be ok with a
drivers/{acpi => nvdimm}/nfit and {arch/powerpc/platforms/pseries =>
drivers/nvdimm}/papr_scm.c move as well to keep all the consumers of
the nvdimm related code together with the core.
^ permalink raw reply
* Re: [PATCH v3 06/27] ocxl: Tally up the LPC memory on a link & allow it to be mapped
From: Alastair D'Silva @ 2020-02-26 0:29 UTC (permalink / raw)
To: Frederic Barrat
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Andrew Donnellan, Arnd Bergmann, Greg Kurz,
Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Vishal Verma, Paul Mackerras, Andrew Morton, linuxppc-dev,
David S. Miller
In-Reply-To: <4c8f704b-5607-5ca0-c00e-01e412117f6b@linux.ibm.com>
On Tue, 2020-02-25 at 17:30 +0100, Frederic Barrat wrote:
>
> Le 21/02/2020 à 04:26, Alastair D'Silva a écrit :
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > Tally up the LPC memory on an OpenCAPI link & allow it to be mapped
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> > drivers/misc/ocxl/core.c | 10 ++++++
> > drivers/misc/ocxl/link.c | 53
> > +++++++++++++++++++++++++++++++
> > drivers/misc/ocxl/ocxl_internal.h | 33 +++++++++++++++++++
> > 3 files changed, 96 insertions(+)
> >
> > diff --git a/drivers/misc/ocxl/core.c b/drivers/misc/ocxl/core.c
> > index b7a09b21ab36..2531c6cf19a0 100644
> > --- a/drivers/misc/ocxl/core.c
> > +++ b/drivers/misc/ocxl/core.c
> > @@ -230,8 +230,18 @@ static int configure_afu(struct ocxl_afu *afu,
> > u8 afu_idx, struct pci_dev *dev)
> > if (rc)
> > goto err_free_pasid;
> >
> > + if (afu->config.lpc_mem_size || afu-
> > >config.special_purpose_mem_size) {
> > + rc = ocxl_link_add_lpc_mem(afu->fn->link, afu-
> > >config.lpc_mem_offset,
> > + afu->config.lpc_mem_size +
> > + afu-
> > >config.special_purpose_mem_size);
> > + if (rc)
> > + goto err_free_mmio;
> > + }
> > +
> > return 0;
> >
> > +err_free_mmio:
> > + unmap_mmio_areas(afu);
> > err_free_pasid:
> > reclaim_afu_pasid(afu);
> > err_free_actag:
> > diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
> > index 58d111afd9f6..1e039cc5ebe5 100644
> > --- a/drivers/misc/ocxl/link.c
> > +++ b/drivers/misc/ocxl/link.c
> > @@ -84,6 +84,11 @@ struct ocxl_link {
> > int dev;
> > atomic_t irq_available;
> > struct spa *spa;
> > + struct mutex lpc_mem_lock; /* protects lpc_mem & lpc_mem_sz */
> > + u64 lpc_mem_sz; /* Total amount of LPC memory presented on the
> > link */
> > + u64 lpc_mem;
> > + int lpc_consumers;
> > +
> > void *platform_data;
> > };
> > static struct list_head links_list = LIST_HEAD_INIT(links_list);
> > @@ -396,6 +401,8 @@ static int alloc_link(struct pci_dev *dev, int
> > PE_mask, struct ocxl_link **out_l
> > if (rc)
> > goto err_spa;
> >
> > + mutex_init(&link->lpc_mem_lock);
> > +
> > /* platform specific hook */
> > rc = pnv_ocxl_spa_setup(dev, link->spa->spa_mem, PE_mask,
> > &link->platform_data);
> > @@ -711,3 +718,49 @@ void ocxl_link_free_irq(void *link_handle, int
> > hw_irq)
> > atomic_inc(&link->irq_available);
> > }
> > EXPORT_SYMBOL_GPL(ocxl_link_free_irq);
> > +
> > +int ocxl_link_add_lpc_mem(void *link_handle, u64 offset, u64 size)
> > +{
> > + struct ocxl_link *link = (struct ocxl_link *) link_handle;
> > +
> > + // Check for overflow
> > + if (offset > (offset + size))
> > + return -EINVAL;
> > +
> > + mutex_lock(&link->lpc_mem_lock);
> > + link->lpc_mem_sz = max(link->lpc_mem_sz, offset + size);
> > +
> > + mutex_unlock(&link->lpc_mem_lock);
> > +
> > + return 0;
> > +}
> > +
> > +u64 ocxl_link_lpc_map(void *link_handle, struct pci_dev *pdev)
> > +{
> > + struct ocxl_link *link = (struct ocxl_link *) link_handle;
> > +
> > + mutex_lock(&link->lpc_mem_lock);
> > +
> > + if(!link->lpc_mem)
> > + link->lpc_mem = pnv_ocxl_platform_lpc_setup(pdev, link-
> > >lpc_mem_sz);
> > +
> > + if(link->lpc_mem)
> > + link->lpc_consumers++;
> > + mutex_unlock(&link->lpc_mem_lock);
> > +
> > + return link->lpc_mem;
> > +}
> > +
> > +void ocxl_link_lpc_release(void *link_handle, struct pci_dev
> > *pdev)
> > +{
> > + struct ocxl_link *link = (struct ocxl_link *) link_handle;
> > +
> > + mutex_lock(&link->lpc_mem_lock);
> > + WARN_ON(--link->lpc_consumers < 0);
>
> Here, we always decrement the lpc_consumers count. However, it was
> only
> incremented if the mapping was setup correctly in opal.
>
> We could arguably claim that ocxl_link_lpc_release() should only be
> called if ocxl_link_lpc_map() succeeded, but it would make error
> path
> handling easier if we only decrement the lpc_consumers count if
> link->lpc_mem is set. So that we can just call
> ocxl_link_lpc_release()
> in error paths without having to worry about triggering the WARN_ON
> message.
>
> Fred
>
>
Ok, this makes sense.
>
> > + if (link->lpc_consumers == 0) {
> > + pnv_ocxl_platform_lpc_release(pdev);
> > + link->lpc_mem = 0;
> > + }
> > +
> > + mutex_unlock(&link->lpc_mem_lock);
> > +}
> > diff --git a/drivers/misc/ocxl/ocxl_internal.h
> > b/drivers/misc/ocxl/ocxl_internal.h
> > index 198e4e4bc51d..d0c8c4838f42 100644
> > --- a/drivers/misc/ocxl/ocxl_internal.h
> > +++ b/drivers/misc/ocxl/ocxl_internal.h
> > @@ -142,4 +142,37 @@ int ocxl_irq_offset_to_id(struct ocxl_context
> > *ctx, u64 offset);
> > u64 ocxl_irq_id_to_offset(struct ocxl_context *ctx, int irq_id);
> > void ocxl_afu_irq_free_all(struct ocxl_context *ctx);
> >
> > +/**
> > + * ocxl_link_add_lpc_mem() - Increment the amount of memory
> > required by an OpenCAPI link
> > + *
> > + * @link_handle: The OpenCAPI link handle
> > + * @offset: The offset of the memory to add
> > + * @size: The amount of memory to increment by
> > + *
> > + * Returns 0 on success, negative on overflow
> > + */
> > +int ocxl_link_add_lpc_mem(void *link_handle, u64 offset, u64
> > size);
> > +
> > +/**
> > + * ocxl_link_lpc_map() - Map the LPC memory for an OpenCAPI device
> > + * Since LPC memory belongs to a link, the whole LPC memory
> > available
> > + * on the link must be mapped in order to make it accessible to a
> > device.
> > + * @link_handle: The OpenCAPI link handle
> > + * @pdev: A device that is on the link
> > + *
> > + * Returns the address of the mapped LPC memory, or 0 on error
> > + */
> > +u64 ocxl_link_lpc_map(void *link_handle, struct pci_dev *pdev);
> > +
> > +/**
> > + * ocxl_link_lpc_release() - Release the LPC memory device for an
> > OpenCAPI device
> > + *
> > + * Offlines LPC memory on an OpenCAPI link for a device. If this
> > is the
> > + * last device on the link to release the memory, unmap it from
> > the link.
> > + *
> > + * @link_handle: The OpenCAPI link handle
> > + * @pdev: A device that is on the link
> > + */
> > +void ocxl_link_lpc_release(void *link_handle, struct pci_dev
> > *pdev);
> > +
> > #endif /* _OCXL_INTERNAL_H_ */
> >
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply
* Re: [PATCH v3 03/27] powerpc: Map & release OpenCAPI LPC memory
From: Alastair D'Silva @ 2020-02-26 0:19 UTC (permalink / raw)
To: Frederic Barrat
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Andrew Donnellan, Arnd Bergmann, Greg Kurz,
Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, linux-mm, Greg Kroah-Hartman, linux-kernel,
Vishal Verma, Paul Mackerras, Andrew Morton, linuxppc-dev,
David S. Miller
In-Reply-To: <69991128-3cf1-a8ba-4d9f-9ff90f1783db@linux.ibm.com>
On Tue, 2020-02-25 at 11:02 +0100, Frederic Barrat wrote:
>
> Le 21/02/2020 à 04:26, Alastair D'Silva a écrit :
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > This patch adds platform support to map & release LPC memory.
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> > arch/powerpc/include/asm/pnv-ocxl.h | 4 +++
> > arch/powerpc/platforms/powernv/ocxl.c | 43
> > +++++++++++++++++++++++++++
> > 2 files changed, 47 insertions(+)
> >
> > diff --git a/arch/powerpc/include/asm/pnv-ocxl.h
> > b/arch/powerpc/include/asm/pnv-ocxl.h
> > index 7de82647e761..0b2a6707e555 100644
> > --- a/arch/powerpc/include/asm/pnv-ocxl.h
> > +++ b/arch/powerpc/include/asm/pnv-ocxl.h
> > @@ -32,5 +32,9 @@ extern int pnv_ocxl_spa_remove_pe_from_cache(void
> > *platform_data, int pe_handle)
> >
> > extern int pnv_ocxl_alloc_xive_irq(u32 *irq, u64 *trigger_addr);
> > extern void pnv_ocxl_free_xive_irq(u32 irq);
> > +#ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
> > +u64 pnv_ocxl_platform_lpc_setup(struct pci_dev *pdev, u64 size);
> > +void pnv_ocxl_platform_lpc_release(struct pci_dev *pdev);
> > +#endif
>
> This breaks the compilation of the ocxl driver if
> CONFIG_MEMORY_HOTPLUG=n
>
> Those functions still make sense even without memory hotplug, for
> example in the context of the implementation you had to access
> opencapi
> LPC memory through mmap(). The #ifdef is really needed only around
> the
> check_hotplug_memory_addressable() call.
>
> Fred
Hmm, we do still need sparsemem though. Let me think about his some
more.
>
>
> > #endif /* _ASM_PNV_OCXL_H */
> > diff --git a/arch/powerpc/platforms/powernv/ocxl.c
> > b/arch/powerpc/platforms/powernv/ocxl.c
> > index 8c65aacda9c8..f2edbcc67361 100644
> > --- a/arch/powerpc/platforms/powernv/ocxl.c
> > +++ b/arch/powerpc/platforms/powernv/ocxl.c
> > @@ -475,6 +475,49 @@ void pnv_ocxl_spa_release(void *platform_data)
> > }
> > EXPORT_SYMBOL_GPL(pnv_ocxl_spa_release);
> >
> > +#ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
> > +u64 pnv_ocxl_platform_lpc_setup(struct pci_dev *pdev, u64 size)
> > +{
> > + struct pci_controller *hose = pci_bus_to_host(pdev->bus);
> > + struct pnv_phb *phb = hose->private_data;
> > + u32 bdfn = pci_dev_id(pdev);
> > + __be64 base_addr_be64;
> > + u64 base_addr;
> > + int rc;
> > +
> > + rc = opal_npu_mem_alloc(phb->opal_id, bdfn, size,
> > &base_addr_be64);
> > + if (rc) {
> > + dev_warn(&pdev->dev,
> > + "OPAL could not allocate LPC memory, rc=%d\n",
> > rc);
> > + return 0;
> > + }
> > +
> > + base_addr = be64_to_cpu(base_addr_be64);
> > +
> > + rc = check_hotplug_memory_addressable(base_addr >> PAGE_SHIFT,
> > + size >> PAGE_SHIFT);
> > + if (rc)
> > + return 0;
> > +
> > + return base_addr;
> > +}
> > +EXPORT_SYMBOL_GPL(pnv_ocxl_platform_lpc_setup);
> > +
> > +void pnv_ocxl_platform_lpc_release(struct pci_dev *pdev)
> > +{
> > + struct pci_controller *hose = pci_bus_to_host(pdev->bus);
> > + struct pnv_phb *phb = hose->private_data;
> > + u32 bdfn = pci_dev_id(pdev);
> > + int rc;
> > +
> > + rc = opal_npu_mem_release(phb->opal_id, bdfn);
> > + if (rc)
> > + dev_warn(&pdev->dev,
> > + "OPAL reported rc=%d when releasing LPC
> > memory\n", rc);
> > +}
> > +EXPORT_SYMBOL_GPL(pnv_ocxl_platform_lpc_release);
> > +#endif
> > +
> > int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int
> > pe_handle)
> > {
> > struct spa_data *data = (struct spa_data *) platform_data;
> >
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply
* RE: [PATCH v3 00/27] Add support for OpenCAPI Persistent Memory devices
From: Alastair D'Silva @ 2020-02-26 0:13 UTC (permalink / raw)
To: Oliver O'Halloran
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Paul Mackerras, Krzysztof Kozlowski, Mauro Carvalho Chehab,
Ira Weiny, Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Matthew Wilcox, Anju T Sudhakar,
Mahesh Salgaonkar, Andrew Donnellan, Arnd Bergmann, Greg Kurz,
Nicholas Piggin, Cédric Le Goater, Dan Williams,
Hari Bathini, Linux MM, Greg Kroah-Hartman,
Linux Kernel Mailing List, Vishal Verma, Frederic Barrat,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <CAOSf1CHYEJf02EV0kYMk+D9s=4PiTXSM1eFcRGYe7XJrHvtAtA@mail.gmail.com>
On Mon, 2020-02-24 at 17:51 +1100, Oliver O'Halloran wrote:
> On Mon, Feb 24, 2020 at 3:43 PM Alastair D'Silva <
> alastair@au1.ibm.com> wrote:
> > On Sun, 2020-02-23 at 20:37 -0800, Matthew Wilcox wrote:
> > > On Mon, Feb 24, 2020 at 03:34:07PM +1100, Alastair D'Silva wrote:
> > > > V3:
> > > > - Rebase against next/next-20200220
> > > > - Move driver to arch/powerpc/platforms/powernv, we now
> > > > expect
> > > > this
> > > > driver to go upstream via the powerpc tree
> > >
> > > That's rather the opposite direction of normal; mostly drivers
> > > live
> > > under
> > > drivers/ and not in arch/. It's easier for drivers to get
> > > overlooked
> > > when doing tree-wide changes if they're hiding.
> >
> > This is true, however, given that it was not all that desirable to
> > have
> > it under drivers/nvdimm, it's sister driver (for the same hardware)
> > is
> > also under arch, and that we don't expect this driver to be used on
> > any
> > platform other than powernv, we think this was the most reasonable
> > place to put it.
>
> Historically powernv specific platform drivers go in their respective
> subsystem trees rather than in arch/ and I'd prefer we kept it that
> way. When I added the papr_scm driver I put it in the pseries
> platform
> directory because most of the pseries paravirt code lives there for
> some reason; I don't know why. Luckily for me that followed the same
> model that Dan used when he put the NFIT driver in drivers/acpi/ and
> the libnvdimm core in drivers/nvdimm/ so we didn't have anything to
> argue about. However, as Matthew pointed out, it is at odds with how
> most subsystems operate. Is there any particular reason we're doing
> things this way or should we think about moving libnvdimm users to
> drivers/nvdimm/?
>
> Oliver
I'm not too fussed where it ends up, as long as it ends up somewhere :)
From what I can tell, the issue is that we have both "infrastructure"
drivers, and end-device drivers. To me, it feels like drivers/nvdimm
should contain both, and I think this feels like the right approach.
I could move it back to drivers/nvdimm/ocxl, but I felt that it was
only tolerated there, not desired. This could be cleared up with a
response from Dan Williams, and if it is indeed dersired, this is my
preferred location.
I think a case could also be made for drivers/ocxl, simply because we
don't expect more than a handful of drivers to ever live there (I
expect most users will drive their devices from userspace via libocxl).
In defence of keeping it in arch/powerpc/powernv, I highly doubt this
driver will end up being used on any platform other than this. Even
though OpenCAPI was engineered as an open standard, there is some
competition from industry giants with a competing standard on a much
more popular platform.
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply
* Re: MCE handler gets NIP wrong on MPC8378
From: Radu Rendec @ 2020-02-26 0:01 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev
In-Reply-To: <CAD5jUk-WzPLYSAxDuWFa3fWcZpT97suySVDEBvUn7V+N01bzTw@mail.gmail.com>
On 02/20/2020 at 12:48 PM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Le 20/02/2020 à 18:34, Radu Rendec a écrit :
> > On 02/20/2020 at 11:25 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >> Le 20/02/2020 à 17:02, Radu Rendec a écrit :
> >>> On 02/20/2020 at 3:38 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >>>> On 02/19/2020 10:39 PM, Radu Rendec wrote:
> >>>>> On 02/19/2020 at 4:21 PM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >>>>>>> Interesting.
> >>>>>>>
> >>>>>>> 0x900 is the adress of the timer interrupt.
> >>>>>>>
> >>>>>>> Would the MCE occur just after the timer interrupt ?
> >>>>>
> >>>>> I doubt that. I'm using a small test module to artificially trigger the
> >>>>> MCE. Basically it's just this (the full code is in my original post):
> >>>>>
> >>>>> bad_addr_base = ioremap(0xf0000000, 0x100);
> >>>>> x = ioread32(bad_addr_base);
> >>>>>
> >>>>> I find it hard to believe that every time I load the module the lwbrx
> >>>>> instruction that triggers the MCE is executed exactly after the timer
> >>>>> interrupt (or that the timer interrupt always occurs close to the lwbrx
> >>>>> instruction).
> >>>>
> >>>> Can you try to see how much time there is between your read and the MCE ?
> >>>> The below should allow it, you'll see first value in r13 and the other
> >>>> in r14 (mce.c is your test code)
> >>>>
> >>>> Also provide the timebase frequency as reported in /proc/cpuinfo
> >>>
> >>> I just ran a test: r13 is 0xda8e0f91 and r14 is 0xdaae0f9c.
> >>>
> >>> # cat /proc/cpuinfo
> >>> processor : 0
> >>> cpu : e300c4
> >>> clock : 800.000004MHz
> >>> revision : 1.1 (pvr 8086 1011)
> >>> bogomips : 200.00
> >>> timebase : 100000000
> >>>
> >>> The difference between r14 and r13 is 0x20000b. Assuming TB is
> >>> incremented with 'timebase' frequency, that means 20.97 milliseconds
> >>> (although the e300 manual says TB is "incremented once every four core
> >>> input clock cycles").
> >>
> >> I wouldn't be surprised that the internal CPU clock be twice the input
> >> clock.
> >>
> >> So that's long enough to surely get a timer interrupt during every bad
> >> access.
> >>
> >> Now we have to understand why SRR1 contains the address of the timer
> >> exception entry and not the address of the bad access.
> >>
> >> The value of SRR1 confirms that it comes from 0x900 as MSR[IR] and [DR]
> >> are cleared when interrupts are enabled.
> >>
> >> Maybe you should file a support case at NXP. They are usually quite
> >> professionnal at responding.
> >
> > I already did (quite some time ago), but it started off as "why does the
> > MCE occur in the first place". That part has already been figured out,
> > but unfortunately I don't have a viable solution to it. Like you said,
> > now the focus has shifted to understanding why the SRR0 value is not
> > what we expect.
>
> Yes now the point is to understand why it starts processing the timer
> interrupt at 0x900 (with IR and DR cleared as observed in SRR1) just
> before taking the Machine Check.
>
> Allthough the execution of the decrementer interrupt is queue for after
> the completion of the failing memory access, I'd expect the Machine
> Check to take priority.
>
> Note that I have never observed such a behaviour on MPC8321 which has an
> e300c2 core.
I apologize for the silence during the past few days, I've been diverted
with something else. This is the feedback that I got from NXP:
| The e300 core uses SRR0/1 for both non-critical interrupts and machine
| check interrupts and if they happen simultaneously a problem can occur
| where the return address from the first exception is lost when handling
| the second exception concurrently. This only occurs in the rare case
| when the software ISR hasn't had the time to save SRR0/1 to the sw stack.
|
| If the ability to nest interrupts is desired, software then saves off
| enough state (i.e. the contents of SRR0, SRR1, etc) that will allow it
| to recover (i.e. resume handling the current interrupt) if another
| interrupt occurs.
So basically what they describe is a race condition between the MCE and
a regular interrupt, where the regular interrupt (the timer interrupt,
in our case) kicks in after the MCE handler is entered into but before
it saves SRR0. This not only requires very precise timing, but would
also end up with a saved SRR0 value that points back somewhere inside
the MCE handler.
But I've thought about something else. We already timed it and we know
it consistently takes around 20 ms between the faulty read and the MCE
handler execution. I'm thinking that the faulty read is essentially a
failed transaction on the internal bus, because no peripheral replies
to the access on the bad address. The 20 ms is probably the bus timeout.
How does this scenario look to you?
- The faulty read starts to execute. A new internal bus transaction is
started, the bad address is put on the bus and the CPU waits for a
peripheral to reply.
- The timer interrupt kicks in. The CPU saves NIP to SRR0 and NIP
becomes 0x900. But the CPU cannot start executing immediately from
address 0x900 because the bus is blocked.
- Nobody replies and eventually the bus transaction fails. An MCE is
triggered to handle the failed bus transaction.
- The MCE has higher priority than the timer interrupt, so it's handled
immediately. The CPU saves NIP to SRR0 and NIP becomes 0x200.
- The CPU starts executing the MCE handler with 0x900 in SRR0.
This is pure speculation and I have absolutely no idea about the e300
core internal architecture. But it's my best guess. I've sent something
similar to NXP support. Let's see what they come up with.
By the way, I have successfully tested a fix that uses __do_inl instead
of ioread32 and disables interrupts around the __do_inl call. If I was
even close with my speculation above, then I guess the only thing we
could fix in the kernel would be to modify __do_inl and co. to disable
interrupts around the potentially dangerous access. The benefit would be
that the MCE could be recovered from. For ioread32, there is no real
benefit in doing that (other than printing the correct NIP address in
the crash dump) because it doesn't instrument the exception tables
anyway so it's non-recoverable.
Best regards,
Radu
^ permalink raw reply
* Re: [PATCH v3 26/32] powerpc/64: system call zero volatile registers when returning
From: Segher Boessenkool @ 2020-02-25 21:20 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: Michal Suchanek, linuxppc-dev
In-Reply-To: <20200225173541.1549955-27-npiggin@gmail.com>
Hi!
On Wed, Feb 26, 2020 at 03:35:35AM +1000, Nicholas Piggin wrote:
> Kernel addresses and potentially other sensitive data could be leaked
> in volatile registers after a syscall.
> cmpdi r3,0
> bne .Lsyscall_restore_regs
> + li r0,0
> + li r4,0
> + li r5,0
> + li r6,0
> + li r7,0
> + li r8,0
> + li r9,0
> + li r10,0
> + li r11,0
> + li r12,0
> + mtctr r0
> + mtspr SPRN_XER,r0
> .Lsyscall_restore_regs_cont:
What about LR? Is that taken care of later?
This also deserves a big fat comment imo, it is very important after
all, and not so obvious.
Segher
^ permalink raw reply
* Re: [PATCH] evh_bytechan: fix out of bounds accesses
From: Stephen Rothwell @ 2020-02-25 20:56 UTC (permalink / raw)
To: Laurentiu Tudor
Cc: Timur Tabi, b08248, Greg Kroah-Hartman, Jiri Slaby, york sun,
PowerPC Mailing List, Scott Wood
In-Reply-To: <37b145e2-a953-c0e6-f0fa-7ef420edfd16@nxp.com>
[-- Attachment #1: Type: text/plain, Size: 2090 bytes --]
Hi Laurentiu,
On Tue, 25 Feb 2020 11:54:17 +0200 Laurentiu Tudor <laurentiu.tudor@nxp.com> wrote:
>
> On 21.02.2020 01:57, Stephen Rothwell wrote:
> >
> > On Thu, 16 Jan 2020 11:37:14 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >>
> >> On Wed, 15 Jan 2020 14:01:35 -0600 Scott Wood <swood@redhat.com> wrote:
> >>>
> >>> On Thu, 2020-01-16 at 06:42 +1100, Stephen Rothwell wrote:
> >>>>
> >>>> On Wed, 15 Jan 2020 07:25:45 -0600 Timur Tabi <timur@kernel.org> wrote:
> >>>>> On 1/14/20 12:31 AM, Stephen Rothwell wrote:
> >>>>>> +/**
> >>>>>> + * ev_byte_channel_send - send characters to a byte stream
> >>>>>> + * @handle: byte stream handle
> >>>>>> + * @count: (input) num of chars to send, (output) num chars sent
> >>>>>> + * @bp: pointer to chars to send
> >>>>>> + *
> >>>>>> + * Returns 0 for success, or an error code.
> >>>>>> + */
> >>>>>> +static unsigned int ev_byte_channel_send(unsigned int handle,
> >>>>>> + unsigned int *count, const char *bp)
> >>>>>
> >>>>> Well, now you've moved this into the .c file and it is no longer
> >>>>> available to other callers. Anything wrong with keeping it in the .h
> >>>>> file?
> >>>>
> >>>> There are currently no other callers - are there likely to be in the
> >>>> future? Even if there are, is it time critical enough that it needs to
> >>>> be inlined everywhere?
> >>>
> >>> It's not performance critical and there aren't likely to be other users --
> >>> just a matter of what's cleaner. FWIW I'd rather see the original patch,
> >>> that keeps the raw asm hcall stuff as simple wrappers in one place.
> >>
> >> And I don't mind either way :-)
> >>
> >> I just want to get rid of the warnings.
> >
> > Any progress with this?
>
> I think that the consensus was to pick up the original patch that is,
> this one: https://patchwork.ozlabs.org/patch/1220186/
>
> I've tested it too, so please feel free to add a:
>
> Tested-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
So, whose tree should his go via?
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH] macintosh: therm_windtunnel: fix regression when instantiating devices
From: Wolfram Sang @ 2020-02-25 15:00 UTC (permalink / raw)
To: John Paul Adrian Glaubitz
Cc: Erhard Furtner, Mathieu Malaterre, debian-powerpc, linux-i2c,
linuxppc-dev
In-Reply-To: <0fe4740a-f331-f885-c60a-6735c4c8e1fa@physik.fu-berlin.de>
[-- Attachment #1: Type: text/plain, Size: 373 bytes --]
On Tue, Feb 25, 2020 at 03:41:22PM +0100, John Paul Adrian Glaubitz wrote:
> Hello!
>
> On 2/25/20 3:12 PM, Wolfram Sang wrote:
> > Adding the Debian-PPC List to reach further people maybe willing to
> > test.
>
> This might be related [1].
IIUC, this is the same as
https://bugzilla.kernel.org/show_bug.cgi?id=199471.
I don't think my patch helps here.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox