Re: [EDT][PATCH 1/1] hw_breakpoint.c :cpu hotplug handling

Re: [EDT][PATCH 1/1] hw_breakpoint.c :cpu hotplug handling

[Vaneet Narang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 1455 bytes --]

EP-2DAD0AFA905A4ACB804C4F82A001242F

On Wed, May 13, 2015 at 06:24:06AM +0100, Maninder Singh wrote:
>> EP-2DAD0AFA905A4ACB804C4F82A001242F
>> 
>> Subject: [PATCH 1/1] hw_breakpoint.c :cpu hotplug handling
>> 
>> This patch adds support for CPU hotplug, It re-installl all installed watchpoints and breakpoints
>> back on H/W in case of cpu-hot plug.

>Not sure why this is needed -- the scheduler should reinstall the
>breakpoints when the debugged task gets scheduled in via
>arch_install_hw_breakpoint.
>
>Will

I agree with you this reinstalling has to be either take care by scheduler or Debug tool. 
In current implementation we clear H/W registers but we don't clear slots (wp_on_reg / bp_on_reg) for both watchpoint or breakpoint.
So it makes mandatory for debug tool to uninstall breakpoints when CPU goes offline, because if we don't 
uninstall which I think is not required and when CPU comes online, we will not be able to reinstall them back because there will be no free slots.
Despite of the fact that H/W registers are free but still we wouldn't be able to install breakpoints.
Logically we should clear these slots or we should reinstall but if you think reinstall has to be taken care by scheduler or debugger
then at least we should clear these slots.

Regards,
Vaneet Narang
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [EDT][PATCH 1/1] hw_breakpoint.c :cpu hotplug handling

[Will Deacon]

On Mon, May 18, 2015 at 05:46:31AM +0100, Vaneet Narang wrote:
> EP-2DAD0AFA905A4ACB804C4F82A001242F
> 
> On Wed, May 13, 2015 at 06:24:06AM +0100, Maninder Singh wrote:
> >> EP-2DAD0AFA905A4ACB804C4F82A001242F
> >> 
> >> Subject: [PATCH 1/1] hw_breakpoint.c :cpu hotplug handling
> >> 
> >> This patch adds support for CPU hotplug, It re-installl all installed watchpoints and breakpoints
> >> back on H/W in case of cpu-hot plug.
> 
> >Not sure why this is needed -- the scheduler should reinstall the
> >breakpoints when the debugged task gets scheduled in via
> >arch_install_hw_breakpoint.
> >
> >Will
> 
> I agree with you this reinstalling has to be either take care by scheduler
> or Debug tool. 

Which debug tool? If it's a ptrace user, then the scheduler should still
manage this, as ptrace will call into perf to create the breakpoints.

> In current implementation we clear H/W registers but we don't clear slots
> (wp_on_reg / bp_on_reg) for both watchpoint or breakpoint.

Ok, so are you saying that when the CPU is hotplugged off, the active
breakpoints aren't being uninstalled by perf? I thought this would have
happened as a result of the scheduler migrating the current task away
from the dying CPU.

Will

[EDT][PATCH 1/1] hw_breakpoint.c :cpu hotplug handling

[Maninder Singh]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 2782 bytes --]

EP-2DAD0AFA905A4ACB804C4F82A001242F

Subject: [PATCH 1/1] hw_breakpoint.c :cpu hotplug handling

This patch adds support for CPU hotplug, It re-installl all installed watchpoints and breakpoints
back on H/W in case of cpu-hot plug.

Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
Reviewed-by: Amit Arora <amit.arora@samsung.com>
Reviewed-by: Ajeet Yadav <ajeet.y@samsung.com>
---
 arch/arm/kernel/hw_breakpoint.c |   56 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index dc7d0a9..172bfa8 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -388,6 +388,56 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 	return 0;
 }
 
+/*
+ * reInstall a perf counter breakpoint.
+ */
+void arch_reinstall_hw_breakpoint(int index, int type)
+{
+	struct arch_hw_breakpoint *info;
+	struct perf_event **slots;
+	int ctrl_base, val_base;
+	u32 addr, ctrl;
+	struct perf_event *bp;
+
+	if (type == ARM_ENTRY_BREAKPOINT) {
+		/* Breakpoint */
+		ctrl_base = ARM_BASE_BCR;
+		val_base = ARM_BASE_BVR;
+		slots = (struct perf_event **)__get_cpu_var(bp_on_reg);
+	} else {
+		/* Watchpoint */
+		ctrl_base = ARM_BASE_WCR;
+		val_base = ARM_BASE_WVR;
+		slots = (struct perf_event **)__get_cpu_var(wp_on_reg);
+	}
+
+	bp = slots[index];
+	if (!bp)
+		return;
+
+	info = counter_arch_bp(bp);
+	addr = info->address;
+	ctrl = encode_ctrl_reg(info->ctrl) | 0x1;
+
+
+	/* Override the breakpoint data with the step data. */
+	if (info->step_ctrl.enabled) {
+		addr = info->trigger & ~0x3;
+		ctrl = encode_ctrl_reg(info->step_ctrl);
+		if (info->ctrl.type != ARM_BREAKPOINT_EXECUTE) {
+			index = 0;
+			ctrl_base = ARM_BASE_BCR + core_num_brps;
+			val_base = ARM_BASE_BVR + core_num_brps;
+		}
+	}
+
+	/* Setup the address register. */
+	write_wb_reg(val_base + index, addr);
+
+	/* Setup the control register. */
+	write_wb_reg(ctrl_base + index, ctrl);
+}
+
 void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 {
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
@@ -1019,6 +1069,12 @@ clear_vcr:
 out_mdbgen:
 	if (enable_monitor_mode())
 		cpumask_or(&debug_err_mask, &debug_err_mask, cpumask_of(cpu));
+
+	for (i = 0; i < core_num_brps; ++i)
+		arch_reinstall_hw_breakpoint(i, ARM_ENTRY_BREAKPOINT);
+	for (i = 0; i < core_num_wrps; ++i)
+		arch_reinstall_hw_breakpoint(i, ARM_ENTRY_SYNC_WATCHPOINT);
+
 }
 
 static int dbg_reset_notify(struct notifier_block *self,
-- 
1.7.1


Thanks 
Maninder Singhÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [EDT][PATCH 1/1] hw_breakpoint.c :cpu hotplug handling

[Baruch Siach]

Hi Maninder Singh,

On Wed, May 13, 2015 at 05:24:07AM +0000, Maninder Singh wrote:
> EP-2DAD0AFA905A4ACB804C4F82A001242F
> 
> Subject: [PATCH 1/1] hw_breakpoint.c :cpu hotplug handling
> 
> This patch adds support for CPU hotplug, It re-installl all installed watchpoints and breakpoints
> back on H/W in case of cpu-hot plug.
> 
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> Reviewed-by: Amit Arora <amit.arora@samsung.com>
> Reviewed-by: Ajeet Yadav <ajeet.y@samsung.com>
> ---
>  arch/arm/kernel/hw_breakpoint.c |   56 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 56 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> index dc7d0a9..172bfa8 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -388,6 +388,56 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
>  	return 0;
>  }
>  
> +/*
> + * reInstall a perf counter breakpoint.
> + */
> +void arch_reinstall_hw_breakpoint(int index, int type)

Static?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [EDT][PATCH 1/1] hw_breakpoint.c :cpu hotplug handling

[Will Deacon]

On Wed, May 13, 2015 at 06:24:06AM +0100, Maninder Singh wrote:
> EP-2DAD0AFA905A4ACB804C4F82A001242F
> 
> Subject: [PATCH 1/1] hw_breakpoint.c :cpu hotplug handling
> 
> This patch adds support for CPU hotplug, It re-installl all installed watchpoints and breakpoints
> back on H/W in case of cpu-hot plug.

Not sure why this is needed -- the scheduler should reinstall the
breakpoints when the debugged task gets scheduled in via
arch_install_hw_breakpoint.

Will