* [PATCH v3 2/4] powerpc/sstep: support emulation for vsx vector paired storage access instructions
From: Balamuruhan S @ 2020-07-31 8:16 UTC (permalink / raw)
To: mpe
Cc: ravi.bangoria, Balamuruhan S, paulus, sandipan, naveen.n.rao,
linuxppc-dev
In-Reply-To: <20200731081637.1837559-1-bala24@linux.ibm.com>
add emulate_step() changes to support vsx vector paired storage
access instructions that provides octword operands loads/stores
between storage and set of 64 Vector Scalar Registers (VSRs).
Suggested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Suggested-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
arch/powerpc/lib/sstep.c | 77 +++++++++++++++++++++++++++++++---------
1 file changed, 60 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 22147257d74d..01e1a3adc406 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -280,6 +280,19 @@ static nokprobe_inline void do_byte_reverse(void *ptr, int nb)
up[1] = tmp;
break;
}
+ case 32: {
+ unsigned long *up = (unsigned long *)ptr;
+ unsigned long tmp;
+
+ tmp = byterev_8(up[0]);
+ up[0] = byterev_8(up[3]);
+ up[3] = tmp;
+ tmp = byterev_8(up[2]);
+ up[2] = byterev_8(up[1]);
+ up[1] = tmp;
+ break;
+ }
+
#endif
default:
WARN_ON_ONCE(1);
@@ -710,6 +723,8 @@ void emulate_vsx_load(struct instruction_op *op, union vsx_reg *reg,
reg->d[0] = reg->d[1] = 0;
switch (op->element_size) {
+ case 32:
+ /* [p]lxvp[x] */
case 16:
/* whole vector; lxv[x] or lxvl[l] */
if (size == 0)
@@ -718,7 +733,7 @@ void emulate_vsx_load(struct instruction_op *op, union vsx_reg *reg,
if (IS_LE && (op->vsx_flags & VSX_LDLEFT))
rev = !rev;
if (rev)
- do_byte_reverse(reg, 16);
+ do_byte_reverse(reg, size);
break;
case 8:
/* scalar loads, lxvd2x, lxvdsx */
@@ -794,6 +809,20 @@ void emulate_vsx_store(struct instruction_op *op, const union vsx_reg *reg,
size = GETSIZE(op->type);
switch (op->element_size) {
+ case 32:
+ /* [p]stxvp[x] */
+ if (size == 0)
+ break;
+ if (rev) {
+ /* reverse 32 bytes */
+ buf.d[0] = byterev_8(reg->d[3]);
+ buf.d[1] = byterev_8(reg->d[2]);
+ buf.d[2] = byterev_8(reg->d[1]);
+ buf.d[3] = byterev_8(reg->d[0]);
+ reg = &buf;
+ }
+ memcpy(mem, reg, size);
+ break;
case 16:
/* stxv, stxvx, stxvl, stxvll */
if (size == 0)
@@ -862,28 +891,35 @@ static nokprobe_inline int do_vsx_load(struct instruction_op *op,
bool cross_endian)
{
int reg = op->reg;
- u8 mem[16];
- union vsx_reg buf;
+ int i, nr_vsx_regs;
+ u8 mem[32];
+ union vsx_reg buf[2];
int size = GETSIZE(op->type);
if (!address_ok(regs, ea, size) || copy_mem_in(mem, ea, size, regs))
return -EFAULT;
- emulate_vsx_load(op, &buf, mem, cross_endian);
+ nr_vsx_regs = size / sizeof(__vector128);
+ emulate_vsx_load(op, buf, mem, cross_endian);
preempt_disable();
if (reg < 32) {
/* FP regs + extensions */
if (regs->msr & MSR_FP) {
- load_vsrn(reg, &buf);
+ for (i = 0; i < nr_vsx_regs; i++)
+ load_vsrn(reg + i, &buf[i].v);
} else {
- current->thread.fp_state.fpr[reg][0] = buf.d[0];
- current->thread.fp_state.fpr[reg][1] = buf.d[1];
+ for (i = 0; i < nr_vsx_regs; i++) {
+ current->thread.fp_state.fpr[reg + i][0] = buf[i].d[0];
+ current->thread.fp_state.fpr[reg + i][1] = buf[i].d[1];
+ }
}
} else {
if (regs->msr & MSR_VEC)
- load_vsrn(reg, &buf);
+ for (i = 0; i < nr_vsx_regs; i++)
+ load_vsrn(reg + i, &buf[i].v);
else
- current->thread.vr_state.vr[reg - 32] = buf.v;
+ for (i = 0; i < nr_vsx_regs; i++)
+ current->thread.vr_state.vr[reg - 32 + i] = buf[i].v;
}
preempt_enable();
return 0;
@@ -894,30 +930,37 @@ static nokprobe_inline int do_vsx_store(struct instruction_op *op,
bool cross_endian)
{
int reg = op->reg;
- u8 mem[16];
- union vsx_reg buf;
+ int i, nr_vsx_regs;
+ u8 mem[32];
+ union vsx_reg buf[2];
int size = GETSIZE(op->type);
if (!address_ok(regs, ea, size))
return -EFAULT;
+ nr_vsx_regs = size / sizeof(__vector128);
preempt_disable();
if (reg < 32) {
/* FP regs + extensions */
if (regs->msr & MSR_FP) {
- store_vsrn(reg, &buf);
+ for (i = 0; i < nr_vsx_regs; i++)
+ store_vsrn(reg + i, &buf[i].v);
} else {
- buf.d[0] = current->thread.fp_state.fpr[reg][0];
- buf.d[1] = current->thread.fp_state.fpr[reg][1];
+ for (i = 0; i < nr_vsx_regs; i++) {
+ buf[i].d[0] = current->thread.fp_state.fpr[reg + i][0];
+ buf[i].d[1] = current->thread.fp_state.fpr[reg + i][1];
+ }
}
} else {
if (regs->msr & MSR_VEC)
- store_vsrn(reg, &buf);
+ for (i = 0; i < nr_vsx_regs; i++)
+ store_vsrn(reg + i, &buf[i].v);
else
- buf.v = current->thread.vr_state.vr[reg - 32];
+ for (i = 0; i < nr_vsx_regs; i++)
+ buf[i].v = current->thread.vr_state.vr[reg - 32 + i];
}
preempt_enable();
- emulate_vsx_store(op, &buf, mem, cross_endian);
+ emulate_vsx_store(op, buf, mem, cross_endian);
return copy_mem_out(mem, ea, size, regs);
}
#endif /* CONFIG_VSX */
--
2.24.1
^ permalink raw reply related
* [PATCH v3 3/4] powerpc ppc-opcode: add encoding macros for vsx vector paired instructions
From: Balamuruhan S @ 2020-07-31 8:16 UTC (permalink / raw)
To: mpe
Cc: ravi.bangoria, Balamuruhan S, paulus, sandipan, naveen.n.rao,
linuxppc-dev
In-Reply-To: <20200731081637.1837559-1-bala24@linux.ibm.com>
add instruction encoding, extended opcodes, regs and DQ immediate macro
for new vsx vector paired instructions,
* Load VSX Vector Paired (lxvp)
* Load VSX Vector Paired Indexed (lxvpx)
* Prefixed Load VSX Vector Paired (plxvp)
* Store VSX Vector Paired (stxvp)
* Store VSX Vector Paired Indexed (stxvpx)
* Prefixed Store VSX Vector Paired (pstxvp)
Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
arch/powerpc/include/asm/ppc-opcode.h | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index 4c0bdafb6a7b..6ad23f47d06a 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -78,6 +78,7 @@
#define IMM_L(i) ((uintptr_t)(i) & 0xffff)
#define IMM_DS(i) ((uintptr_t)(i) & 0xfffc)
+#define IMM_DQ(i) (((uintptr_t)(i) & 0xfff) << 4)
/*
* 16-bit immediate helper macros: HA() is for use with sign-extending instrs
@@ -272,6 +273,8 @@
#define PPC_INST_STFD 0xd8000000
#define PPC_PREFIX_MLS 0x06000000
#define PPC_PREFIX_8LS 0x04000000
+#define PPC_PLXVP_EX_OP 0xe8000000
+#define PPC_PSTXVP_EX_OP 0xf8000000
/* Prefixed instructions */
#define PPC_INST_PLD 0xe4000000
@@ -296,6 +299,8 @@
#define __PPC_XS(s) ((((s) & 0x1f) << 21) | (((s) & 0x20) >> 5))
#define __PPC_XT(s) __PPC_XS(s)
#define __PPC_T_TLB(t) (((t) & 0x3) << 21)
+#define __PPC_TP(tp) (((tp) & 0xf) << 22)
+#define __PPC_TX(tx) (((tx) & 0x1) << 21)
#define __PPC_WC(w) (((w) & 0x3) << 21)
#define __PPC_WS(w) (((w) & 0x1f) << 11)
#define __PPC_SH(s) __PPC_WS(s)
@@ -387,6 +392,18 @@
#define PPC_RAW_STXVD2X(s, a, b) (0x7c000798 | VSX_XX1((s), a, b))
#define PPC_RAW_LXVD2X(s, a, b) (0x7c000698 | VSX_XX1((s), a, b))
#define PPC_RAW_MFVRD(a, t) (0x7c000066 | VSX_XX1((t) + 32, a, R0))
+#define PPC_LXVP(tp, tx, a, i) \
+ (0x18000000 | __PPC_TP(tp) | __PPC_TX(tx) | ___PPC_RA(a) | IMM_DQ(i))
+#define PPC_STXVP(sp, sx, a, i) \
+ (0x18000001 | __PPC_TP(sp) | __PPC_TX(sx) | ___PPC_RA(a) | IMM_DQ(i) | 0x1)
+#define PPC_LXVPX(tp, tx, a, b) \
+ (0x7c00029a | __PPC_TP(tp) | __PPC_TX(tx) | ___PPC_RA(a) | ___PPC_RB(b))
+#define PPC_STXVPX(sp, sx, a, b) \
+ (0x7c00039a | __PPC_TP(sp) | __PPC_TX(sx) | ___PPC_RA(a) | ___PPC_RB(b))
+#define PPC_PLXVP(a, i, pr, tp, tx) \
+ ((PPC_PREFIX_8LS | __PPC_PRFX_R(pr) | IMM_H(i)) << 32 | (PPC_PLXVP_EX_OP | __PPC_TP(tp) | __PPC_TX(tx) | ___PPC_RA(a) | IMM_L(i)))
+#define PPC_PSTXVP(a, i, pr, sp, sx) \
+ ((PPC_PREFIX_8LS | __PPC_PRFX_R(pr) | IMM_H(i)) << 32 | (PPC_PSTXVP_EX_OP | __PPC_TP(sp) | __PPC_TX(sx) | ___PPC_RA(a) | IMM_L(i)))
#define PPC_RAW_MTVRD(t, a) (0x7c000166 | VSX_XX1((t) + 32, a, R0))
#define PPC_RAW_VPMSUMW(t, a, b) (0x10000488 | VSX_XX3((t), a, b))
#define PPC_RAW_VPMSUMD(t, a, b) (0x100004c8 | VSX_XX3((t), a, b))
--
2.24.1
^ permalink raw reply related
* [PATCH v3 4/4] powerpc sstep: add testcases for vsx load/store instructions
From: Balamuruhan S @ 2020-07-31 8:16 UTC (permalink / raw)
To: mpe
Cc: ravi.bangoria, Balamuruhan S, paulus, sandipan, naveen.n.rao,
linuxppc-dev
In-Reply-To: <20200731081637.1837559-1-bala24@linux.ibm.com>
add testcases for vsx load/store vector paired instructions,
* Load VSX Vector Paired (lxvp)
* Load VSX Vector Paired Indexed (lxvpx)
* Prefixed Load VSX Vector Paired (plxvp)
* Store VSX Vector Paired (stxvp)
* Store VSX Vector Paired Indexed (stxvpx)
* Prefixed Store VSX Vector Paired (pstxvp)
Suggested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
arch/powerpc/lib/test_emulate_step.c | 252 +++++++++++++++++++++++++++
1 file changed, 252 insertions(+)
diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
index d242e9f72e0c..f16934b80511 100644
--- a/arch/powerpc/lib/test_emulate_step.c
+++ b/arch/powerpc/lib/test_emulate_step.c
@@ -612,6 +612,255 @@ static void __init test_lxvd2x_stxvd2x(void)
}
#endif /* CONFIG_VSX */
+#ifdef CONFIG_VSX
+static void __init test_lxvp_stxvp(void)
+{
+ struct pt_regs regs;
+ union {
+ vector128 a;
+ u32 b[4];
+ } c[2];
+ u32 cached_b[8];
+ int stepped = -1;
+
+ init_pt_regs(®s);
+
+ /*** lxvp ***/
+
+ cached_b[0] = c[0].b[0] = 18233;
+ cached_b[1] = c[0].b[1] = 34863571;
+ cached_b[2] = c[0].b[2] = 834;
+ cached_b[3] = c[0].b[3] = 6138911;
+ cached_b[4] = c[1].b[0] = 1234;
+ cached_b[5] = c[1].b[1] = 5678;
+ cached_b[6] = c[1].b[2] = 91011;
+ cached_b[7] = c[1].b[3] = 121314;
+
+ regs.gpr[4] = (unsigned long)&c[0].a;
+
+ /*
+ * lxvp XTp,DQ(RA)
+ * XTp = 32×TX + 2×Tp
+ * let TX=1 Tp=1 RA=4 DQ=0
+ */
+ stepped = emulate_step(®s, ppc_inst(PPC_LXVP(1, 1, 4, 0)));
+
+ if (stepped == 1 && cpu_has_feature(CPU_FTR_VSX)) {
+ show_result("lxvp", "PASS");
+ } else {
+ if (!cpu_has_feature(CPU_FTR_VSX))
+ show_result("lxvp", "PASS (!CPU_FTR_VSX)");
+ else
+ show_result("lxvp", "FAIL");
+ }
+
+ /*** stxvp ***/
+
+ c[0].b[0] = 21379463;
+ c[0].b[1] = 87;
+ c[0].b[2] = 374234;
+ c[0].b[3] = 4;
+ c[1].b[0] = 90;
+ c[1].b[1] = 122;
+ c[1].b[2] = 555;
+ c[1].b[3] = 32144;
+
+ /*
+ * stxvp XSp,DQ(RA)
+ * XSp = 32×SX + 2×Sp
+ * let SX=1 Sp=1 RA=4 DQ=0
+ */
+ stepped = emulate_step(®s, ppc_inst(PPC_STXVP(1, 1, 4, 0)));
+
+ if (stepped == 1 && cached_b[0] == c[0].b[0] && cached_b[1] == c[0].b[1] &&
+ cached_b[2] == c[0].b[2] && cached_b[3] == c[0].b[3] &&
+ cached_b[4] == c[1].b[0] && cached_b[5] == c[1].b[1] &&
+ cached_b[6] == c[1].b[2] && cached_b[7] == c[1].b[3] &&
+ cpu_has_feature(CPU_FTR_VSX)) {
+ show_result("stxvp", "PASS");
+ } else {
+ if (!cpu_has_feature(CPU_FTR_VSX))
+ show_result("stxvp", "PASS (!CPU_FTR_VSX)");
+ else
+ show_result("stxvp", "FAIL");
+ }
+}
+#else
+static void __init test_lxvp_stxvp(void)
+{
+ show_result("lxvp", "SKIP (CONFIG_VSX is not set)");
+ show_result("stxvp", "SKIP (CONFIG_VSX is not set)");
+}
+#endif /* CONFIG_VSX */
+
+#ifdef CONFIG_VSX
+static void __init test_lxvpx_stxvpx(void)
+{
+ struct pt_regs regs;
+ union {
+ vector128 a;
+ u32 b[4];
+ } c[2];
+ u32 cached_b[8];
+ int stepped = -1;
+
+ init_pt_regs(®s);
+
+ /*** lxvpx ***/
+
+ cached_b[0] = c[0].b[0] = 18233;
+ cached_b[1] = c[0].b[1] = 34863571;
+ cached_b[2] = c[0].b[2] = 834;
+ cached_b[3] = c[0].b[3] = 6138911;
+ cached_b[4] = c[1].b[0] = 1234;
+ cached_b[5] = c[1].b[1] = 5678;
+ cached_b[6] = c[1].b[2] = 91011;
+ cached_b[7] = c[1].b[3] = 121314;
+
+ regs.gpr[3] = (unsigned long)&c[0].a;
+ regs.gpr[4] = 0;
+
+ /*
+ * lxvpx XTp,RA,RB
+ * XTp = 32×TX + 2×Tp
+ * let TX=1 Tp=1 RA=3 RB=4
+ */
+ stepped = emulate_step(®s, ppc_inst(PPC_LXVPX(1, 1, 3, 4)));
+
+ if (stepped == 1 && cpu_has_feature(CPU_FTR_VSX)) {
+ show_result("lxvpx", "PASS");
+ } else {
+ if (!cpu_has_feature(CPU_FTR_VSX))
+ show_result("lxvpx", "PASS (!CPU_FTR_VSX)");
+ else
+ show_result("lxvpx", "FAIL");
+ }
+
+ /*** stxvpx ***/
+
+ c[0].b[0] = 21379463;
+ c[0].b[1] = 87;
+ c[0].b[2] = 374234;
+ c[0].b[3] = 4;
+ c[1].b[0] = 90;
+ c[1].b[1] = 122;
+ c[1].b[2] = 555;
+ c[1].b[3] = 32144;
+
+ /*
+ * stxvpx XSp,RA,RB
+ * XSp = 32×SX + 2×Sp
+ * let SX=1 Sp=1 RA=3 RB=4
+ */
+ stepped = emulate_step(®s, ppc_inst(PPC_STXVPX(1, 1, 3, 4)));
+
+ if (stepped == 1 && cached_b[0] == c[0].b[0] && cached_b[1] == c[0].b[1] &&
+ cached_b[2] == c[0].b[2] && cached_b[3] == c[0].b[3] &&
+ cached_b[4] == c[1].b[0] && cached_b[5] == c[1].b[1] &&
+ cached_b[6] == c[1].b[2] && cached_b[7] == c[1].b[3] &&
+ cpu_has_feature(CPU_FTR_VSX)) {
+ show_result("stxvpx", "PASS");
+ } else {
+ if (!cpu_has_feature(CPU_FTR_VSX))
+ show_result("stxvpx", "PASS (!CPU_FTR_VSX)");
+ else
+ show_result("stxvpx", "FAIL");
+ }
+}
+#else
+static void __init test_lxvpx_stxvpx(void)
+{
+ show_result("lxvpx", "SKIP (CONFIG_VSX is not set)");
+ show_result("stxvpx", "SKIP (CONFIG_VSX is not set)");
+}
+#endif /* CONFIG_VSX */
+
+#ifdef CONFIG_VSX
+static void __init test_plxvp_pstxvp(void)
+{
+ struct ppc_inst instr;
+ struct pt_regs regs;
+ union {
+ vector128 a;
+ u32 b[4];
+ } c[2];
+ u32 cached_b[8];
+ int stepped = -1;
+
+ /*
+ * plxvp XTp,D(RA),R
+ * XSp = 32×SX + 2×Sp
+ * let RA=3 R=0 D=d0||d1=0 R=0 Sp=1 SX=1
+ */
+ instr = ppc_inst_prefix(PPC_PLXVP(3, 0, 0, 1, 1) >> 32,
+ PPC_PLXVP(3, 0, 0, 1, 1) & 0xffffffff);
+
+ /*** plxvpx ***/
+
+ cached_b[0] = c[0].b[0] = 18233;
+ cached_b[1] = c[0].b[1] = 34863571;
+ cached_b[2] = c[0].b[2] = 834;
+ cached_b[3] = c[0].b[3] = 6138911;
+ cached_b[4] = c[1].b[0] = 1234;
+ cached_b[5] = c[1].b[1] = 5678;
+ cached_b[6] = c[1].b[2] = 91011;
+ cached_b[7] = c[1].b[3] = 121314;
+
+ init_pt_regs(®s);
+ regs.gpr[3] = (unsigned long)&c[0].a;
+
+ stepped = emulate_step(®s, instr);
+ if (stepped == 1 && cpu_has_feature(CPU_FTR_VSX)) {
+ show_result("plxvpx", "PASS");
+ } else {
+ if (!cpu_has_feature(CPU_FTR_VSX))
+ show_result("plxvpx", "PASS (!CPU_FTR_VSX)");
+ else
+ show_result("plxvpx", "FAIL");
+ }
+
+ /*** pstxvpx ***/
+
+ c[0].b[0] = 21379463;
+ c[0].b[1] = 87;
+ c[0].b[2] = 374234;
+ c[0].b[3] = 4;
+ c[1].b[0] = 90;
+ c[1].b[1] = 122;
+ c[1].b[2] = 555;
+ c[1].b[3] = 32144;
+
+ /*
+ * pstxvpx XTp,D(RA),R
+ * XSp = 32×SX + 2×Sp
+ * let RA=3 D=d0||d1=0 R=0 Sp=1 SX=1
+ */
+ instr = ppc_inst_prefix(PPC_PSTXVP(3, 0, 0, 1, 1) >> 32,
+ PPC_PSTXVP(3, 0, 0, 1, 1) & 0xffffffff);
+
+ stepped = emulate_step(®s, instr);
+
+ if (stepped == 1 && cached_b[0] == c[0].b[0] && cached_b[1] == c[0].b[1] &&
+ cached_b[2] == c[0].b[2] && cached_b[3] == c[0].b[3] &&
+ cached_b[4] == c[1].b[0] && cached_b[5] == c[1].b[1] &&
+ cached_b[6] == c[1].b[2] && cached_b[7] == c[1].b[3] &&
+ cpu_has_feature(CPU_FTR_VSX)) {
+ show_result("pstxvpx", "PASS");
+ } else {
+ if (!cpu_has_feature(CPU_FTR_VSX))
+ show_result("pstxvpx", "PASS (!CPU_FTR_VSX)");
+ else
+ show_result("pstxvpx", "FAIL");
+ }
+}
+#else
+static void __init test_plxvp_pstxvp(void)
+{
+ show_result("plxvpx", "SKIP (CONFIG_VSX is not set)");
+ show_result("pstxvpx", "SKIP (CONFIG_VSX is not set)");
+}
+#endif /* CONFIG_VSX */
+
static void __init run_tests_load_store(void)
{
test_ld();
@@ -628,6 +877,9 @@ static void __init run_tests_load_store(void)
test_plfd_pstfd();
test_lvx_stvx();
test_lxvd2x_stxvd2x();
+ test_lxvp_stxvp();
+ test_lxvpx_stxvpx();
+ test_plxvp_pstxvp();
}
struct compute_test {
--
2.24.1
^ permalink raw reply related
* Re: [PATCH] KVM: PPC: Book3S HV: fix a oops in kvmppc_uvmem_page_free()
From: Ram Pai @ 2020-07-31 8:37 UTC (permalink / raw)
To: Bharata B Rao
Cc: ldufour, cclaudio, kvm-ppc, sathnaga, aneesh.kumar, sukadev,
linuxppc-dev, bauerman, david
In-Reply-To: <20200731042940.GA20199@in.ibm.com>
On Fri, Jul 31, 2020 at 09:59:40AM +0530, Bharata B Rao wrote:
> On Thu, Jul 30, 2020 at 04:25:26PM -0700, Ram Pai wrote:
> > Observed the following oops while stress-testing, using multiple
> > secureVM on a distro kernel. However this issue theoritically exists in
> > 5.5 kernel and later.
> >
> > This issue occurs when the total number of requested device-PFNs exceed
> > the total-number of available device-PFNs. PFN migration fails to
> > allocate a device-pfn, which causes migrate_vma_finalize() to trigger
> > kvmppc_uvmem_page_free() on a page, that is not associated with any
> > device-pfn. kvmppc_uvmem_page_free() blindly tries to access the
> > contents of the private data which can be null, leading to the following
> > kernel fault.
> >
> > --------------------------------------------------------------------------
> > Unable to handle kernel paging request for data at address 0x00000011
> > Faulting instruction address: 0xc00800000e36e110
> > Oops: Kernel access of bad area, sig: 11 [#1]
> > LE SMP NR_CPUS=2048 NUMA PowerNV
> > ....
> > MSR: 900000000280b033 <SF,HV,VEC,VSX,EE,FP,ME,IR,DR,RI,LE>
> > CR: 24424822 XER: 00000000
> > CFAR: c000000000e3d764 DAR: 0000000000000011 DSISR: 40000000 IRQMASK: 0
> > GPR00: c00800000e36e0a4 c000001f1d59f610 c00800000e38a400 0000000000000000
> > GPR04: c000001fa5000000 fffffffffffffffe ffffffffffffffff c000201fffeaf300
> > GPR08: 00000000000001f0 0000000000000000 0000000000000f80 c00800000e373608
> > GPR12: c000000000e3d710 c000201fffeaf300 0000000000000001 00007fef87360000
> > GPR16: 00007fff97db4410 c000201c3b66a578 ffffffffffffffff 0000000000000000
> > GPR20: 0000000119db9ad0 000000000000000a fffffffffffffffc 0000000000000001
> > GPR24: c000201c3b660000 c000001f1d59f7a0 c0000000004cffb0 0000000000000001
> > GPR28: 0000000000000000 c00a001ff003e000 c00800000e386150 0000000000000f80
> > NIP [c00800000e36e110] kvmppc_uvmem_page_free+0xc8/0x210 [kvm_hv]
> > LR [c00800000e36e0a4] kvmppc_uvmem_page_free+0x5c/0x210 [kvm_hv]
> > Call Trace:
> > [c000000000512010] free_devmap_managed_page+0xd0/0x100
> > [c0000000003f71d0] put_devmap_managed_page+0xa0/0xc0
> > [c0000000004d24bc] migrate_vma_finalize+0x32c/0x410
> > [c00800000e36e828] kvmppc_svm_page_in.constprop.5+0xa0/0x460 [kvm_hv]
> > [c00800000e36eddc] kvmppc_uv_migrate_mem_slot.isra.2+0x1f4/0x230 [kvm_hv]
> > [c00800000e36fa98] kvmppc_h_svm_init_done+0x90/0x170 [kvm_hv]
> > [c00800000e35bb14] kvmppc_pseries_do_hcall+0x1ac/0x10a0 [kvm_hv]
> > [c00800000e35edf4] kvmppc_vcpu_run_hv+0x83c/0x1060 [kvm_hv]
> > [c00800000e95eb2c] kvmppc_vcpu_run+0x34/0x48 [kvm]
> > [c00800000e95a2dc] kvm_arch_vcpu_ioctl_run+0x374/0x830 [kvm]
> > [c00800000e9433b4] kvm_vcpu_ioctl+0x45c/0x7c0 [kvm]
> > [c0000000005451d0] do_vfs_ioctl+0xe0/0xaa0
> > [c000000000545d64] sys_ioctl+0xc4/0x160
> > [c00000000000b408] system_call+0x5c/0x70
> > Instruction dump:
> > a12d1174 2f890000 409e0158 a1271172 3929ffff b1271172 7c2004ac 39200000
> > 913e0140 39200000 e87d0010 f93d0010 <89230011> e8c30000 e9030008 2f890000
> > --------------------------------------------------------------------------
> >
> > Fix the oops..
> >
> > fixes: ca9f49 ("KVM: PPC: Book3S HV: Support for running secure guests")
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> > arch/powerpc/kvm/book3s_hv_uvmem.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> > index 2806983..f4002bf 100644
> > --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> > +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> > @@ -1018,13 +1018,15 @@ static void kvmppc_uvmem_page_free(struct page *page)
> > {
> > unsigned long pfn = page_to_pfn(page) -
> > (kvmppc_uvmem_pgmap.res.start >> PAGE_SHIFT);
> > - struct kvmppc_uvmem_page_pvt *pvt;
> > + struct kvmppc_uvmem_page_pvt *pvt = page->zone_device_data;
> > +
> > + if (!pvt)
> > + return;
> >
> > spin_lock(&kvmppc_uvmem_bitmap_lock);
> > bitmap_clear(kvmppc_uvmem_bitmap, pfn, 1);
> > spin_unlock(&kvmppc_uvmem_bitmap_lock);
> >
> > - pvt = page->zone_device_data;
> > page->zone_device_data = NULL;
> > if (pvt->remove_gfn)
> > kvmppc_gfn_remove(pvt->gpa >> PAGE_SHIFT, pvt->kvm);
>
> In our case, device pages that are in use are always associated with a valid
> pvt member. See kvmppc_uvmem_get_page() which returns failure if it
> runs out of device pfns and that will result in proper failure of
> page-in calls.
looked at the code, and yes that code path looks correct. So my
reasoning behind the root cause of this bug is incorrect. However the
bug is surfacing and there must be a reason.
>
> For the case where we run out of device pfns, migrate_vma_finalize() will
> restore the original PTE and will not replace the PTE with device private PTE.
>
> Also kvmppc_uvmem_page_free() (=dev_pagemap_ops.page_free()) is never
> called for non-device-private pages.
Yes. it should not be called. But as seen above in the stack trace, it is called.
What would cause the HMM to call ->page_free() on a page that is not
associated with that device's pfn?
>
> This could be a use-after-free case possibly arising out of the new state
> changes in HV. If so, this fix will only mask the bug and not address the
> original problem.
I can verify by rerunning the tests, without the new state changes. But
I do not see how those changes can cause this fault?
This could also be caused by a duplicate ->page_free() call due to some
bug in the migrate_page path? Could there be a race between
migrate_page() and a page_fault ?
Regardless, kvmppc_uvmem_page_free() needs to be fixed. It should not
access contents of pvt, without verifing pvt is valid.
>
> Regards,
> Bharata.
--
Ram Pai
^ permalink raw reply
* Re: [PATCH v4 07/10] Powerpc/numa: Detect support for coregroup
From: Srikar Dronamraju @ 2020-07-31 9:18 UTC (permalink / raw)
To: Michael Ellerman
Cc: Nathan Lynch, Gautham R Shenoy, Michael Neuling, Peter Zijlstra,
Jordan Niethe, LKML, Nicholas Piggin, Valentin Schneider,
Oliver O'Halloran, linuxppc-dev, Ingo Molnar
In-Reply-To: <8736585djw.fsf@mpe.ellerman.id.au>
* Michael Ellerman <mpe@ellerman.id.au> [2020-07-31 17:49:55]:
> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> > Add support for grouping cores based on the device-tree classification.
> > - The last domain in the associativity domains always refers to the
> > core.
> > - If primary reference domain happens to be the penultimate domain in
> > the associativity domains device-tree property, then there are no
> > coregroups. However if its not a penultimate domain, then there are
> > coregroups. There can be more than one coregroup. For now we would be
> > interested in the last or the smallest coregroups.
>
> This still doesn't tell me what a coregroup actually represents.
>
> I get that it's a grouping of cores, and that the device tree specifies
> it for us, but grouping based on what?
>
We have just abstracted the fact that we are creating a sub-group of cores
within a DIE. We are limiting to one sub-group per core. However this would
allow the firmware the flexibility to vary the grouping. Once the firmware
starts using this group, we could add more code to detect the type of
grouping and adjust the sd domain flags accordingly.
> I think the answer is we aren't being told by firmware, it's just a
> grouping based on some opaque performance characteristic and we just
> have to take that as given.
>
This is partially true. At this time, we dont have firmwares that can
exploit this code. Once the firmwares start using this grouping, we could
add more code to align the grouping to the scheduler topology.
> But please explain that clearly in the change log and the code comments.
>
Okay, I will do the needful.
> cheers
>
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH v4 06/10] powerpc/smp: Generalize 2nd sched domain
From: Srikar Dronamraju @ 2020-07-31 9:29 UTC (permalink / raw)
To: Michael Ellerman
Cc: Nathan Lynch, Gautham R Shenoy, Michael Neuling, Peter Zijlstra,
Jordan Niethe, LKML, Nicholas Piggin, Valentin Schneider,
Oliver O'Halloran, linuxppc-dev, Ingo Molnar
In-Reply-To: <875za45dr2.fsf@mpe.ellerman.id.au>
* Michael Ellerman <mpe@ellerman.id.au> [2020-07-31 17:45:37]:
> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> > Currently "CACHE" domain happens to be the 2nd sched domain as per
> > powerpc_topology. This domain will collapse if cpumask of l2-cache is
> > same as SMT domain. However we could generalize this domain such that it
> > could mean either be a "CACHE" domain or a "BIGCORE" domain.
> >
> > While setting up the "CACHE" domain, check if shared_cache is already
> > set.
>
> PeterZ asked for some overview of what you're doing and why, you
> responded to his mail, but I was expecting to see that text incorporated
> here somewhere.
>
Okay, do you want that as part of the code or documentation dir or the
changelog?
> He also asked for some comments, which I would also like to see.
>
>
> I'm also not clear why we want to rename it to "bigcore", that's not a
> commonly understood term, I don't think it's clear to new readers what
> it means.
>
> Leaving it as the shared cache domain, and having a comment mentioning
> that "bigcores" share a cache, would be clearer I think.
>
Today, Shared cache is equal to Big Core. However in not too distant future,
Shared cache domain and Big Core may not be the same. For example lets
assume that L2 cache were to Shrink per small core with the firmware
exposing the core as a bigcore. Then with the current design, we have a SMT
== SHARED CACHE, and a DIE. We would not have any domain at the publicised 8
thread level. Keeping the Bigcore as a domain and mapping the shared
cache, (I am resetting the domain name as CACHE if BIGCORE==SHARED_CACHE),
helps us in this scenario.
> cheers
>
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH v4 08/10] powerpc/smp: Allocate cpumask only after searching thread group
From: Srikar Dronamraju @ 2020-07-31 9:49 UTC (permalink / raw)
To: Michael Ellerman
Cc: Nathan Lynch, Gautham R Shenoy, Michael Neuling, Peter Zijlstra,
Jordan Niethe, LKML, Nicholas Piggin, Valentin Schneider,
Oliver O'Halloran, linuxppc-dev, Ingo Molnar
In-Reply-To: <87zh7g3yvk.fsf@mpe.ellerman.id.au>
* Michael Ellerman <mpe@ellerman.id.au> [2020-07-31 17:52:15]:
> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> > If allocated earlier and the search fails, then cpumask need to be
> > freed. However cpu_l1_cache_map can be allocated after we search thread
> > group.
>
> It's not freed anywhere AFAICS?
>
Yes, its never freed. Infact we are never checking if
zalloc_cpumask_var_node fails. Its not just this cpumask, but historically
all the other existing cpumasks in arch/powerpc/kernel/smp.c are never
freed/checked. I did dig into this a bit and it appears that ..
(Please do correct me if I am wrong!! )
Powerpc using cpumask_var_t for all of the percpu variables. And it dont seem
to enable CONFIG_CPUMASK_OFFSTACK even from the MAXSMP config.
So from include/linux/cpumask.h
typedef struct cpumask cpumask_var_t[1];
and
zalloc_cpumask_var_node ends up being cpumask_clear
So I think we are historically we seem to assume we are always
!CPUMASK_OFFSTACK and hence we dont need to check for return as well as
free..
I would look forward to your comments on how we should handle this going
forward. But I would keep this the same for this patchset.
One of the questions that I have is if we most likely are to be in
!CONFIG_CPUMASK_OFFSTACK, then should be migrate to cpumask_t for percpu
variables.
The reason being we end up using NR_CPU cpumask for each percpu cpumask
variable instead of using NR_CPU cpumask_t pointer.
> And even after this change there's still an error path that doesn't free
> it, isn't there?
>
> cheers
>
> > Cc: linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
> > Cc: LKML <linux-kernel@vger.kernel.org>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: Nicholas Piggin <npiggin@gmail.com>
> > Cc: Anton Blanchard <anton@ozlabs.org>
> > Cc: Oliver O'Halloran <oohall@gmail.com>
> > Cc: Nathan Lynch <nathanl@linux.ibm.com>
> > Cc: Michael Neuling <mikey@neuling.org>
> > Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Valentin Schneider <valentin.schneider@arm.com>
> > Cc: Jordan Niethe <jniethe5@gmail.com>
> > Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > ---
> > arch/powerpc/kernel/smp.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > index 698000c7f76f..dab96a1203ec 100644
> > --- a/arch/powerpc/kernel/smp.c
> > +++ b/arch/powerpc/kernel/smp.c
> > @@ -797,10 +797,6 @@ static int init_cpu_l1_cache_map(int cpu)
> > if (err)
> > goto out;
> >
> > - zalloc_cpumask_var_node(&per_cpu(cpu_l1_cache_map, cpu),
> > - GFP_KERNEL,
> > - cpu_to_node(cpu));
> > -
> > cpu_group_start = get_cpu_thread_group_start(cpu, &tg);
> >
> > if (unlikely(cpu_group_start == -1)) {
> > @@ -809,6 +805,9 @@ static int init_cpu_l1_cache_map(int cpu)
> > goto out;
> > }
> >
> > + zalloc_cpumask_var_node(&per_cpu(cpu_l1_cache_map, cpu),
> > + GFP_KERNEL, cpu_to_node(cpu));
> > +
> > for (i = first_thread; i < first_thread + threads_per_core; i++) {
> > int i_group_start = get_cpu_thread_group_start(i, &tg);
> >
> > --
> > 2.17.1
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH v4 10/10] powerpc/smp: Implement cpu_to_coregroup_id
From: Srikar Dronamraju @ 2020-07-31 9:58 UTC (permalink / raw)
To: Michael Ellerman
Cc: Nathan Lynch, Gautham R Shenoy, Michael Neuling, Peter Zijlstra,
Jordan Niethe, LKML, Nicholas Piggin, Valentin Schneider,
Oliver O'Halloran, linuxppc-dev, Ingo Molnar
In-Reply-To: <87wo2k3yeq.fsf@mpe.ellerman.id.au>
* Michael Ellerman <mpe@ellerman.id.au> [2020-07-31 18:02:21]:
> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> > Lookup the coregroup id from the associativity array.
>
Thanks Michael for all your comments and inputs.
> It's slightly strange that this is called in patch 9, but only properly
> implemented here in patch 10.
>
> I'm not saying you have to squash them together, but it would be good if
> the change log for patch 9 mentioned that a subsequent commit will
> complete the implementation and how that affects the behaviour.
>
I probably got influenced by few LKML community members who always add a
stub and implement the gory details in a subsequent patch. I will surely add
the change log in patch 9 about the subsequent patches.
> cheers
>
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH] KVM: PPC: Book3S HV: fix a oops in kvmppc_uvmem_page_free()
From: Bharata B Rao @ 2020-07-31 10:02 UTC (permalink / raw)
To: Ram Pai
Cc: ldufour, cclaudio, kvm-ppc, sathnaga, aneesh.kumar, sukadev,
linuxppc-dev, bauerman, david
In-Reply-To: <20200731083700.GB5787@oc0525413822.ibm.com>
On Fri, Jul 31, 2020 at 01:37:00AM -0700, Ram Pai wrote:
> On Fri, Jul 31, 2020 at 09:59:40AM +0530, Bharata B Rao wrote:
> > On Thu, Jul 30, 2020 at 04:25:26PM -0700, Ram Pai wrote:
> > In our case, device pages that are in use are always associated with a valid
> > pvt member. See kvmppc_uvmem_get_page() which returns failure if it
> > runs out of device pfns and that will result in proper failure of
> > page-in calls.
>
> looked at the code, and yes that code path looks correct. So my
> reasoning behind the root cause of this bug is incorrect. However the
> bug is surfacing and there must be a reason.
>
> >
> > For the case where we run out of device pfns, migrate_vma_finalize() will
> > restore the original PTE and will not replace the PTE with device private PTE.
> >
> > Also kvmppc_uvmem_page_free() (=dev_pagemap_ops.page_free()) is never
> > called for non-device-private pages.
>
> Yes. it should not be called. But as seen above in the stack trace, it is called.
>
> What would cause the HMM to call ->page_free() on a page that is not
> associated with that device's pfn?
I believe it is being called for a device private page, you can verify
it when you hit it next time?
>
> >
> > This could be a use-after-free case possibly arising out of the new state
> > changes in HV. If so, this fix will only mask the bug and not address the
> > original problem.
>
> I can verify by rerunning the tests, without the new state changes. But
> I do not see how those changes can cause this fault?
>
> This could also be caused by a duplicate ->page_free() call due to some
> bug in the migrate_page path? Could there be a race between
> migrate_page() and a page_fault ?
>
>
> Regardless, kvmppc_uvmem_page_free() needs to be fixed. It should not
> access contents of pvt, without verifing pvt is valid.
We don't expect pvt to be NULL here. Checking for NULL and returning
isn't the right fix, I think.
Regards,
Bharata.
^ permalink raw reply
* Re: [PATCH] ASoC: fsl_sai: Fix value of FSL_SAI_CR1_RFW_MASK
From: Fabio Estevam @ 2020-07-31 10:38 UTC (permalink / raw)
To: Shengjiu Wang
Cc: Linux-ALSA, Timur Tabi, Xiubo Li, linux-kernel, Takashi Iwai,
Liam Girdwood, Jaroslav Kysela, Nicolin Chen, Mark Brown,
linuxppc-dev
In-Reply-To: <1596176895-28724-1-git-send-email-shengjiu.wang@nxp.com>
Hi Shengjiu,
On Fri, Jul 31, 2020 at 3:32 AM Shengjiu Wang <shengjiu.wang@nxp.com> wrote:
>
> The fifo_depth is 64 on i.MX8QM/i.MX8QXP, 128 on i.MX8MQ, 16 on
> i.MX7ULP.
>
> Original FSL_SAI_CR1_RFW_MASK value 0x1F is not suitable for
> these platform, the FIFO watermark mask should be updated
> according to the fifo_depth.
>
> Fixes: a860fac42097 ("ASoC: fsl_sai: Add support for imx7ulp/imx8mq")
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
^ permalink raw reply
* [RFC PATCH 1/2] powerpc/numa: Introduce logical numa id
From: Aneesh Kumar K.V @ 2020-07-31 11:19 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Nathan Lynch, Aneesh Kumar K.V, Srikar Dronamraju
We use ibm,associativity and ibm,associativity-lookup-arrays to derive the numa
node numbers. These device tree properties are firmware indicated grouping of
resources based on their hierarchy in the platform. These numbers (group id) are
not sequential and hypervisor/firmware can follow different numbering schemes.
For ex: on powernv platforms, we group them in the below order.
* - CCM node ID
* - HW card ID
* - HW module ID
* - Chip ID
* - Core ID
Based on ibm,associativity-reference-points we use one of the above group ids as
Linux NUMA node id. (On PowerNV platform Chip ID is used). This results
in Linux reporting non-linear NUMA node id and which also results in Linux
reporting empty node 0 NUMA nodes.
This can be resolved by mapping the firmware provided group id to a logical Linux
NUMA id. In this patch, we do this only for pseries platforms considering the
firmware group id is a virtualized entity and users would not have drawn any
conclusion based on the Linux Numa Node id.
On PowerNV platform since we have historically mapped Chip ID as Linux NUMA node
id, we keep the existing Linux NUMA node id numbering.
Before Fix:
# numactl -H
available: 2 nodes (0-1)
node 0 cpus:
node 0 size: 0 MB
node 0 free: 0 MB
node 1 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71
node 1 size: 50912 MB
node 1 free: 45248 MB
node distances:
node 0 1
0: 10 40
1: 40 10
after fix
# numactl -H
available: 1 nodes (0)
node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71
node 0 size: 50912 MB
node 0 free: 49724 MB
node distances:
node 0
0: 10
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/topology.h | 1 +
arch/powerpc/mm/numa.c | 49 ++++++++++++++++++++++-------
2 files changed, 39 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
index f0b6300e7dd3..15b0424a27a8 100644
--- a/arch/powerpc/include/asm/topology.h
+++ b/arch/powerpc/include/asm/topology.h
@@ -118,5 +118,6 @@ int get_physical_package_id(int cpu);
#endif
#endif
+int firmware_group_id_to_nid(int firmware_gid);
#endif /* __KERNEL__ */
#endif /* _ASM_POWERPC_TOPOLOGY_H */
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index e437a9ac4956..6c659aada55b 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -221,25 +221,51 @@ static void initialize_distance_lookup_table(int nid,
}
}
+static u32 nid_map[MAX_NUMNODES] = {[0 ... MAX_NUMNODES - 1] = NUMA_NO_NODE};
+
+int firmware_group_id_to_nid(int firmware_gid)
+{
+ static int last_nid = 0;
+
+ /*
+ * For PowerNV we don't change the node id. This helps to avoid
+ * confusion w.r.t the expected node ids. On pseries, node numbers
+ * are virtualized. Hence do logical node id for pseries.
+ */
+ if (!firmware_has_feature(FW_FEATURE_LPAR))
+ return firmware_gid;
+
+ if (firmware_gid == -1)
+ return NUMA_NO_NODE;
+
+ if (nid_map[firmware_gid] == NUMA_NO_NODE)
+ nid_map[firmware_gid] = last_nid++;
+
+ return nid_map[firmware_gid];
+}
+
/* Returns nid in the range [0..MAX_NUMNODES-1], or -1 if no useful numa
* info is found.
*/
static int associativity_to_nid(const __be32 *associativity)
{
int nid = NUMA_NO_NODE;
+ int firmware_gid = -1;
if (!numa_enabled)
goto out;
if (of_read_number(associativity, 1) >= min_common_depth)
- nid = of_read_number(&associativity[min_common_depth], 1);
+ firmware_gid = of_read_number(&associativity[min_common_depth], 1);
/* POWER4 LPAR uses 0xffff as invalid node */
- if (nid == 0xffff || nid >= MAX_NUMNODES)
- nid = NUMA_NO_NODE;
+ if (firmware_gid == 0xffff || firmware_gid >= MAX_NUMNODES)
+ firmware_gid = -1;
+
+ nid = firmware_group_id_to_nid(firmware_gid);
if (nid > 0 &&
- of_read_number(associativity, 1) >= distance_ref_points_depth) {
+ of_read_number(associativity, 1) >= distance_ref_points_depth) {
/*
* Skip the length field and send start of associativity array
*/
@@ -432,24 +458,25 @@ static int of_get_assoc_arrays(struct assoc_arrays *aa)
static int of_drconf_to_nid_single(struct drmem_lmb *lmb)
{
struct assoc_arrays aa = { .arrays = NULL };
- int default_nid = NUMA_NO_NODE;
- int nid = default_nid;
+ int nid = NUMA_NO_NODE, firmware_gid;
int rc, index;
if ((min_common_depth < 0) || !numa_enabled)
- return default_nid;
+ return NUMA_NO_NODE;
rc = of_get_assoc_arrays(&aa);
if (rc)
- return default_nid;
+ return NUMA_NO_NODE;
if (min_common_depth <= aa.array_sz &&
!(lmb->flags & DRCONF_MEM_AI_INVALID) && lmb->aa_index < aa.n_arrays) {
index = lmb->aa_index * aa.array_sz + min_common_depth - 1;
- nid = of_read_number(&aa.arrays[index], 1);
+ firmware_gid = of_read_number(&aa.arrays[index], 1);
- if (nid == 0xffff || nid >= MAX_NUMNODES)
- nid = default_nid;
+ if (firmware_gid == 0xffff || firmware_gid >= MAX_NUMNODES)
+ firmware_gid = -1;
+
+ nid = firmware_group_id_to_nid(firmware_gid);
if (nid > 0) {
index = lmb->aa_index * aa.array_sz;
--
2.26.2
^ permalink raw reply related
* [RFC PATCH 2/2] powerpc/powernv/cpufreq: Don't assume chip id is same as Linux node id
From: Aneesh Kumar K.V @ 2020-07-31 11:22 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Nathan Lynch, Aneesh Kumar K.V, Srikar Dronamraju
In-Reply-To: <20200731111916.243569-1-aneesh.kumar@linux.ibm.com>
On PowerNV platforms we always have 1:1 mapping between chip ID and
firmware group id. Use the helper to convert firmware group id to
node id instead of directly using chip ID as Linux node id.
NOTE: This doesn't have any functional change. On PowerNV platforms
we continue to have 1:1 mapping between firmware group id and
Linux node id.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
drivers/cpufreq/powernv-cpufreq.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 8646eb197cd9..ca82b6ac0989 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -27,6 +27,7 @@
#include <asm/reg.h>
#include <asm/smp.h> /* Required for cpu_sibling_mask() in UP configs */
#include <asm/opal.h>
+#include <asm/topology.h>
#include <linux/timer.h>
#define POWERNV_MAX_PSTATES_ORDER 8
@@ -1069,8 +1070,14 @@ static int init_chip_info(void)
}
for (i = 0; i < nr_chips; i++) {
+ unsigned int nid;
+
chips[i].id = chip[i];
- cpumask_copy(&chips[i].mask, cpumask_of_node(chip[i]));
+ /*
+ * On powervn platforms firmware group id is same as chipd id.
+ */
+ nid = firmware_group_id_to_nid(chip[i]);
+ cpumask_copy(&chips[i].mask, cpumask_of_node(nid));
INIT_WORK(&chips[i].throttle, powernv_cpufreq_work_fn);
for_each_cpu(cpu, &chips[i].mask)
per_cpu(chip_info, cpu) = &chips[i];
--
2.26.2
^ permalink raw reply related
* Re: [PATCH v4 10/10] powerpc/smp: Implement cpu_to_coregroup_id
From: Michael Ellerman @ 2020-07-31 11:29 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Nathan Lynch, Gautham R Shenoy, Michael Neuling, Peter Zijlstra,
Jordan Niethe, LKML, Nicholas Piggin, Valentin Schneider,
Oliver O'Halloran, linuxppc-dev, Ingo Molnar
In-Reply-To: <20200731095808.GB18776@linux.vnet.ibm.com>
Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> * Michael Ellerman <mpe@ellerman.id.au> [2020-07-31 18:02:21]:
>
>> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
>> > Lookup the coregroup id from the associativity array.
>
> Thanks Michael for all your comments and inputs.
>
>> It's slightly strange that this is called in patch 9, but only properly
>> implemented here in patch 10.
>>
>> I'm not saying you have to squash them together, but it would be good if
>> the change log for patch 9 mentioned that a subsequent commit will
>> complete the implementation and how that affects the behaviour.
>
> I probably got influenced by few LKML community members who always add a
> stub and implement the gory details in a subsequent patch. I will surely add
> the change log in patch 9 about the subsequent patches.
That's OK, it's a valid way to do things, and can be good for keeping
the size of individual patches down to make them easier to review.
But yeah a mention in the change log of the preceeding patch is helpful
for anyone looking at that commit on its own in the future.
cheers
^ permalink raw reply
* Re: [PATCH v4 07/10] Powerpc/numa: Detect support for coregroup
From: Michael Ellerman @ 2020-07-31 11:31 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Nathan Lynch, Gautham R Shenoy, Michael Neuling, Peter Zijlstra,
Jordan Niethe, LKML, Nicholas Piggin, Valentin Schneider,
Oliver O'Halloran, linuxppc-dev, Ingo Molnar
In-Reply-To: <20200731091859.GG14603@linux.vnet.ibm.com>
Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> * Michael Ellerman <mpe@ellerman.id.au> [2020-07-31 17:49:55]:
>
>> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
>> > Add support for grouping cores based on the device-tree classification.
>> > - The last domain in the associativity domains always refers to the
>> > core.
>> > - If primary reference domain happens to be the penultimate domain in
>> > the associativity domains device-tree property, then there are no
>> > coregroups. However if its not a penultimate domain, then there are
>> > coregroups. There can be more than one coregroup. For now we would be
>> > interested in the last or the smallest coregroups.
>>
>> This still doesn't tell me what a coregroup actually represents.
>>
>> I get that it's a grouping of cores, and that the device tree specifies
>> it for us, but grouping based on what?
>
> We have just abstracted the fact that we are creating a sub-group of cores
> within a DIE. We are limiting to one sub-group per core. However this would
> allow the firmware the flexibility to vary the grouping. Once the firmware
> starts using this group, we could add more code to detect the type of
> grouping and adjust the sd domain flags accordingly.
OK. That's good info to have in the change log.
>> I think the answer is we aren't being told by firmware, it's just a
>> grouping based on some opaque performance characteristic and we just
>> have to take that as given.
>>
>
> This is partially true. At this time, we dont have firmwares that can
> exploit this code. Once the firmwares start using this grouping, we could
> add more code to align the grouping to the scheduler topology.
>
>> But please explain that clearly in the change log and the code comments.
>>
>
> Okay, I will do the needful.
Thanks.
cheers
^ permalink raw reply
* [PATCH 1/2] powerpc/vmemmap: Fix memory leak with vmemmap list allocation failures.
From: Aneesh Kumar K.V @ 2020-07-31 11:34 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V
If we fail to allocate vmemmap list, we don't keep track of allocated
vmemmap block buf. Hence on section deactivate we skip vmemmap block
buf free. This results in memory leak.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/mm/init_64.c | 35 ++++++++++++++++++++++++++++-------
1 file changed, 28 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
index 152aa0200cef..8a198504c0e1 100644
--- a/arch/powerpc/mm/init_64.c
+++ b/arch/powerpc/mm/init_64.c
@@ -162,16 +162,16 @@ static __meminit struct vmemmap_backing * vmemmap_list_alloc(int node)
return next++;
}
-static __meminit void vmemmap_list_populate(unsigned long phys,
- unsigned long start,
- int node)
+static __meminit int vmemmap_list_populate(unsigned long phys,
+ unsigned long start,
+ int node)
{
struct vmemmap_backing *vmem_back;
vmem_back = vmemmap_list_alloc(node);
if (unlikely(!vmem_back)) {
- WARN_ON(1);
- return;
+ pr_debug("vmemap list allocation failed\n");
+ return -ENOMEM;
}
vmem_back->phys = phys;
@@ -179,6 +179,7 @@ static __meminit void vmemmap_list_populate(unsigned long phys,
vmem_back->list = vmemmap_list;
vmemmap_list = vmem_back;
+ return 0;
}
static bool altmap_cross_boundary(struct vmem_altmap *altmap, unsigned long start,
@@ -199,6 +200,7 @@ static bool altmap_cross_boundary(struct vmem_altmap *altmap, unsigned long star
int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
struct vmem_altmap *altmap)
{
+ bool altmap_alloc;
unsigned long page_size = 1 << mmu_psize_defs[mmu_vmemmap_psize].shift;
/* Align to the page size of the linear mapping. */
@@ -228,13 +230,32 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
p = altmap_alloc_block_buf(page_size, altmap);
if (!p)
pr_debug("altmap block allocation failed, falling back to system memory");
+ else
+ altmap_alloc = true;
}
- if (!p)
+ if (!p) {
p = vmemmap_alloc_block_buf(page_size, node);
+ altmap_alloc = false;
+ }
if (!p)
return -ENOMEM;
- vmemmap_list_populate(__pa(p), start, node);
+ if (vmemmap_list_populate(__pa(p), start, node)) {
+ /*
+ * If we don't populate vmemap list, we don't have
+ * the ability to free the allocated vmemmap
+ * pages in section_deactivate. Hence free them
+ * here.
+ */
+ int nr_pfns = page_size >> PAGE_SHIFT;
+ unsigned long page_order = get_order(page_size);
+
+ if (altmap_alloc)
+ vmem_altmap_free(altmap, nr_pfns);
+ else
+ free_pages((unsigned long)p, page_order);
+ return -ENOMEM;
+ }
pr_debug(" * %016lx..%016lx allocated at %p\n",
start, start + page_size, p);
--
2.26.2
^ permalink raw reply related
* [PATCH 2/2] powerpc/vmemmap: Don't warn if we don't find a mapping vmemmap list entry
From: Aneesh Kumar K.V @ 2020-07-31 11:35 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: Aneesh Kumar K.V
In-Reply-To: <20200731113500.248306-1-aneesh.kumar@linux.ibm.com>
Now that we are handling vmemmap list allocation failure correctly, don't
WARN in section deactivate when we don't find a mapping vmemmap list entry.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/mm/init_64.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
index 8a198504c0e1..3ffe8b84aab2 100644
--- a/arch/powerpc/mm/init_64.c
+++ b/arch/powerpc/mm/init_64.c
@@ -285,10 +285,8 @@ static unsigned long vmemmap_list_free(unsigned long start)
vmem_back_prev = vmem_back;
}
- if (unlikely(!vmem_back)) {
- WARN_ON(1);
+ if (unlikely(!vmem_back))
return 0;
- }
/* remove it from vmemmap_list */
if (vmem_back == vmemmap_list) /* remove head */
--
2.26.2
^ permalink raw reply related
* [PATCH v4 00/12] PCI: Remove '*val = 0' from pcie_capability_read_*()
From: Saheed O. Bolarinwa @ 2020-07-31 11:02 UTC (permalink / raw)
To: helgaas, Mike Marciniszyn, Dennis Dalessandro, Doug Ledford,
Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
David S. Miller, Kalle Valo, Jakub Kicinski, Rafael J. Wysocki,
Len Brown, Russell Currey, Sam Bobroff, Oliver O'Halloran
Cc: linuxppc-dev, linux-rdma, Saheed O. Bolarinwa, skhan,
linux-wireless, linux-kernel, QCA ath9k Development, linux-acpi,
netdev, linux-pci, bjorn, linux-kernel-mentees
v4 CHANGES:
- Drop uses of pcie_capability_read_*() return value. This related to
[1] which is pointing towards making the accessors return void.
- Remove patches found to be unnecessary
- Reword some commit messages
v3 CHANGES:
- Split previous PATCH 6/13 into two : PATCH 6/14 and PATCH 7/14
- Fix commit message of PATCH 5/14
- Update Patch numbering and Commit messages
- Add 'Acked by Greg KH' to PATCH 2/14
- Add PATCH version
v2 CHANGES:
- Fix missing comma, causing the email cc error
- Fix typos and numbering errors in commit messages
- Add commit message to 13/13
- Add two more patches: PATCH 3/13 and PATCH 4/13
MERGING:
- Patch 6/12 depends on Patch 5/12. However Patch 5/12 has no dependency.
Please, merge PATCH 6/12 only after Patch 5/12.
- Patch 12/12 depends on all preceding patches. Please merge Patch 12/12
only after other patches in this series have been merged.
- All other patches have no dependencies besides those mentioned above and
can be merge individually.
PATCH 5/12:
Set the default case in the switch-statement to set status
to "Power On".
PATCH 1/12 to 11/12:
Use the value read by pcie_capability_read_*() to determine success or
failure. This is done by checking if it is ~0, while maintaining the
functions' behaviour. This ensures that the changes in PATCH 12/12 does
not introduce any bug.
PATCH 12/12:
There are several reasons why a PCI capability read may fail whether the
device is present or not. If this happens, pcie_capability_read_*() will
return -EINVAL/PCIBIOS_BAD_REGISTER_NUMBER or PCIBIOS_DEVICE_NOT_FOUND
and *val is set to 0.
This behaviour if further ensured by this code inside
pcie_capability_read_*()
ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);
/*
* Reset *val to 0 if pci_read_config_dword() fails, it may
* have been written as 0xFFFFFFFF if hardware error happens
* during pci_read_config_dword().
*/
if (ret)
*val = 0;
return ret;
a) Since all pci_generic_config_read() does is read a register value,
it may return success after reading a ~0 which *may* have been fabricated
by the PCI host bridge due to a read timeout. Hence pci_read_config_*()
will return success with a fabricated ~0 in *val, indicating a problem.
In this case, the assumed behaviour of pcie_capability_read_*() will be
wrong. To avoid error slipping through, more checks are necessary.
b) pci_read_config_*() will return PCIBIOS_DEVICE_NOT_FOUND only if
dev->error_state = pci_channel_io_perm_failure (i.e.
pci_dev_is_disconnected()) or if pci_generic_config_read() can't find the
device. In both cases *val is initially set to ~0 but as shown in the code
above pcie_capability_read_*() resets it back to 0. Even with this effort,
drivers still have to perform validation checks more so if 0 is a valid
value.
Most drivers only consider the case (b) and in some cases, there is the
expectation that on timeout *val has a fabricated value of ~0, which *may*
not always be true as explained in (a).
In any case, checks need to be done to validate the value read and maybe
confirm which error has occurred. It is better left to the drivers to do.
Check the return value of pcie_capability_read_dword() to ensure success
and avoid bug as a result of Patch 14/14.
Remove the reset of *val to 0 when pci_read_config_*() fails.
[1] https://lore.kernel.org/linux-pci/20200714234625.GA428442@bjorn-Precision-5520/
Saheed O. Bolarinwa (12):
IB/hfi1: Check if pcie_capability_read_*() reads ~0
misc: rtsx: Check if pcie_capability_read_*() reads ~0
ath9k: Check if pcie_capability_read_*() reads ~0
iwlegacy: Check if pcie_capability_read_*() reads ~0
PCI: pciehp: Set "Power On" as the default get_power_status
PCI: pciehp: Check if pcie_capability_read_*() reads ~0
PCI/ACPI: Check if pcie_capability_read_*() reads ~0
PCI: Check if pcie_capability_read_*() reads ~0
PCI/PM: Check if pcie_capability_read_*() reads ~0
PCI/AER: Check if pcie_capability_read_*() reads ~0
PCI/ASPM: Check if pcie_capability_read_*() reads ~0
PCI: Remove '*val = 0' from pcie_capability_read_*()
drivers/infiniband/hw/hfi1/aspm.c | 6 ++--
drivers/misc/cardreader/rts5227.c | 2 +-
drivers/misc/cardreader/rts5249.c | 2 +-
drivers/misc/cardreader/rts5260.c | 2 +-
drivers/misc/cardreader/rts5261.c | 2 +-
drivers/net/wireless/ath/ath9k/pci.c | 3 +-
drivers/net/wireless/intel/iwlegacy/common.c | 2 +-
drivers/pci/access.c | 14 --------
drivers/pci/hotplug/pciehp_hpc.c | 13 +++++---
drivers/pci/pci-acpi.c | 4 +--
drivers/pci/pci.c | 34 ++++++++++++++------
drivers/pci/pcie/aer.c | 2 +-
drivers/pci/pcie/aspm.c | 10 +++---
drivers/pci/probe.c | 12 +++----
14 files changed, 56 insertions(+), 52 deletions(-)
--
2.18.4
^ permalink raw reply
* Re: [PATCH v4 08/10] powerpc/smp: Allocate cpumask only after searching thread group
From: Michael Ellerman @ 2020-07-31 12:14 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Nathan Lynch, Gautham R Shenoy, Michael Neuling, Peter Zijlstra,
Jordan Niethe, LKML, Nicholas Piggin, Valentin Schneider,
Oliver O'Halloran, linuxppc-dev, Ingo Molnar
In-Reply-To: <20200731094938.GA18776@linux.vnet.ibm.com>
Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> * Michael Ellerman <mpe@ellerman.id.au> [2020-07-31 17:52:15]:
>
>> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
>> > If allocated earlier and the search fails, then cpumask need to be
>> > freed. However cpu_l1_cache_map can be allocated after we search thread
>> > group.
>>
>> It's not freed anywhere AFAICS?
>
> Yes, its never freed. Infact we are never checking if
> zalloc_cpumask_var_node fails. Its not just this cpumask, but historically
> all the other existing cpumasks in arch/powerpc/kernel/smp.c are never
> freed/checked. I did dig into this a bit and it appears that ..
> (Please do correct me if I am wrong!! )
That's correct.
> Powerpc using cpumask_var_t for all of the percpu variables. And it dont seem
> to enable CONFIG_CPUMASK_OFFSTACK even from the MAXSMP config.
I remember Rusty adding that code, but I don't know if we ever
considered enabling CPUMASK_OFFSTACK.
Probably we meant to but never got around to doing it.
> So from include/linux/cpumask.h
>
> typedef struct cpumask cpumask_var_t[1];
> and
> zalloc_cpumask_var_node ends up being cpumask_clear
>
> So I think we are historically we seem to assume we are always
> !CPUMASK_OFFSTACK and hence we dont need to check for return as well as
> free..
Right.
> I would look forward to your comments on how we should handle this going
> forward. But I would keep this the same for this patchset.
Agreed, just clarify in the change log that it's not freed at the moment
because of CPU_MASK_OFFSTACK=n
> One of the questions that I have is if we most likely are to be in
> !CONFIG_CPUMASK_OFFSTACK, then should be migrate to cpumask_t for percpu
> variables.
I don't think so, cpumask_t is semi-deprecated AIUI.
> The reason being we end up using NR_CPU cpumask for each percpu cpumask
> variable instead of using NR_CPU cpumask_t pointer.
Our current defconfigs have NR_CPUS=2048, which is probably just small
enough to continue using OFFSTACK=n.
But we allow configuring NR_CPUS up to 8192, which surely would need
OFFSTACK=y in order to work.
So I think we need to stick with cpumask_var_t, but we should test with
OFFSTACK=y, and should probably be a bit more careful with checking the
allocations succeed.
And then we should select OFFSTACK=y for NR_CPUS above some threshold.
cheers
^ permalink raw reply
* Re: [PATCH v4 06/10] powerpc/smp: Generalize 2nd sched domain
From: Michael Ellerman @ 2020-07-31 12:22 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Nathan Lynch, Gautham R Shenoy, Michael Neuling, Peter Zijlstra,
Jordan Niethe, LKML, Nicholas Piggin, Valentin Schneider,
Oliver O'Halloran, linuxppc-dev, Ingo Molnar
In-Reply-To: <20200731092901.GH14603@linux.vnet.ibm.com>
Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> * Michael Ellerman <mpe@ellerman.id.au> [2020-07-31 17:45:37]:
>
>> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
>> > Currently "CACHE" domain happens to be the 2nd sched domain as per
>> > powerpc_topology. This domain will collapse if cpumask of l2-cache is
>> > same as SMT domain. However we could generalize this domain such that it
>> > could mean either be a "CACHE" domain or a "BIGCORE" domain.
>> >
>> > While setting up the "CACHE" domain, check if shared_cache is already
>> > set.
>>
>> PeterZ asked for some overview of what you're doing and why, you
>> responded to his mail, but I was expecting to see that text incorporated
>> here somewhere.
>>
>
> Okay, do you want that as part of the code or documentation dir or the
> changelog?
I guess a comment is best, as that's most likely to be seen by people
looking at the code in future.
A little bit of overview in the change log is also good.
>> He also asked for some comments, which I would also like to see.
>>
>>
>> I'm also not clear why we want to rename it to "bigcore", that's not a
>> commonly understood term, I don't think it's clear to new readers what
>> it means.
>>
>> Leaving it as the shared cache domain, and having a comment mentioning
>> that "bigcores" share a cache, would be clearer I think.
>>
>
> Today, Shared cache is equal to Big Core. However in not too distant future,
> Shared cache domain and Big Core may not be the same. For example lets
> assume that L2 cache were to Shrink per small core with the firmware
> exposing the core as a bigcore. Then with the current design, we have a SMT
> == SHARED CACHE, and a DIE. We would not have any domain at the publicised 8
> thread level. Keeping the Bigcore as a domain and mapping the shared
> cache, (I am resetting the domain name as CACHE if BIGCORE==SHARED_CACHE),
> helps us in this scenario.
Yeah OK.
In that scenario it's not really clear what the 8 thread level domain
expresses, if the two "halves" of the bigcore have separate L2s. But
presumably there is still some benefit to exposing it.
cheers
^ permalink raw reply
* [PATCH v4 10/12] PCI/AER: Check if pcie_capability_read_*() reads ~0
From: Saheed O. Bolarinwa @ 2020-07-31 11:43 UTC (permalink / raw)
To: helgaas, Russell Currey, Sam Bobroff, Oliver O'Halloran
Cc: Saheed O. Bolarinwa, skhan, linux-kernel, linux-pci, bjorn,
linuxppc-dev, linux-kernel-mentees
In-Reply-To: <20200731114329.100848-1-refactormyself@gmail.com>
On failure pcie_capability_read_*() sets it's last parameter, val
to 0. However, with Patch 12/12, it is possible that val is set
to ~0 on failure. This would introduce a bug because
(x & x) == (~0 & x).
Since ~0 is an invalid value in here,
Add extra check for ~0 to the if condition to confirm failure.
Suggested-by: Bjorn Helgaas <bjorn@helgaas.com>
Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
---
drivers/pci/pcie/aer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 3acf56683915..dbeabc370efc 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -829,7 +829,7 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
/* Check if AER is enabled */
pcie_capability_read_word(dev, PCI_EXP_DEVCTL, ®16);
- if (!(reg16 & PCI_EXP_AER_FLAGS))
+ if ((reg16 == (u16)~0) || !(reg16 & PCI_EXP_AER_FLAGS))
return false;
if (!aer)
--
2.18.4
^ permalink raw reply related
* [PATCH v4 12/12] PCI: Remove '*val = 0' from pcie_capability_read_*()
From: Saheed O. Bolarinwa @ 2020-07-31 11:43 UTC (permalink / raw)
To: helgaas
Cc: Greg Kroah-Hartman, linux-wireless, QCA ath9k Development,
Oliver O'Halloran, linux-acpi, linux-rdma, Jason Gunthorpe,
Doug Ledford, linux-pci, Jakub Kicinski, linux-kernel-mentees,
Len Brown, Arnd Bergmann, skhan, bjorn, Kalle Valo,
Mike Marciniszyn, Sam Bobroff, Saheed O. Bolarinwa,
Dennis Dalessandro, Rafael J. Wysocki, linux-kernel, netdev,
linuxppc-dev, David S. Miller
In-Reply-To: <20200731114329.100848-1-refactormyself@gmail.com>
There are several reasons why a PCI capability read may fail whether the
device is present or not. If this happens, pcie_capability_read_*() will
return -EINVAL/PCIBIOS_BAD_REGISTER_NUMBER or PCIBIOS_DEVICE_NOT_FOUND
and *val is set to 0.
This behaviour if further ensured by this code inside
pcie_capability_read_*()
ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);
/*
* Reset *val to 0 if pci_read_config_dword() fails, it may
* have been written as 0xFFFFFFFF if hardware error happens
* during pci_read_config_dword().
*/
if (ret)
*val = 0;
return ret;
a) Since all pci_generic_config_read() does is read a register value,
it may return success after reading a ~0 which *may* have been fabricated
by the PCI host bridge due to a read timeout. Hence pci_read_config_*()
will return success with a fabricated ~0 in *val, indicating a problem.
In this case, the assumed behaviour of pcie_capability_read_*() will be
wrong. To avoid error slipping through, more checks are necessary.
b) pci_read_config_*() will return PCIBIOS_DEVICE_NOT_FOUND only if
dev->error_state = pci_channel_io_perm_failure (i.e.
pci_dev_is_disconnected()) or if pci_generic_config_read() can't find the
device. In both cases *val is initially set to ~0 but as shown in the code
above pcie_capability_read_*() resets it back to 0. Even with this effort,
drivers still have to perform validation checks more so if 0 is a valid
value.
Most drivers only consider the case (b) and in some cases, there is the
expectation that on timeout *val has a fabricated value of ~0, which *may*
not always be true as explained in (a).
In any case, checks need to be done to validate the value read and maybe
confirm which error has occurred. It is better left to the drivers to do.
Remove the reset of *val to 0 when pci_read_config_*() fails.
Suggested-by: Bjorn Helgaas <bjorn@helgaas.com>
Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.com>
---
drivers/pci/access.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 79c4a2ef269a..ec95edbb1ac8 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -413,13 +413,6 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val)
if (pcie_capability_reg_implemented(dev, pos)) {
ret = pci_read_config_word(dev, pci_pcie_cap(dev) + pos, val);
- /*
- * Reset *val to 0 if pci_read_config_word() fails, it may
- * have been written as 0xFFFF if hardware error happens
- * during pci_read_config_word().
- */
- if (ret)
- *val = 0;
return ret;
}
@@ -448,13 +441,6 @@ int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val)
if (pcie_capability_reg_implemented(dev, pos)) {
ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);
- /*
- * Reset *val to 0 if pci_read_config_dword() fails, it may
- * have been written as 0xFFFFFFFF if hardware error happens
- * during pci_read_config_dword().
- */
- if (ret)
- *val = 0;
return ret;
}
--
2.18.4
^ permalink raw reply related
* [GIT PULL] Please pull powerpc/linux.git powerpc-5.8-8 tag
From: Michael Ellerman @ 2020-07-31 13:05 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Aneesh Kumar K.V, linuxppc-dev, linux-kernel, npiggin
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Hi Linus,
Please pull one more powerpc fix for 5.8:
The following changes since commit f0479c4bcbd92d1a457d4a43bcab79f29d11334a:
selftests/powerpc: Use proper error code to check fault address (2020-07-15 23:10:17 +1000)
are available in the git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git tags/powerpc-5.8-8
for you to fetch changes up to 909adfc66b9a1db21b5e8733e9ebfa6cd5135d74:
powerpc/64s/hash: Fix hash_preload running with interrupts enabled (2020-07-27 17:02:09 +1000)
- ------------------------------------------------------------------
powerpc fixes for 5.8 #8
Fix a bug introduced by the changes we made to lockless page table walking this
cycle. When using the hash MMU, and perf with callchain recording, we can
deadlock if the PMI interrupts a hash fault, and the callchain recording then
takes a hash fault on the same page.
Thanks to:
Nicholas Piggin, Aneesh Kumar K.V, Anton Blanchard, Athira Rajeev.
- ------------------------------------------------------------------
Nicholas Piggin (1):
powerpc/64s/hash: Fix hash_preload running with interrupts enabled
arch/powerpc/kernel/exceptions-64s.S | 14 ++++++++---
arch/powerpc/mm/book3s64/hash_utils.c | 25 ++++++++++++++++++++
arch/powerpc/perf/core-book3s.c | 6 +++++
3 files changed, 42 insertions(+), 3 deletions(-)
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEEJFGtCPCthwEv2Y/bUevqPMjhpYAFAl8kFsQACgkQUevqPMjh
pYAkbg/9HzJB8KbjAVJpoXHOf9+aHylhZKU2pcVtdoYE07VV6JabWnlI5flvTSEY
3Dvr3h/X0k2Z3n52+s1kYlW27ErU+o9Hbz1j4O41Ardiy14GmQ02FSi5iXjhKxuk
AjX2MN76n1Te+1ee2Ib/Ubu8KOrbxLIya223tJ0TNu7RWuYVaCO/lNXPPTwm2JWI
s9lryRAHNc4qkPMytsWi/gzNml3IP6cqnGVt1H5fmnOwQXUYbSjdmhBa7AwjCsA1
xJBMkxBxWOnUSknRCxaCCUTQ3sD+sJMzp6verlwEIb+HRh08iJyVzuWCpT9m0ZOG
5F3CvhIxwLatDsJ9N5EeGI2V4qBstBlHEUJJAjkwKgZxzmEm7j9H+ItAc421eo2A
t/SYJTK4JltllCDyB22jCxdEgJh+opv8rki7U+INI01I8gHzqsr/0CHleGtMN5Cq
fUBf25LOMUCpjeAO207j4DzeY6rTXz9H07XWrR4su//4S9bNkYUYpFfeE/3Kwwdq
vZCxjm+fxxLPyAW+E2D2EGsyzIJootrAzly5T7gun8qwgCSgoCxI+YV9/DLmYaGf
OjcG6+X58sfn1FTc8cD6udKCIMKh0JmSccZRqw1Ijp4EiGMnDZUbP9jg/aSciD8a
+oco9QGvvxYQTMut4XUkXtqqZqaTISgWFD/QB9f4KGJ1Rs8tW5Y=
=oaTB
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: explicitly reschedule during drmem_lmb list traversal
From: Michael Ellerman @ 2020-07-31 13:16 UTC (permalink / raw)
To: Nathan Lynch, Laurent Dufour; +Cc: tyreld, cheloha, linuxppc-dev
In-Reply-To: <875za511z6.fsf@linux.ibm.com>
Nathan Lynch <nathanl@linux.ibm.com> writes:
> Michael Ellerman <mpe@ellerman.id.au> writes:
>> Nathan Lynch <nathanl@linux.ibm.com> writes:
>>> Laurent Dufour <ldufour@linux.ibm.com> writes:
>>>> Le 28/07/2020 à 19:37, Nathan Lynch a écrit :
>>>>> The drmem lmb list can have hundreds of thousands of entries, and
>>>>> unfortunately lookups take the form of linear searches. As long as
>>>>> this is the case, traversals have the potential to monopolize the CPU
>>>>> and provoke lockup reports, workqueue stalls, and the like unless
>>>>> they explicitly yield.
>>>>>
>>>>> Rather than placing cond_resched() calls within various
>>>>> for_each_drmem_lmb() loop blocks in the code, put it in the iteration
>>>>> expression of the loop macro itself so users can't omit it.
>>>>
>>>> Is that not too much to call cond_resched() on every LMB?
>>>>
>>>> Could that be less frequent, every 10, or 100, I don't really know ?
>>>
>>> Everything done within for_each_drmem_lmb is relatively heavyweight
>>> already. E.g. calling dlpar_remove_lmb()/dlpar_add_lmb() can take dozens
>>> of milliseconds. I don't think cond_resched() is an expensive check in
>>> this context.
>>
>> Hmm, mostly.
>>
>> But there are quite a few cases like drmem_update_dt_v1():
>>
>> for_each_drmem_lmb(lmb) {
>> dr_cell->base_addr = cpu_to_be64(lmb->base_addr);
>> dr_cell->drc_index = cpu_to_be32(lmb->drc_index);
>> dr_cell->aa_index = cpu_to_be32(lmb->aa_index);
>> dr_cell->flags = cpu_to_be32(drmem_lmb_flags(lmb));
>>
>> dr_cell++;
>> }
>>
>> Which will compile to a pretty tight loop at the moment.
>>
>> Or drmem_update_dt_v2() which has two loops over all lmbs.
>>
>> And although the actual TIF check is cheap the function call to do it is
>> not free.
>>
>> So I worry this is going to make some of those long loops take even
>> longer.
>
> That's fair, and I was wrong - some of the loop bodies are relatively
> simple, not doing allocations or taking locks, etc.
>
> One way to deal is to keep for_each_drmem_lmb() as-is and add a new
> iterator that can reschedule, e.g. for_each_drmem_lmb_slow().
If we did that, how many call-sites would need converting?
Is it ~2 or ~20 or ~200?
> On the other hand... it's probably not too strong to say that the
> drmem/hotplug code is in crisis with respect to correctness and
> algorithmic complexity, so those are my overriding concerns right
> now. Yes, this change will pessimize loops that are reinitializing the
> entire drmem_lmb array on every DLPAR operation, but:
>
> 1. it doesn't make any user of for_each_drmem_lmb() less correct;
> 2. why is this code doing that in the first place, other than to
> accommodate a poor data structure choice?
>
> The duration of the system calls where this code runs are measured in
> minutes or hours on large configurations because of all the behaviors
> that are at best O(n) with the amount of memory assigned to the
> partition. For simplicity's sake I'd rather defer lower-level
> performance considerations like this until the drmem data structures'
> awful lookup properties are fixed -- hopefully in the 5.10 timeframe.
Yeah fair enough.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: explicitly reschedule during drmem_lmb list traversal
From: Nathan Lynch @ 2020-07-31 13:52 UTC (permalink / raw)
To: Michael Ellerman, Laurent Dufour; +Cc: tyreld, cheloha, linuxppc-dev
In-Reply-To: <87ft974yf7.fsf@mpe.ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
> Nathan Lynch <nathanl@linux.ibm.com> writes:
>> Michael Ellerman <mpe@ellerman.id.au> writes:
>>> Nathan Lynch <nathanl@linux.ibm.com> writes:
>>>> Laurent Dufour <ldufour@linux.ibm.com> writes:
>>>>> Le 28/07/2020 à 19:37, Nathan Lynch a écrit :
>>>>>> The drmem lmb list can have hundreds of thousands of entries, and
>>>>>> unfortunately lookups take the form of linear searches. As long as
>>>>>> this is the case, traversals have the potential to monopolize the CPU
>>>>>> and provoke lockup reports, workqueue stalls, and the like unless
>>>>>> they explicitly yield.
>>>>>>
>>>>>> Rather than placing cond_resched() calls within various
>>>>>> for_each_drmem_lmb() loop blocks in the code, put it in the iteration
>>>>>> expression of the loop macro itself so users can't omit it.
>>>>>
>>>>> Is that not too much to call cond_resched() on every LMB?
>>>>>
>>>>> Could that be less frequent, every 10, or 100, I don't really know ?
>>>>
>>>> Everything done within for_each_drmem_lmb is relatively heavyweight
>>>> already. E.g. calling dlpar_remove_lmb()/dlpar_add_lmb() can take dozens
>>>> of milliseconds. I don't think cond_resched() is an expensive check in
>>>> this context.
>>>
>>> Hmm, mostly.
>>>
>>> But there are quite a few cases like drmem_update_dt_v1():
>>>
>>> for_each_drmem_lmb(lmb) {
>>> dr_cell->base_addr = cpu_to_be64(lmb->base_addr);
>>> dr_cell->drc_index = cpu_to_be32(lmb->drc_index);
>>> dr_cell->aa_index = cpu_to_be32(lmb->aa_index);
>>> dr_cell->flags = cpu_to_be32(drmem_lmb_flags(lmb));
>>>
>>> dr_cell++;
>>> }
>>>
>>> Which will compile to a pretty tight loop at the moment.
>>>
>>> Or drmem_update_dt_v2() which has two loops over all lmbs.
>>>
>>> And although the actual TIF check is cheap the function call to do it is
>>> not free.
>>>
>>> So I worry this is going to make some of those long loops take even
>>> longer.
>>
>> That's fair, and I was wrong - some of the loop bodies are relatively
>> simple, not doing allocations or taking locks, etc.
>>
>> One way to deal is to keep for_each_drmem_lmb() as-is and add a new
>> iterator that can reschedule, e.g. for_each_drmem_lmb_slow().
>
> If we did that, how many call-sites would need converting?
> Is it ~2 or ~20 or ~200?
At a glance I would convert 15-20 out of the 24 users in the tree I'm
looking at. Let me know if I should do a v2 with that approach.
^ permalink raw reply
* Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.8-8 tag
From: pr-tracker-bot @ 2020-07-31 16:45 UTC (permalink / raw)
To: Michael Ellerman
Cc: linuxppc-dev, Linus Torvalds, linux-kernel, npiggin,
Aneesh Kumar K.V
In-Reply-To: <87ime34yya.fsf@mpe.ellerman.id.au>
The pull request you sent on Fri, 31 Jul 2020 23:05:17 +1000:
> https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git tags/powerpc-5.8-8
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/deacdb3e3979979016fcd0ffd518c320a62ad166
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ 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