[PATCH 0/4] perf: Support AMD range breakpoints

[PATCH 0/4] perf: Support AMD range breakpoints

[Frederic Weisbecker]

Hi,

I've made it wait too much so I'm posting that for review now. This
implements range breakpoints on some AMD machine by extending the length
field.

Any comment?

Thanks.

Jacob Shin (4):
  perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
  perf tools: allow user to specify hardware breakpoint bp_len
  perf tools: add hardware breakpoint bp_len test cases
  perf/x86: Remove get_hbp_len and replace with bp_len

 arch/x86/include/asm/cpufeature.h        |  2 ++
 arch/x86/include/asm/debugreg.h          |  5 +++
 arch/x86/include/asm/hw_breakpoint.h     |  1 +
 arch/x86/include/uapi/asm/msr-index.h    |  4 +++
 arch/x86/kernel/cpu/amd.c                | 19 +++++++++++
 arch/x86/kernel/hw_breakpoint.c          | 45 ++++++++++---------------
 tools/perf/Documentation/perf-record.txt |  7 ++--
 tools/perf/tests/parse-events.c          | 58 ++++++++++++++++++++++++++++++++
 tools/perf/util/parse-events.c           | 21 ++++++------
 tools/perf/util/parse-events.h           |  2 +-
 tools/perf/util/parse-events.l           |  1 +
 tools/perf/util/parse-events.y           | 26 ++++++++++++--
 12 files changed, 148 insertions(+), 43 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8

[Frederic Weisbecker]

From: Jacob Shin <jacob.w.shin@gmail.com>

Implement hardware breakpoint address mask for AMD Family 16h and
above processors. CPUID feature bit indicates hardware support for
DRn_ADDR_MASK MSRs. These masks further qualify DRn/DR7 hardware
breakpoint addresses to allow matching of larger addresses ranges.

Valuable advice and pseudo code from Oleg Nesterov <oleg@redhat.com>

Signed-off-by: Jacob Shin <jacob.w.shin@gmail.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: xiakaixu <xiakaixu@huawei.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 arch/x86/include/asm/cpufeature.h     |  2 ++
 arch/x86/include/asm/debugreg.h       |  5 +++++
 arch/x86/include/asm/hw_breakpoint.h  |  1 +
 arch/x86/include/uapi/asm/msr-index.h |  4 ++++
 arch/x86/kernel/cpu/amd.c             | 19 +++++++++++++++++++
 arch/x86/kernel/hw_breakpoint.c       | 20 ++++++++++++++++----
 6 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index e265ff9..6bd564d 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -170,6 +170,7 @@
 #define X86_FEATURE_TOPOEXT	(6*32+22) /* topology extensions CPUID leafs */
 #define X86_FEATURE_PERFCTR_CORE (6*32+23) /* core performance counter extensions */
 #define X86_FEATURE_PERFCTR_NB  (6*32+24) /* NB performance counter extensions */
+#define X86_FEATURE_BPEXT	(6*32+26) /* data breakpoint extension */
 #define X86_FEATURE_PERFCTR_L2	(6*32+28) /* L2 performance counter extensions */
 
 /*
@@ -338,6 +339,7 @@ extern const char * const x86_power_flags[32];
 #define cpu_has_cx16		boot_cpu_has(X86_FEATURE_CX16)
 #define cpu_has_eager_fpu	boot_cpu_has(X86_FEATURE_EAGER_FPU)
 #define cpu_has_topoext		boot_cpu_has(X86_FEATURE_TOPOEXT)
+#define cpu_has_bpext		boot_cpu_has(X86_FEATURE_BPEXT)
 
 #ifdef CONFIG_X86_64
 
diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
index 4b528a9..145b009 100644
--- a/arch/x86/include/asm/debugreg.h
+++ b/arch/x86/include/asm/debugreg.h
@@ -114,5 +114,10 @@ static inline void debug_stack_usage_inc(void) { }
 static inline void debug_stack_usage_dec(void) { }
 #endif /* X86_64 */
 
+#ifdef CONFIG_CPU_SUP_AMD
+extern void set_dr_addr_mask(unsigned long mask, int dr);
+#else
+static inline void set_dr_addr_mask(unsigned long mask, int dr) { }
+#endif
 
 #endif /* _ASM_X86_DEBUGREG_H */
diff --git a/arch/x86/include/asm/hw_breakpoint.h b/arch/x86/include/asm/hw_breakpoint.h
index ef1c4d2..6c98be8 100644
--- a/arch/x86/include/asm/hw_breakpoint.h
+++ b/arch/x86/include/asm/hw_breakpoint.h
@@ -12,6 +12,7 @@
  */
 struct arch_hw_breakpoint {
 	unsigned long	address;
+	unsigned long	mask;
 	u8		len;
 	u8		type;
 };
diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
index c827ace..d6b767c 100644
--- a/arch/x86/include/uapi/asm/msr-index.h
+++ b/arch/x86/include/uapi/asm/msr-index.h
@@ -208,6 +208,10 @@
 /* Fam 16h MSRs */
 #define MSR_F16H_L2I_PERF_CTL		0xc0010230
 #define MSR_F16H_L2I_PERF_CTR		0xc0010231
+#define MSR_F16H_DR1_ADDR_MASK		0xc0011019
+#define MSR_F16H_DR2_ADDR_MASK		0xc001101a
+#define MSR_F16H_DR3_ADDR_MASK		0xc001101b
+#define MSR_F16H_DR0_ADDR_MASK		0xc0011027
 
 /* Fam 15h MSRs */
 #define MSR_F15H_PERF_CTL		0xc0010200
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index ce8b8ff..2d1fe7a 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -887,3 +887,22 @@ static bool cpu_has_amd_erratum(struct cpuinfo_x86 *cpu, const int *erratum)
 
 	return false;
 }
+
+void set_dr_addr_mask(unsigned long mask, int dr)
+{
+	if (!cpu_has_bpext)
+		return;
+
+	switch (dr) {
+	case 0:
+		wrmsr(MSR_F16H_DR0_ADDR_MASK, mask, 0);
+		break;
+	case 1:
+	case 2:
+	case 3:
+		wrmsr(MSR_F16H_DR1_ADDR_MASK - 1 + dr, mask, 0);
+		break;
+	default:
+		break;
+	}
+}
diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index a67b47c..fa24eb5 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -127,6 +127,8 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 	*dr7 |= encode_dr7(i, info->len, info->type);
 
 	set_debugreg(*dr7, 7);
+	if (info->mask)
+		set_dr_addr_mask(info->mask, i);
 
 	return 0;
 }
@@ -162,6 +164,8 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 	*dr7 &= ~__encode_dr7(i, info->len, info->type);
 
 	set_debugreg(*dr7, 7);
+	if (info->mask)
+		set_dr_addr_mask(0, i);
 }
 
 static int get_hbp_len(u8 hbp_len)
@@ -278,6 +282,8 @@ static int arch_build_bp_info(struct perf_event *bp)
 	}
 
 	/* Len */
+	info->mask = 0;
+
 	switch (bp->attr.bp_len) {
 	case HW_BREAKPOINT_LEN_1:
 		info->len = X86_BREAKPOINT_LEN_1;
@@ -294,11 +300,17 @@ static int arch_build_bp_info(struct perf_event *bp)
 		break;
 #endif
 	default:
-		return -EINVAL;
+		if (!is_power_of_2(bp->attr.bp_len))
+			return -EINVAL;
+		if (!cpu_has_bpext)
+			return -EOPNOTSUPP;
+		info->mask = bp->attr.bp_len - 1;
+		info->len = X86_BREAKPOINT_LEN_1;
 	}
 
 	return 0;
 }
+
 /*
  * Validate the arch-specific HW Breakpoint register settings
  */
@@ -313,11 +325,11 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 	if (ret)
 		return ret;
 
-	ret = -EINVAL;
-
 	switch (info->len) {
 	case X86_BREAKPOINT_LEN_1:
 		align = 0;
+		if (info->mask)
+			align = info->mask;
 		break;
 	case X86_BREAKPOINT_LEN_2:
 		align = 1;
@@ -331,7 +343,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 		break;
 #endif
 	default:
-		return ret;
+		WARN_ON_ONCE(1);
 	}
 
 	/*
-- 
1.8.3.1

Re: [PATCH 1/4] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8

[Frederic Weisbecker]

Hi Jacob, Suravee,

On Thu, May 29, 2014 at 05:26:50PM +0200, Frederic Weisbecker wrote:
> From: Jacob Shin <jacob.w.shin@gmail.com>
> 
> Implement hardware breakpoint address mask for AMD Family 16h and
> above processors. CPUID feature bit indicates hardware support for
> DRn_ADDR_MASK MSRs. These masks further qualify DRn/DR7 hardware
> breakpoint addresses to allow matching of larger addresses ranges.
> 
> Valuable advice and pseudo code from Oleg Nesterov <oleg@redhat.com>
> 
> Signed-off-by: Jacob Shin <jacob.w.shin@gmail.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: xiakaixu <xiakaixu@huawei.com>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  arch/x86/include/asm/cpufeature.h     |  2 ++
>  arch/x86/include/asm/debugreg.h       |  5 +++++
>  arch/x86/include/asm/hw_breakpoint.h  |  1 +
>  arch/x86/include/uapi/asm/msr-index.h |  4 ++++
>  arch/x86/kernel/cpu/amd.c             | 19 +++++++++++++++++++
>  arch/x86/kernel/hw_breakpoint.c       | 20 ++++++++++++++++----
>  6 files changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> index e265ff9..6bd564d 100644
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -170,6 +170,7 @@
>  #define X86_FEATURE_TOPOEXT	(6*32+22) /* topology extensions CPUID leafs */
>  #define X86_FEATURE_PERFCTR_CORE (6*32+23) /* core performance counter extensions */
>  #define X86_FEATURE_PERFCTR_NB  (6*32+24) /* NB performance counter extensions */
> +#define X86_FEATURE_BPEXT	(6*32+26) /* data breakpoint extension */
>  #define X86_FEATURE_PERFCTR_L2	(6*32+28) /* L2 performance counter extensions */
>  
>  /*
> @@ -338,6 +339,7 @@ extern const char * const x86_power_flags[32];
>  #define cpu_has_cx16		boot_cpu_has(X86_FEATURE_CX16)
>  #define cpu_has_eager_fpu	boot_cpu_has(X86_FEATURE_EAGER_FPU)
>  #define cpu_has_topoext		boot_cpu_has(X86_FEATURE_TOPOEXT)
> +#define cpu_has_bpext		boot_cpu_has(X86_FEATURE_BPEXT)
>  
>  #ifdef CONFIG_X86_64
>  
> diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
> index 4b528a9..145b009 100644
> --- a/arch/x86/include/asm/debugreg.h
> +++ b/arch/x86/include/asm/debugreg.h
> @@ -114,5 +114,10 @@ static inline void debug_stack_usage_inc(void) { }
>  static inline void debug_stack_usage_dec(void) { }
>  #endif /* X86_64 */
>  
> +#ifdef CONFIG_CPU_SUP_AMD
> +extern void set_dr_addr_mask(unsigned long mask, int dr);
> +#else
> +static inline void set_dr_addr_mask(unsigned long mask, int dr) { }
> +#endif

I see this symbol in the code but it's not defined anywhere in a Kconfig file.
Maybe you (or I) forgot to include a file in your patches?

Thanks.

Re: [PATCH 1/4] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8

[Suravee Suthikulpanit]

On 05/30/2014 08:33 AM, Frederic Weisbecker wrote:
>> diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
>> >index 4b528a9..145b009 100644
>> >--- a/arch/x86/include/asm/debugreg.h
>> >+++ b/arch/x86/include/asm/debugreg.h
>> >@@ -114,5 +114,10 @@ static inline void debug_stack_usage_inc(void) { }
>> >  static inline void debug_stack_usage_dec(void) { }
>> >  #endif /* X86_64 */
>> >
>> >+#ifdef CONFIG_CPU_SUP_AMD
>> >+extern void set_dr_addr_mask(unsigned long mask, int dr);
>> >+#else
>> >+static inline void set_dr_addr_mask(unsigned long mask, int dr) { }
>> >+#endif
> I see this symbol in the code but it's not defined anywhere in a Kconfig file.
> Maybe you (or I) forgot to include a file in your patches?
>
> Thanks.

If you are referring to the CONFIG_CPU_SUP_AMD, this is in 
arch/x86/Kconfig.cpu

Suravee

Re: [PATCH 1/4] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8

[Frederic Weisbecker]

On Fri, May 30, 2014 at 08:55:55PM -0500, Suravee Suthikulpanit wrote:
> 
> 
> On 05/30/2014 08:33 AM, Frederic Weisbecker wrote:
> >>diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
> >>>index 4b528a9..145b009 100644
> >>>--- a/arch/x86/include/asm/debugreg.h
> >>>+++ b/arch/x86/include/asm/debugreg.h
> >>>@@ -114,5 +114,10 @@ static inline void debug_stack_usage_inc(void) { }
> >>>  static inline void debug_stack_usage_dec(void) { }
> >>>  #endif /* X86_64 */
> >>>
> >>>+#ifdef CONFIG_CPU_SUP_AMD
> >>>+extern void set_dr_addr_mask(unsigned long mask, int dr);
> >>>+#else
> >>>+static inline void set_dr_addr_mask(unsigned long mask, int dr) { }
> >>>+#endif
> >I see this symbol in the code but it's not defined anywhere in a Kconfig file.
> >Maybe you (or I) forgot to include a file in your patches?
> >
> >Thanks.
> 
> If you are referring to the CONFIG_CPU_SUP_AMD, this is in
> arch/x86/Kconfig.cpu

Ah I thought it was created in the patchset. Nevermind.

[PATCH 2/4] perf tools: allow user to specify hardware breakpoint bp_len

[Frederic Weisbecker]

From: Jacob Shin <jacob.w.shin@gmail.com>

Currently bp_len is given a default value of 4. Allow user to override it:

  $ perf stat -e mem:0x1000/8
                            ^
                            bp_len

If no value is given, it will default to 4 as it did before.

Signed-off-by: Jacob Shin <jacob.w.shin@gmail.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: xiakaixu <xiakaixu@huawei.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 tools/perf/Documentation/perf-record.txt |  7 +++++--
 tools/perf/util/parse-events.c           | 21 +++++++++++----------
 tools/perf/util/parse-events.h           |  2 +-
 tools/perf/util/parse-events.l           |  1 +
 tools/perf/util/parse-events.y           | 26 ++++++++++++++++++++++++--
 5 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index c71b0f3..5cac20d 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -33,12 +33,15 @@ OPTIONS
         - a raw PMU event (eventsel+umask) in the form of rNNN where NNN is a
 	  hexadecimal event descriptor.
 
-        - a hardware breakpoint event in the form of '\mem:addr[:access]'
+        - a hardware breakpoint event in the form of '\mem:addr[/len][:access]'
           where addr is the address in memory you want to break in.
           Access is the memory access type (read, write, execute) it can
-          be passed as follows: '\mem:addr[:[r][w][x]]'.
+          be passed as follows: '\mem:addr[:[r][w][x]]'. len is the range,
+          number of bytes from specified addr, which the breakpoint will cover.
           If you want to profile read-write accesses in 0x1000, just set
           'mem:0x1000:rw'.
+          If you want to profile write accesses in [0x1000~1008), just set
+          'mem:0x1000/8:w'.
 
 --filter=<filter>::
         Event filter.
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 1e15df1..8dd9287 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -516,7 +516,7 @@ do {					\
 }
 
 int parse_events_add_breakpoint(struct list_head *list, int *idx,
-				void *ptr, char *type)
+				void *ptr, char *type, u64 len)
 {
 	struct perf_event_attr attr;
 
@@ -526,14 +526,15 @@ int parse_events_add_breakpoint(struct list_head *list, int *idx,
 	if (parse_breakpoint_type(type, &attr))
 		return -EINVAL;
 
-	/*
-	 * We should find a nice way to override the access length
-	 * Provide some defaults for now
-	 */
-	if (attr.bp_type == HW_BREAKPOINT_X)
-		attr.bp_len = sizeof(long);
-	else
-		attr.bp_len = HW_BREAKPOINT_LEN_4;
+	/* Provide some defaults if len is not specified */
+	if (!len) {
+		if (attr.bp_type == HW_BREAKPOINT_X)
+			len = sizeof(long);
+		else
+			len = HW_BREAKPOINT_LEN_4;
+	}
+
+	attr.bp_len = len;
 
 	attr.type = PERF_TYPE_BREAKPOINT;
 	attr.sample_period = 1;
@@ -1262,7 +1263,7 @@ void print_events(const char *event_glob, bool name_only)
 		printf("\n");
 
 		printf("  %-50s [%s]\n",
-		       "mem:<addr>[:access]",
+		       "mem:<addr>[/len][:access]",
 			event_type_descriptors[PERF_TYPE_BREAKPOINT]);
 		printf("\n");
 	}
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index df094b4..00a5eb4 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -92,7 +92,7 @@ int parse_events_add_numeric(struct list_head *list, int *idx,
 int parse_events_add_cache(struct list_head *list, int *idx,
 			   char *type, char *op_result1, char *op_result2);
 int parse_events_add_breakpoint(struct list_head *list, int *idx,
-				void *ptr, char *type);
+				void *ptr, char *type, u64 len);
 int parse_events_add_pmu(struct list_head *list, int *idx,
 			 char *pmu , struct list_head *head_config);
 void parse_events__set_leader(char *name, struct list_head *list);
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 3432995..199e18a 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -141,6 +141,7 @@ branch_type		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE
 <mem>{
 {modifier_bp}		{ return str(yyscanner, PE_MODIFIER_BP); }
 :			{ return ':'; }
+"/"			{ return '/'; }
 {num_dec}		{ return value(yyscanner, 10); }
 {num_hex}		{ return value(yyscanner, 16); }
 	/*
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 0bc87ba..456f4de 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -276,6 +276,28 @@ PE_NAME_CACHE_TYPE
 }
 
 event_legacy_mem:
+PE_PREFIX_MEM PE_VALUE '/' PE_VALUE ':' PE_MODIFIER_BP sep_dc
+{
+	struct parse_events_evlist *data = _data;
+	struct list_head *list;
+
+	ALLOC_LIST(list);
+	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
+					     (void *) $2, $6, $4));
+	$$ = list;
+}
+|
+PE_PREFIX_MEM PE_VALUE '/' PE_VALUE sep_dc
+{
+	struct parse_events_evlist *data = _data;
+	struct list_head *list;
+
+	ALLOC_LIST(list);
+	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
+					     (void *) $2, NULL, $4));
+	$$ = list;
+}
+|
 PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
 {
 	struct parse_events_evlist *data = _data;
@@ -283,7 +305,7 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
 
 	ALLOC_LIST(list);
 	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
-					     (void *) $2, $4));
+					     (void *) $2, $4, 0));
 	$$ = list;
 }
 |
@@ -294,7 +316,7 @@ PE_PREFIX_MEM PE_VALUE sep_dc
 
 	ALLOC_LIST(list);
 	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
-					     (void *) $2, NULL));
+					     (void *) $2, NULL, 0));
 	$$ = list;
 }
 
-- 
1.8.3.1

Re: [PATCH 2/4] perf tools: allow user to specify hardware breakpoint bp_len

[Jiri Olsa]

On Thu, May 29, 2014 at 05:26:51PM +0200, Frederic Weisbecker wrote:
> From: Jacob Shin <jacob.w.shin@gmail.com>
> 
> Currently bp_len is given a default value of 4. Allow user to override it:
> 
>   $ perf stat -e mem:0x1000/8
>                             ^
>                             bp_len
> 
> If no value is given, it will default to 4 as it did before.

Namhyung,
both perf tols patches from this patchset mess up with hists
tests..  I havent found any connection yet.. any idea? ;-)

[root@krava perf]# ./perf test 
...
19: Test software clock events have valid period values    : Ok
20: Test converting perf time to TSC                       : Ok
21: Test object code reading                               : Ok
22: Test sample parsing                                    : Ok
23: Test using a dummy software event to keep tracking     : Ok
24: Test parsing with no sample_id_all bit set             : Ok
25: Test dwarf unwind                                      : Ok
26: Test filtering hist entries                            : FAILED!
27: Test mmap thread lookup                                : Ok
28: Test thread mg sharing                                 : Ok
29: Test output sorting of hist entries                    : FAILED!
30: Test cumulation of child hist entries                  : FAILED!

thanks,
jirka

> 
> Signed-off-by: Jacob Shin <jacob.w.shin@gmail.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: xiakaixu <xiakaixu@huawei.com>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  tools/perf/Documentation/perf-record.txt |  7 +++++--
>  tools/perf/util/parse-events.c           | 21 +++++++++++----------
>  tools/perf/util/parse-events.h           |  2 +-
>  tools/perf/util/parse-events.l           |  1 +
>  tools/perf/util/parse-events.y           | 26 ++++++++++++++++++++++++--
>  5 files changed, 42 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> index c71b0f3..5cac20d 100644
> --- a/tools/perf/Documentation/perf-record.txt
> +++ b/tools/perf/Documentation/perf-record.txt
> @@ -33,12 +33,15 @@ OPTIONS
>          - a raw PMU event (eventsel+umask) in the form of rNNN where NNN is a
>  	  hexadecimal event descriptor.
>  
> -        - a hardware breakpoint event in the form of '\mem:addr[:access]'
> +        - a hardware breakpoint event in the form of '\mem:addr[/len][:access]'
>            where addr is the address in memory you want to break in.
>            Access is the memory access type (read, write, execute) it can
> -          be passed as follows: '\mem:addr[:[r][w][x]]'.
> +          be passed as follows: '\mem:addr[:[r][w][x]]'. len is the range,
> +          number of bytes from specified addr, which the breakpoint will cover.
>            If you want to profile read-write accesses in 0x1000, just set
>            'mem:0x1000:rw'.
> +          If you want to profile write accesses in [0x1000~1008), just set
> +          'mem:0x1000/8:w'.
>  
>  --filter=<filter>::
>          Event filter.
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 1e15df1..8dd9287 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -516,7 +516,7 @@ do {					\
>  }
>  
>  int parse_events_add_breakpoint(struct list_head *list, int *idx,
> -				void *ptr, char *type)
> +				void *ptr, char *type, u64 len)
>  {
>  	struct perf_event_attr attr;
>  
> @@ -526,14 +526,15 @@ int parse_events_add_breakpoint(struct list_head *list, int *idx,
>  	if (parse_breakpoint_type(type, &attr))
>  		return -EINVAL;
>  
> -	/*
> -	 * We should find a nice way to override the access length
> -	 * Provide some defaults for now
> -	 */
> -	if (attr.bp_type == HW_BREAKPOINT_X)
> -		attr.bp_len = sizeof(long);
> -	else
> -		attr.bp_len = HW_BREAKPOINT_LEN_4;
> +	/* Provide some defaults if len is not specified */
> +	if (!len) {
> +		if (attr.bp_type == HW_BREAKPOINT_X)
> +			len = sizeof(long);
> +		else
> +			len = HW_BREAKPOINT_LEN_4;
> +	}
> +
> +	attr.bp_len = len;
>  
>  	attr.type = PERF_TYPE_BREAKPOINT;
>  	attr.sample_period = 1;
> @@ -1262,7 +1263,7 @@ void print_events(const char *event_glob, bool name_only)
>  		printf("\n");
>  
>  		printf("  %-50s [%s]\n",
> -		       "mem:<addr>[:access]",
> +		       "mem:<addr>[/len][:access]",
>  			event_type_descriptors[PERF_TYPE_BREAKPOINT]);
>  		printf("\n");
>  	}
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index df094b4..00a5eb4 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -92,7 +92,7 @@ int parse_events_add_numeric(struct list_head *list, int *idx,
>  int parse_events_add_cache(struct list_head *list, int *idx,
>  			   char *type, char *op_result1, char *op_result2);
>  int parse_events_add_breakpoint(struct list_head *list, int *idx,
> -				void *ptr, char *type);
> +				void *ptr, char *type, u64 len);
>  int parse_events_add_pmu(struct list_head *list, int *idx,
>  			 char *pmu , struct list_head *head_config);
>  void parse_events__set_leader(char *name, struct list_head *list);
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 3432995..199e18a 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -141,6 +141,7 @@ branch_type		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE
>  <mem>{
>  {modifier_bp}		{ return str(yyscanner, PE_MODIFIER_BP); }
>  :			{ return ':'; }
> +"/"			{ return '/'; }
>  {num_dec}		{ return value(yyscanner, 10); }
>  {num_hex}		{ return value(yyscanner, 16); }
>  	/*
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 0bc87ba..456f4de 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -276,6 +276,28 @@ PE_NAME_CACHE_TYPE
>  }
>  
>  event_legacy_mem:
> +PE_PREFIX_MEM PE_VALUE '/' PE_VALUE ':' PE_MODIFIER_BP sep_dc
> +{
> +	struct parse_events_evlist *data = _data;
> +	struct list_head *list;
> +
> +	ALLOC_LIST(list);
> +	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
> +					     (void *) $2, $6, $4));
> +	$$ = list;
> +}
> +|
> +PE_PREFIX_MEM PE_VALUE '/' PE_VALUE sep_dc
> +{
> +	struct parse_events_evlist *data = _data;
> +	struct list_head *list;
> +
> +	ALLOC_LIST(list);
> +	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
> +					     (void *) $2, NULL, $4));
> +	$$ = list;
> +}
> +|
>  PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
>  {
>  	struct parse_events_evlist *data = _data;
> @@ -283,7 +305,7 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
>  
>  	ALLOC_LIST(list);
>  	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
> -					     (void *) $2, $4));
> +					     (void *) $2, $4, 0));
>  	$$ = list;
>  }
>  |
> @@ -294,7 +316,7 @@ PE_PREFIX_MEM PE_VALUE sep_dc
>  
>  	ALLOC_LIST(list);
>  	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
> -					     (void *) $2, NULL));
> +					     (void *) $2, NULL, 0));
>  	$$ = list;
>  }
>  
> -- 
> 1.8.3.1
> 

Re: [PATCH 2/4] perf tools: allow user to specify hardware breakpoint bp_len

[Namhyung Kim]

Hi Jiri,

On Fri, 30 May 2014 15:39:06 +0200, Jiri Olsa wrote:
> On Thu, May 29, 2014 at 05:26:51PM +0200, Frederic Weisbecker wrote:
>> From: Jacob Shin <jacob.w.shin@gmail.com>
>> 
>> Currently bp_len is given a default value of 4. Allow user to override it:
>> 
>>   $ perf stat -e mem:0x1000/8
>>                             ^
>>                             bp_len
>> 
>> If no value is given, it will default to 4 as it did before.
>
> Namhyung,
> both perf tols patches from this patchset mess up with hists
> tests..  I havent found any connection yet.. any idea? ;-)

So you already found the problem in the hpp->elide change and that's the
reason of the failure, right? :)

Thanks,
Namhyung

Re: [PATCH 2/4] perf tools: allow user to specify hardware breakpoint bp_len

[Jiri Olsa]

On Tue, Jun 03, 2014 at 10:36:22AM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Fri, 30 May 2014 15:39:06 +0200, Jiri Olsa wrote:
> > On Thu, May 29, 2014 at 05:26:51PM +0200, Frederic Weisbecker wrote:
> >> From: Jacob Shin <jacob.w.shin@gmail.com>
> >> 
> >> Currently bp_len is given a default value of 4. Allow user to override it:
> >> 
> >>   $ perf stat -e mem:0x1000/8
> >>                             ^
> >>                             bp_len
> >> 
> >> If no value is given, it will default to 4 as it did before.
> >
> > Namhyung,
> > both perf tols patches from this patchset mess up with hists
> > tests..  I havent found any connection yet.. any idea? ;-)
> 
> So you already found the problem in the hpp->elide change and that's the
> reason of the failure, right? :)

I haven't got a chance to test it yet.. but it looks like
thats the case.. I'll retest soon ;-)

jirka

Re: [PATCH 2/4] perf tools: allow user to specify hardware breakpoint bp_len

[Suravee Suthikulanit]

On 6/3/2014 6:55 AM, Jiri Olsa wrote:
> On Tue, Jun 03, 2014 at 10:36:22AM +0900, Namhyung Kim wrote:
>> Hi Jiri,
>>
>> On Fri, 30 May 2014 15:39:06 +0200, Jiri Olsa wrote:
>>> On Thu, May 29, 2014 at 05:26:51PM +0200, Frederic Weisbecker wrote:
>>>> From: Jacob Shin <jacob.w.shin@gmail.com>
>>>>
>>>> Currently bp_len is given a default value of 4. Allow user to override it:
>>>>
>>>>    $ perf stat -e mem:0x1000/8
>>>>                              ^
>>>>                              bp_len
>>>>
>>>> If no value is given, it will default to 4 as it did before.
>>>
>>> Namhyung,
>>> both perf tols patches from this patchset mess up with hists
>>> tests..  I havent found any connection yet.. any idea? ;-)
>>
>> So you already found the problem in the hpp->elide change and that's the
>> reason of the failure, right? :)
>
> I haven't got a chance to test it yet.. but it looks like
> thats the case.. I'll retest soon ;-)
>
> jirka
>
Anything I can do to help here?

Thanks,

Suravee

Re: [PATCH 2/4] perf tools: allow user to specify hardware breakpoint bp_len

[Suravee Suthikulanit]

Frederic,

3.17 windows is now open. Do you think you could get this one in?

Thanks

Suravee

On 6/6/2014 11:30 AM, Suravee Suthikulanit wrote:
> This message has been archived. View the original item
> <http://ausev2.amd.com/EnterpriseVault/ViewMessage.asp?VaultId=1C7248B8A7CDE234D884C352F0CB2021B1110000amdvault.amd.com&SavesetId=201407115122720~201406061630160000~Z~F134EBF86540766B73DA6ADB45060FE1>
> On 6/3/2014 6:55 AM, Jiri Olsa wrote:
>  > On Tue, Jun 03, 2014 at 10:36:22AM +0900, Namhyung Kim wrote:
>  >> Hi Jiri,
>  >>
>  >> On Fri, 30 May 2014 15:39:06 +0200, Jiri Olsa wrote:
>  >>> On Thu, May 29, 2014 at 05:26:51PM +0200, Frederic Weisbecker wrote:
>  >>>> From: Jacob Shin <jacob.w.shin@gmail.com>
>  >>>>
>  >>>> Currently bp_len is given a default value of 4. Allow user to
> override it:
>  >>>>
>  >>>>    $ perf stat -e mem:0x1000/8
>  >>>>                              ^
>  >>>>                              bp_len
>  >>>>
>  >>>> If no value is given, it will default to 4 as it did before.
>  >>>
>  >>> Namhyung,
>  >>> both perf tols patches from this patchset mess up with hists
>  >>> tests..  I havent found any connection yet.. any idea? ;-)
>  >>
>  >> So you already found the problem in the hpp->elide change and that's the
>  >> reason of the failure, right? :)
>  >
>  > I haven't got a chance to test it yet.. but it looks like
>  > thats the case.. I'll retest soon ;-)
>  >
>  > jirka
>  >
> Anything I can do to help here?
>
> Thanks,
>
> Suravee
>

[PATCH 3/4] perf tools: add hardware breakpoint bp_len test cases

[Frederic Weisbecker]

From: Jacob Shin <jacob.w.shin@gmail.com>

Signed-off-by: Jacob Shin <jacob.w.shin@gmail.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: xiakaixu <xiakaixu@huawei.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 tools/perf/tests/parse-events.c | 58 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index deba669..41ef91d 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -1114,6 +1114,49 @@ static int test__pinned_group(struct perf_evlist *evlist)
 	return 0;
 }
 
+static int test__checkevent_breakpoint_len(struct perf_evlist *evlist)
+{
+	struct perf_evsel *evsel = perf_evlist__first(evlist);
+
+	TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->nr_entries);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_BREAKPOINT == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config", 0 == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong bp_type", (HW_BREAKPOINT_R | HW_BREAKPOINT_W) ==
+					 evsel->attr.bp_type);
+	TEST_ASSERT_VAL("wrong bp_len", HW_BREAKPOINT_LEN_1 ==
+					evsel->attr.bp_len);
+
+	return 0;
+}
+
+static int test__checkevent_breakpoint_len_w(struct perf_evlist *evlist)
+{
+	struct perf_evsel *evsel = perf_evlist__first(evlist);
+
+	TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->nr_entries);
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_BREAKPOINT == evsel->attr.type);
+	TEST_ASSERT_VAL("wrong config", 0 == evsel->attr.config);
+	TEST_ASSERT_VAL("wrong bp_type", HW_BREAKPOINT_W ==
+					 evsel->attr.bp_type);
+	TEST_ASSERT_VAL("wrong bp_len", HW_BREAKPOINT_LEN_2 ==
+					evsel->attr.bp_len);
+
+	return 0;
+}
+
+static int
+test__checkevent_breakpoint_len_rw_modifier(struct perf_evlist *evlist)
+{
+	struct perf_evsel *evsel = perf_evlist__first(evlist);
+
+	TEST_ASSERT_VAL("wrong exclude_user", !evsel->attr.exclude_user);
+	TEST_ASSERT_VAL("wrong exclude_kernel", evsel->attr.exclude_kernel);
+	TEST_ASSERT_VAL("wrong exclude_hv", evsel->attr.exclude_hv);
+	TEST_ASSERT_VAL("wrong precise_ip", !evsel->attr.precise_ip);
+
+	return test__checkevent_breakpoint_rw(evlist);
+}
+
 static int count_tracepoints(void)
 {
 	char events_path[PATH_MAX];
@@ -1389,6 +1432,21 @@ static struct evlist_test test__events[] = {
 		.check = test__pinned_group,
 		.id    = 41,
 	},
+	{
+		.name  = "mem:0/1",
+		.check = test__checkevent_breakpoint_len,
+		.id    = 42,
+	},
+	{
+		.name  = "mem:0/2:w",
+		.check = test__checkevent_breakpoint_len_w,
+		.id    = 43,
+	},
+	{
+		.name  = "mem:0/4:rw:u",
+		.check = test__checkevent_breakpoint_len_rw_modifier,
+		.id    = 44
+	},
 #if defined(__s390x__)
 	{
 		.name  = "kvm-s390:kvm_s390_create_vm",
-- 
1.8.3.1

Re: [PATCH 3/4] perf tools: add hardware breakpoint bp_len test cases

[Suravee Suthikulanit]

On 5/29/2014 10:26 AM, Frederic Weisbecker wrote:
> @@ -1389,6 +1432,21 @@ static struct evlist_test test__events[] = {
>   		.check = test__pinned_group,
>   		.id    = 41,
>   	},
> +	{
> +		.name  = "mem:0/1",
> +		.check = test__checkevent_breakpoint_len,
> +		.id    = 42,
> +	},
> +	{
> +		.name  = "mem:0/2:w",
> +		.check = test__checkevent_breakpoint_len_w,
> +		.id    = 43,
> +	},
> +	{
> +		.name  = "mem:0/4:rw:u",
> +		.check = test__checkevent_breakpoint_len_rw_modifier,
> +		.id    = 44
> +	},
>   #if defined(__s390x__)
>   	{
>   		.name  = "kvm-s390:kvm_s390_create_vm",
Frederic,

This hunk seems to fail to apply on the main tree.  Which tree are you 
working on?

Suravee

Re: [PATCH 3/4] perf tools: add hardware breakpoint bp_len test cases

[Frederic Weisbecker]

On Fri, May 30, 2014 at 09:49:25AM -0500, Suravee Suthikulanit wrote:
> On 5/29/2014 10:26 AM, Frederic Weisbecker wrote:
> >@@ -1389,6 +1432,21 @@ static struct evlist_test test__events[] = {
> >  		.check = test__pinned_group,
> >  		.id    = 41,
> >  	},
> >+	{
> >+		.name  = "mem:0/1",
> >+		.check = test__checkevent_breakpoint_len,
> >+		.id    = 42,
> >+	},
> >+	{
> >+		.name  = "mem:0/2:w",
> >+		.check = test__checkevent_breakpoint_len_w,
> >+		.id    = 43,
> >+	},
> >+	{
> >+		.name  = "mem:0/4:rw:u",
> >+		.check = test__checkevent_breakpoint_len_rw_modifier,
> >+		.id    = 44
> >+	},
> >  #if defined(__s390x__)
> >  	{
> >  		.name  = "kvm-s390:kvm_s390_create_vm",
> Frederic,
> 
> This hunk seems to fail to apply on the main tree.  Which tree are
> you working on?

I forgot to say it's rebased on top of:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
	perf/core
 
> Suravee
> 

[PATCH 4/4] perf/x86: Remove get_hbp_len and replace with bp_len

[Frederic Weisbecker]

From: Jacob Shin <jacob.w.shin@gmail.com>

Clean up the logic for determining the breakpoint length

Signed-off-by: Jacob Shin <jacob.w.shin@gmail.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: xiakaixu <xiakaixu@huawei.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 arch/x86/kernel/hw_breakpoint.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index fa24eb5..2a9000c 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -168,29 +168,6 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 		set_dr_addr_mask(0, i);
 }
 
-static int get_hbp_len(u8 hbp_len)
-{
-	unsigned int len_in_bytes = 0;
-
-	switch (hbp_len) {
-	case X86_BREAKPOINT_LEN_1:
-		len_in_bytes = 1;
-		break;
-	case X86_BREAKPOINT_LEN_2:
-		len_in_bytes = 2;
-		break;
-	case X86_BREAKPOINT_LEN_4:
-		len_in_bytes = 4;
-		break;
-#ifdef CONFIG_X86_64
-	case X86_BREAKPOINT_LEN_8:
-		len_in_bytes = 8;
-		break;
-#endif
-	}
-	return len_in_bytes;
-}
-
 /*
  * Check for virtual address in kernel space.
  */
@@ -201,7 +178,7 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 
 	va = info->address;
-	len = get_hbp_len(info->len);
+	len = bp->attr.bp_len;
 
 	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
 }
-- 
1.8.3.1

Re: [PATCH 0/4] perf: Support AMD range breakpoints

[Jiri Olsa]

On Thu, May 29, 2014 at 05:26:49PM +0200, Frederic Weisbecker wrote:
> Hi,
> 
> I've made it wait too much so I'm posting that for review now. This
> implements range breakpoints on some AMD machine by extending the length
> field.
> 
> Any comment?
hi,
perf tools patches look ok to me.. let me know
if I should queue them

thanks,
jirka

Re: [PATCH 0/4] perf: Support AMD range breakpoints

[Suravee Suthikulpanit]

I have also tested this patch set on the family16h system, and it seems 
to be working okay.  Let me know if I can help with anything else to get 
this patch in.

Suravee

On 05/30/2014 08:06 AM, Jiri Olsa wrote:
> On Thu, May 29, 2014 at 05:26:49PM +0200, Frederic Weisbecker wrote:
>> Hi,
>>
>> I've made it wait too much so I'm posting that for review now. This
>> implements range breakpoints on some AMD machine by extending the length
>> field.
>>
>> Any comment?
> hi,
> perf tools patches look ok to me.. let me know
> if I should queue them
>
> thanks,
> jirka
>