* [PATCH v4 08/16] powerpc/watchpoint: Disable all available watchpoints when !dawr_force_enable
From: Ravi Bangoria @ 2020-04-30 4:34 UTC (permalink / raw)
To: mpe, mikey
Cc: apopple, ravi.bangoria, peterz, fweisbec, oleg, npiggin,
linux-kernel, paulus, jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <20200430043417.30948-1-ravi.bangoria@linux.ibm.com>
Instead of disabling only first watchpoint, disable all available
watchpoints while clearing dawr_force_enable.
Callback function is used only for disabling watchpoint, rename it
to disable_dawrs_cb(). And null_brk parameter is not really required
while disabling watchpoint, remove it.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Reviewed-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/kernel/dawr.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/dawr.c b/arch/powerpc/kernel/dawr.c
index 8114ad3a8574..500f52fa4711 100644
--- a/arch/powerpc/kernel/dawr.c
+++ b/arch/powerpc/kernel/dawr.c
@@ -50,9 +50,13 @@ int set_dawr(int nr, struct arch_hw_breakpoint *brk)
return 0;
}
-static void set_dawr_cb(void *info)
+static void disable_dawrs_cb(void *info)
{
- set_dawr(0, info);
+ struct arch_hw_breakpoint null_brk = {0};
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++)
+ set_dawr(i, &null_brk);
}
static ssize_t dawr_write_file_bool(struct file *file,
@@ -74,7 +78,7 @@ static ssize_t dawr_write_file_bool(struct file *file,
/* If we are clearing, make sure all CPUs have the DAWR cleared */
if (!dawr_force_enable)
- smp_call_function(set_dawr_cb, &null_brk, 0);
+ smp_call_function(disable_dawrs_cb, NULL, 0);
return rc;
}
--
2.21.1
^ permalink raw reply related
* [PATCH v4 09/16] powerpc/watchpoint: Convert thread_struct->hw_brk to an array
From: Ravi Bangoria @ 2020-04-30 4:34 UTC (permalink / raw)
To: mpe, mikey
Cc: apopple, ravi.bangoria, peterz, fweisbec, oleg, npiggin,
linux-kernel, paulus, jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <20200430043417.30948-1-ravi.bangoria@linux.ibm.com>
So far powerpc hw supported only one watchpoint. But Future Power
architecture is introducing 2nd DAWR. Convert thread_struct->hw_brk
into an array.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Reviewed-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/include/asm/processor.h | 2 +-
arch/powerpc/kernel/process.c | 60 ++++++++++++++---------
arch/powerpc/kernel/ptrace/ptrace-noadv.c | 40 ++++++++++-----
arch/powerpc/kernel/ptrace/ptrace32.c | 4 +-
arch/powerpc/kernel/signal.c | 13 +++--
5 files changed, 78 insertions(+), 41 deletions(-)
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index a71bdd6bc284..668c02c67b61 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -187,7 +187,7 @@ struct thread_struct {
*/
struct perf_event *last_hit_ubp;
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
- struct arch_hw_breakpoint hw_brk; /* info on the hardware breakpoint */
+ struct arch_hw_breakpoint hw_brk[HBP_NUM_MAX]; /* hardware breakpoint info */
unsigned long trap_nr; /* last trap # on this thread */
u8 load_slb; /* Ages out SLB preload cache entries */
u8 load_fp;
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 351fbd8d2c5b..6d1b7cede900 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -711,21 +711,49 @@ void switch_booke_debug_regs(struct debug_reg *new_debug)
EXPORT_SYMBOL_GPL(switch_booke_debug_regs);
#else /* !CONFIG_PPC_ADV_DEBUG_REGS */
#ifndef CONFIG_HAVE_HW_BREAKPOINT
-static void set_breakpoint(struct arch_hw_breakpoint *brk)
+static void set_breakpoint(int i, struct arch_hw_breakpoint *brk)
{
preempt_disable();
- __set_breakpoint(0, brk);
+ __set_breakpoint(i, brk);
preempt_enable();
}
static void set_debug_reg_defaults(struct thread_struct *thread)
{
- thread->hw_brk.address = 0;
- thread->hw_brk.type = 0;
- thread->hw_brk.len = 0;
- thread->hw_brk.hw_len = 0;
- if (ppc_breakpoint_available())
- set_breakpoint(&thread->hw_brk);
+ int i;
+ struct arch_hw_breakpoint null_brk = {0};
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ thread->hw_brk[i] = null_brk;
+ if (ppc_breakpoint_available())
+ set_breakpoint(i, &thread->hw_brk[i]);
+ }
+}
+
+static inline bool hw_brk_match(struct arch_hw_breakpoint *a,
+ struct arch_hw_breakpoint *b)
+{
+ if (a->address != b->address)
+ return false;
+ if (a->type != b->type)
+ return false;
+ if (a->len != b->len)
+ return false;
+ /* no need to check hw_len. it's calculated from address and len */
+ return true;
+}
+
+static void switch_hw_breakpoint(struct task_struct *new)
+{
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (likely(hw_brk_match(this_cpu_ptr(¤t_brk[i]),
+ &new->thread.hw_brk[i])))
+ continue;
+
+ __set_breakpoint(i, &new->thread.hw_brk[i]);
+ }
}
#endif /* !CONFIG_HAVE_HW_BREAKPOINT */
#endif /* CONFIG_PPC_ADV_DEBUG_REGS */
@@ -829,19 +857,6 @@ bool ppc_breakpoint_available(void)
}
EXPORT_SYMBOL_GPL(ppc_breakpoint_available);
-static inline bool hw_brk_match(struct arch_hw_breakpoint *a,
- struct arch_hw_breakpoint *b)
-{
- if (a->address != b->address)
- return false;
- if (a->type != b->type)
- return false;
- if (a->len != b->len)
- return false;
- /* no need to check hw_len. it's calculated from address and len */
- return true;
-}
-
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
static inline bool tm_enabled(struct task_struct *tsk)
@@ -1174,8 +1189,7 @@ struct task_struct *__switch_to(struct task_struct *prev,
* schedule DABR
*/
#ifndef CONFIG_HAVE_HW_BREAKPOINT
- if (unlikely(!hw_brk_match(this_cpu_ptr(¤t_brk[0]), &new->thread.hw_brk)))
- __set_breakpoint(0, &new->thread.hw_brk);
+ switch_hw_breakpoint(new);
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
#endif
diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
index 12962302d6a4..0dbb35392dd2 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
@@ -67,11 +67,16 @@ int ptrace_get_debugreg(struct task_struct *child, unsigned long addr,
/* We only support one DABR and no IABRS at the moment */
if (addr > 0)
return -EINVAL;
- dabr_fake = ((child->thread.hw_brk.address & (~HW_BRK_TYPE_DABR)) |
- (child->thread.hw_brk.type & HW_BRK_TYPE_DABR));
+ dabr_fake = ((child->thread.hw_brk[0].address & (~HW_BRK_TYPE_DABR)) |
+ (child->thread.hw_brk[0].type & HW_BRK_TYPE_DABR));
return put_user(dabr_fake, datalp);
}
+/*
+ * ptrace_set_debugreg() fakes DABR and DABR is only one. So even if
+ * internal hw supports more than one watchpoint, we support only one
+ * watchpoint with this interface.
+ */
int ptrace_set_debugreg(struct task_struct *task, unsigned long addr, unsigned long data)
{
#ifdef CONFIG_HAVE_HW_BREAKPOINT
@@ -137,7 +142,7 @@ int ptrace_set_debugreg(struct task_struct *task, unsigned long addr, unsigned l
return ret;
thread->ptrace_bps[0] = bp;
- thread->hw_brk = hw_brk;
+ thread->hw_brk[0] = hw_brk;
return 0;
}
@@ -159,12 +164,24 @@ int ptrace_set_debugreg(struct task_struct *task, unsigned long addr, unsigned l
if (set_bp && (!ppc_breakpoint_available()))
return -ENODEV;
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
- task->thread.hw_brk = hw_brk;
+ task->thread.hw_brk[0] = hw_brk;
return 0;
}
+static int find_empty_hw_brk(struct thread_struct *thread)
+{
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!thread->hw_brk[i].address)
+ return i;
+ }
+ return -1;
+}
+
long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_info)
{
+ int i;
#ifdef CONFIG_HAVE_HW_BREAKPOINT
int len = 0;
struct thread_struct *thread = &child->thread;
@@ -223,15 +240,16 @@ long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_inf
if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT)
return -EINVAL;
- if (child->thread.hw_brk.address)
+ i = find_empty_hw_brk(&child->thread);
+ if (i < 0)
return -ENOSPC;
if (!ppc_breakpoint_available())
return -ENODEV;
- child->thread.hw_brk = brk;
+ child->thread.hw_brk[i] = brk;
- return 1;
+ return i + 1;
}
long ppc_del_hwdebug(struct task_struct *child, long data)
@@ -241,7 +259,7 @@ long ppc_del_hwdebug(struct task_struct *child, long data)
struct thread_struct *thread = &child->thread;
struct perf_event *bp;
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
- if (data != 1)
+ if (data < 1 || data > nr_wp_slots())
return -EINVAL;
#ifdef CONFIG_HAVE_HW_BREAKPOINT
@@ -254,11 +272,11 @@ long ppc_del_hwdebug(struct task_struct *child, long data)
}
return ret;
#else /* CONFIG_HAVE_HW_BREAKPOINT */
- if (child->thread.hw_brk.address == 0)
+ if (child->thread.hw_brk[data - 1].address == 0)
return -ENOENT;
- child->thread.hw_brk.address = 0;
- child->thread.hw_brk.type = 0;
+ child->thread.hw_brk[data - 1].address = 0;
+ child->thread.hw_brk[data - 1].type = 0;
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
return 0;
diff --git a/arch/powerpc/kernel/ptrace/ptrace32.c b/arch/powerpc/kernel/ptrace/ptrace32.c
index 7976ddf29c0e..7589a9665ffb 100644
--- a/arch/powerpc/kernel/ptrace/ptrace32.c
+++ b/arch/powerpc/kernel/ptrace/ptrace32.c
@@ -259,8 +259,8 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
ret = put_user(child->thread.debug.dac1, (u32 __user *)data);
#else
dabr_fake = (
- (child->thread.hw_brk.address & (~HW_BRK_TYPE_DABR)) |
- (child->thread.hw_brk.type & HW_BRK_TYPE_DABR));
+ (child->thread.hw_brk[0].address & (~HW_BRK_TYPE_DABR)) |
+ (child->thread.hw_brk[0].type & HW_BRK_TYPE_DABR));
ret = put_user(dabr_fake, (u32 __user *)data);
#endif
break;
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index 1a04a4b18741..a926457cf57c 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -262,15 +262,20 @@ static void do_signal(struct task_struct *tsk)
return; /* no signals delivered */
}
-#ifndef CONFIG_PPC_ADV_DEBUG_REGS
/*
* Reenable the DABR before delivering the signal to
* user space. The DABR will have been cleared if it
* triggered inside the kernel.
*/
- if (tsk->thread.hw_brk.address && tsk->thread.hw_brk.type)
- __set_breakpoint(0, &tsk->thread.hw_brk);
-#endif
+ if (!IS_ENABLED(CONFIG_PPC_ADV_DEBUG_REGS)) {
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (tsk->thread.hw_brk[i].address && tsk->thread.hw_brk[i].type)
+ __set_breakpoint(i, &tsk->thread.hw_brk[i]);
+ }
+ }
+
/* Re-enable the breakpoints for the signal stack */
thread_change_pc(tsk, tsk->thread.regs);
--
2.21.1
^ permalink raw reply related
* [PATCH v4 10/16] powerpc/watchpoint: Use loop for thread_struct->ptrace_bps
From: Ravi Bangoria @ 2020-04-30 4:34 UTC (permalink / raw)
To: mpe, mikey
Cc: apopple, ravi.bangoria, peterz, fweisbec, oleg, npiggin,
linux-kernel, paulus, jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <20200430043417.30948-1-ravi.bangoria@linux.ibm.com>
ptrace_bps is already an array of size HBP_NUM_MAX. But we use
hardcoded index 0 while fetching/updating it. Convert such code
to loop over array.
ptrace interface to use multiple watchpoint remains same. eg:
two PPC_PTRACE_SETHWDEBUG calls will create two watchpoint if
hw underneath supports it.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Reviewed-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/kernel/hw_breakpoint.c | 7 ++++--
arch/powerpc/kernel/process.c | 6 ++++-
arch/powerpc/kernel/ptrace/ptrace-noadv.c | 28 +++++++++++++++++------
3 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 5826f1f2cab9..772b2c953220 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -419,10 +419,13 @@ NOKPROBE_SYMBOL(hw_breakpoint_exceptions_notify);
*/
void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
{
+ int i;
struct thread_struct *t = &tsk->thread;
- unregister_hw_breakpoint(t->ptrace_bps[0]);
- t->ptrace_bps[0] = NULL;
+ for (i = 0; i < nr_wp_slots(); i++) {
+ unregister_hw_breakpoint(t->ptrace_bps[i]);
+ t->ptrace_bps[i] = NULL;
+ }
}
void hw_breakpoint_pmu_read(struct perf_event *bp)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 6d1b7cede900..41a59a37383b 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1604,6 +1604,9 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
void (*f)(void);
unsigned long sp = (unsigned long)task_stack_page(p) + THREAD_SIZE;
struct thread_info *ti = task_thread_info(p);
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+ int i;
+#endif
klp_init_thread_info(p);
@@ -1663,7 +1666,8 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
p->thread.ksp_limit = (unsigned long)end_of_stack(p);
#endif
#ifdef CONFIG_HAVE_HW_BREAKPOINT
- p->thread.ptrace_bps[0] = NULL;
+ for (i = 0; i < nr_wp_slots(); i++)
+ p->thread.ptrace_bps[i] = NULL;
#endif
p->thread.fp_save_area = NULL;
diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
index 0dbb35392dd2..08cb8c1b504c 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
@@ -168,6 +168,19 @@ int ptrace_set_debugreg(struct task_struct *task, unsigned long addr, unsigned l
return 0;
}
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+static int find_empty_ptrace_bp(struct thread_struct *thread)
+{
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!thread->ptrace_bps[i])
+ return i;
+ }
+ return -1;
+}
+#endif
+
static int find_empty_hw_brk(struct thread_struct *thread)
{
int i;
@@ -217,8 +230,9 @@ long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_inf
len = 1;
else
return -EINVAL;
- bp = thread->ptrace_bps[0];
- if (bp)
+
+ i = find_empty_ptrace_bp(thread);
+ if (i < 0)
return -ENOSPC;
/* Create a new breakpoint request if one doesn't exist already */
@@ -228,13 +242,13 @@ long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_inf
arch_bp_generic_fields(brk.type, &attr.bp_type);
bp = register_user_hw_breakpoint(&attr, ptrace_triggered, NULL, child);
- thread->ptrace_bps[0] = bp;
+ thread->ptrace_bps[i] = bp;
if (IS_ERR(bp)) {
- thread->ptrace_bps[0] = NULL;
+ thread->ptrace_bps[i] = NULL;
return PTR_ERR(bp);
}
- return 1;
+ return i + 1;
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT)
@@ -263,10 +277,10 @@ long ppc_del_hwdebug(struct task_struct *child, long data)
return -EINVAL;
#ifdef CONFIG_HAVE_HW_BREAKPOINT
- bp = thread->ptrace_bps[0];
+ bp = thread->ptrace_bps[data - 1];
if (bp) {
unregister_hw_breakpoint(bp);
- thread->ptrace_bps[0] = NULL;
+ thread->ptrace_bps[data - 1] = NULL;
} else {
ret = -ENOENT;
}
--
2.21.1
^ permalink raw reply related
* [PATCH v4 11/16] powerpc/watchpoint: Introduce is_ptrace_bp() function
From: Ravi Bangoria @ 2020-04-30 4:34 UTC (permalink / raw)
To: mpe, mikey
Cc: apopple, ravi.bangoria, peterz, fweisbec, oleg, npiggin,
linux-kernel, paulus, jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <20200430043417.30948-1-ravi.bangoria@linux.ibm.com>
Introduce is_ptrace_bp() function and move the check inside the
function. We will utilize it more in later set of patches.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Reviewed-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/kernel/hw_breakpoint.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 772b2c953220..319a761b7412 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -90,6 +90,11 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
hw_breakpoint_disable();
}
+static bool is_ptrace_bp(struct perf_event *bp)
+{
+ return bp->overflow_handler == ptrace_triggered;
+}
+
/*
* Perform cleanup of arch-specific counters during unregistration
* of the perf-event
@@ -324,7 +329,7 @@ int hw_breakpoint_handler(struct die_args *args)
* one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
* generated in do_dabr().
*/
- if (bp->overflow_handler == ptrace_triggered) {
+ if (is_ptrace_bp(bp)) {
perf_bp_event(bp, regs);
rc = NOTIFY_DONE;
goto out;
--
2.21.1
^ permalink raw reply related
* [PATCH v4 12/16] powerpc/watchpoint: Use builtin ALIGN*() macros
From: Ravi Bangoria @ 2020-04-30 4:34 UTC (permalink / raw)
To: mpe, mikey
Cc: apopple, ravi.bangoria, peterz, fweisbec, oleg, npiggin,
linux-kernel, paulus, jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <20200430043417.30948-1-ravi.bangoria@linux.ibm.com>
Currently we calculate hw aligned start and end addresses manually.
Replace them with builtin ALIGN_DOWN() and ALIGN() macros.
So far end_addr was inclusive but this patch makes it exclusive (by
avoiding -1) for better readability.
Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Reviewed-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/include/asm/hw_breakpoint.h | 5 +++--
arch/powerpc/kernel/hw_breakpoint.c | 12 ++++++------
arch/powerpc/kernel/process.c | 8 ++++----
arch/powerpc/kernel/ptrace/ptrace-noadv.c | 2 +-
4 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index d472b2eb757e..add5aa076919 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -34,10 +34,11 @@ struct arch_hw_breakpoint {
#define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \
HW_BRK_TYPE_HYP)
+/* Minimum granularity */
#ifdef CONFIG_PPC_8xx
-#define HW_BREAKPOINT_ALIGN 0x3
+#define HW_BREAKPOINT_SIZE 0x4
#else
-#define HW_BREAKPOINT_ALIGN 0x7
+#define HW_BREAKPOINT_SIZE 0x8
#endif
#define DABR_MAX_LEN 8
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 319a761b7412..ab0dd22fed5f 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -145,10 +145,10 @@ int arch_bp_generic_fields(int type, int *gen_bp_type)
* <---8 bytes--->
*
* In this case, we should configure hw as:
- * start_addr = address & ~HW_BREAKPOINT_ALIGN
+ * start_addr = address & ~(HW_BREAKPOINT_SIZE - 1)
* len = 16 bytes
*
- * @start_addr and @end_addr are inclusive.
+ * @start_addr is inclusive but @end_addr is exclusive.
*/
static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
{
@@ -156,14 +156,14 @@ static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
u16 hw_len;
unsigned long start_addr, end_addr;
- start_addr = hw->address & ~HW_BREAKPOINT_ALIGN;
- end_addr = (hw->address + hw->len - 1) | HW_BREAKPOINT_ALIGN;
- hw_len = end_addr - start_addr + 1;
+ start_addr = ALIGN_DOWN(hw->address, HW_BREAKPOINT_SIZE);
+ end_addr = ALIGN(hw->address + hw->len, HW_BREAKPOINT_SIZE);
+ hw_len = end_addr - start_addr;
if (dawr_enabled()) {
max_len = DAWR_MAX_LEN;
/* DAWR region can't cross 512 bytes boundary */
- if ((start_addr >> 9) != (end_addr >> 9))
+ if (ALIGN(start_addr, SZ_512M) != ALIGN(end_addr - 1, SZ_512M))
return -EINVAL;
} else if (IS_ENABLED(CONFIG_PPC_8xx)) {
/* 8xx can setup a range without limitation */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 41a59a37383b..dcf9c5b4ac59 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -800,12 +800,12 @@ static inline int set_breakpoint_8xx(struct arch_hw_breakpoint *brk)
unsigned long lctrl1 = LCTRL1_CTE_GT | LCTRL1_CTF_LT | LCTRL1_CRWE_RW |
LCTRL1_CRWF_RW;
unsigned long lctrl2 = LCTRL2_LW0EN | LCTRL2_LW0LADC | LCTRL2_SLW0EN;
- unsigned long start_addr = brk->address & ~HW_BREAKPOINT_ALIGN;
- unsigned long end_addr = (brk->address + brk->len - 1) | HW_BREAKPOINT_ALIGN;
+ unsigned long start_addr = ALIGN_DOWN(brk->address, HW_BREAKPOINT_SIZE);
+ unsigned long end_addr = ALIGN(brk->address + brk->len, HW_BREAKPOINT_SIZE);
if (start_addr == 0)
lctrl2 |= LCTRL2_LW0LA_F;
- else if (end_addr == ~0U)
+ else if (end_addr == 0)
lctrl2 |= LCTRL2_LW0LA_E;
else
lctrl2 |= LCTRL2_LW0LA_EandF;
@@ -821,7 +821,7 @@ static inline int set_breakpoint_8xx(struct arch_hw_breakpoint *brk)
lctrl1 |= LCTRL1_CRWE_WO | LCTRL1_CRWF_WO;
mtspr(SPRN_CMPE, start_addr - 1);
- mtspr(SPRN_CMPF, end_addr + 1);
+ mtspr(SPRN_CMPF, end_addr);
mtspr(SPRN_LCTRL1, lctrl1);
mtspr(SPRN_LCTRL2, lctrl2);
diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
index 08cb8c1b504c..697c7e4b5877 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
@@ -216,7 +216,7 @@ long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_inf
if ((unsigned long)bp_info->addr >= TASK_SIZE)
return -EIO;
- brk.address = bp_info->addr & ~HW_BREAKPOINT_ALIGN;
+ brk.address = ALIGN_DOWN(bp_info->addr, HW_BREAKPOINT_SIZE);
brk.type = HW_BRK_TYPE_TRANSLATE;
brk.len = DABR_MAX_LEN;
if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
--
2.21.1
^ permalink raw reply related
* [PATCH v4 13/16] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint
From: Ravi Bangoria @ 2020-04-30 4:34 UTC (permalink / raw)
To: mpe, mikey
Cc: apopple, ravi.bangoria, peterz, fweisbec, oleg, npiggin,
linux-kernel, paulus, jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <20200430043417.30948-1-ravi.bangoria@linux.ibm.com>
Currently we assume that we have only one watchpoint supported by hw.
Get rid of that assumption and use dynamic loop instead. This should
make supporting more watchpoints very easy.
With more than one watchpoint, exception handler need to know which
DAWR caused the exception, and hw currently does not provide it. So
we need sw logic for the same. To figure out which DAWR caused the
exception, check all different combinations of user specified range,
dawr address range, actual access range and dawrx constrains. For ex,
if user specified range and actual access range overlaps but dawrx is
configured for readonly watchpoint and the instruction is store, this
DAWR must not have caused exception.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Reviewed-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/include/asm/processor.h | 2 +-
arch/powerpc/include/asm/sstep.h | 2 +
arch/powerpc/kernel/hw_breakpoint.c | 400 +++++++++++++++++++++------
arch/powerpc/kernel/process.c | 3 -
4 files changed, 315 insertions(+), 92 deletions(-)
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index 668c02c67b61..251f50eec9fa 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -185,7 +185,7 @@ struct thread_struct {
* Helps identify source of single-step exception and subsequent
* hw-breakpoint enablement
*/
- struct perf_event *last_hit_ubp;
+ struct perf_event *last_hit_ubp[HBP_NUM_MAX];
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
struct arch_hw_breakpoint hw_brk[HBP_NUM_MAX]; /* hardware breakpoint info */
unsigned long trap_nr; /* last trap # on this thread */
diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
index 769f055509c9..38919b27a6fa 100644
--- a/arch/powerpc/include/asm/sstep.h
+++ b/arch/powerpc/include/asm/sstep.h
@@ -48,6 +48,8 @@ enum instruction_type {
#define INSTR_TYPE_MASK 0x1f
+#define OP_IS_LOAD(type) ((LOAD <= (type) && (type) <= LOAD_VSX) || (type) == LARX)
+#define OP_IS_STORE(type) ((STORE <= (type) && (type) <= STORE_VSX) || (type) == STCX)
#define OP_IS_LOAD_STORE(type) (LOAD <= (type) && (type) <= STCX)
/* Compute flags, ORed in with type */
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index ab0dd22fed5f..28d57d841642 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -30,7 +30,7 @@
* Stores the breakpoints currently in use on each breakpoint address
* register for every cpu
*/
-static DEFINE_PER_CPU(struct perf_event *, bp_per_reg);
+static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM_MAX]);
/*
* Returns total number of data or instruction breakpoints available.
@@ -42,6 +42,17 @@ int hw_breakpoint_slots(int type)
return 0; /* no instruction breakpoints available */
}
+static bool single_step_pending(void)
+{
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (current->thread.last_hit_ubp[i])
+ return true;
+ }
+ return false;
+}
+
/*
* Install a perf counter breakpoint.
*
@@ -54,16 +65,26 @@ int hw_breakpoint_slots(int type)
int arch_install_hw_breakpoint(struct perf_event *bp)
{
struct arch_hw_breakpoint *info = counter_arch_bp(bp);
- struct perf_event **slot = this_cpu_ptr(&bp_per_reg);
+ struct perf_event **slot;
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ slot = this_cpu_ptr(&bp_per_reg[i]);
+ if (!*slot) {
+ *slot = bp;
+ break;
+ }
+ }
- *slot = bp;
+ if (WARN_ONCE(i == nr_wp_slots(), "Can't find any breakpoint slot"))
+ return -EBUSY;
/*
* Do not install DABR values if the instruction must be single-stepped.
* If so, DABR will be populated in single_step_dabr_instruction().
*/
- if (current->thread.last_hit_ubp != bp)
- __set_breakpoint(0, info);
+ if (!single_step_pending())
+ __set_breakpoint(i, info);
return 0;
}
@@ -79,15 +100,22 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
*/
void arch_uninstall_hw_breakpoint(struct perf_event *bp)
{
- struct perf_event **slot = this_cpu_ptr(&bp_per_reg);
+ struct arch_hw_breakpoint null_brk = {0};
+ struct perf_event **slot;
+ int i;
- if (*slot != bp) {
- WARN_ONCE(1, "Can't find the breakpoint");
- return;
+ for (i = 0; i < nr_wp_slots(); i++) {
+ slot = this_cpu_ptr(&bp_per_reg[i]);
+ if (*slot == bp) {
+ *slot = NULL;
+ break;
+ }
}
- *slot = NULL;
- hw_breakpoint_disable();
+ if (WARN_ONCE(i == nr_wp_slots(), "Can't find any breakpoint slot"))
+ return;
+
+ __set_breakpoint(i, &null_brk);
}
static bool is_ptrace_bp(struct perf_event *bp)
@@ -107,8 +135,14 @@ void arch_unregister_hw_breakpoint(struct perf_event *bp)
* restoration variables to prevent dangling pointers.
* FIXME, this should not be using bp->ctx at all! Sayeth peterz.
*/
- if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L))
- bp->ctx->task->thread.last_hit_ubp = NULL;
+ if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L)) {
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (bp->ctx->task->thread.last_hit_ubp[i] == bp)
+ bp->ctx->task->thread.last_hit_ubp[i] = NULL;
+ }
+ }
}
/*
@@ -220,90 +254,215 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs)
{
struct arch_hw_breakpoint *info;
+ int i;
- if (likely(!tsk->thread.last_hit_ubp))
- return;
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (unlikely(tsk->thread.last_hit_ubp[i]))
+ goto reset;
+ }
+ return;
- info = counter_arch_bp(tsk->thread.last_hit_ubp);
+reset:
regs->msr &= ~MSR_SE;
- __set_breakpoint(0, info);
- tsk->thread.last_hit_ubp = NULL;
+ for (i = 0; i < nr_wp_slots(); i++) {
+ info = counter_arch_bp(__this_cpu_read(bp_per_reg[i]));
+ __set_breakpoint(i, info);
+ tsk->thread.last_hit_ubp[i] = NULL;
+ }
}
-static bool dar_within_range(unsigned long dar, struct arch_hw_breakpoint *info)
+static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info)
{
return ((info->address <= dar) && (dar - info->address < info->len));
}
-static bool
-dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
+static bool dar_user_range_overlaps(unsigned long dar, int size,
+ struct arch_hw_breakpoint *info)
{
- return ((dar <= info->address + info->len - 1) &&
- (dar + size - 1 >= info->address));
+ return ((dar < info->address + info->len) &&
+ (dar + size > info->address));
+}
+
+static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info)
+{
+ unsigned long hw_start_addr, hw_end_addr;
+
+ hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
+ hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
+
+ return ((hw_start_addr <= dar) && (hw_end_addr > dar));
+}
+
+static bool dar_hw_range_overlaps(unsigned long dar, int size,
+ struct arch_hw_breakpoint *info)
+{
+ unsigned long hw_start_addr, hw_end_addr;
+
+ hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
+ hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
+
+ return ((dar < hw_end_addr) && (dar + size > hw_start_addr));
}
/*
- * Handle debug exception notifications.
+ * If hw has multiple DAWR registers, we also need to check all
+ * dawrx constraint bits to confirm this is _really_ a valid event.
*/
-static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
- struct arch_hw_breakpoint *info)
+static bool check_dawrx_constraints(struct pt_regs *regs, int type,
+ struct arch_hw_breakpoint *info)
{
- unsigned int instr = 0;
- int ret, type, size;
- struct instruction_op op;
- unsigned long addr = info->address;
+ if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ))
+ return false;
- if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
- goto fail;
+ if (OP_IS_STORE(type) && !(info->type & HW_BRK_TYPE_WRITE))
+ return false;
- ret = analyse_instr(&op, regs, instr);
- type = GETTYPE(op.type);
- size = GETSIZE(op.type);
+ if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL))
+ return false;
- if (!ret && (type == LARX || type == STCX)) {
- printk_ratelimited("Breakpoint hit on instruction that can't be emulated."
- " Breakpoint at 0x%lx will be disabled.\n", addr);
- goto disable;
- }
+ if (user_mode(regs) && !(info->type & HW_BRK_TYPE_USER))
+ return false;
+
+ return true;
+}
+
+/*
+ * Returns true if the event is valid wrt dawr configuration,
+ * including extraneous exception. Otherwise return false.
+ */
+static bool check_constraints(struct pt_regs *regs, unsigned int instr,
+ int type, int size,
+ struct arch_hw_breakpoint *info)
+{
+ bool in_user_range = dar_in_user_range(regs->dar, info);
+ bool dawrx_constraints;
/*
- * If it's extraneous event, we still need to emulate/single-
- * step the instruction, but we don't generate an event.
+ * 8xx supports only one breakpoint and thus we can
+ * unconditionally return true.
*/
- if (size && !dar_range_overlaps(regs->dar, size, info))
- info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+ if (IS_ENABLED(CONFIG_PPC_8xx)) {
+ if (!in_user_range)
+ info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+ return true;
+ }
- /* Do not emulate user-space instructions, instead single-step them */
- if (user_mode(regs)) {
- current->thread.last_hit_ubp = bp;
- regs->msr |= MSR_SE;
+ if (unlikely(instr == -1)) {
+ if (in_user_range)
+ return true;
+
+ if (dar_in_hw_range(regs->dar, info)) {
+ info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+ return true;
+ }
return false;
}
- if (!emulate_step(regs, instr))
- goto fail;
+ dawrx_constraints = check_dawrx_constraints(regs, type, info);
- return true;
+ if (dar_user_range_overlaps(regs->dar, size, info))
+ return dawrx_constraints;
+
+ if (dar_hw_range_overlaps(regs->dar, size, info)) {
+ if (dawrx_constraints) {
+ info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
+ return true;
+ }
+ }
+ return false;
+}
+
+static int get_instr_detail(struct pt_regs *regs, int *type, int *size,
+ bool *larx_stcx)
+{
+ unsigned int instr = 0;
+ struct instruction_op op;
+
+ if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
+ return -1;
+
+ analyse_instr(&op, regs, instr);
-fail:
/*
- * We've failed in reliably handling the hw-breakpoint. Unregister
- * it and throw a warning message to let the user know about it.
+ * Set size = 8 if analyse_instr() fails. If it's a userspace
+ * watchpoint(valid or extraneous), we can notify user about it.
+ * If it's a kernel watchpoint, instruction emulation will fail
+ * in stepping_handler() and watchpoint will be disabled.
*/
- WARN(1, "Unable to handle hardware breakpoint. Breakpoint at "
- "0x%lx will be disabled.", addr);
+ *type = GETTYPE(op.type);
+ *size = !(*type == UNKNOWN) ? GETSIZE(op.type) : 8;
+ *larx_stcx = (*type == LARX || *type == STCX);
-disable:
+ return instr;
+}
+
+/*
+ * We've failed in reliably handling the hw-breakpoint. Unregister
+ * it and throw a warning message to let the user know about it.
+ */
+static void handler_error(struct perf_event *bp, struct arch_hw_breakpoint *info)
+{
+ WARN(1, "Unable to handle hardware breakpoint."
+ "Breakpoint at 0x%lx will be disabled.",
+ info->address);
+ perf_event_disable_inatomic(bp);
+}
+
+static void larx_stcx_err(struct perf_event *bp, struct arch_hw_breakpoint *info)
+{
+ printk_ratelimited("Breakpoint hit on instruction that can't "
+ "be emulated. Breakpoint at 0x%lx will be "
+ "disabled.\n", info->address);
perf_event_disable_inatomic(bp);
- return false;
+}
+
+static bool stepping_handler(struct pt_regs *regs, struct perf_event **bp,
+ struct arch_hw_breakpoint **info, int *hit,
+ unsigned int instr)
+{
+ int i;
+ int stepped;
+
+ /* Do not emulate user-space instructions, instead single-step them */
+ if (user_mode(regs)) {
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!hit[i])
+ continue;
+ current->thread.last_hit_ubp[i] = bp[i];
+ info[i] = NULL;
+ }
+ regs->msr |= MSR_SE;
+ return false;
+ }
+
+ stepped = emulate_step(regs, instr);
+ if (!stepped) {
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!hit[i])
+ continue;
+ handler_error(bp[i], info[i]);
+ info[i] = NULL;
+ }
+ return false;
+ }
+ return true;
}
int hw_breakpoint_handler(struct die_args *args)
{
+ bool err = false;
int rc = NOTIFY_STOP;
- struct perf_event *bp;
+ struct perf_event *bp[HBP_NUM_MAX] = {0};
struct pt_regs *regs = args->regs;
- struct arch_hw_breakpoint *info;
+ struct arch_hw_breakpoint *info[HBP_NUM_MAX] = {0};
+ int i;
+ int hit[HBP_NUM_MAX] = {0};
+ int nr_hit = 0;
+ bool ptrace_bp = false;
+ unsigned int instr = 0;
+ int type = 0;
+ int size = 0;
+ bool larx_stcx = false;
/* Disable breakpoints during exception handling */
hw_breakpoint_disable();
@@ -316,12 +475,39 @@ int hw_breakpoint_handler(struct die_args *args)
*/
rcu_read_lock();
- bp = __this_cpu_read(bp_per_reg);
- if (!bp) {
+ if (!IS_ENABLED(CONFIG_PPC_8xx))
+ instr = get_instr_detail(regs, &type, &size, &larx_stcx);
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ bp[i] = __this_cpu_read(bp_per_reg[i]);
+ if (!bp[i])
+ continue;
+
+ info[i] = counter_arch_bp(bp[i]);
+ info[i]->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ;
+
+ if (check_constraints(regs, instr, type, size, info[i])) {
+ if (!IS_ENABLED(CONFIG_PPC_8xx) && instr == -1) {
+ handler_error(bp[i], info[i]);
+ info[i] = NULL;
+ err = 1;
+ continue;
+ }
+
+ if (is_ptrace_bp(bp[i]))
+ ptrace_bp = true;
+ hit[i] = 1;
+ nr_hit++;
+ }
+ }
+
+ if (err)
+ goto reset;
+
+ if (!nr_hit) {
rc = NOTIFY_DONE;
goto out;
}
- info = counter_arch_bp(bp);
/*
* Return early after invoking user-callback function without restoring
@@ -329,29 +515,50 @@ int hw_breakpoint_handler(struct die_args *args)
* one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
* generated in do_dabr().
*/
- if (is_ptrace_bp(bp)) {
- perf_bp_event(bp, regs);
+ if (ptrace_bp) {
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!hit[i])
+ continue;
+ perf_bp_event(bp[i], regs);
+ info[i] = NULL;
+ }
rc = NOTIFY_DONE;
- goto out;
+ goto reset;
}
- info->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ;
- if (IS_ENABLED(CONFIG_PPC_8xx)) {
- if (!dar_within_range(regs->dar, info))
- info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
- } else {
- if (!stepping_handler(regs, bp, info))
- goto out;
+ if (!IS_ENABLED(CONFIG_PPC_8xx)) {
+ if (larx_stcx) {
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!hit[i])
+ continue;
+ larx_stcx_err(bp[i], info[i]);
+ info[i] = NULL;
+ }
+ goto reset;
+ }
+
+ if (!stepping_handler(regs, bp, info, hit, instr))
+ goto reset;
}
/*
* As a policy, the callback is invoked in a 'trigger-after-execute'
* fashion
*/
- if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
- perf_bp_event(bp, regs);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!hit[i])
+ continue;
+ if (!(info[i]->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
+ perf_bp_event(bp[i], regs);
+ }
+
+reset:
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!info[i])
+ continue;
+ __set_breakpoint(i, info[i]);
+ }
- __set_breakpoint(0, info);
out:
rcu_read_unlock();
return rc;
@@ -366,26 +573,43 @@ static int single_step_dabr_instruction(struct die_args *args)
struct pt_regs *regs = args->regs;
struct perf_event *bp = NULL;
struct arch_hw_breakpoint *info;
+ int i;
+ bool found = false;
- bp = current->thread.last_hit_ubp;
/*
* Check if we are single-stepping as a result of a
* previous HW Breakpoint exception
*/
- if (!bp)
- return NOTIFY_DONE;
+ for (i = 0; i < nr_wp_slots(); i++) {
+ bp = current->thread.last_hit_ubp[i];
+
+ if (!bp)
+ continue;
+
+ found = true;
+ info = counter_arch_bp(bp);
+
+ /*
+ * We shall invoke the user-defined callback function in the
+ * single stepping handler to confirm to 'trigger-after-execute'
+ * semantics
+ */
+ if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
+ perf_bp_event(bp, regs);
+ current->thread.last_hit_ubp[i] = NULL;
+ }
- info = counter_arch_bp(bp);
+ if (!found)
+ return NOTIFY_DONE;
- /*
- * We shall invoke the user-defined callback function in the single
- * stepping handler to confirm to 'trigger-after-execute' semantics
- */
- if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ))
- perf_bp_event(bp, regs);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ bp = __this_cpu_read(bp_per_reg[i]);
+ if (!bp)
+ continue;
- __set_breakpoint(0, info);
- current->thread.last_hit_ubp = NULL;
+ info = counter_arch_bp(bp);
+ __set_breakpoint(i, info);
+ }
/*
* If the process was being single-stepped by ptrace, let the
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index dcf9c5b4ac59..98bfd0153291 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -629,9 +629,6 @@ void do_break (struct pt_regs *regs, unsigned long address,
if (debugger_break_match(regs))
return;
- /* Clear the breakpoint */
- hw_breakpoint_disable();
-
/* Deliver the signal to userspace */
force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)address);
}
--
2.21.1
^ permalink raw reply related
* [PATCH v4 14/16] powerpc/watchpoint: Don't allow concurrent perf and ptrace events
From: Ravi Bangoria @ 2020-04-30 4:34 UTC (permalink / raw)
To: mpe, mikey
Cc: apopple, ravi.bangoria, peterz, fweisbec, oleg, npiggin,
linux-kernel, paulus, jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <20200430043417.30948-1-ravi.bangoria@linux.ibm.com>
With Book3s DAWR, ptrace and perf watchpoints on powerpc behaves
differently. Ptrace watchpoint works in one-shot mode and generates
signal before executing instruction. It's ptrace user's job to
single-step the instruction and re-enable the watchpoint. OTOH, in
case of perf watchpoint, kernel emulates/single-steps the instruction
and then generates event. If perf and ptrace creates two events with
same or overlapping address ranges, it's ambiguous to decide who
should single-step the instruction. Because of this issue, don't
allow perf and ptrace watchpoint at the same time if their address
range overlaps.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Reviewed-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/include/asm/hw_breakpoint.h | 2 +
arch/powerpc/kernel/hw_breakpoint.c | 221 +++++++++++++++++++++++
kernel/events/hw_breakpoint.c | 16 ++
3 files changed, 239 insertions(+)
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index add5aa076919..f42a55eb77d2 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -70,6 +70,8 @@ extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
unsigned long val, void *data);
int arch_install_hw_breakpoint(struct perf_event *bp);
void arch_uninstall_hw_breakpoint(struct perf_event *bp);
+int arch_reserve_bp_slot(struct perf_event *bp);
+void arch_release_bp_slot(struct perf_event *bp);
void arch_unregister_hw_breakpoint(struct perf_event *bp);
void hw_breakpoint_pmu_read(struct perf_event *bp);
extern void flush_ptrace_hw_breakpoint(struct task_struct *tsk);
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 28d57d841642..c8623708c9c7 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -123,6 +123,227 @@ static bool is_ptrace_bp(struct perf_event *bp)
return bp->overflow_handler == ptrace_triggered;
}
+struct breakpoint {
+ struct list_head list;
+ struct perf_event *bp;
+ bool ptrace_bp;
+};
+
+static DEFINE_PER_CPU(struct breakpoint *, cpu_bps[HBP_NUM_MAX]);
+static LIST_HEAD(task_bps);
+
+static struct breakpoint *alloc_breakpoint(struct perf_event *bp)
+{
+ struct breakpoint *tmp;
+
+ tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return ERR_PTR(-ENOMEM);
+ tmp->bp = bp;
+ tmp->ptrace_bp = is_ptrace_bp(bp);
+ return tmp;
+}
+
+static bool bp_addr_range_overlap(struct perf_event *bp1, struct perf_event *bp2)
+{
+ __u64 bp1_saddr, bp1_eaddr, bp2_saddr, bp2_eaddr;
+
+ bp1_saddr = ALIGN_DOWN(bp1->attr.bp_addr, HW_BREAKPOINT_SIZE);
+ bp1_eaddr = ALIGN(bp1->attr.bp_addr + bp1->attr.bp_len, HW_BREAKPOINT_SIZE);
+ bp2_saddr = ALIGN_DOWN(bp2->attr.bp_addr, HW_BREAKPOINT_SIZE);
+ bp2_eaddr = ALIGN(bp2->attr.bp_addr + bp2->attr.bp_len, HW_BREAKPOINT_SIZE);
+
+ return (bp1_saddr < bp2_eaddr && bp1_eaddr > bp2_saddr);
+}
+
+static bool alternate_infra_bp(struct breakpoint *b, struct perf_event *bp)
+{
+ return is_ptrace_bp(bp) ? !b->ptrace_bp : b->ptrace_bp;
+}
+
+static bool can_co_exist(struct breakpoint *b, struct perf_event *bp)
+{
+ return !(alternate_infra_bp(b, bp) && bp_addr_range_overlap(b->bp, bp));
+}
+
+static int task_bps_add(struct perf_event *bp)
+{
+ struct breakpoint *tmp;
+
+ tmp = alloc_breakpoint(bp);
+ if (IS_ERR(tmp))
+ return PTR_ERR(tmp);
+
+ list_add(&tmp->list, &task_bps);
+ return 0;
+}
+
+static void task_bps_remove(struct perf_event *bp)
+{
+ struct list_head *pos, *q;
+
+ list_for_each_safe(pos, q, &task_bps) {
+ struct breakpoint *tmp = list_entry(pos, struct breakpoint, list);
+
+ if (tmp->bp == bp) {
+ list_del(&tmp->list);
+ kfree(tmp);
+ break;
+ }
+ }
+}
+
+/*
+ * If any task has breakpoint from alternate infrastructure,
+ * return true. Otherwise return false.
+ */
+static bool all_task_bps_check(struct perf_event *bp)
+{
+ struct breakpoint *tmp;
+
+ list_for_each_entry(tmp, &task_bps, list) {
+ if (!can_co_exist(tmp, bp))
+ return true;
+ }
+ return false;
+}
+
+/*
+ * If same task has breakpoint from alternate infrastructure,
+ * return true. Otherwise return false.
+ */
+static bool same_task_bps_check(struct perf_event *bp)
+{
+ struct breakpoint *tmp;
+
+ list_for_each_entry(tmp, &task_bps, list) {
+ if (tmp->bp->hw.target == bp->hw.target &&
+ !can_co_exist(tmp, bp))
+ return true;
+ }
+ return false;
+}
+
+static int cpu_bps_add(struct perf_event *bp)
+{
+ struct breakpoint **cpu_bp;
+ struct breakpoint *tmp;
+ int i = 0;
+
+ tmp = alloc_breakpoint(bp);
+ if (IS_ERR(tmp))
+ return PTR_ERR(tmp);
+
+ cpu_bp = per_cpu_ptr(cpu_bps, bp->cpu);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!cpu_bp[i]) {
+ cpu_bp[i] = tmp;
+ break;
+ }
+ }
+ return 0;
+}
+
+static void cpu_bps_remove(struct perf_event *bp)
+{
+ struct breakpoint **cpu_bp;
+ int i = 0;
+
+ cpu_bp = per_cpu_ptr(cpu_bps, bp->cpu);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!cpu_bp[i])
+ continue;
+
+ if (cpu_bp[i]->bp == bp) {
+ kfree(cpu_bp[i]);
+ cpu_bp[i] = NULL;
+ break;
+ }
+ }
+}
+
+static bool cpu_bps_check(int cpu, struct perf_event *bp)
+{
+ struct breakpoint **cpu_bp;
+ int i;
+
+ cpu_bp = per_cpu_ptr(cpu_bps, cpu);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (cpu_bp[i] && !can_co_exist(cpu_bp[i], bp))
+ return true;
+ }
+ return false;
+}
+
+static bool all_cpu_bps_check(struct perf_event *bp)
+{
+ int cpu;
+
+ for_each_online_cpu(cpu) {
+ if (cpu_bps_check(cpu, bp))
+ return true;
+ }
+ return false;
+}
+
+/*
+ * We don't use any locks to serialize accesses to cpu_bps or task_bps
+ * because are already inside nr_bp_mutex.
+ */
+int arch_reserve_bp_slot(struct perf_event *bp)
+{
+ int ret;
+
+ /* ptrace breakpoint */
+ if (is_ptrace_bp(bp)) {
+ if (all_cpu_bps_check(bp))
+ return -ENOSPC;
+
+ if (same_task_bps_check(bp))
+ return -ENOSPC;
+
+ return task_bps_add(bp);
+ }
+
+ /* perf breakpoint */
+ if (is_kernel_addr(bp->attr.bp_addr))
+ return 0;
+
+ if (bp->hw.target && bp->cpu == -1) {
+ if (same_task_bps_check(bp))
+ return -ENOSPC;
+
+ return task_bps_add(bp);
+ } else if (!bp->hw.target && bp->cpu != -1) {
+ if (all_task_bps_check(bp))
+ return -ENOSPC;
+
+ return cpu_bps_add(bp);
+ }
+
+ if (same_task_bps_check(bp))
+ return -ENOSPC;
+
+ ret = cpu_bps_add(bp);
+ if (ret)
+ return ret;
+ ret = task_bps_add(bp);
+ if (ret)
+ cpu_bps_remove(bp);
+
+ return ret;
+}
+
+void arch_release_bp_slot(struct perf_event *bp)
+{
+ if (!is_kernel_addr(bp->attr.bp_addr)) {
+ if (bp->hw.target)
+ task_bps_remove(bp);
+ if (bp->cpu != -1)
+ cpu_bps_remove(bp);
+ }
+}
+
/*
* Perform cleanup of arch-specific counters during unregistration
* of the perf-event
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 3cc8416ec844..b48d7039a015 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -213,6 +213,15 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
list_del(&bp->hw.bp_list);
}
+__weak int arch_reserve_bp_slot(struct perf_event *bp)
+{
+ return 0;
+}
+
+__weak void arch_release_bp_slot(struct perf_event *bp)
+{
+}
+
/*
* Function to perform processor-specific cleanup during unregistration
*/
@@ -270,6 +279,7 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
struct bp_busy_slots slots = {0};
enum bp_type_idx type;
int weight;
+ int ret;
/* We couldn't initialize breakpoint constraints on boot */
if (!constraints_initialized)
@@ -294,6 +304,10 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
if (slots.pinned + (!!slots.flexible) > nr_slots[type])
return -ENOSPC;
+ ret = arch_reserve_bp_slot(bp);
+ if (ret)
+ return ret;
+
toggle_bp_slot(bp, true, type, weight);
return 0;
@@ -317,6 +331,8 @@ static void __release_bp_slot(struct perf_event *bp, u64 bp_type)
enum bp_type_idx type;
int weight;
+ arch_release_bp_slot(bp);
+
type = find_slot_idx(bp_type);
weight = hw_breakpoint_weight(bp);
toggle_bp_slot(bp, false, type, weight);
--
2.21.1
^ permalink raw reply related
* [PATCH v4 15/16] powerpc/watchpoint/xmon: Don't allow breakpoint overwriting
From: Ravi Bangoria @ 2020-04-30 4:34 UTC (permalink / raw)
To: mpe, mikey
Cc: apopple, ravi.bangoria, peterz, fweisbec, oleg, npiggin,
linux-kernel, paulus, jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <20200430043417.30948-1-ravi.bangoria@linux.ibm.com>
Xmon allows overwriting breakpoints because it's supported by only
one dawr. But with multiple dawrs, overwriting becomes ambiguous
or unnecessary complicated. So let's not allow it.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Reviewed-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/xmon/xmon.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index d8c0f01e4b24..99e9138661e4 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -1382,6 +1382,10 @@ bpt_cmds(void)
printf("Hardware data breakpoint not supported on this cpu\n");
break;
}
+ if (dabr.enabled) {
+ printf("Couldn't find free breakpoint register\n");
+ break;
+ }
mode = 7;
cmd = inchar();
if (cmd == 'r')
--
2.21.1
^ permalink raw reply related
* [PATCH v4 16/16] powerpc/watchpoint/xmon: Support 2nd dawr
From: Ravi Bangoria @ 2020-04-30 4:34 UTC (permalink / raw)
To: mpe, mikey
Cc: apopple, ravi.bangoria, peterz, fweisbec, oleg, npiggin,
linux-kernel, paulus, jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <20200430043417.30948-1-ravi.bangoria@linux.ibm.com>
Add support for 2nd DAWR in xmon. With this, we can have two
simultaneous breakpoints from xmon.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Reviewed-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/xmon/xmon.c | 101 ++++++++++++++++++++++++++-------------
1 file changed, 69 insertions(+), 32 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 99e9138661e4..01da49b666db 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -111,7 +111,7 @@ struct bpt {
#define NBPTS 256
static struct bpt bpts[NBPTS];
-static struct bpt dabr;
+static struct bpt dabr[HBP_NUM_MAX];
static struct bpt *iabr;
static unsigned bpinstr = 0x7fe00008; /* trap */
@@ -787,10 +787,17 @@ static int xmon_sstep(struct pt_regs *regs)
static int xmon_break_match(struct pt_regs *regs)
{
+ int i;
+
if ((regs->msr & (MSR_IR|MSR_PR|MSR_64BIT)) != (MSR_IR|MSR_64BIT))
return 0;
- if (dabr.enabled == 0)
- return 0;
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (dabr[i].enabled)
+ goto found;
+ }
+ return 0;
+
+found:
xmon_core(regs, 0);
return 1;
}
@@ -929,13 +936,16 @@ static void insert_bpts(void)
static void insert_cpu_bpts(void)
{
+ int i;
struct arch_hw_breakpoint brk;
- if (dabr.enabled) {
- brk.address = dabr.address;
- brk.type = (dabr.enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
- brk.len = DABR_MAX_LEN;
- __set_breakpoint(0, &brk);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (dabr[i].enabled) {
+ brk.address = dabr[i].address;
+ brk.type = (dabr[i].enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
+ brk.len = 8;
+ __set_breakpoint(i, &brk);
+ }
}
if (iabr)
@@ -1349,6 +1359,35 @@ static long check_bp_loc(unsigned long addr)
return 1;
}
+static int find_free_data_bpt(void)
+{
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!dabr[i].enabled)
+ return i;
+ }
+ printf("Couldn't find free breakpoint register\n");
+ return -1;
+}
+
+static void print_data_bpts(void)
+{
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!dabr[i].enabled)
+ continue;
+
+ printf(" data "REG" [", dabr[i].address);
+ if (dabr[i].enabled & 1)
+ printf("r");
+ if (dabr[i].enabled & 2)
+ printf("w");
+ printf("]\n");
+ }
+}
+
static char *breakpoint_help_string =
"Breakpoint command usage:\n"
"b show breakpoints\n"
@@ -1382,10 +1421,9 @@ bpt_cmds(void)
printf("Hardware data breakpoint not supported on this cpu\n");
break;
}
- if (dabr.enabled) {
- printf("Couldn't find free breakpoint register\n");
+ i = find_free_data_bpt();
+ if (i < 0)
break;
- }
mode = 7;
cmd = inchar();
if (cmd == 'r')
@@ -1394,15 +1432,15 @@ bpt_cmds(void)
mode = 6;
else
termch = cmd;
- dabr.address = 0;
- dabr.enabled = 0;
- if (scanhex(&dabr.address)) {
- if (!is_kernel_addr(dabr.address)) {
+ dabr[i].address = 0;
+ dabr[i].enabled = 0;
+ if (scanhex(&dabr[i].address)) {
+ if (!is_kernel_addr(dabr[i].address)) {
printf(badaddr);
break;
}
- dabr.address &= ~HW_BRK_TYPE_DABR;
- dabr.enabled = mode | BP_DABR;
+ dabr[i].address &= ~HW_BRK_TYPE_DABR;
+ dabr[i].enabled = mode | BP_DABR;
}
force_enable_xmon();
@@ -1441,7 +1479,9 @@ bpt_cmds(void)
for (i = 0; i < NBPTS; ++i)
bpts[i].enabled = 0;
iabr = NULL;
- dabr.enabled = 0;
+ for (i = 0; i < nr_wp_slots(); i++)
+ dabr[i].enabled = 0;
+
printf("All breakpoints cleared\n");
break;
}
@@ -1475,14 +1515,7 @@ bpt_cmds(void)
if (xmon_is_ro || !scanhex(&a)) {
/* print all breakpoints */
printf(" type address\n");
- if (dabr.enabled) {
- printf(" data "REG" [", dabr.address);
- if (dabr.enabled & 1)
- printf("r");
- if (dabr.enabled & 2)
- printf("w");
- printf("]\n");
- }
+ print_data_bpts();
for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
if (!bp->enabled)
continue;
@@ -1942,8 +1975,13 @@ static void dump_207_sprs(void)
printf("hfscr = %.16lx dhdes = %.16lx rpr = %.16lx\n",
mfspr(SPRN_HFSCR), mfspr(SPRN_DHDES), mfspr(SPRN_RPR));
- printf("dawr = %.16lx dawrx = %.16lx ciabr = %.16lx\n",
- mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0), mfspr(SPRN_CIABR));
+ printf("dawr0 = %.16lx dawrx0 = %.16lx\n",
+ mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0));
+ if (nr_wp_slots() > 1) {
+ printf("dawr1 = %.16lx dawrx1 = %.16lx\n",
+ mfspr(SPRN_DAWR1), mfspr(SPRN_DAWRX1));
+ }
+ printf("ciabr = %.16lx\n", mfspr(SPRN_CIABR));
#endif
}
@@ -3873,10 +3911,9 @@ static void clear_all_bpt(void)
bpts[i].enabled = 0;
/* Clear any data or iabr breakpoints */
- if (iabr || dabr.enabled) {
- iabr = NULL;
- dabr.enabled = 0;
- }
+ iabr = NULL;
+ for (i = 0; i < nr_wp_slots(); i++)
+ dabr[i].enabled = 0;
}
#ifdef CONFIG_DEBUG_FS
--
2.21.1
^ permalink raw reply related
* Re: [PATCH 2/2] powerpc/spufs: stop using access_ok
From: Christoph Hellwig @ 2020-04-30 5:39 UTC (permalink / raw)
To: Jeremy Kerr; +Cc: linuxppc-dev, Christoph Hellwig, Arnd Bergmann
In-Reply-To: <9c629b09cf25d143c7787548516c1f276bd09aa5.camel@ozlabs.org>
On Thu, Apr 30, 2020 at 08:39:00AM +0800, Jeremy Kerr wrote:
> Hi Christophe,
>
> > > Just use the proper non __-prefixed get/put_user variants where
> > > that is not done yet.
> >
> > But it means you are doing the access_ok() check everytime, which is
> > what is to be avoided by doing the access_ok() once then using the
> > __-prefixed variant.
>
> 5 out of 8 of these are just a access_ok(); simple_read_from_buffer().
>
> For the cases where it's multiple __put/get_user()s, the max will be 5.
> (for the mbox access). Is that worth optimising the access_ok() checks?
access_ok is just trivial comparism to the segment limit, I don't
think it has a relavant performance impact.
^ permalink raw reply
* Re: [RFC 1/3] powernv/cpuidle : Support for pre-entry and post exit of stop state in firmware
From: Abhishek @ 2020-04-30 5:52 UTC (permalink / raw)
To: Nicholas Piggin, linux-kernel, linuxppc-dev
Cc: ego, mikey, psampat, oohall, skiboot
In-Reply-To: <1588035100.usm3gb816q.astroid@bobo.none>
Hi Nick,
Have you posted out the kernel side of "opal v4" patchset?
I could only find the opal patchset.
Thanks,
Abhishek
On 04/28/2020 06:38 AM, Nicholas Piggin wrote:
> Thanks for picking this up and pushing it along. I do plan to come back
> and take another look at it all, but what we do need to do first is get
> a coherent approach to this proposed new calling convention and OS ops.
>
> It's fine to work on this in the meantime, but to start merging things
> my idea is:
>
> - OPAL must leave r13-r15 untouched for the OS.
> - OS ops are made available only for a "v4" OS that uses the new
> calling convention, including kernel stack.
> - OS ops baseline (all OSes must provide) will be console / printk
> facility, trap handling and crash/symbol decoding on behalf of OPAL,
> and runtime virtual memory.
>
> Other OS ops features can be added in the versioned structure, including
> this.
>
> I'm trying to get back to cleaning these things up and start getting
> them merged now. Any comments or review on those would be helpful.
>
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [PATCH v6 1/4] powerpc: Document details on H_SCM_HEALTH hcall
From: Michael Ellerman @ 2020-04-30 6:04 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev, linux-nvdimm
Cc: Vaibhav Jain, Santosh Sivaraj, Oliver O'Halloran,
Dan Williams, Aneesh Kumar K . V
In-Reply-To: <20200420070711.223545-2-vaibhav@linux.ibm.com>
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> Add documentation to 'papr_hcalls.rst' describing the bitmap flags
> that are returned from H_SCM_HEALTH hcall as per the PAPR-SCM
> specification.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
>
> v5..v6
> * New patch in the series
> ---
> Documentation/powerpc/papr_hcalls.rst | 43 ++++++++++++++++++++++++---
> 1 file changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/powerpc/papr_hcalls.rst b/Documentation/powerpc/papr_hcalls.rst
> index 3493631a60f8..9a5ba5eaf323 100644
> --- a/Documentation/powerpc/papr_hcalls.rst
> +++ b/Documentation/powerpc/papr_hcalls.rst
> @@ -220,13 +220,48 @@ from the LPAR memory.
> **H_SCM_HEALTH**
>
> | Input: drcIndex
> -| Out: *health-bitmap, health-bit-valid-bitmap*
> +| Out: *health-bitmap (r4), health-bit-valid-bitmap (r5)*
> | Return Value: *H_Success, H_Parameter, H_Hardware*
>
> Given a DRC Index return the info on predictive failure and overall health of
> -the NVDIMM. The asserted bits in the health-bitmap indicate a single predictive
> -failure and health-bit-valid-bitmap indicate which bits in health-bitmap are
> -valid.
> +the NVDIMM. The asserted bits in the health-bitmap indicate one or more states
> +(described in table below) of the NVDIMM and health-bit-valid-bitmap indicate
> +which bits in health-bitmap are valid.
> +
> +Health Bitmap Flags:
> +
> ++------+-----------------------------------------------------------------------+
> +| Bit | Definition |
> ++======+=======================================================================+
> +| 00 | SCM device is unable to persist memory contents. |
> +| | If the system is powered down, nothing will be saved. |
> ++------+-----------------------------------------------------------------------+
Are these correct bit numbers or backward IBM big endian bit numbers?
ie. which bit is LSB?
cheers
^ permalink raw reply
* Re: [PATCH 2/2] powerpc/spufs: stop using access_ok
From: Christophe Leroy @ 2020-04-30 6:18 UTC (permalink / raw)
To: Christoph Hellwig, Jeremy Kerr; +Cc: linuxppc-dev, Arnd Bergmann
In-Reply-To: <20200430053901.GA6981@lst.de>
Le 30/04/2020 à 07:39, Christoph Hellwig a écrit :
> On Thu, Apr 30, 2020 at 08:39:00AM +0800, Jeremy Kerr wrote:
>> Hi Christophe,
>>
>>>> Just use the proper non __-prefixed get/put_user variants where
>>>> that is not done yet.
>>>
>>> But it means you are doing the access_ok() check everytime, which is
>>> what is to be avoided by doing the access_ok() once then using the
>>> __-prefixed variant.
>>
>> 5 out of 8 of these are just a access_ok(); simple_read_from_buffer().
>>
>> For the cases where it's multiple __put/get_user()s, the max will be 5.
>> (for the mbox access). Is that worth optimising the access_ok() checks?
>
> access_ok is just trivial comparism to the segment limit, I don't
> think it has a relavant performance impact.
>
I think it has an impact. See the difference between the two following
trivial functions:
int test1(unsigned long val, unsigned long *addr)
{
return __put_user(val, addr);
}
int test2(unsigned long val, unsigned long *addr)
{
return put_user(val, addr);
}
00000000 <test1>:
0: 39 20 00 00 li r9,0
4: 90 64 00 00 stw r3,0(r4)
8: 7d 23 4b 78 mr r3,r9
c: 4e 80 00 20 blr
00000000 <test2>:
0: 81 22 04 38 lwz r9,1080(r2)
4: 7c 6a 1b 78 mr r10,r3
8: 7f 89 20 40 cmplw cr7,r9,r4
c: 41 9c 00 24 blt cr7,30 <test2+0x30>
10: 7d 24 48 50 subf r9,r4,r9
14: 38 60 ff f2 li r3,-14
18: 2b 89 00 02 cmplwi cr7,r9,2
1c: 4c 9d 00 20 blelr cr7
20: 39 20 00 00 li r9,0
24: 91 44 00 00 stw r10,0(r4)
28: 7d 23 4b 78 mr r3,r9
2c: 4e 80 00 20 blr
30: 38 60 ff f2 li r3,-14
34: 4e 80 00 20 blr
It looks like GCC is smart enough to read the limit in task struct only
once when we have two consecutive put_user() but there is still some
difference:
int test3(unsigned long val, unsigned long *addr)
{
return put_user(val, addr) ? : put_user(val, addr + 1);
}
int test4(unsigned long val, unsigned long *addr)
{
if (!access_ok(addr, sizeof(*addr)))
return -EFAULT;
return __put_user(val, addr) ? : __put_user(val, addr + 1);
}
00000000 <test3>:
0: 81 42 04 38 lwz r10,1080(r2)
4: 7f 8a 20 40 cmplw cr7,r10,r4
8: 41 9c 00 48 blt cr7,50 <test3+0x50>
c: 7d 04 50 50 subf r8,r4,r10
10: 39 20 ff f2 li r9,-14
14: 2b 88 00 02 cmplwi cr7,r8,2
18: 40 9d 00 30 ble cr7,48 <test3+0x48>
1c: 39 20 00 00 li r9,0
20: 90 64 00 00 stw r3,0(r4)
24: 2f 89 00 00 cmpwi cr7,r9,0
28: 40 9e 00 20 bne cr7,48 <test3+0x48>
2c: 38 84 00 04 addi r4,r4,4
30: 7f 8a 20 40 cmplw cr7,r10,r4
34: 41 9c 00 1c blt cr7,50 <test3+0x50>
38: 7d 44 50 50 subf r10,r4,r10
3c: 2b 8a 00 02 cmplwi cr7,r10,2
40: 40 9d 00 10 ble cr7,50 <test3+0x50>
44: 90 64 00 00 stw r3,0(r4)
48: 7d 23 4b 78 mr r3,r9
4c: 4e 80 00 20 blr
50: 39 20 ff f2 li r9,-14
54: 4b ff ff f4 b 48 <test3+0x48>
Disassembly of section .text.test4:
00000000 <test4>:
0: 81 22 04 38 lwz r9,1080(r2)
4: 7f 89 20 40 cmplw cr7,r9,r4
8: 41 9c 00 34 blt cr7,3c <test4+0x3c>
c: 7d 44 48 50 subf r10,r4,r9
10: 39 20 ff f2 li r9,-14
14: 2b 8a 00 06 cmplwi cr7,r10,6
18: 40 9d 00 1c ble cr7,34 <test4+0x34>
1c: 39 20 00 00 li r9,0
20: 90 64 00 00 stw r3,0(r4)
24: 2f 89 00 00 cmpwi cr7,r9,0
28: 40 9e 00 0c bne cr7,34 <test4+0x34>
2c: 38 84 00 04 addi r4,r4,4
30: 90 64 00 00 stw r3,0(r4)
34: 7d 23 4b 78 mr r3,r9
38: 4e 80 00 20 blr
3c: 39 20 ff f2 li r9,-14
40: 4b ff ff f4 b 34 <test4+0x34>
^ permalink raw reply
* Re: [PATCH v2 3/3] mm/page_alloc: Keep memoryless cpuless node 0 offline
From: Srikar Dronamraju @ 2020-04-30 7:18 UTC (permalink / raw)
To: Michal Hocko
Cc: Linus Torvalds, linux-kernel, linux-mm, Mel Gorman,
Kirill A. Shutemov, Andrew Morton, linuxppc-dev,
Christopher Lameter, Vlastimil Babka
In-Reply-To: <20200429122211.GD28637@dhcp22.suse.cz>
* Michal Hocko <mhocko@kernel.org> [2020-04-29 14:22:11]:
> On Wed 29-04-20 07:11:45, Srikar Dronamraju wrote:
> > > >
> > > > By marking, N_ONLINE as NODE_MASK_NONE, lets stop assuming that Node 0 is
> > > > always online.
> > > >
> > > > ...
> > > >
> > > > --- a/mm/page_alloc.c
> > > > +++ b/mm/page_alloc.c
> > > > @@ -116,8 +116,10 @@ EXPORT_SYMBOL(latent_entropy);
> > > > */
> > > > nodemask_t node_states[NR_NODE_STATES] __read_mostly = {
> > > > [N_POSSIBLE] = NODE_MASK_ALL,
> > > > +#ifdef CONFIG_NUMA
> > > > + [N_ONLINE] = NODE_MASK_NONE,
> > > > +#else
> > > > [N_ONLINE] = { { [0] = 1UL } },
> > > > -#ifndef CONFIG_NUMA
> > > > [N_NORMAL_MEMORY] = { { [0] = 1UL } },
> > > > #ifdef CONFIG_HIGHMEM
> > > > [N_HIGH_MEMORY] = { { [0] = 1UL } },
> > >
> > > So on all other NUMA machines, when does node 0 get marked online?
> > >
> > > This change means that for some time during boot, such machines will
> > > now be running with node 0 marked as offline. What are the
> > > implications of this? Will something break?
> >
> > Till the nodes are detected, marking Node 0 as online tends to be redundant.
> > Because the system doesn't know if its a NUMA or a non-NUMA system.
> > Once we detect the nodes, we online them immediately. Hence I don't see any
> > side-effects or negative implications of this change.
> >
> > However if I am missing anything, please do let me know.
> >
> > >From my part, I have tested this on
> > 1. Non-NUMA Single node but CPUs and memory coming from zero node.
> > 2. Non-NUMA Single node but CPUs and memory coming from non-zero node.
> > 3. NUMA Multi node but with CPUs and memory from node 0.
> > 4. NUMA Multi node but with no CPUs and memory from node 0.
>
> Have you tested on something else than ppc? Each arch does the NUMA
> setup separately and this is a big mess. E.g. x86 marks even memory less
> nodes (see init_memory_less_node) as online.
>
while I have predominantly tested on ppc, I did test on X86 with CONFIG_NUMA
enabled/disabled on both single node and multi node machines.
However, I dont have a cpuless/memoryless x86 system.
> Honestly I have hard time to evaluate the effect of this patch. It makes
> some sense to assume all nodes offline before they get online but this
> is a land mine territory.
>
> I am also not sure what kind of problem this is going to address. You
> have mentioned numa balancing without many details.
1. On a machine with just one node with node number not being 0,
the current setup will end up showing 2 online nodes. And when there are
more than one online nodes, numa_balancing gets enabled.
Without patch
$ grep numa /proc/vmstat
numa_hit 95179
numa_miss 0
numa_foreign 0
numa_interleave 3764
numa_local 95179
numa_other 0
numa_pte_updates 1206973 <----------
numa_huge_pte_updates 4654 <----------
numa_hint_faults 19560 <----------
numa_hint_faults_local 19560 <----------
numa_pages_migrated 0
With patch
$ grep numa /proc/vmstat
numa_hit 322338756
numa_miss 0
numa_foreign 0
numa_interleave 3790
numa_local 322338756
numa_other 0
numa_pte_updates 0 <----------
numa_huge_pte_updates 0 <----------
numa_hint_faults 0 <----------
numa_hint_faults_local 0 <----------
numa_pages_migrated 0
So we have a redundant page hinting numa faults which we can avoid.
2. Few people have complained about existence of this dummy node when
parsing lscpu and numactl o/p. They somehow start to think that the tools
are reporting incorrectly or the kernel is not able to recognize resources
connected to the node.
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH v1 2/3] mm/memory_hotplug: Introduce MHP_DRIVER_MANAGED
From: David Hildenbrand @ 2020-04-30 7:19 UTC (permalink / raw)
To: linux-kernel
Cc: virtio-dev, linux-hyperv, Michal Hocko, linux-acpi, Baoquan He,
linux-nvdimm, linux-s390, Michael S . Tsirkin, Dave Hansen,
virtualization, linux-mm, Wei Yang, Eric Biederman, Pankaj Gupta,
xen-devel, Andrew Morton, Michal Hocko, linuxppc-dev,
Dan Williams, Pavel Tatashin
In-Reply-To: <20200429160803.109056-3-david@redhat.com>
On 29.04.20 18:08, David Hildenbrand wrote:
> Some paravirtualized devices that add memory via add_memory() and
> friends (esp. virtio-mem) don't want to create entries in
> /sys/firmware/memmap/ - primarily to hinder kexec from adding this
> memory to the boot memmap of the kexec kernel.
>
> In fact, such memory is never exposed via the firmware (e.g., e820), but
> only via the device, so exposing this memory via /sys/firmware/memmap/ is
> wrong:
> "kexec needs the raw firmware-provided memory map to setup the
> parameter segment of the kernel that should be booted with
> kexec. Also, the raw memory map is useful for debugging. For
> that reason, /sys/firmware/memmap is an interface that provides
> the raw memory map to userspace." [1]
>
> We want to let user space know that memory which is always detected,
> added, and managed via a (device) driver - like memory managed by
> virtio-mem - is special. It cannot be used for placing kexec segments
> and the (device) driver is responsible for re-adding memory that
> (eventually shrunk/grown/defragmented) memory after a reboot/kexec. It
> should e.g., not be added to a fixed up firmware memmap. However, it should
> be dumped by kdump.
>
> Also, such memory could behave differently than an ordinary DIMM - e.g.,
> memory managed by virtio-mem can have holes inside added memory resource,
> which should not be touched, especially for writing.
>
> Let's expose that memory as "System RAM (driver managed)" e.g., via
> /pro/iomem.
>
> We don't have to worry about firmware_map_remove() on the removal path.
> If there is no entry, it will simply return with -EINVAL.
>
> [1] https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-firmware-memmap
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> Cc: Wei Yang <richard.weiyang@gmail.com>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> include/linux/memory_hotplug.h | 8 ++++++++
> mm/memory_hotplug.c | 20 ++++++++++++++++----
> 2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index bf0e3edb8688..cc538584b39e 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -68,6 +68,14 @@ struct mhp_params {
> pgprot_t pgprot;
> };
>
> +/* Flags used for add_memory() and friends. */
> +
> +/*
> + * Don't create entries in /sys/firmware/memmap/ and expose memory as
> + * "System RAM (driver managed)" in e.g., /proc/iomem
> + */
> +#define MHP_DRIVER_MANAGED 1
> +
> /*
> * Zone resizing functions
> *
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index ebdf6541d074..cfa0721280aa 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -98,11 +98,11 @@ void mem_hotplug_done(void)
> u64 max_mem_size = U64_MAX;
>
> /* add this memory to iomem resource */
> -static struct resource *register_memory_resource(u64 start, u64 size)
> +static struct resource *register_memory_resource(u64 start, u64 size,
> + const char *resource_name)
> {
> struct resource *res;
> unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
> - char *resource_name = "System RAM";
>
> /*
> * Make sure value parsed from 'mem=' only restricts memory adding
> @@ -1058,7 +1058,8 @@ int __ref add_memory_resource(int nid, struct resource *res,
> BUG_ON(ret);
>
> /* create new memmap entry */
> - firmware_map_add_hotplug(start, start + size, "System RAM");
> + if (!(flags & MHP_DRIVER_MANAGED))
> + firmware_map_add_hotplug(start, start + size, "System RAM");
>
> /* device_online() will take the lock when calling online_pages() */
> mem_hotplug_done();
> @@ -1081,10 +1082,21 @@ int __ref add_memory_resource(int nid, struct resource *res,
> /* requires device_hotplug_lock, see add_memory_resource() */
> int __ref __add_memory(int nid, u64 start, u64 size, unsigned long flags)
> {
> + const char *resource_name = "System RAM";
> struct resource *res;
> int ret;
>
> - res = register_memory_resource(start, size);
> + /*
> + * Indicate that memory managed by a driver is special. It's always
> + * detected and added via a driver, should not be given to the kexec
> + * kernel for booting when manually crafting the firmware memmap, and
> + * no kexec segments should be placed on it. However, kdump should
> + * dump this memory.
> + */
> + if (flags & MHP_DRIVER_MANAGED)
> + resource_name = "System RAM (driver managed)";
> +
> + res = register_memory_resource(start, size, resource_name);
> if (IS_ERR(res))
> return PTR_ERR(res);
>
>
BTW, I was wondering if this is actually also something that
drivers/dax/kmem.c wants to use for adding memory.
Just because we decided to use some DAX memory in the current kernel as
system ram, doesn't mean we should make that decision for the kexec
kernel (e.g., using it as initial memory, placing kexec binaries onto
it, etc.). This is also not what we would observe during a real reboot.
I can see that the "System RAM" resource will show up as child resource
under the device e.g., in /proc/iomem.
However, entries in /sys/firmware/memmap/ are created as "System RAM".
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v1 2/3] mm/memory_hotplug: Introduce MHP_DRIVER_MANAGED
From: Dan Williams @ 2020-04-30 8:11 UTC (permalink / raw)
To: David Hildenbrand
Cc: virtio-dev, linux-hyperv, Michal Hocko, Pavel Tatashin,
Baoquan He, Linux MM, Wei Yang, linux-s390, linux-nvdimm,
Linux Kernel Mailing List, Dave Hansen, virtualization,
Linux ACPI, Michael S . Tsirkin, Eric Biederman, Pankaj Gupta,
xen-devel, Andrew Morton, Michal Hocko, linuxppc-dev
In-Reply-To: <a7305cd8-8b2f-1d8f-7654-ecf777c46df6@redhat.com>
On Thu, Apr 30, 2020 at 12:20 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 29.04.20 18:08, David Hildenbrand wrote:
> > Some paravirtualized devices that add memory via add_memory() and
> > friends (esp. virtio-mem) don't want to create entries in
> > /sys/firmware/memmap/ - primarily to hinder kexec from adding this
> > memory to the boot memmap of the kexec kernel.
> >
> > In fact, such memory is never exposed via the firmware (e.g., e820), but
> > only via the device, so exposing this memory via /sys/firmware/memmap/ is
> > wrong:
> > "kexec needs the raw firmware-provided memory map to setup the
> > parameter segment of the kernel that should be booted with
> > kexec. Also, the raw memory map is useful for debugging. For
> > that reason, /sys/firmware/memmap is an interface that provides
> > the raw memory map to userspace." [1]
> >
> > We want to let user space know that memory which is always detected,
> > added, and managed via a (device) driver - like memory managed by
> > virtio-mem - is special. It cannot be used for placing kexec segments
> > and the (device) driver is responsible for re-adding memory that
> > (eventually shrunk/grown/defragmented) memory after a reboot/kexec. It
> > should e.g., not be added to a fixed up firmware memmap. However, it should
> > be dumped by kdump.
> >
> > Also, such memory could behave differently than an ordinary DIMM - e.g.,
> > memory managed by virtio-mem can have holes inside added memory resource,
> > which should not be touched, especially for writing.
> >
> > Let's expose that memory as "System RAM (driver managed)" e.g., via
> > /pro/iomem.
> >
> > We don't have to worry about firmware_map_remove() on the removal path.
> > If there is no entry, it will simply return with -EINVAL.
> >
> > [1] https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-firmware-memmap
> >
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> > Cc: Wei Yang <richard.weiyang@gmail.com>
> > Cc: Baoquan He <bhe@redhat.com>
> > Cc: Eric Biederman <ebiederm@xmission.com>
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> > include/linux/memory_hotplug.h | 8 ++++++++
> > mm/memory_hotplug.c | 20 ++++++++++++++++----
> > 2 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> > index bf0e3edb8688..cc538584b39e 100644
> > --- a/include/linux/memory_hotplug.h
> > +++ b/include/linux/memory_hotplug.h
> > @@ -68,6 +68,14 @@ struct mhp_params {
> > pgprot_t pgprot;
> > };
> >
> > +/* Flags used for add_memory() and friends. */
> > +
> > +/*
> > + * Don't create entries in /sys/firmware/memmap/ and expose memory as
> > + * "System RAM (driver managed)" in e.g., /proc/iomem
> > + */
> > +#define MHP_DRIVER_MANAGED 1
> > +
> > /*
> > * Zone resizing functions
> > *
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index ebdf6541d074..cfa0721280aa 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -98,11 +98,11 @@ void mem_hotplug_done(void)
> > u64 max_mem_size = U64_MAX;
> >
> > /* add this memory to iomem resource */
> > -static struct resource *register_memory_resource(u64 start, u64 size)
> > +static struct resource *register_memory_resource(u64 start, u64 size,
> > + const char *resource_name)
> > {
> > struct resource *res;
> > unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
> > - char *resource_name = "System RAM";
> >
> > /*
> > * Make sure value parsed from 'mem=' only restricts memory adding
> > @@ -1058,7 +1058,8 @@ int __ref add_memory_resource(int nid, struct resource *res,
> > BUG_ON(ret);
> >
> > /* create new memmap entry */
> > - firmware_map_add_hotplug(start, start + size, "System RAM");
> > + if (!(flags & MHP_DRIVER_MANAGED))
> > + firmware_map_add_hotplug(start, start + size, "System RAM");
> >
> > /* device_online() will take the lock when calling online_pages() */
> > mem_hotplug_done();
> > @@ -1081,10 +1082,21 @@ int __ref add_memory_resource(int nid, struct resource *res,
> > /* requires device_hotplug_lock, see add_memory_resource() */
> > int __ref __add_memory(int nid, u64 start, u64 size, unsigned long flags)
> > {
> > + const char *resource_name = "System RAM";
> > struct resource *res;
> > int ret;
> >
> > - res = register_memory_resource(start, size);
> > + /*
> > + * Indicate that memory managed by a driver is special. It's always
> > + * detected and added via a driver, should not be given to the kexec
> > + * kernel for booting when manually crafting the firmware memmap, and
> > + * no kexec segments should be placed on it. However, kdump should
> > + * dump this memory.
> > + */
> > + if (flags & MHP_DRIVER_MANAGED)
> > + resource_name = "System RAM (driver managed)";
> > +
> > + res = register_memory_resource(start, size, resource_name);
> > if (IS_ERR(res))
> > return PTR_ERR(res);
> >
> >
>
> BTW, I was wondering if this is actually also something that
> drivers/dax/kmem.c wants to use for adding memory.
>
> Just because we decided to use some DAX memory in the current kernel as
> system ram, doesn't mean we should make that decision for the kexec
> kernel (e.g., using it as initial memory, placing kexec binaries onto
> it, etc.). This is also not what we would observe during a real reboot.
Agree.
> I can see that the "System RAM" resource will show up as child resource
> under the device e.g., in /proc/iomem.
>
> However, entries in /sys/firmware/memmap/ are created as "System RAM".
True. Do you think this rename should just be limited to what type
/sys/firmware/memmap/ emits? I have the concern, but no proof
currently, that there are /proc/iomem walkers that explicitly look for
"System RAM", but might be thrown off by "System RAM (driver
managed)". I was not aware of /sys/firmware/memmap until about 5
minutes ago.
^ permalink raw reply
* Re: [PATCH v1 2/3] mm/memory_hotplug: Introduce MHP_DRIVER_MANAGED
From: David Hildenbrand @ 2020-04-30 8:20 UTC (permalink / raw)
To: Dan Williams
Cc: virtio-dev, linux-hyperv, Michal Hocko, Pavel Tatashin,
Baoquan He, Linux MM, Wei Yang, linux-s390, linux-nvdimm,
Linux Kernel Mailing List, Dave Hansen, virtualization,
Linux ACPI, Michael S . Tsirkin, Eric Biederman, Pankaj Gupta,
xen-devel, Andrew Morton, Michal Hocko, linuxppc-dev
In-Reply-To: <CAPcyv4i04+QLxiOyz04_eef2DFetEFKBUmi2A4xxw9abQD8hNQ@mail.gmail.com>
On 30.04.20 10:11, Dan Williams wrote:
> On Thu, Apr 30, 2020 at 12:20 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 29.04.20 18:08, David Hildenbrand wrote:
>>> Some paravirtualized devices that add memory via add_memory() and
>>> friends (esp. virtio-mem) don't want to create entries in
>>> /sys/firmware/memmap/ - primarily to hinder kexec from adding this
>>> memory to the boot memmap of the kexec kernel.
>>>
>>> In fact, such memory is never exposed via the firmware (e.g., e820), but
>>> only via the device, so exposing this memory via /sys/firmware/memmap/ is
>>> wrong:
>>> "kexec needs the raw firmware-provided memory map to setup the
>>> parameter segment of the kernel that should be booted with
>>> kexec. Also, the raw memory map is useful for debugging. For
>>> that reason, /sys/firmware/memmap is an interface that provides
>>> the raw memory map to userspace." [1]
>>>
>>> We want to let user space know that memory which is always detected,
>>> added, and managed via a (device) driver - like memory managed by
>>> virtio-mem - is special. It cannot be used for placing kexec segments
>>> and the (device) driver is responsible for re-adding memory that
>>> (eventually shrunk/grown/defragmented) memory after a reboot/kexec. It
>>> should e.g., not be added to a fixed up firmware memmap. However, it should
>>> be dumped by kdump.
>>>
>>> Also, such memory could behave differently than an ordinary DIMM - e.g.,
>>> memory managed by virtio-mem can have holes inside added memory resource,
>>> which should not be touched, especially for writing.
>>>
>>> Let's expose that memory as "System RAM (driver managed)" e.g., via
>>> /pro/iomem.
>>>
>>> We don't have to worry about firmware_map_remove() on the removal path.
>>> If there is no entry, it will simply return with -EINVAL.
>>>
>>> [1] https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-firmware-memmap
>>>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
>>> Cc: Wei Yang <richard.weiyang@gmail.com>
>>> Cc: Baoquan He <bhe@redhat.com>
>>> Cc: Eric Biederman <ebiederm@xmission.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> include/linux/memory_hotplug.h | 8 ++++++++
>>> mm/memory_hotplug.c | 20 ++++++++++++++++----
>>> 2 files changed, 24 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>>> index bf0e3edb8688..cc538584b39e 100644
>>> --- a/include/linux/memory_hotplug.h
>>> +++ b/include/linux/memory_hotplug.h
>>> @@ -68,6 +68,14 @@ struct mhp_params {
>>> pgprot_t pgprot;
>>> };
>>>
>>> +/* Flags used for add_memory() and friends. */
>>> +
>>> +/*
>>> + * Don't create entries in /sys/firmware/memmap/ and expose memory as
>>> + * "System RAM (driver managed)" in e.g., /proc/iomem
>>> + */
>>> +#define MHP_DRIVER_MANAGED 1
>>> +
>>> /*
>>> * Zone resizing functions
>>> *
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index ebdf6541d074..cfa0721280aa 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -98,11 +98,11 @@ void mem_hotplug_done(void)
>>> u64 max_mem_size = U64_MAX;
>>>
>>> /* add this memory to iomem resource */
>>> -static struct resource *register_memory_resource(u64 start, u64 size)
>>> +static struct resource *register_memory_resource(u64 start, u64 size,
>>> + const char *resource_name)
>>> {
>>> struct resource *res;
>>> unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
>>> - char *resource_name = "System RAM";
>>>
>>> /*
>>> * Make sure value parsed from 'mem=' only restricts memory adding
>>> @@ -1058,7 +1058,8 @@ int __ref add_memory_resource(int nid, struct resource *res,
>>> BUG_ON(ret);
>>>
>>> /* create new memmap entry */
>>> - firmware_map_add_hotplug(start, start + size, "System RAM");
>>> + if (!(flags & MHP_DRIVER_MANAGED))
>>> + firmware_map_add_hotplug(start, start + size, "System RAM");
>>>
>>> /* device_online() will take the lock when calling online_pages() */
>>> mem_hotplug_done();
>>> @@ -1081,10 +1082,21 @@ int __ref add_memory_resource(int nid, struct resource *res,
>>> /* requires device_hotplug_lock, see add_memory_resource() */
>>> int __ref __add_memory(int nid, u64 start, u64 size, unsigned long flags)
>>> {
>>> + const char *resource_name = "System RAM";
>>> struct resource *res;
>>> int ret;
>>>
>>> - res = register_memory_resource(start, size);
>>> + /*
>>> + * Indicate that memory managed by a driver is special. It's always
>>> + * detected and added via a driver, should not be given to the kexec
>>> + * kernel for booting when manually crafting the firmware memmap, and
>>> + * no kexec segments should be placed on it. However, kdump should
>>> + * dump this memory.
>>> + */
>>> + if (flags & MHP_DRIVER_MANAGED)
>>> + resource_name = "System RAM (driver managed)";
>>> +
>>> + res = register_memory_resource(start, size, resource_name);
>>> if (IS_ERR(res))
>>> return PTR_ERR(res);
>>>
>>>
>>
>> BTW, I was wondering if this is actually also something that
>> drivers/dax/kmem.c wants to use for adding memory.
>>
>> Just because we decided to use some DAX memory in the current kernel as
>> system ram, doesn't mean we should make that decision for the kexec
>> kernel (e.g., using it as initial memory, placing kexec binaries onto
>> it, etc.). This is also not what we would observe during a real reboot.
>
> Agree.
>
>> I can see that the "System RAM" resource will show up as child resource
>> under the device e.g., in /proc/iomem.
>>
>> However, entries in /sys/firmware/memmap/ are created as "System RAM".
>
> True. Do you think this rename should just be limited to what type
> /sys/firmware/memmap/ emits? I have the concern, but no proof
We could split this patch into
MHP_NO_FIRMWARE_MEMMAP (create firmware memmap entries)
and
MHP_DRIVER_MANAGED (name of the resource)
See below, the latter might not be needed.
> currently, that there are /proc/iomem walkers that explicitly look for
> "System RAM", but might be thrown off by "System RAM (driver
> managed)". I was not aware of /sys/firmware/memmap until about 5
> minutes ago.
The only two users of /proc/iomem I am aware of are kexec-tools and some
s390x tools.
kexec-tools on x86-64 uses /sys/firmware/memmap to craft the initial
memmap, but uses /proc/iomem to
a) Find places for kexec images
b) Detect memory regions to dump via kdump
I am not yet sure if we really need the "System RAM (driver managed)"
part. If we can teach kexec-tools to
a) Don't place kexec images on "System RAM" that has a parent resource
(most likely requires kexec-tools changes)
b) Consider for kdump "System RAM" that has a parent resource
we might be able to avoid renaming that. (I assume that's already done)
E.g., regarding virtio-mem (patch #3) I am currently also looking into
creating a parent resource instead, like dax/kmem to avoid the rename:
:/# cat /proc/iomem
00000000-00000fff : Reserved
[...]
100000000-13fffffff : System RAM
140000000-33fffffff : virtio0
140000000-147ffffff : System RAM
148000000-14fffffff : System RAM
150000000-157ffffff : System RAM
340000000-303fffffff : virtio1
340000000-347ffffff : System RAM
3280000000-32ffffffff : PCI Bus 0000:00
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v1 2/3] mm/memory_hotplug: Introduce MHP_DRIVER_MANAGED
From: Dan Williams @ 2020-04-30 8:34 UTC (permalink / raw)
To: David Hildenbrand
Cc: virtio-dev, linux-hyperv, Michal Hocko, Pavel Tatashin,
Baoquan He, Linux MM, Wei Yang, linux-s390, linux-nvdimm,
Linux Kernel Mailing List, Dave Hansen, virtualization,
Linux ACPI, Michael S . Tsirkin, Eric Biederman, Pankaj Gupta,
xen-devel, Andrew Morton, Michal Hocko, linuxppc-dev
In-Reply-To: <e32522cd-31bb-e129-47a6-9ec13b570506@redhat.com>
On Thu, Apr 30, 2020 at 1:21 AM David Hildenbrand <david@redhat.com> wrote:
> >> Just because we decided to use some DAX memory in the current kernel as
> >> system ram, doesn't mean we should make that decision for the kexec
> >> kernel (e.g., using it as initial memory, placing kexec binaries onto
> >> it, etc.). This is also not what we would observe during a real reboot.
> >
> > Agree.
> >
> >> I can see that the "System RAM" resource will show up as child resource
> >> under the device e.g., in /proc/iomem.
> >>
> >> However, entries in /sys/firmware/memmap/ are created as "System RAM".
> >
> > True. Do you think this rename should just be limited to what type
> > /sys/firmware/memmap/ emits? I have the concern, but no proof
>
> We could split this patch into
>
> MHP_NO_FIRMWARE_MEMMAP (create firmware memmap entries)
>
> and
>
> MHP_DRIVER_MANAGED (name of the resource)
>
> See below, the latter might not be needed.
>
> > currently, that there are /proc/iomem walkers that explicitly look for
> > "System RAM", but might be thrown off by "System RAM (driver
> > managed)". I was not aware of /sys/firmware/memmap until about 5
> > minutes ago.
>
> The only two users of /proc/iomem I am aware of are kexec-tools and some
> s390x tools.
>
> kexec-tools on x86-64 uses /sys/firmware/memmap to craft the initial
> memmap, but uses /proc/iomem to
> a) Find places for kexec images
> b) Detect memory regions to dump via kdump
>
> I am not yet sure if we really need the "System RAM (driver managed)"
> part. If we can teach kexec-tools to
> a) Don't place kexec images on "System RAM" that has a parent resource
> (most likely requires kexec-tools changes)
> b) Consider for kdump "System RAM" that has a parent resource
> we might be able to avoid renaming that. (I assume that's already done)
>
> E.g., regarding virtio-mem (patch #3) I am currently also looking into
> creating a parent resource instead, like dax/kmem to avoid the rename:
>
> :/# cat /proc/iomem
> 00000000-00000fff : Reserved
> [...]
> 100000000-13fffffff : System RAM
> 140000000-33fffffff : virtio0
> 140000000-147ffffff : System RAM
> 148000000-14fffffff : System RAM
> 150000000-157ffffff : System RAM
> 340000000-303fffffff : virtio1
> 340000000-347ffffff : System RAM
> 3280000000-32ffffffff : PCI Bus 0000:00
Looks good to me if it flies with kexec-tools.
^ permalink raw reply
* Re: [PATCH v6 2/4] powerpc/papr_scm: Fetch nvdimm health information from PHYP
From: Michael Ellerman @ 2020-04-30 9:11 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev, linux-nvdimm
Cc: Vaibhav Jain, Santosh Sivaraj, Oliver O'Halloran,
Dan Williams, Aneesh Kumar K . V
In-Reply-To: <20200420070711.223545-3-vaibhav@linux.ibm.com>
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> Implement support for fetching nvdimm health information via
> H_SCM_HEALTH hcall as documented in Ref[1]. The hcall returns a pair
> of 64-bit big-endian integers, bitwise-and of which is then stored in
> 'struct papr_scm_priv' and subsequently partially exposed to
> user-space via newly introduced dimm specific attribute
> 'papr/flags'. Since the hcall is costly, the health information is
> cached and only re-queried, 60s after the previous successful hcall.
>
> The patch also adds a new asm header named 'papr_scm.h' describing the
> interface between PHYP and guest kernel. A documentation text
> describing flags reported by the the new sysfs attribute 'papr/flags'
> is also introduced at Documentation/ABI/testing/sysfs-bus-papr-scm.
>
> [1] commit 58b278f568f0 ("powerpc: Provide initial documentation for
> PAPR hcalls")
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog
>
> v5..v6 :
> * Change the flags sysfs attribute from 'papr_flags' to 'papr/flags'
> [Dan Williams]
> * Include documentation for 'papr/flags' attr [Dan Williams]
> * Change flag 'save_fail' to 'flush_fail' [Dan Williams]
> * Caching of health bitmap to reduce expensive hcalls [Dan Williams]
> * Removed usage of PPC_BIT from 'papr-scm.h' header [Mpe]
> * Replaced two __be64 integers from papr_scm_priv to a single u64
> integer [Mpe]
> * Updated patch description to reflect the changes made in this
> version.
> * Removed avoidable usage of 'papr_scm_priv.dimm_mutex' from
> flags_show() [Dan Williams]
>
> v4..v5 :
> * None
>
> v3..v4 :
> * None
>
> v2..v3 :
> * Removed PAPR_SCM_DIMM_HEALTH_NON_CRITICAL as a condition for
> NVDIMM unarmed [Aneesh]
>
> v1..v2 :
> * New patch in the series.
> ---
> Documentation/ABI/testing/sysfs-bus-papr-scm | 27 ++++
> arch/powerpc/include/asm/papr_scm.h | 49 ++++++++
> arch/powerpc/platforms/pseries/papr_scm.c | 126 ++++++++++++++++++-
> 3 files changed, 200 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-papr-scm
> create mode 100644 arch/powerpc/include/asm/papr_scm.h
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-papr-scm b/Documentation/ABI/testing/sysfs-bus-papr-scm
> new file mode 100644
> index 000000000000..001e4d34ab5c
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-papr-scm
> @@ -0,0 +1,27 @@
> +What: /sys/bus/nd/devices/nmemX/papr/flags
> +Date: Apr, 2020
> +KernelVersion: v5.8
> +Contact: linuxppc-dev <linuxppc-dev@lists.ozlabs.org>, linux-nvdimm@lists.01.org,
> +Description:
> + (RO) Report flags indicating various states of a
> + papr-scm NVDIMM device. Each flag maps to a one or
> + more bits set in the dimm-health-bitmap retrieved in
> + response to H_SCM_HEALTH hcall. The details of the bit
> + flags returned in response to this hcall is available
> + at 'Documentation/powerpc/papr_hcalls.rst' . Below are
> + the flags reported in this sysfs file:
> +
> + * "not_armed" : Indicating that nvdimm contents will not
NVDIMM?
> + survive a power cycle.
> + * "flush_fail" : Indicating that nvdimm contents
> + couldn't be flushed during last
> + shutdown event.
> + * "restore_fail": Indicating that nvdimm contents
> + couldn't be restored during dimm
DIMM?
> + initialization.
> + * "encrypted" : Dimm contents are encrypted.
> + * "smart_notify": There is health event for the nvdimm.
> + * "scrubbed" : Indicating that contents of the
> + nvdimm have been scrubbed.
> + * "locked" : Indicating that nvdimm contents cant
> + be modified until next power cycle.
Some of these strings are not very meaningful to me, I would choose
different values.
But I assume you are using these because they are defined somewhere
else, where is that?
> diff --git a/arch/powerpc/include/asm/papr_scm.h b/arch/powerpc/include/asm/papr_scm.h
> new file mode 100644
> index 000000000000..b51c048e906a
> --- /dev/null
> +++ b/arch/powerpc/include/asm/papr_scm.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Structures and defines needed to manage nvdimms for spapr guests.
> + */
> +#ifndef _ASM_POWERPC_PAPR_SCM_H_
> +#define _ASM_POWERPC_PAPR_SCM_H_
> +
> +#include <linux/types.h>
> +#include <asm/bitsperlong.h>
> +
> +/* DIMM health bitmap bitmap indicators */
> +
> +/* SCM device is unable to persist memory contents */
> +#define PAPR_SCM_DIMM_UNARMED (1ULL << (63 - 0))
> +/* SCM device failed to persist memory contents */
> +#define PAPR_SCM_DIMM_SHUTDOWN_DIRTY (1ULL << (63 - 1))
> +/* SCM device contents are persisted from previous IPL */
> +#define PAPR_SCM_DIMM_SHUTDOWN_CLEAN (1ULL << (63 - 2))
> +/* SCM device contents are not persisted from previous IPL */
> +#define PAPR_SCM_DIMM_EMPTY (1ULL << (63 - 3))
> +/* SCM device memory life remaining is critically low */
> +#define PAPR_SCM_DIMM_HEALTH_CRITICAL (1ULL << (63 - 4))
> +/* SCM device will be garded off next IPL due to failure */
> +#define PAPR_SCM_DIMM_HEALTH_FATAL (1ULL << (63 - 5))
> +/* SCM contents cannot persist due to current platform health status */
> +#define PAPR_SCM_DIMM_HEALTH_UNHEALTHY (1ULL << (63 - 6))
> +/* SCM device is unable to persist memory contents in certain conditions */
> +#define PAPR_SCM_DIMM_HEALTH_NON_CRITICAL (1ULL << (63 - 7))
> +/* SCM device is encrypted */
> +#define PAPR_SCM_DIMM_ENCRYPTED (1ULL << (63 - 8))
> +/* SCM device has been scrubbed and locked */
> +#define PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED (1ULL << (63 - 9))
> +
> +/* Bits status indicators for health bitmap indicating unarmed dimm */
> +#define PAPR_SCM_DIMM_UNARMED_MASK (PAPR_SCM_DIMM_UNARMED | \
> + PAPR_SCM_DIMM_HEALTH_UNHEALTHY)
> +
> +/* Bits status indicators for health bitmap indicating unflushed dimm */
> +#define PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK (PAPR_SCM_DIMM_SHUTDOWN_DIRTY)
> +
> +/* Bits status indicators for health bitmap indicating unrestored dimm */
> +#define PAPR_SCM_DIMM_BAD_RESTORE_MASK (PAPR_SCM_DIMM_EMPTY)
> +
> +/* Bit status indicators for smart event notification */
> +#define PAPR_SCM_DIMM_SMART_EVENT_MASK (PAPR_SCM_DIMM_HEALTH_CRITICAL | \
> + PAPR_SCM_DIMM_HEALTH_FATAL | \
> + PAPR_SCM_DIMM_HEALTH_UNHEALTHY)
These are only ever used in papr_scm.c AFAICS, so put them in there.
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 0b4467e378e5..f8fe579e6f2e 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -14,6 +14,7 @@
> #include <linux/delay.h>
>
> #include <asm/plpar_wrappers.h>
> +#include <asm/papr_scm.h>
>
> #define BIND_ANY_ADDR (~0ul)
>
> @@ -39,6 +40,15 @@ struct papr_scm_priv {
> struct resource res;
> struct nd_region *region;
> struct nd_interleave_set nd_set;
> +
> + /* Protect dimm health data from concurrent read/writes */
> + struct mutex dimm_mutex;
health_mutex then?
> + /* Last time the health information of the dimm was updated */
> + unsigned long lasthealth_jiffies;
> +
> + /* Health information for the dimm */
> + u64 health_bitmap;
> };
>
> static int drc_pmem_bind(struct papr_scm_priv *p)
> @@ -144,6 +154,56 @@ static int drc_pmem_query_n_bind(struct papr_scm_priv *p)
> return drc_pmem_bind(p);
> }
>
> +/* Min interval in seconds between successive H_SCM_HEALTH hcalls */
> +#define MIN_HEALTH_QUERY_INTERVAL 60
> +
> +/*
> + * Issue hcall if needed to retrieve dimm health info. Information is cached
> + * and subsequent calls may return success without issueing the hcall.
^
issuing
> + * Use 'force == true' to force issue of the hcall ignoring the cache
> + * timeout.
> + */
> +static int drc_pmem_query_health(struct papr_scm_priv *p, bool force)
The force API is a bit clunky.
I think it would make more sense if you had the caching logic in a
wrapper function, or even in flags_show() directly.
And then have papr_scm_get_health() just call the underlying routine
that always does the hcall.
I think you can safely read lasthealth_jiffies without the mutex.
> +{
> + unsigned long ret[PLPAR_HCALL_BUFSIZE];
> + s64 rc;
> + unsigned long cache_timeout;
My eyes! ;)
unsigned long cache_timeout, ret[PLPAR_HCALL_BUFSIZE];
s64 rc;
Please :)
> +
> + /* Protect concurrent modifications to papr_scm_priv */
> + rc = mutex_lock_interruptible(&p->dimm_mutex);
> + if (rc)
> + return rc;
> +
> + /* Jiffies offset for which the health data is assumed to be same */
> + cache_timeout = p->lasthealth_jiffies +
> + msecs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL * 1000);
> +
> + /* Dont issue the hcall if health information is relatively new */
> + if (!force && time_after(cache_timeout, jiffies)) {
> + rc = 0;
> + goto out;
> + }
> +
> + /* issue the hcall */
> + rc = plpar_hcall(H_SCM_HEALTH, ret, p->drc_index);
> + if (rc != H_SUCCESS) {
> + dev_err(&p->pdev->dev,
> + "Failed to query health information, Err:%lld\n", rc);
> + rc = -ENXIO;
> + goto out;
> + }
> +
> + p->lasthealth_jiffies = jiffies;
> + p->health_bitmap = ret[0] & ret[1];
I said in v5 that you needn't store health_bitmap as __be64, but you do
still need to convert it to CPU endian at some point.
I think ret[] should actually be __be64, and this is where you should
convert it to CPU endian, before assigning to health_bitmap.
> +
> + dev_dbg(&p->pdev->dev,
> + "Queried dimm health info. Bitmap:0x%016lx Mask:0x%016lx\n",
> + ret[0], ret[1]);
> +
> +out:
> + mutex_unlock(&p->dimm_mutex);
> + return rc;
> +}
>
> static int papr_scm_meta_get(struct papr_scm_priv *p,
> struct nd_cmd_get_config_data_hdr *hdr)
> @@ -304,6 +364,65 @@ static inline int papr_scm_node(int node)
> return min_node;
> }
>
> +static ssize_t flags_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct nvdimm *dimm = to_nvdimm(dev);
> + struct papr_scm_priv *p = nvdimm_provider_data(dimm);
> + int rc;
> + u64 health;
> +
> + rc = drc_pmem_query_health(p, false);
> + if (rc)
> + return rc;
> +
> + /*
> + * Copy the LE byte-ordered health_bitmap locally, check for various
> + * masks and update the sysfs out buffer.
> + */
> + health = p->health_bitmap;
Why are you copying it locally? Because you don't hold the lock and
you're worried it will mutate? The compiler may just ignore you and
reload it anyway.
You'd need to use READ_ONCE(), or do it with the lock held. Or you can
just decide that it's OK for an update to race vs the display.
> +
> + if (health & PAPR_SCM_DIMM_UNARMED_MASK)
> + rc += sprintf(buf, "not_armed ");
I'd still be happier if you used snprintf() or seq_puts(), but I won't
die in a ditch over it.
> + if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK)
> + rc += sprintf(buf + rc, "save_fail ");
> +
> + if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK)
> + rc += sprintf(buf + rc, "restore_fail ");
> +
> + if (health & PAPR_SCM_DIMM_ENCRYPTED)
> + rc += sprintf(buf + rc, "encrypted ");
> +
> + if (health & PAPR_SCM_DIMM_SMART_EVENT_MASK)
> + rc += sprintf(buf + rc, "smart_notify ");
> +
> + if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED)
> + rc += sprintf(buf + rc, "scrubbed locked ");
> +
> + if (rc > 0)
> + rc += sprintf(buf + rc, "\n");
> +
> + return rc;
> +}
> +DEVICE_ATTR_RO(flags);
cheers
^ permalink raw reply
* Re: [PATCH v6 3/4] ndctl/papr_scm, uapi: Add support for PAPR nvdimm specific methods
From: Michael Ellerman @ 2020-04-30 9:17 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev, linux-nvdimm
Cc: Vaibhav Jain, Santosh Sivaraj, Oliver O'Halloran,
Dan Williams, Aneesh Kumar K . V
In-Reply-To: <20200420070711.223545-4-vaibhav@linux.ibm.com>
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> Introduce support for Papr nvDimm Specific Methods (PDSM) in papr_scm
> modules and add the command family to the white list of NVDIMM command
> sets. Also advertise support for ND_CMD_CALL for the dimm
> command mask and implement necessary scaffolding in the module to
> handle ND_CMD_CALL ioctl and PDSM requests that we receive.
>
> The layout of the PDSM request as we expect from libnvdimm/libndctl is
> described in newly introduced uapi header 'papr_scm_pdsm.h' which
> defines a new 'struct nd_pdsm_cmd_pkg' header. This header is used
> to communicate the PDSM request via member
> 'nd_pkg_papr_scm->nd_command' and size of payload that need to be
> sent/received for servicing the PDSM.
>
> A new function is_cmd_valid() is implemented that reads the args to
> papr_scm_ndctl() and performs sanity tests on them. A new function
> papr_scm_service_pdsm() is introduced and is called from
> papr_scm_ndctl() in case of a PDSM request is received via ND_CMD_CALL
> command from libnvdimm.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog
>
> v5..v6 :
> * Changed the usage of the term DSM to PDSM to distinguish it from the
> ACPI term [ Dan Williams ]
> * Renamed papr_scm_dsm.h to papr_scm_pdsm.h and updated various struct
> to reflect the new terminology.
> * Updated the patch description and title to reflect the new terminology.
> * Squashed patch to introduce new command family in 'ndctl.h' with
> this patch [ Dan Williams ]
> * Updated the papr_scm_pdsm method starting index from 0x10000 to 0x0
> [ Dan Williams ]
> * Removed redundant license text from the papr_scm_psdm.h file.
> [ Dan Williams ]
> * s/envelop/envelope/ at various places [ Dan Williams ]
> * Added '__packed' attribute to command package header to gaurd
> against different compiler adding paddings between the fields.
> [ Dan Williams]
> * Converted various pr_debug to dev_debug [ Dan Williams ]
>
> v4..v5 :
> * None
>
> v3..v4 :
> * None
>
> v2..v3 :
> * Updated the patch prefix to 'ndctl/uapi' [Aneesh]
>
> v1..v2 :
> * None
> ---
> arch/powerpc/include/uapi/asm/papr_scm_pdsm.h | 153 ++++++++++++++++++
> arch/powerpc/platforms/pseries/papr_scm.c | 101 +++++++++++-
> include/uapi/linux/ndctl.h | 1 +
> 3 files changed, 249 insertions(+), 6 deletions(-)
> create mode 100644 arch/powerpc/include/uapi/asm/papr_scm_pdsm.h
>
> diff --git a/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h b/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h
> new file mode 100644
> index 000000000000..ec48b5c7fc18
> --- /dev/null
> +++ b/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h
> @@ -0,0 +1,153 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +/*
> + * PAPR-SCM Dimm specific methods (PDSM) and structs for libndctl
> + *
> + * (C) Copyright IBM 2020
> + *
> + * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
> + */
> +
> +#ifndef _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_
> +#define _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_
> +
> +#include <linux/types.h>
> +
> +#ifdef __KERNEL__
> +#include <linux/ndctl.h>
> +#else
> +#include <ndctl.h>
> +#endif
We shouldn't be checking __KERNEL__ in uapi headers, something has gone
wrong if this is necessary.
> +
> +/*
> + * PDSM Envelope:
> + *
> + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
> + * 'envelopes' which consists of a header and user-defined payload sections.
> + * The header is described by 'struct nd_pdsm_cmd_pkg' which expects a
> + * payload following it and offset of which relative to the struct is provided
> + * by 'nd_pdsm_cmd_pkg.payload_offset'. *
> + *
> + * +-------------+---------------------+---------------------------+
> + * | 64-Bytes | 8-Bytes | Max 184-Bytes |
> + * +-------------+---------------------+---------------------------+
> + * | nd_pdsm_cmd_pkg | |
> + * |-------------+ | |
> + * | nd_cmd_pkg | | |
> + * +-------------+---------------------+---------------------------+
> + * | nd_family | | |
> + * | nd_size_out | cmd_status | |
> + * | nd_size_in | payload_version | PAYLOAD |
> + * | nd_command | payload_offset -----> |
> + * | nd_fw_size | | |
> + * +-------------+---------------------+---------------------------+
> + *
> + * PDSM Header:
> + *
> + * The header is defined as 'struct nd_pdsm_cmd_pkg' which embeds a
> + * 'struct nd_cmd_pkg' instance. The PDSM command is assigned to member
> + * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelope which is
> + * contained in 'struct nd_cmd_pkg', the header also has members following
> + * members:
> + *
> + * 'cmd_status' : (Out) Errors if any encountered while servicing PDSM.
> + * 'payload_version' : (In/Out) Version number associated with the payload.
> + * 'payload_offset' : (In)Relative offset of payload from start of envelope.
> + *
> + * PDSM Payload:
> + *
> + * The layout of the PDSM Payload is defined by various structs shared between
> + * papr_scm and libndctl so that contents of payload can be interpreted. During
> + * servicing of a PDSM the papr_scm module will read input args from the payload
> + * field by casting its contents to an appropriate struct pointer based on the
> + * PDSM command. Similarly the output of servicing the PDSM command will be
> + * copied to the payload field using the same struct.
> + *
> + * 'libnvdimm' enforces a hard limit of 256 bytes on the envelope size, which
> + * leaves around 184 bytes for the envelope payload (ignoring any padding that
> + * the compiler may silently introduce).
> + *
> + * Payload Version:
> + *
> + * A 'payload_version' field is present in PDSM header that indicates a specific
> + * version of the structure present in PDSM Payload for a given PDSM command.
> + * This provides backward compatibility in case the PDSM Payload structure
> + * evolves and different structures are supported by 'papr_scm' and 'libndctl'.
> + *
> + * When sending a PDSM Payload to 'papr_scm', 'libndctl' should send the version
> + * of the payload struct it supports via 'payload_version' field. The 'papr_scm'
> + * module when servicing the PDSM envelope checks the 'payload_version' and then
> + * uses 'payload struct version' == MIN('payload_version field',
> + * 'max payload-struct-version supported by papr_scm') to service the PDSM.
> + * After servicing the PDSM, 'papr_scm' put the negotiated version of payload
> + * struct in returned 'payload_version' field.
> + *
> + * Libndctl on receiving the envelope back from papr_scm again checks the
> + * 'payload_version' field and based on it use the appropriate version dsm
> + * struct to parse the results.
> + *
> + * Backward Compatibility:
> + *
> + * Above scheme of exchanging different versioned PDSM struct between libndctl
> + * and papr_scm should provide backward compatibility until following two
> + * assumptions/conditions when defining new PDSM structs hold:
> + *
> + * Let T(X) = { set of attributes in PDSM struct 'T' versioned X }
> + *
> + * 1. T(X) is a proper subset of T(Y) if X > Y.
> + * i.e Each new version of PDSM struct should retain existing struct
> + * attributes from previous version
> + *
> + * 2. If an entity (libndctl or papr_scm) supports a PDSM struct T(X) then
> + * it should also support T(1), T(2)...T(X - 1).
> + * i.e When adding support for new version of a PDSM struct, libndctl
> + * and papr_scm should retain support of the existing PDSM struct
> + * version they support.
> + */
> +
> +#ifndef __packed
> +#define __packed __attribute__((packed))
> +#endif
I'm not sure it's kosher to be defining __packed in a uapi header.
> +
> +/* Papr-scm-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
> +struct nd_pdsm_cmd_pkg {
> + struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */
> + __s32 cmd_status; /* Out: Sub-cmd status returned back */
> + __u16 payload_offset; /* In: offset from start of struct */
> + __u16 payload_version; /* In/Out: version of the payload */
> + __u8 payload[]; /* In/Out: Sub-cmd data buffer */
> +} __packed;
> +
> +/*
> + * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
> + * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
> + */
> +enum papr_scm_pdsm {
> + PAPR_SCM_PDSM_MIN = 0x0,
> + PAPR_SCM_PDSM_MAX,
> +};
> +
> +/* Helpers to evaluate the size of PDSM envelope */
> +/* Calculate the papr_scm-header size */
> +#define ND_PDSM_ENVELOPE_CONTENT_HDR_SIZE \
> + (sizeof(struct nd_pdsm_cmd_pkg) - sizeof(struct nd_cmd_pkg))
> +
> +/* Given a type calculate envelope-content size (papr_scm-header + payload) */
> +#define ND_PDSM_ENVELOPE_CONTENT_SIZE(_type_) \
> + (sizeof(_type_) + ND_PDSM_ENVELOPE_CONTENT_HDR_SIZE)
Those seem unused? Do we really need to make them part of the uapi for
all time?
> +/* Convert a libnvdimm nd_cmd_pkg to pdsm specific pkg */
> +static struct nd_pdsm_cmd_pkg *nd_to_pdsm_cmd_pkg(struct nd_cmd_pkg *cmd)
> +{
> + return (struct nd_pdsm_cmd_pkg *) cmd;
> +}
> +
> +/* Return the payload pointer for a given pcmd */
> +static void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
That should probably be static inline?
> +{
> + if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
> + return NULL;
> + else
> + return (void *)((__u8 *) pcmd + pcmd->payload_offset);
> +}
> +
> +#endif /* _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_ */
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index f8fe579e6f2e..20da1b837017 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -15,13 +15,15 @@
>
> #include <asm/plpar_wrappers.h>
> #include <asm/papr_scm.h>
> +#include <asm/papr_scm_pdsm.h>
>
> #define BIND_ANY_ADDR (~0ul)
>
> #define PAPR_SCM_DIMM_CMD_MASK \
> ((1ul << ND_CMD_GET_CONFIG_SIZE) | \
> (1ul << ND_CMD_GET_CONFIG_DATA) | \
> - (1ul << ND_CMD_SET_CONFIG_DATA))
> + (1ul << ND_CMD_SET_CONFIG_DATA) | \
> + (1ul << ND_CMD_CALL))
>
> struct papr_scm_priv {
> struct platform_device *pdev;
> @@ -306,16 +308,97 @@ static int papr_scm_meta_set(struct papr_scm_priv *p,
> return 0;
> }
>
> -int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> - unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
> +/*
> + * Validate the inputs args to dimm-control function and return '0' if valid.
> + * This also does initial sanity validation to ND_CMD_CALL sub-command packages.
> + */
> +static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
> + unsigned int buf_len)
> {
> - struct nd_cmd_get_config_size *get_size_hdr;
> + unsigned long cmd_mask = PAPR_SCM_DIMM_CMD_MASK;
> + struct nd_pdsm_cmd_pkg *pkg = nd_to_pdsm_cmd_pkg(buf);
> struct papr_scm_priv *p;
>
> /* Only dimm-specific calls are supported atm */
> if (!nvdimm)
> return -EINVAL;
>
> + /* get the provider date from struct nvdimm */
> + p = nvdimm_provider_data(nvdimm);
> +
> + if (!test_bit(cmd, &cmd_mask)) {
> + dev_dbg(&p->pdev->dev, "Unsupported cmd=%u\n", cmd);
> + return -EINVAL;
> + } else if (cmd == ND_CMD_CALL) {
> +
> + /* Verify the envelope package */
> + if (!buf || buf_len < sizeof(struct nd_pdsm_cmd_pkg)) {
> + dev_dbg(&p->pdev->dev, "Invalid pkg size=%u\n",
> + buf_len);
> + return -EINVAL;
> + }
> +
> + /* Verify that the PDSM family is valid */
> + if (pkg->hdr.nd_family != NVDIMM_FAMILY_PAPR_SCM) {
> + dev_dbg(&p->pdev->dev, "Invalid pkg family=0x%llx\n",
> + pkg->hdr.nd_family);
> + return -EINVAL;
> +
> + }
> +
> + /* We except a payload with all PDSM commands */
> + if (pdsm_cmd_to_payload(pkg) == NULL) {
> + dev_dbg(&p->pdev->dev,
> + "Empty payload for sub-command=0x%llx\n",
> + pkg->hdr.nd_command);
> + return -EINVAL;
> + }
> + }
> +
> + /* Command looks valid */
> + return 0;
> +}
> +
> +static int papr_scm_service_pdsm(struct papr_scm_priv *p,
> + struct nd_pdsm_cmd_pkg *call_pkg)
> +{
> + /* unknown subcommands return error in packages */
> + if (call_pkg->hdr.nd_command <= PAPR_SCM_PDSM_MIN ||
> + call_pkg->hdr.nd_command >= PAPR_SCM_PDSM_MAX) {
> + dev_dbg(&p->pdev->dev, "Invalid PDSM request 0x%llx\n",
> + call_pkg->hdr.nd_command);
> + call_pkg->cmd_status = -EINVAL;
> + return 0;
> + }
> +
> + /* Depending on the DSM command call appropriate service routine */
> + switch (call_pkg->hdr.nd_command) {
> + default:
> + dev_dbg(&p->pdev->dev, "Unsupported PDSM request 0x%llx\n",
> + call_pkg->hdr.nd_command);
> + call_pkg->cmd_status = -ENOENT;
> + return 0;
> + }
> +}
> +
> +int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> + unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
> +{
> + struct nd_cmd_get_config_size *get_size_hdr;
> + struct papr_scm_priv *p;
> + struct nd_pdsm_cmd_pkg *call_pkg = NULL;
> + int rc;
> +
> + /* Use a local variable in case cmd_rc pointer is NULL */
> + if (cmd_rc == NULL)
> + cmd_rc = &rc;
> +
> + *cmd_rc = is_cmd_valid(nvdimm, cmd, buf, buf_len);
> + if (*cmd_rc) {
> + pr_debug("Invalid cmd=0x%x. Err=%d\n", cmd, *cmd_rc);
> + return *cmd_rc;
> + }
> +
> p = nvdimm_provider_data(nvdimm);
>
> switch (cmd) {
> @@ -336,13 +419,19 @@ int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
> *cmd_rc = papr_scm_meta_set(p, buf);
> break;
>
> + case ND_CMD_CALL:
> + call_pkg = nd_to_pdsm_cmd_pkg(buf);
> + *cmd_rc = papr_scm_service_pdsm(p, call_pkg);
> + break;
> +
> default:
> - return -EINVAL;
> + dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
> + *cmd_rc = -EINVAL;
> }
>
> dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc);
>
> - return 0;
> + return *cmd_rc;
> }
>
> static inline int papr_scm_node(int node)
> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
> index de5d90212409..99fb60600ef8 100644
> --- a/include/uapi/linux/ndctl.h
> +++ b/include/uapi/linux/ndctl.h
> @@ -244,6 +244,7 @@ struct nd_cmd_pkg {
> #define NVDIMM_FAMILY_HPE2 2
> #define NVDIMM_FAMILY_MSFT 3
> #define NVDIMM_FAMILY_HYPERV 4
> +#define NVDIMM_FAMILY_PAPR_SCM 5
Would like an ack from Dan for that change.
cheers
^ permalink raw reply
* [PATCH v2 0/3] mm/memory_hotplug: Allow to not create firmware memmap entries
From: David Hildenbrand @ 2020-04-30 10:29 UTC (permalink / raw)
To: linux-kernel
Cc: linux-hyperv, Michal Hocko, Michael S . Tsirkin, Jason Wang,
Dave Hansen, Heiko Carstens, Pingfan Liu, virtualization,
linux-mm, Paul Mackerras, K. Y. Srinivasan, Dan Williams,
virtio-dev, Wei Liu, Stefano Stabellini, Dave Jiang, Baoquan He,
linux-nvdimm, David Hildenbrand, linux-acpi, Wei Yang, xen-devel,
Len Brown, Nathan Lynch, Stephen Hemminger, Pavel Tatashin,
Vasily Gorbik, linux-s390, Haiyang Zhang, Leonardo Bras,
Boris Ostrovsky, Michal Hocko, Christian Borntraeger,
Oscar Salvador, Juergen Gross, Pankaj Gupta, Greg Kroah-Hartman,
Rafael J. Wysocki, Thomas Gleixner, Eric Biederman, Vishal Verma,
Andrew Morton, linuxppc-dev
This is the follow up of [1]:
[PATCH v1 0/3] mm/memory_hotplug: Make virtio-mem play nicely with
kexec-tools
I realized that this is not only helpful for virtio-mem, but also for
dax/kmem - it's a fix for that use case (see patch #3) of persistent
memory.
Also, while testing, I discovered that kexec-tools will *not* add dax/kmem
memory (anything not directly under the root when parsing /proc/iomem) to
the elfcorehdr, so this memory will never get included in a dump. This
probably has to be fixed in kexec-tools - virtio-mem will require this as
well.
v1 -> v2:
- Don't change the resource name
- Rename the flag to MHP_NO_FIRMWARE_MEMMAP to reflect what it is doing
- Rephrase subjects/descriptions
- Use the flag for dax/kmem
I'll have to rebase virtio-mem on these changes, there will be a resend.
[1] https://lkml.kernel.org/r/20200429160803.109056-1-david@redhat.com
David Hildenbrand (3):
mm/memory_hotplug: Prepare passing flags to add_memory() and friends
mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
device-dax: Add system ram (add_memory()) with MHP_NO_FIRMWARE_MEMMAP
arch/powerpc/platforms/powernv/memtrace.c | 2 +-
arch/powerpc/platforms/pseries/hotplug-memory.c | 2 +-
drivers/acpi/acpi_memhotplug.c | 2 +-
drivers/base/memory.c | 2 +-
drivers/dax/kmem.c | 3 ++-
drivers/hv/hv_balloon.c | 2 +-
drivers/s390/char/sclp_cmd.c | 2 +-
drivers/xen/balloon.c | 2 +-
include/linux/memory_hotplug.h | 15 ++++++++++++---
mm/memory_hotplug.c | 14 ++++++++------
10 files changed, 29 insertions(+), 17 deletions(-)
--
2.25.3
^ permalink raw reply
* [PATCH v2 1/3] mm/memory_hotplug: Prepare passing flags to add_memory() and friends
From: David Hildenbrand @ 2020-04-30 10:29 UTC (permalink / raw)
To: linux-kernel
Cc: linux-hyperv, Michal Hocko, Michael S . Tsirkin, Jason Wang,
Heiko Carstens, Pingfan Liu, virtualization, linux-mm,
Paul Mackerras, K. Y. Srinivasan, Boris Ostrovsky, virtio-dev,
Wei Liu, Stefano Stabellini, Dave Jiang, Baoquan He, linux-nvdimm,
Vishal Verma, David Hildenbrand, linux-acpi, Wei Yang, xen-devel,
Len Brown, Nathan Lynch, Vasily Gorbik, linux-s390, Haiyang Zhang,
Leonardo Bras, Stephen Hemminger, Dan Williams, Michal Hocko,
Christian Borntraeger, Oscar Salvador, Juergen Gross,
Pankaj Gupta, Greg Kroah-Hartman, Rafael J. Wysocki,
Thomas Gleixner, Eric Biederman, Andrew Morton, linuxppc-dev
In-Reply-To: <20200430102908.10107-1-david@redhat.com>
We soon want to pass flags - prepare for that.
This patch is based on a similar patch by Oscar Salvador:
https://lkml.kernel.org/r/20190625075227.15193-3-osalvador@suse.de
Acked-by: Wei Liu <wei.liu@kernel.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Pingfan Liu <kernelfans@gmail.com>
Cc: Leonardo Bras <leobras.c@gmail.com>
Cc: Nathan Lynch <nathanl@linux.ibm.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Cc: linux-nvdimm@lists.01.org
Cc: linux-hyperv@vger.kernel.org
Cc: linux-s390@vger.kernel.org
Cc: virtualization@lists.linux-foundation.org
Cc: xen-devel@lists.xenproject.org
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/powerpc/platforms/powernv/memtrace.c | 2 +-
arch/powerpc/platforms/pseries/hotplug-memory.c | 2 +-
drivers/acpi/acpi_memhotplug.c | 2 +-
drivers/base/memory.c | 2 +-
drivers/dax/kmem.c | 2 +-
drivers/hv/hv_balloon.c | 2 +-
drivers/s390/char/sclp_cmd.c | 2 +-
drivers/xen/balloon.c | 2 +-
include/linux/memory_hotplug.h | 7 ++++---
mm/memory_hotplug.c | 11 ++++++-----
10 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/memtrace.c b/arch/powerpc/platforms/powernv/memtrace.c
index 13b369d2cc45..a7475d18c671 100644
--- a/arch/powerpc/platforms/powernv/memtrace.c
+++ b/arch/powerpc/platforms/powernv/memtrace.c
@@ -224,7 +224,7 @@ static int memtrace_online(void)
ent->mem = 0;
}
- if (add_memory(ent->nid, ent->start, ent->size)) {
+ if (add_memory(ent->nid, ent->start, ent->size, 0)) {
pr_err("Failed to add trace memory to node %d\n",
ent->nid);
ret += 1;
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 5ace2f9a277e..ae44eba46ca0 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -646,7 +646,7 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
block_sz = memory_block_size_bytes();
/* Add the memory */
- rc = __add_memory(lmb->nid, lmb->base_addr, block_sz);
+ rc = __add_memory(lmb->nid, lmb->base_addr, block_sz, 0);
if (rc) {
invalidate_lmb_associativity_index(lmb);
return rc;
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index e294f44a7850..d91b3584d4b2 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -207,7 +207,7 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
if (node < 0)
node = memory_add_physaddr_to_nid(info->start_addr);
- result = __add_memory(node, info->start_addr, info->length);
+ result = __add_memory(node, info->start_addr, info->length, 0);
/*
* If the memory block has been used by the kernel, add_memory()
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 2b09b68b9f78..c0ef7d9e310a 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -432,7 +432,7 @@ static ssize_t probe_store(struct device *dev, struct device_attribute *attr,
nid = memory_add_physaddr_to_nid(phys_addr);
ret = __add_memory(nid, phys_addr,
- MIN_MEMORY_BLOCK_SIZE * sections_per_block);
+ MIN_MEMORY_BLOCK_SIZE * sections_per_block, 0);
if (ret)
goto out;
diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
index 3d0a7e702c94..e159184e0ba0 100644
--- a/drivers/dax/kmem.c
+++ b/drivers/dax/kmem.c
@@ -65,7 +65,7 @@ int dev_dax_kmem_probe(struct device *dev)
new_res->flags = IORESOURCE_SYSTEM_RAM;
new_res->name = dev_name(dev);
- rc = add_memory(numa_node, new_res->start, resource_size(new_res));
+ rc = add_memory(numa_node, new_res->start, resource_size(new_res), 0);
if (rc) {
release_resource(new_res);
kfree(new_res);
diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 32e3bc0aa665..0194bed1a573 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -726,7 +726,7 @@ static void hv_mem_hot_add(unsigned long start, unsigned long size,
nid = memory_add_physaddr_to_nid(PFN_PHYS(start_pfn));
ret = add_memory(nid, PFN_PHYS((start_pfn)),
- (HA_CHUNK << PAGE_SHIFT));
+ (HA_CHUNK << PAGE_SHIFT), 0);
if (ret) {
pr_err("hot_add memory failed error is %d\n", ret);
diff --git a/drivers/s390/char/sclp_cmd.c b/drivers/s390/char/sclp_cmd.c
index a864b21af602..a6a908244c74 100644
--- a/drivers/s390/char/sclp_cmd.c
+++ b/drivers/s390/char/sclp_cmd.c
@@ -406,7 +406,7 @@ static void __init add_memory_merged(u16 rn)
if (!size)
goto skip_add;
for (addr = start; addr < start + size; addr += block_size)
- add_memory(0, addr, block_size);
+ add_memory(0, addr, block_size, 0);
skip_add:
first_rn = rn;
num = 1;
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 0c142bcab79d..6ec0373fa624 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -347,7 +347,7 @@ static enum bp_state reserve_additional_memory(void)
mutex_unlock(&balloon_mutex);
/* add_memory_resource() requires the device_hotplug lock */
lock_device_hotplug();
- rc = add_memory_resource(nid, resource);
+ rc = add_memory_resource(nid, resource, 0);
unlock_device_hotplug();
mutex_lock(&balloon_mutex);
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 7dca9cd6076b..0151fb935c09 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -339,9 +339,10 @@ extern void set_zone_contiguous(struct zone *zone);
extern void clear_zone_contiguous(struct zone *zone);
extern void __ref free_area_init_core_hotplug(int nid);
-extern int __add_memory(int nid, u64 start, u64 size);
-extern int add_memory(int nid, u64 start, u64 size);
-extern int add_memory_resource(int nid, struct resource *resource);
+extern int __add_memory(int nid, u64 start, u64 size, unsigned long flags);
+extern int add_memory(int nid, u64 start, u64 size, unsigned long flags);
+extern int add_memory_resource(int nid, struct resource *resource,
+ unsigned long flags);
extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
unsigned long nr_pages, struct vmem_altmap *altmap);
extern void remove_pfn_range_from_zone(struct zone *zone,
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 555137bd0882..c01be92693e3 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1004,7 +1004,8 @@ static int online_memory_block(struct memory_block *mem, void *arg)
*
* we are OK calling __meminit stuff here - we have CONFIG_MEMORY_HOTPLUG
*/
-int __ref add_memory_resource(int nid, struct resource *res)
+int __ref add_memory_resource(int nid, struct resource *res,
+ unsigned long flags)
{
struct mhp_params params = { .pgprot = PAGE_KERNEL };
u64 start, size;
@@ -1082,7 +1083,7 @@ int __ref add_memory_resource(int nid, struct resource *res)
}
/* requires device_hotplug_lock, see add_memory_resource() */
-int __ref __add_memory(int nid, u64 start, u64 size)
+int __ref __add_memory(int nid, u64 start, u64 size, unsigned long flags)
{
struct resource *res;
int ret;
@@ -1091,18 +1092,18 @@ int __ref __add_memory(int nid, u64 start, u64 size)
if (IS_ERR(res))
return PTR_ERR(res);
- ret = add_memory_resource(nid, res);
+ ret = add_memory_resource(nid, res, flags);
if (ret < 0)
release_memory_resource(res);
return ret;
}
-int add_memory(int nid, u64 start, u64 size)
+int add_memory(int nid, u64 start, u64 size, unsigned long flags)
{
int rc;
lock_device_hotplug();
- rc = __add_memory(nid, start, size);
+ rc = __add_memory(nid, start, size, flags);
unlock_device_hotplug();
return rc;
--
2.25.3
^ permalink raw reply related
* [PATCH v2 2/3] mm/memory_hotplug: Introduce MHP_NO_FIRMWARE_MEMMAP
From: David Hildenbrand @ 2020-04-30 10:29 UTC (permalink / raw)
To: linux-kernel
Cc: virtio-dev, linux-hyperv, Michal Hocko, linux-acpi, Baoquan He,
linux-nvdimm, linux-s390, Michael S . Tsirkin, David Hildenbrand,
virtualization, linux-mm, Wei Yang, Eric Biederman, Pankaj Gupta,
xen-devel, Andrew Morton, Michal Hocko, linuxppc-dev
In-Reply-To: <20200430102908.10107-1-david@redhat.com>
Some devices/drivers that add memory via add_memory() and friends (e.g.,
dax/kmem, but also virtio-mem in the future) don't want to create entries
in /sys/firmware/memmap/ - primarily to hinder kexec from adding this
memory to the boot memmap of the kexec kernel.
In fact, such memory is never exposed via the firmware memmap as System
RAM (e.g., e820), so exposing this memory via /sys/firmware/memmap/ is
wrong:
"kexec needs the raw firmware-provided memory map to setup the
parameter segment of the kernel that should be booted with
kexec. Also, the raw memory map is useful for debugging. For
that reason, /sys/firmware/memmap is an interface that provides
the raw memory map to userspace." [1]
We don't have to worry about firmware_map_remove() on the removal path.
If there is no entry, it will simply return with -EINVAL.
[1] https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-firmware-memmap
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
include/linux/memory_hotplug.h | 8 ++++++++
mm/memory_hotplug.c | 3 ++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 0151fb935c09..4ca418a731eb 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -68,6 +68,14 @@ struct mhp_params {
pgprot_t pgprot;
};
+/* Flags used for add_memory() and friends. */
+
+/*
+ * Don't create entries in /sys/firmware/memmap/. The memory is detected and
+ * added via a device driver, not via the initial (firmware) memmap.
+ */
+#define MHP_NO_FIRMWARE_MEMMAP 1
+
/*
* Zone resizing functions
*
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index c01be92693e3..e94ede9cad00 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1062,7 +1062,8 @@ int __ref add_memory_resource(int nid, struct resource *res,
BUG_ON(ret);
/* create new memmap entry */
- firmware_map_add_hotplug(start, start + size, "System RAM");
+ if (!(flags & MHP_NO_FIRMWARE_MEMMAP))
+ firmware_map_add_hotplug(start, start + size, "System RAM");
/* device_online() will take the lock when calling online_pages() */
mem_hotplug_done();
--
2.25.3
^ permalink raw reply related
* [PATCH v2 3/3] device-dax: Add system ram (add_memory()) with MHP_NO_FIRMWARE_MEMMAP
From: David Hildenbrand @ 2020-04-30 10:29 UTC (permalink / raw)
To: linux-kernel
Cc: virtio-dev, linux-hyperv, Michal Hocko, linux-acpi, Baoquan He,
linux-nvdimm, linux-s390, Michael S . Tsirkin, David Hildenbrand,
Dave Hansen, virtualization, linux-mm, Wei Yang, Eric Biederman,
Pankaj Gupta, xen-devel, Andrew Morton, Michal Hocko,
linuxppc-dev, Dan Williams, Pavel Tatashin
In-Reply-To: <20200430102908.10107-1-david@redhat.com>
Currently, when adding memory, we create entries in /sys/firmware/memmap/
as "System RAM". This does not reflect the reality and will lead to
kexec-tools to add that memory to the fixed-up initial memmap for a
kexec kernel (loaded via kexec_load()). The memory will be considered
initial System RAM by the kexec kernel.
We should let the kexec kernel decide how to use that memory - just as
we do during an ordinary reboot.
Before configuring the namespace:
[root@localhost ~]# cat /proc/iomem
...
140000000-33fffffff : Persistent Memory
140000000-33fffffff : namespace0.0
3280000000-32ffffffff : PCI Bus 0000:00
After configuring the namespace:
[root@localhost ~]# cat /proc/iomem
...
140000000-33fffffff : Persistent Memory
140000000-1481fffff : namespace0.0
148200000-33fffffff : dax0.0
3280000000-32ffffffff : PCI Bus 0000:00
After loading kmem:
[root@localhost ~]# cat /proc/iomem
...
140000000-33fffffff : Persistent Memory
140000000-1481fffff : namespace0.0
150000000-33fffffff : dax0.0
150000000-33fffffff : System RAM
3280000000-32ffffffff : PCI Bus 0000:00
After a proper reboot:
[root@localhost ~]# cat /proc/iomem
...
140000000-33fffffff : Persistent Memory
140000000-1481fffff : namespace0.0
148200000-33fffffff : dax0.0
3280000000-32ffffffff : PCI Bus 0000:00
Within the kexec kernel before this change:
[root@localhost ~]# cat /proc/iomem
...
140000000-33fffffff : Persistent Memory
140000000-1481fffff : namespace0.0
150000000-33fffffff : System RAM
3280000000-32ffffffff : PCI Bus 0000:00
Within the kexec kernel after this change:
[root@localhost ~]# cat /proc/iomem
...
140000000-33fffffff : Persistent Memory
140000000-1481fffff : namespace0.0
148200000-33fffffff : dax0.0
3280000000-32ffffffff : PCI Bus 0000:00
/sys/firmware/memmap/ before this change:
0000000000000000-000000000009fc00 (System RAM)
000000000009fc00-00000000000a0000 (Reserved)
00000000000f0000-0000000000100000 (Reserved)
0000000000100000-00000000bffdf000 (System RAM)
00000000bffdf000-00000000c0000000 (Reserved)
00000000feffc000-00000000ff000000 (Reserved)
00000000fffc0000-0000000100000000 (Reserved)
0000000100000000-0000000140000000 (System RAM)
0000000150000000-0000000340000000 (System RAM)
/sys/firmware/memmap/ after a proper reboot:
0000000000000000-000000000009fc00 (System RAM)
000000000009fc00-00000000000a0000 (Reserved)
00000000000f0000-0000000000100000 (Reserved)
0000000000100000-00000000bffdf000 (System RAM)
00000000bffdf000-00000000c0000000 (Reserved)
00000000feffc000-00000000ff000000 (Reserved)
00000000fffc0000-0000000100000000 (Reserved)
0000000100000000-0000000140000000 (System RAM)
/sys/firmware/memmap/ after this change:
0000000000000000-000000000009fc00 (System RAM)
000000000009fc00-00000000000a0000 (Reserved)
00000000000f0000-0000000000100000 (Reserved)
0000000000100000-00000000bffdf000 (System RAM)
00000000bffdf000-00000000c0000000 (Reserved)
00000000feffc000-00000000ff000000 (Reserved)
00000000fffc0000-0000000100000000 (Reserved)
0000000100000000-0000000140000000 (System RAM)
kexec-tools already seem to basically ignore any System RAM that's not
on top level when searching for areas to place kexec images - but also
for determining crash areas to dump via kdump. This behavior is not
changed by this patch. kexec-tools probably has to be fixed to also
include this memory in system dumps.
Note: kexec_file_load() does the right thing already within the kernel.
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
drivers/dax/kmem.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
index e159184e0ba0..929823a79816 100644
--- a/drivers/dax/kmem.c
+++ b/drivers/dax/kmem.c
@@ -65,7 +65,8 @@ int dev_dax_kmem_probe(struct device *dev)
new_res->flags = IORESOURCE_SYSTEM_RAM;
new_res->name = dev_name(dev);
- rc = add_memory(numa_node, new_res->start, resource_size(new_res), 0);
+ rc = add_memory(numa_node, new_res->start, resource_size(new_res),
+ MHP_NO_FIRMWARE_MEMMAP);
if (rc) {
release_resource(new_res);
kfree(new_res);
--
2.25.3
^ permalink raw reply related
* Re: [PATCH v6 3/4] ndctl/papr_scm, uapi: Add support for PAPR nvdimm specific methods
From: Vaibhav Jain @ 2020-04-30 11:07 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev, linux-nvdimm; +Cc: Aneesh Kumar K . V
In-Reply-To: <87lfmde3il.fsf@mpe.ellerman.id.au>
Hi Mpe,
Thanks again for reviewing this patchset.
Michael Ellerman <mpe@ellerman.id.au> writes:
> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
>> Introduce support for Papr nvDimm Specific Methods (PDSM) in papr_scm
>> modules and add the command family to the white list of NVDIMM command
>> sets. Also advertise support for ND_CMD_CALL for the dimm
>> command mask and implement necessary scaffolding in the module to
>> handle ND_CMD_CALL ioctl and PDSM requests that we receive.
>>
>> The layout of the PDSM request as we expect from libnvdimm/libndctl is
>> described in newly introduced uapi header 'papr_scm_pdsm.h' which
>> defines a new 'struct nd_pdsm_cmd_pkg' header. This header is used
>> to communicate the PDSM request via member
>> 'nd_pkg_papr_scm->nd_command' and size of payload that need to be
>> sent/received for servicing the PDSM.
>>
>> A new function is_cmd_valid() is implemented that reads the args to
>> papr_scm_ndctl() and performs sanity tests on them. A new function
>> papr_scm_service_pdsm() is introduced and is called from
>> papr_scm_ndctl() in case of a PDSM request is received via ND_CMD_CALL
>> command from libnvdimm.
>>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> Changelog
>>
>> v5..v6 :
>> * Changed the usage of the term DSM to PDSM to distinguish it from the
>> ACPI term [ Dan Williams ]
>> * Renamed papr_scm_dsm.h to papr_scm_pdsm.h and updated various struct
>> to reflect the new terminology.
>> * Updated the patch description and title to reflect the new terminology.
>> * Squashed patch to introduce new command family in 'ndctl.h' with
>> this patch [ Dan Williams ]
>> * Updated the papr_scm_pdsm method starting index from 0x10000 to 0x0
>> [ Dan Williams ]
>> * Removed redundant license text from the papr_scm_psdm.h file.
>> [ Dan Williams ]
>> * s/envelop/envelope/ at various places [ Dan Williams ]
>> * Added '__packed' attribute to command package header to gaurd
>> against different compiler adding paddings between the fields.
>> [ Dan Williams]
>> * Converted various pr_debug to dev_debug [ Dan Williams ]
>>
>> v4..v5 :
>> * None
>>
>> v3..v4 :
>> * None
>>
>> v2..v3 :
>> * Updated the patch prefix to 'ndctl/uapi' [Aneesh]
>>
>> v1..v2 :
>> * None
>> ---
>> arch/powerpc/include/uapi/asm/papr_scm_pdsm.h | 153 ++++++++++++++++++
>> arch/powerpc/platforms/pseries/papr_scm.c | 101 +++++++++++-
>> include/uapi/linux/ndctl.h | 1 +
>> 3 files changed, 249 insertions(+), 6 deletions(-)
>> create mode 100644 arch/powerpc/include/uapi/asm/papr_scm_pdsm.h
>>
>> diff --git a/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h b/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h
>> new file mode 100644
>> index 000000000000..ec48b5c7fc18
>> --- /dev/null
>> +++ b/arch/powerpc/include/uapi/asm/papr_scm_pdsm.h
>> @@ -0,0 +1,153 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
>> +/*
>> + * PAPR-SCM Dimm specific methods (PDSM) and structs for libndctl
>> + *
>> + * (C) Copyright IBM 2020
>> + *
>> + * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
>> + */
>> +
>> +#ifndef _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_
>> +#define _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_
>> +
>> +#include <linux/types.h>
>> +
>> +#ifdef __KERNEL__
>> +#include <linux/ndctl.h>
>> +#else
>> +#include <ndctl.h>
>> +#endif
>
> We shouldn't be checking __KERNEL__ in uapi headers, something has gone
> wrong if this is necessary.
This header file is shared with userspace libndctl changes as proposed at
https://lore.kernel.org/linux-nvdimm/20200420075556.272174-5-vaibhav@linux.ibm.com/
The definition of "struct nd_cmd_pkg" on which 'struct nd_pdsm_cmd_pkg'
depenends on, is available in different locations in kernel and libndctl
>
>> +
>> +/*
>> + * PDSM Envelope:
>> + *
>> + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
>> + * 'envelopes' which consists of a header and user-defined payload sections.
>> + * The header is described by 'struct nd_pdsm_cmd_pkg' which expects a
>> + * payload following it and offset of which relative to the struct is provided
>> + * by 'nd_pdsm_cmd_pkg.payload_offset'. *
>> + *
>> + * +-------------+---------------------+---------------------------+
>> + * | 64-Bytes | 8-Bytes | Max 184-Bytes |
>> + * +-------------+---------------------+---------------------------+
>> + * | nd_pdsm_cmd_pkg | |
>> + * |-------------+ | |
>> + * | nd_cmd_pkg | | |
>> + * +-------------+---------------------+---------------------------+
>> + * | nd_family | | |
>> + * | nd_size_out | cmd_status | |
>> + * | nd_size_in | payload_version | PAYLOAD |
>> + * | nd_command | payload_offset -----> |
>> + * | nd_fw_size | | |
>> + * +-------------+---------------------+---------------------------+
>> + *
>> + * PDSM Header:
>> + *
>> + * The header is defined as 'struct nd_pdsm_cmd_pkg' which embeds a
>> + * 'struct nd_cmd_pkg' instance. The PDSM command is assigned to member
>> + * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelope which is
>> + * contained in 'struct nd_cmd_pkg', the header also has members following
>> + * members:
>> + *
>> + * 'cmd_status' : (Out) Errors if any encountered while servicing PDSM.
>> + * 'payload_version' : (In/Out) Version number associated with the payload.
>> + * 'payload_offset' : (In)Relative offset of payload from start of envelope.
>> + *
>> + * PDSM Payload:
>> + *
>> + * The layout of the PDSM Payload is defined by various structs shared between
>> + * papr_scm and libndctl so that contents of payload can be interpreted. During
>> + * servicing of a PDSM the papr_scm module will read input args from the payload
>> + * field by casting its contents to an appropriate struct pointer based on the
>> + * PDSM command. Similarly the output of servicing the PDSM command will be
>> + * copied to the payload field using the same struct.
>> + *
>> + * 'libnvdimm' enforces a hard limit of 256 bytes on the envelope size, which
>> + * leaves around 184 bytes for the envelope payload (ignoring any padding that
>> + * the compiler may silently introduce).
>> + *
>> + * Payload Version:
>> + *
>> + * A 'payload_version' field is present in PDSM header that indicates a specific
>> + * version of the structure present in PDSM Payload for a given PDSM command.
>> + * This provides backward compatibility in case the PDSM Payload structure
>> + * evolves and different structures are supported by 'papr_scm' and 'libndctl'.
>> + *
>> + * When sending a PDSM Payload to 'papr_scm', 'libndctl' should send the version
>> + * of the payload struct it supports via 'payload_version' field. The 'papr_scm'
>> + * module when servicing the PDSM envelope checks the 'payload_version' and then
>> + * uses 'payload struct version' == MIN('payload_version field',
>> + * 'max payload-struct-version supported by papr_scm') to service the PDSM.
>> + * After servicing the PDSM, 'papr_scm' put the negotiated version of payload
>> + * struct in returned 'payload_version' field.
>> + *
>> + * Libndctl on receiving the envelope back from papr_scm again checks the
>> + * 'payload_version' field and based on it use the appropriate version dsm
>> + * struct to parse the results.
>> + *
>> + * Backward Compatibility:
>> + *
>> + * Above scheme of exchanging different versioned PDSM struct between libndctl
>> + * and papr_scm should provide backward compatibility until following two
>> + * assumptions/conditions when defining new PDSM structs hold:
>> + *
>> + * Let T(X) = { set of attributes in PDSM struct 'T' versioned X }
>> + *
>> + * 1. T(X) is a proper subset of T(Y) if X > Y.
>> + * i.e Each new version of PDSM struct should retain existing struct
>> + * attributes from previous version
>> + *
>> + * 2. If an entity (libndctl or papr_scm) supports a PDSM struct T(X) then
>> + * it should also support T(1), T(2)...T(X - 1).
>> + * i.e When adding support for new version of a PDSM struct, libndctl
>> + * and papr_scm should retain support of the existing PDSM struct
>> + * version they support.
>> + */
>> +
>> +#ifndef __packed
>> +#define __packed __attribute__((packed))
>> +#endif
>
> I'm not sure it's kosher to be defining __packed in a uapi header.
>
Similar reason as before. Userspace libndctl doesnt provide a definition
of __packed hence adding it here.
>> +
>> +/* Papr-scm-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
>> +struct nd_pdsm_cmd_pkg {
>> + struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */
>> + __s32 cmd_status; /* Out: Sub-cmd status returned back */
>> + __u16 payload_offset; /* In: offset from start of struct */
>> + __u16 payload_version; /* In/Out: version of the payload */
>> + __u8 payload[]; /* In/Out: Sub-cmd data buffer */
>> +} __packed;
>> +
>> +/*
>> + * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
>> + * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
>> + */
>> +enum papr_scm_pdsm {
>> + PAPR_SCM_PDSM_MIN = 0x0,
>> + PAPR_SCM_PDSM_MAX,
>> +};
>> +
>> +/* Helpers to evaluate the size of PDSM envelope */
>> +/* Calculate the papr_scm-header size */
>> +#define ND_PDSM_ENVELOPE_CONTENT_HDR_SIZE \
>> + (sizeof(struct nd_pdsm_cmd_pkg) - sizeof(struct nd_cmd_pkg))
>> +
>> +/* Given a type calculate envelope-content size (papr_scm-header + payload) */
>> +#define ND_PDSM_ENVELOPE_CONTENT_SIZE(_type_) \
>> + (sizeof(_type_) + ND_PDSM_ENVELOPE_CONTENT_HDR_SIZE)
>
> Those seem unused? Do we really need to make them part of the uapi for
> all time?
This helper macro is used in proposed libndctl changes to calculate returning
payload buffer size from the kernel.
https://lore.kernel.org/linux-nvdimm/20200420075556.272174-7-vaibhav@linux.ibm.com/
>
>> +/* Convert a libnvdimm nd_cmd_pkg to pdsm specific pkg */
>> +static struct nd_pdsm_cmd_pkg *nd_to_pdsm_cmd_pkg(struct nd_cmd_pkg *cmd)
>> +{
>> + return (struct nd_pdsm_cmd_pkg *) cmd;
>> +}
>> +
>> +/* Return the payload pointer for a given pcmd */
>> +static void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
>
> That should probably be static inline?
Agree, will do.
>
>> +{
>> + if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
>> + return NULL;
>> + else
>> + return (void *)((__u8 *) pcmd + pcmd->payload_offset);
>> +}
>> +
>> +#endif /* _UAPI_ASM_POWERPC_PAPR_SCM_PDSM_H_ */
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index f8fe579e6f2e..20da1b837017 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -15,13 +15,15 @@
>>
>> #include <asm/plpar_wrappers.h>
>> #include <asm/papr_scm.h>
>> +#include <asm/papr_scm_pdsm.h>
>>
>> #define BIND_ANY_ADDR (~0ul)
>>
>> #define PAPR_SCM_DIMM_CMD_MASK \
>> ((1ul << ND_CMD_GET_CONFIG_SIZE) | \
>> (1ul << ND_CMD_GET_CONFIG_DATA) | \
>> - (1ul << ND_CMD_SET_CONFIG_DATA))
>> + (1ul << ND_CMD_SET_CONFIG_DATA) | \
>> + (1ul << ND_CMD_CALL))
>>
>> struct papr_scm_priv {
>> struct platform_device *pdev;
>> @@ -306,16 +308,97 @@ static int papr_scm_meta_set(struct papr_scm_priv *p,
>> return 0;
>> }
>>
>> -int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>> - unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
>> +/*
>> + * Validate the inputs args to dimm-control function and return '0' if valid.
>> + * This also does initial sanity validation to ND_CMD_CALL sub-command packages.
>> + */
>> +static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
>> + unsigned int buf_len)
>> {
>> - struct nd_cmd_get_config_size *get_size_hdr;
>> + unsigned long cmd_mask = PAPR_SCM_DIMM_CMD_MASK;
>> + struct nd_pdsm_cmd_pkg *pkg = nd_to_pdsm_cmd_pkg(buf);
>> struct papr_scm_priv *p;
>>
>> /* Only dimm-specific calls are supported atm */
>> if (!nvdimm)
>> return -EINVAL;
>>
>> + /* get the provider date from struct nvdimm */
>> + p = nvdimm_provider_data(nvdimm);
>> +
>> + if (!test_bit(cmd, &cmd_mask)) {
>> + dev_dbg(&p->pdev->dev, "Unsupported cmd=%u\n", cmd);
>> + return -EINVAL;
>> + } else if (cmd == ND_CMD_CALL) {
>> +
>> + /* Verify the envelope package */
>> + if (!buf || buf_len < sizeof(struct nd_pdsm_cmd_pkg)) {
>> + dev_dbg(&p->pdev->dev, "Invalid pkg size=%u\n",
>> + buf_len);
>> + return -EINVAL;
>> + }
>> +
>> + /* Verify that the PDSM family is valid */
>> + if (pkg->hdr.nd_family != NVDIMM_FAMILY_PAPR_SCM) {
>> + dev_dbg(&p->pdev->dev, "Invalid pkg family=0x%llx\n",
>> + pkg->hdr.nd_family);
>> + return -EINVAL;
>> +
>> + }
>> +
>> + /* We except a payload with all PDSM commands */
>> + if (pdsm_cmd_to_payload(pkg) == NULL) {
>> + dev_dbg(&p->pdev->dev,
>> + "Empty payload for sub-command=0x%llx\n",
>> + pkg->hdr.nd_command);
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + /* Command looks valid */
>> + return 0;
>> +}
>> +
>> +static int papr_scm_service_pdsm(struct papr_scm_priv *p,
>> + struct nd_pdsm_cmd_pkg *call_pkg)
>> +{
>> + /* unknown subcommands return error in packages */
>> + if (call_pkg->hdr.nd_command <= PAPR_SCM_PDSM_MIN ||
>> + call_pkg->hdr.nd_command >= PAPR_SCM_PDSM_MAX) {
>> + dev_dbg(&p->pdev->dev, "Invalid PDSM request 0x%llx\n",
>> + call_pkg->hdr.nd_command);
>> + call_pkg->cmd_status = -EINVAL;
>> + return 0;
>> + }
>> +
>> + /* Depending on the DSM command call appropriate service routine */
>> + switch (call_pkg->hdr.nd_command) {
>> + default:
>> + dev_dbg(&p->pdev->dev, "Unsupported PDSM request 0x%llx\n",
>> + call_pkg->hdr.nd_command);
>> + call_pkg->cmd_status = -ENOENT;
>> + return 0;
>> + }
>> +}
>> +
>> +int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>> + unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
>> +{
>> + struct nd_cmd_get_config_size *get_size_hdr;
>> + struct papr_scm_priv *p;
>> + struct nd_pdsm_cmd_pkg *call_pkg = NULL;
>> + int rc;
>> +
>> + /* Use a local variable in case cmd_rc pointer is NULL */
>> + if (cmd_rc == NULL)
>> + cmd_rc = &rc;
>> +
>> + *cmd_rc = is_cmd_valid(nvdimm, cmd, buf, buf_len);
>> + if (*cmd_rc) {
>> + pr_debug("Invalid cmd=0x%x. Err=%d\n", cmd, *cmd_rc);
>> + return *cmd_rc;
>> + }
>> +
>> p = nvdimm_provider_data(nvdimm);
>>
>> switch (cmd) {
>> @@ -336,13 +419,19 @@ int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>> *cmd_rc = papr_scm_meta_set(p, buf);
>> break;
>>
>> + case ND_CMD_CALL:
>> + call_pkg = nd_to_pdsm_cmd_pkg(buf);
>> + *cmd_rc = papr_scm_service_pdsm(p, call_pkg);
>> + break;
>> +
>> default:
>> - return -EINVAL;
>> + dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
>> + *cmd_rc = -EINVAL;
>> }
>>
>> dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc);
>>
>> - return 0;
>> + return *cmd_rc;
>> }
>>
>> static inline int papr_scm_node(int node)
>> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
>> index de5d90212409..99fb60600ef8 100644
>> --- a/include/uapi/linux/ndctl.h
>> +++ b/include/uapi/linux/ndctl.h
>> @@ -244,6 +244,7 @@ struct nd_cmd_pkg {
>> #define NVDIMM_FAMILY_HPE2 2
>> #define NVDIMM_FAMILY_MSFT 3
>> #define NVDIMM_FAMILY_HYPERV 4
>> +#define NVDIMM_FAMILY_PAPR_SCM 5
>
> Would like an ack from Dan for that change.
>
Agree
> cheers
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
~ Vaibhav
^ 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