[PATCH V5 0/3] perf/x86/amd: AMD Family 16h Data Breakpoint Extensions

[PATCH V5 0/3] perf/x86/amd: AMD Family 16h Data Breakpoint Extensions

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Frederic, this is the rebase of the V4 patch onto the linux-3.12.0-rc3 (linux.git),
and retest.

The following patchset enables hardware breakpoint bp_len greater than
HW_BREAKPOINT_LEN_8 on AMD Family 16h and later.

  $ perf stat -e mem:0x1000/16:w a.out
                            ^^
                            bp_len

This will count writes to [0x1000 ~ 0x1010)

V5:
* Rebase onto 3.12.0-rc3.
* Modify the tools/perf/util/parse-events.y due to change in
  parse_events_add_breakpoint().

V4:
Even more per Oleg's suggestion:
* Further simplify info->len and info->mask setting switch statement

V3:
More per Oleg's suggestion:
* Use already existing bp_len instead of changing userland API and
  in kernel turn bp_len into proper AMD hardware breakpoint address
  mask.

V2:
Per Oleg's suggestions:
* Moved testing of bp_addr_mask to validate_hw_breakpoint()
* Changed perf tool syntax to mem:<addr>[/addr_mask][:access]


Jacob Shin (3):
  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

 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          | 47 +++++++++++----------------
 tools/perf/Documentation/perf-record.txt |  7 ++--
 tools/perf/tests/parse-events.c          | 55 ++++++++++++++++++++++++++++++++
 tools/perf/util/parse-events.c           | 17 ++++------
 tools/perf/util/parse-events.h           |  2 +-
 tools/perf/util/parse-events.l           |  1 +
 tools/perf/util/parse-events.y           | 26 +++++++++++++--
 12 files changed, 142 insertions(+), 44 deletions(-)

-- 
1.8.1.2

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

[suravee.suthikulpanit]

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>
---
 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       | 47 ++++++++++++++---------------------
 6 files changed, 49 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index d3f5c63..26609bb 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 */
 
 /*
@@ -332,6 +333,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 bb04650..1f04f6c 100644
--- a/arch/x86/include/uapi/asm/msr-index.h
+++ b/arch/x86/include/uapi/asm/msr-index.h
@@ -205,6 +205,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 903a264..fffc9cb 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -909,3 +909,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 f66ff16..3cb1951 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -128,6 +128,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;
 }
@@ -163,29 +165,8 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 	*dr7 &= ~__encode_dr7(i, info->len, info->type);
 
 	set_debugreg(*dr7, 7);
-}
-
-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;
+	if (info->mask)
+		set_dr_addr_mask(0, i);
 }
 
 /*
@@ -198,7 +179,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);
 }
@@ -279,7 +260,16 @@ static int arch_build_bp_info(struct perf_event *bp)
 	}
 
 	/* Len */
+	info->mask = 0;
+
 	switch (bp->attr.bp_len) {
+	default:
+		if (!is_power_of_2(bp->attr.bp_len))
+			return -EINVAL;
+		if (!cpu_has_bpext)
+			return -EOPNOTSUPP;
+		info->mask = bp->attr.bp_len - 1;
+		/* fall through */
 	case HW_BREAKPOINT_LEN_1:
 		info->len = X86_BREAKPOINT_LEN_1;
 		break;
@@ -294,12 +284,11 @@ static int arch_build_bp_info(struct perf_event *bp)
 		info->len = X86_BREAKPOINT_LEN_8;
 		break;
 #endif
-	default:
-		return -EINVAL;
 	}
 
 	return 0;
 }
+
 /*
  * Validate the arch-specific HW Breakpoint register settings
  */
@@ -314,11 +303,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;
@@ -332,7 +321,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 		break;
 #endif
 	default:
-		return ret;
+		BUG();
 	}
 
 	/*
-- 
1.8.1.2

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

[suravee.suthikulpanit]

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>
---
 tools/perf/Documentation/perf-record.txt |  7 +++++--
 tools/perf/util/parse-events.c           | 17 +++++++----------
 tools/perf/util/parse-events.h           |  2 +-
 tools/perf/util/parse-events.l           |  1 +
 tools/perf/util/parse-events.y           | 26 ++++++++++++++++++++++++--
 5 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index e297b74..e07491f 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 9812531..65d81ae 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -515,7 +515,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;
 
@@ -525,14 +525,11 @@ 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)
+		len = attr.bp_type == HW_BREAKPOINT_X ? sizeof(long) :
+							HW_BREAKPOINT_LEN_4;
+	attr.bp_len = len;
 
 	attr.type = PERF_TYPE_BREAKPOINT;
 	attr.sample_period = 1;
@@ -1238,7 +1235,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 f1cb4c4..54a20df 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -93,7 +93,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 91346b7..2fde75f 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -193,6 +193,7 @@ r{num_raw_hex}		{ return raw(yyscanner); }
 <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 4eb67ec..c9adf4d 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.1.2

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

[suravee.suthikulpanit]

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>
---
 tools/perf/tests/parse-events.c | 55 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 48114d1..9b44722 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -1115,6 +1115,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];
@@ -1347,6 +1390,18 @@ static struct evlist_test test__events[] = {
 		.name  = "{cycles,cache-misses,branch-misses}:D",
 		.check = test__pinned_group,
 	},
+	[42] = {
+		.name  = "mem:0/1",
+		.check = test__checkevent_breakpoint_len,
+	},
+	[43] = {
+		.name  = "mem:0/2:w",
+		.check = test__checkevent_breakpoint_len_w,
+	},
+	[44] = {
+		.name  = "mem:0/4:rw:u",
+		.check = test__checkevent_breakpoint_len_rw_modifier,
+	},
 };
 
 static struct evlist_test test__events_pmu[] = {
-- 
1.8.1.2

Re: [PATCH V5 0/3] perf/x86/amd: AMD Family 16h Data Breakpoint Extensions

[Oleg Nesterov]

On 10/02, suravee.suthikulpanit@amd.com wrote:
>
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> Frederic, this is the rebase of the V4 patch onto the linux-3.12.0-rc3 (linux.git),
> and retest.

But the code is the same? If yes,

Reviewed-by: Oleg Nesterov <oleg@redhat.com>


To remind, a range on executable breakpoint is problematic, but I do
not blame this series. We need to fix the generic code and kill
HW_BREAKPOINT_X.

And, heh, it would be nice if the hardware could tell us the _exact_
address (from the range) which should be blamed.

Oleg.

Re: [PATCH V5 0/3] perf/x86/amd: AMD Family 16h Data Breakpoint Extensions

[Suravee Suthikulpanit]

On 10/2/2013 11:15 AM, Oleg Nesterov wrote:
> On 10/02,suravee.suthikulpanit@amd.com  wrote:
>> >
>> >From: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
>> >
>> >Frederic, this is the rebase of the V4 patch onto the linux-3.12.0-rc3 (linux.git),
>> >and retest.
> But the code is the same? If yes,
>
> Reviewed-by: Oleg Nesterov<oleg@redhat.com>

The only change I made was in the tools/perf/util/parse-events.y due to change in parse_events_add_breakpoint(), but should be the same logic.

Suravee.

Re: [PATCH V5 0/3] perf/x86/amd: AMD Family 16h Data Breakpoint Extensions

[Frederic Weisbecker]

On Wed, Oct 02, 2013 at 11:11:05AM -0500, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Frederic, this is the rebase of the V4 patch onto the linux-3.12.0-rc3 (linux.git),
> and retest.
> 
> The following patchset enables hardware breakpoint bp_len greater than
> HW_BREAKPOINT_LEN_8 on AMD Family 16h and later.
> 
>   $ perf stat -e mem:0x1000/16:w a.out
>                             ^^
>                             bp_len
> 
> This will count writes to [0x1000 ~ 0x1010)

This interface looks good to me. I mean it seems flexible enough to provide
a range of address that fits anyway whenever the architecture supports breakpoint
ranges through either a length or a mask.

Thanks.

> 
> V5:
> * Rebase onto 3.12.0-rc3.
> * Modify the tools/perf/util/parse-events.y due to change in
>   parse_events_add_breakpoint().
> 
> V4:
> Even more per Oleg's suggestion:
> * Further simplify info->len and info->mask setting switch statement
> 
> V3:
> More per Oleg's suggestion:
> * Use already existing bp_len instead of changing userland API and
>   in kernel turn bp_len into proper AMD hardware breakpoint address
>   mask.
> 
> V2:
> Per Oleg's suggestions:
> * Moved testing of bp_addr_mask to validate_hw_breakpoint()
> * Changed perf tool syntax to mem:<addr>[/addr_mask][:access]
> 
> 
> Jacob Shin (3):
>   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
> 
>  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          | 47 +++++++++++----------------
>  tools/perf/Documentation/perf-record.txt |  7 ++--
>  tools/perf/tests/parse-events.c          | 55 ++++++++++++++++++++++++++++++++
>  tools/perf/util/parse-events.c           | 17 ++++------
>  tools/perf/util/parse-events.h           |  2 +-
>  tools/perf/util/parse-events.l           |  1 +
>  tools/perf/util/parse-events.y           | 26 +++++++++++++--
>  12 files changed, 142 insertions(+), 44 deletions(-)
> 
> -- 
> 1.8.1.2
> 
> 

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

[Frederic Weisbecker]

On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpanit@amd.com 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>
> ---
>  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       | 47 ++++++++++++++---------------------
>  6 files changed, 49 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> index d3f5c63..26609bb 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 */

Is this perhaps a bit too generic? Extension can mean about everything and who knows
what other feature Intel and Amd will add to breakpoints in the future.

How about X86_FEATURE_BP_RANGE?

>  #define X86_FEATURE_PERFCTR_L2	(6*32+28) /* L2 performance counter extensions */
>  
>  /*
> @@ -332,6 +333,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;

So it's a bit sad that we have both len and mask. OTOH it's my fault because we have that len
thing that is actually buggy for instruction breakpoints and needs to be sizeof(long) (who knows
what kind of fluorescent bier I drank before writing that).

But Oleg had a patch to fix that.

Oleg?

>  	u8		type;
>  };
> diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
> index bb04650..1f04f6c 100644
> --- a/arch/x86/include/uapi/asm/msr-index.h
> +++ b/arch/x86/include/uapi/asm/msr-index.h
> @@ -205,6 +205,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 903a264..fffc9cb 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -909,3 +909,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 f66ff16..3cb1951 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -128,6 +128,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;
>  }
> @@ -163,29 +165,8 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
>  	*dr7 &= ~__encode_dr7(i, info->len, info->type);
>  
>  	set_debugreg(*dr7, 7);
> -}
> -
> -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;
> +	if (info->mask)
> +		set_dr_addr_mask(0, i);
>  }
>  
>  /*
> @@ -198,7 +179,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);
>  }
> @@ -279,7 +260,16 @@ static int arch_build_bp_info(struct perf_event *bp)
>  	}
>  
>  	/* Len */
> +	info->mask = 0;
> +
>  	switch (bp->attr.bp_len) {
> +	default:
> +		if (!is_power_of_2(bp->attr.bp_len))
> +			return -EINVAL;
> +		if (!cpu_has_bpext)
> +			return -EOPNOTSUPP;
> +		info->mask = bp->attr.bp_len - 1;
> +		/* fall through */
>  	case HW_BREAKPOINT_LEN_1:
>  		info->len = X86_BREAKPOINT_LEN_1;
>  		break;
> @@ -294,12 +284,11 @@ static int arch_build_bp_info(struct perf_event *bp)
>  		info->len = X86_BREAKPOINT_LEN_8;
>  		break;
>  #endif
> -	default:
> -		return -EINVAL;
>  	}
>  
>  	return 0;
>  }
> +
>  /*
>   * Validate the arch-specific HW Breakpoint register settings
>   */
> @@ -314,11 +303,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;
> @@ -332,7 +321,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
>  		break;
>  #endif
>  	default:
> -		return ret;
> +		BUG();

Please use WARN_ON_ONCE(1) instead. BUG() makes the machine unusable, which may make the
user unhappy and the warning hard to log. BUG() is good only when letting things run may
compromize user persistent storage data integrity or so, like a scary filesystem bug for example.


>  	}
>  
>  	/*
> -- 
> 1.8.1.2
> 
> 

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

[Borislav Petkov]

fix tglx's address.

On Thu, Oct 31, 2013 at 10:58:31AM +0100, Frederic Weisbecker wrote:
> Is this perhaps a bit too generic? Extension can mean about everything
> and who knows what other feature Intel and Amd will add to breakpoints
> in the future.

Yeah, but that's the name of the feature. When they come up with another
extension, they'll call it BP_EXT2, most probably...

:-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

[Frederic Weisbecker]

On Thu, Oct 31, 2013 at 11:48:36AM +0100, Borislav Petkov wrote:
> fix tglx's address.
> 
> On Thu, Oct 31, 2013 at 10:58:31AM +0100, Frederic Weisbecker wrote:
> > Is this perhaps a bit too generic? Extension can mean about everything
> > and who knows what other feature Intel and Amd will add to breakpoints
> > in the future.
> 
> Yeah, but that's the name of the feature. When they come up with another
> extension, they'll call it BP_EXT2, most probably...
> 
> :-)

:-)

Ok we can keep that naming then, at least on the feature symbol. But add a comment
on it.

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

[Oleg Nesterov]

On 10/31, Frederic Weisbecker wrote:
>
> On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpanit@amd.com wrote:
> > --- 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;
>
> So it's a bit sad that we have both len and mask.

Yes, we can probably remove it later, in fact iirc it is not strictly
necessary right now. But this is minor, we can do this later and the
code looks simpler this way.

> thing that is actually buggy for instruction breakpoints and needs to
> be sizeof(long) (who knows
> what kind of fluorescent bier I drank before writing that).
>
> But Oleg had a patch to fix that.

Yes, we already discussed some draft patches. And one of the problems
was this series. I mean, the changes we discussed conflict with these
patches, I think we should fix this after this series is merged.

Oleg.

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

[Borislav Petkov]

On Thu, Oct 31, 2013 at 12:23:30PM +0100, Frederic Weisbecker wrote:
> Ok we can keep that naming then, at least on the feature symbol. But
> add a comment on it.

Great, in the latest F16h BKDG the CPUID bit is called
"DataBreakpointExtension". So BPEXT could mean anything :)

So the comment is with the definition of the bit:

+#define X86_FEATURE_BPEXT      (6*32+26) /* data breakpoint extension */

Oh well...

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

[Borislav Petkov]

On Sat, Nov 09, 2013 at 06:22:56AM +0900, Suravee Suthikulpanit wrote:
> Sorry for late reply. And yes, the BDKG called it "Breakpoint
> Extension". Keeping the name consistent with the documentation might
> be best for now to avoid confusion.
>
> So, would you like me to send in a new patch to add this comment?

No need, the X86_FEATURE_BPEXT define has a comment already.

-- 
Regards/Gruss,
Boris.

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

[Frederic Weisbecker]

On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpanit@amd.com 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>
> ---
>  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       | 47 ++++++++++++++---------------------
>  6 files changed, 49 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> index d3f5c63..26609bb 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 */

Does this feature only work on data breakpoint or is instruction breakpoint
address range supported as well?

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

[Suravee Suthikulpanit]

On 11/02/2013 01:34 PM, Borislav Petkov wrote:
> On Thu, Oct 31, 2013 at 12:23:30PM +0100, Frederic Weisbecker wrote:
>> Ok we can keep that naming then, at least on the feature symbol. But
>> add a comment on it.
> Great, in the latest F16h BKDG the CPUID bit is called
> "DataBreakpointExtension". So BPEXT could mean anything :)
>
> So the comment is with the definition of the bit:
>
> +#define X86_FEATURE_BPEXT      (6*32+26) /* data breakpoint extension */
>
> Oh well...
>
Sorry for late reply.  And yes, the BDKG called it "Breakpoint Extension". Keeping the name consistent with the documentation might be best for now to avoid confusion.

So, would you like me to send in a new patch to add this comment?

Suravee

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

[Oleg Nesterov]

On 11/08, Frederic Weisbecker wrote:
>
> On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpanit@amd.com wrote:
> >
> > diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> > index d3f5c63..26609bb 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 */
>
> Does this feature only work on data breakpoint or is instruction breakpoint
> address range supported as well?

IIRC, execute range is supported as well.

But. I can't look at the code now, but iirc this can't really work until
we fix the (already discussed) problems with bp_len && X86_BREAKPOINT_LEN_X.
IOW, we should not blame these patches if it doesn't work.

Oleg.

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

[Frederic Weisbecker]

On Sat, Nov 09, 2013 at 04:11:56PM +0100, Oleg Nesterov wrote:
> On 11/08, Frederic Weisbecker wrote:
> >
> > On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpanit@amd.com wrote:
> > >
> > > diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> > > index d3f5c63..26609bb 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 */
> >
> > Does this feature only work on data breakpoint or is instruction breakpoint
> > address range supported as well?
> 
> IIRC, execute range is supported as well.
> 
> But. I can't look at the code now, but iirc this can't really work until
> we fix the (already discussed) problems with bp_len && X86_BREAKPOINT_LEN_X.
> IOW, we should not blame these patches if it doesn't work.

Yeah, don't worry I don't plan to push back these patches for the sake of that bug,
that would be definetly unfair, especially as I introduced that issue :)

And the patchset looks good overall, except for a few details but it's mostly ok,
I just would like to fix that issue along the way. It would be really nice if we can
avoid having a mask _and_ a len for breakpoints. I mean, that doesn't look right to me,
it's two units basically measuring the same thing, so that's asking for conflicting troubles.

I'm just not sure how to reuse the len to express breakpoint ranges (that was in fact the
initial purpose of it) without breaking the tools.

Any idea?

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

[Oleg Nesterov]

Just in case let me repeat, I can be easily wrong because I forgot
how this series actually look and I don't have the patches now ;)

On 11/09, Frederic Weisbecker wrote:
>
> On Sat, Nov 09, 2013 at 04:11:56PM +0100, Oleg Nesterov wrote:
> > On 11/08, Frederic Weisbecker wrote:
> > >
> > >
> > > Does this feature only work on data breakpoint or is instruction breakpoint
> > > address range supported as well?
> >
> > IIRC, execute range is supported as well.
> >
> > But. I can't look at the code now, but iirc this can't really work until
> > we fix the (already discussed) problems with bp_len && X86_BREAKPOINT_LEN_X.
> > IOW, we should not blame these patches if it doesn't work.
>
> Yeah, don't worry I don't plan to push back these patches for the sake of that bug,
> that would be definetly unfair, especially as I introduced that issue :)
>
> And the patchset looks good overall, except for a few details but it's mostly ok,

OK,

> I just would like to fix that issue along the way. It would be really nice if we can
> avoid having a mask _and_ a len for breakpoints.

Up to you and Suravee, but can't we cleanup this later?

This series was updated many times to address a lot of (sometimes
contradictory) complaints.

> I mean, that doesn't look right to me,
> it's two units basically measuring the same thing, so that's asking for conflicting troubles.

Yes. And we can kill either _len or _mask, not sure what would be more
clean.

At least with the current implementation (again, iirc) mask == len -1.
Although amd supports the more generic masks (but I can't recall the
details).

> I'm just not sure how to reuse the len to express breakpoint ranges (that was in fact the
> initial purpose of it) without breaking the tools.

Confused... user-space still uses len to express the range? Just
the kernel "switches" to mask if len > 8 ?

Oleg.

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

[Frederic Weisbecker]

On Sat, Nov 09, 2013 at 04:54:28PM +0100, Oleg Nesterov wrote:
> Just in case let me repeat, I can be easily wrong because I forgot
> how this series actually look and I don't have the patches now ;)
> 
> On 11/09, Frederic Weisbecker wrote:
> >
> > On Sat, Nov 09, 2013 at 04:11:56PM +0100, Oleg Nesterov wrote:
> > > On 11/08, Frederic Weisbecker wrote:
> > > >
> > > >
> > > > Does this feature only work on data breakpoint or is instruction breakpoint
> > > > address range supported as well?
> > >
> > > IIRC, execute range is supported as well.
> > >
> > > But. I can't look at the code now, but iirc this can't really work until
> > > we fix the (already discussed) problems with bp_len && X86_BREAKPOINT_LEN_X.
> > > IOW, we should not blame these patches if it doesn't work.
> >
> > Yeah, don't worry I don't plan to push back these patches for the sake of that bug,
> > that would be definetly unfair, especially as I introduced that issue :)
> >
> > And the patchset looks good overall, except for a few details but it's mostly ok,
> 
> OK,
> 
> > I just would like to fix that issue along the way. It would be really nice if we can
> > avoid having a mask _and_ a len for breakpoints.
> 
> Up to you and Suravee, but can't we cleanup this later?
> 
> This series was updated many times to address a lot of (sometimes
> contradictory) complaints.

Sure. But I'm confident that we can solve the conflicting mask / len issue easily beside.
I mean, I don't feel confident with merging things as is, otoh it should be easy to fix up.

I can to the fixup myself BTW, no problem with that, and I don't want to hold up this patchset,
but we just need to agree on a proper solution.
 
> > I mean, that doesn't look right to me,
> > it's two units basically measuring the same thing, so that's asking for conflicting troubles.
> 
> Yes. And we can kill either _len or _mask, not sure what would be more
> clean.
> 
> At least with the current implementation (again, iirc) mask == len -1.
> Although amd supports the more generic masks (but I can't recall the
> details).

Right. I think len is probably more powerful. Unless the mask could be used to define
multiple ranges of breakpoints, not sure it's ever going to be used that way though. But we
can still extend the interface if we need a mask later.

> 
> > I'm just not sure how to reuse the len to express breakpoint ranges (that was in fact the
> > initial purpose of it) without breaking the tools.
> 
> Confused... user-space still uses len to express the range? Just
> the kernel "switches" to mask if len > 8 ?

Right but what if we want breakpoints having a size below 8? Like break on instructions
from 0x1000 to 0x1008 ?

Or should we ignore range instruction breakpoints when len < 8?

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

[Oleg Nesterov]

On 11/11, Frederic Weisbecker wrote:
>
> On Sat, Nov 09, 2013 at 04:54:28PM +0100, Oleg Nesterov wrote:
> >
> > Up to you and Suravee, but can't we cleanup this later?
> >
> > This series was updated many times to address a lot of (sometimes
> > contradictory) complaints.
>
> Sure. But I'm confident that we can solve the conflicting mask / len issue easily beside.
> I mean, I don't feel confident with merging things as is, otoh it should be easy to fix up.

I do not really understand where do you see the conflict...

I can be easily wrong, but afaics currently mask / len issue is simply
the implementation detail.

> > > I mean, that doesn't look right to me,
> > > it's two units basically measuring the same thing, so that's asking for conflicting troubles.
> >
> > Yes. And we can kill either _len or _mask, not sure what would be more
> > clean.
> >
> > At least with the current implementation (again, iirc) mask == len -1.
> > Although amd supports the more generic masks (but I can't recall the
> > details).
>
> Right. I think len is probably more powerful. Unless the mask could be used to define
> multiple ranges of breakpoints, not sure it's ever going to be used that way though.

Actually, mask is more powerfull. And initial versions of this patches
(iirc) tried to use mask as an argument which comes from the userspace
(tools/perf, perf_event_attr, etc). But one of reviewers nacked this
interfacer, so we still use len.

> But we
> can still extend the interface if we need a mask later.

Yes, agreed.

> > > I'm just not sure how to reuse the len to express breakpoint ranges (that was in fact the
> > > initial purpose of it) without breaking the tools.
> >
> > Confused... user-space still uses len to express the range? Just
> > the kernel "switches" to mask if len > 8 ?
>
> Right but what if we want breakpoints having a size below 8? Like break on instructions
> from 0x1000 to 0x1008 ?
>
> Or should we ignore range instruction breakpoints when len < 8?

In this case the new code has no effect (iirc), we simply use
X86_BREAKPOINT_LEN_* and "tell the hardware about extended range/mask"
code is never called. IIRC, currently we simply check bp_mask != 0
to distinguish.

Oleg.

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

[Frederic Weisbecker]

2013/11/11 Oleg Nesterov <oleg@redhat.com>:
> On 11/11, Frederic Weisbecker wrote:
>>
>> On Sat, Nov 09, 2013 at 04:54:28PM +0100, Oleg Nesterov wrote:
>> >
>> > Up to you and Suravee, but can't we cleanup this later?
>> >
>> > This series was updated many times to address a lot of (sometimes
>> > contradictory) complaints.
>>
>> Sure. But I'm confident that we can solve the conflicting mask / len issue easily beside.
>> I mean, I don't feel confident with merging things as is, otoh it should be easy to fix up.
>
> I do not really understand where do you see the conflict...
>
> I can be easily wrong, but afaics currently mask / len issue is simply
> the implementation detail.

I think it's like we have an object that has a length, and to create
this object we pass both kilometers and miles. Ok it's a bit different
here because a mask can apply on top of a len. But here it's used to
define essentially the same thing (ie: a range of address)

>
>> > > I mean, that doesn't look right to me,
>> > > it's two units basically measuring the same thing, so that's asking for conflicting troubles.
>> >
>> > Yes. And we can kill either _len or _mask, not sure what would be more
>> > clean.
>> >
>> > At least with the current implementation (again, iirc) mask == len -1.
>> > Although amd supports the more generic masks (but I can't recall the
>> > details).
>>
>> Right. I think len is probably more powerful. Unless the mask could be used to define
>> multiple ranges of breakpoints, not sure it's ever going to be used that way though.
>
> Actually, mask is more powerfull. And initial versions of this patches
> (iirc) tried to use mask as an argument which comes from the userspace
> (tools/perf, perf_event_attr, etc). But one of reviewers nacked this
> interfacer, so we still use len.

Well, we can still reconsider it if needed but to me it seems that
mask is only interesting if we may deal with non contiguous range of
addresses. It doesn't appear to be the case, but I don't have much
tricky usecases in mind.

>
>> But we
>> can still extend the interface if we need a mask later.
>
> Yes, agreed.

Great.

>
>> > > I'm just not sure how to reuse the len to express breakpoint ranges (that was in fact the
>> > > initial purpose of it) without breaking the tools.
>> >
>> > Confused... user-space still uses len to express the range? Just
>> > the kernel "switches" to mask if len > 8 ?
>>
>> Right but what if we want breakpoints having a size below 8? Like break on instructions
>> from 0x1000 to 0x1008 ?
>>
>> Or should we ignore range instruction breakpoints when len < 8?
>
> In this case the new code has no effect (iirc), we simply use
> X86_BREAKPOINT_LEN_* and "tell the hardware about extended range/mask"
> code is never called. IIRC, currently we simply check bp_mask != 0
> to distinguish.

I'm not sure I understand correctly. Do you mean that range below 8
don't rely on extended breakpoint range?

Ideally it would be nice if we drop bp_mask and use extended ranges
only when len > 8. How does that sound?

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

[Oleg Nesterov]

On 12/03, Frederic Weisbecker wrote:
>
> 2013/11/11 Oleg Nesterov <oleg@redhat.com>:
> > On 11/11, Frederic Weisbecker wrote:
> >>
> >> On Sat, Nov 09, 2013 at 04:54:28PM +0100, Oleg Nesterov wrote:
> >> >
> >> > Up to you and Suravee, but can't we cleanup this later?
> >> >
> >> > This series was updated many times to address a lot of (sometimes
> >> > contradictory) complaints.
> >>
> >> Sure. But I'm confident that we can solve the conflicting mask / len issue easily beside.
> >> I mean, I don't feel confident with merging things as is, otoh it should be easy to fix up.
> >
> > I do not really understand where do you see the conflict...
> >
> > I can be easily wrong, but afaics currently mask / len issue is simply
> > the implementation detail.
>
> I think it's like we have an object that has a length, and to create
> this object we pass both kilometers and miles. Ok it's a bit different
> here because a mask can apply on top of a len. But here it's used to
> define essentially the same thing (ie: a range of address)

Yes. perf/etc uses length, the current imlementation uses ->mask to
actually set the range.

> > Actually, mask is more powerfull. And initial versions of this patches
> > (iirc) tried to use mask as an argument which comes from the userspace
> > (tools/perf, perf_event_attr, etc). But one of reviewers nacked this
> > interfacer, so we still use len.
>
> Well, we can still reconsider it if needed but to me it seems that
> mask is only interesting if we may deal with non contiguous range of
> addresses.

And this is what this mask can actually do. Just there is no way (currently)
to pass the mask from userpace.

> >> Right but what if we want breakpoints having a size below 8? Like break on instructions
> >> from 0x1000 to 0x1008 ?
> >>
> >> Or should we ignore range instruction breakpoints when len < 8?
> >
> > In this case the new code has no effect (iirc), we simply use
> > X86_BREAKPOINT_LEN_* and "tell the hardware about extended range/mask"
> > code is never called. IIRC, currently we simply check bp_mask != 0
> > to distinguish.
>
> I'm not sure I understand correctly. Do you mean that range below 8
> don't rely on extended breakpoint range?

IIRC - yes.

> Ideally it would be nice if we drop bp_mask and use extended ranges
> only when len > 8. How does that sound?

Again, iirc, this is what the code does. except (in essence) it checks
mask != 0 instead of len > 8.

And yes, we can probably drop bp_mask (unless we are going to support
the contiguous ranges), just I think we can do this later.

Oleg.

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

[Frederic Weisbecker]

On Wed, Dec 04, 2013 at 02:57:43PM +0100, Oleg Nesterov wrote:
> On 12/03, Frederic Weisbecker wrote:
> >
> > 2013/11/11 Oleg Nesterov <oleg@redhat.com>:
> > > On 11/11, Frederic Weisbecker wrote:
> > >>
> > >> On Sat, Nov 09, 2013 at 04:54:28PM +0100, Oleg Nesterov wrote:
> > >> >
> > >> > Up to you and Suravee, but can't we cleanup this later?
> > >> >
> > >> > This series was updated many times to address a lot of (sometimes
> > >> > contradictory) complaints.
> > >>
> > >> Sure. But I'm confident that we can solve the conflicting mask / len issue easily beside.
> > >> I mean, I don't feel confident with merging things as is, otoh it should be easy to fix up.
> > >
> > > I do not really understand where do you see the conflict...
> > >
> > > I can be easily wrong, but afaics currently mask / len issue is simply
> > > the implementation detail.
> >
> > I think it's like we have an object that has a length, and to create
> > this object we pass both kilometers and miles. Ok it's a bit different
> > here because a mask can apply on top of a len. But here it's used to
> > define essentially the same thing (ie: a range of address)
> 
> Yes. perf/etc uses length, the current imlementation uses ->mask to
> actually set the range.
> 
> > > Actually, mask is more powerfull. And initial versions of this patches
> > > (iirc) tried to use mask as an argument which comes from the userspace
> > > (tools/perf, perf_event_attr, etc). But one of reviewers nacked this
> > > interfacer, so we still use len.
> >
> > Well, we can still reconsider it if needed but to me it seems that
> > mask is only interesting if we may deal with non contiguous range of
> > addresses.
> 
> And this is what this mask can actually do. Just there is no way (currently)
> to pass the mask from userpace.

Ok but are we interested in non contiguous range?

> 
> > >> Right but what if we want breakpoints having a size below 8? Like break on instructions
> > >> from 0x1000 to 0x1008 ?
> > >>
> > >> Or should we ignore range instruction breakpoints when len < 8?
> > >
> > > In this case the new code has no effect (iirc), we simply use
> > > X86_BREAKPOINT_LEN_* and "tell the hardware about extended range/mask"
> > > code is never called. IIRC, currently we simply check bp_mask != 0
> > > to distinguish.
> >
> > I'm not sure I understand correctly. Do you mean that range below 8
> > don't rely on extended breakpoint range?
> 
> IIRC - yes.
> 
> > Ideally it would be nice if we drop bp_mask and use extended ranges
> > only when len > 8. How does that sound?
> 
> Again, iirc, this is what the code does. except (in essence) it checks
> mask != 0 instead of len > 8.

Ok.

> 
> And yes, we can probably drop bp_mask (unless we are going to support
> the contiguous ranges), just I think we can do this later.

The problem is that once we push the bp_mask interface, we won't be able
to remove it later. It's a user ABI.

So I really want to be careful with that and extend bp_len for range breakpoints
then if we find out limitations, only then we can introduce bp_mask.

Suravee, any thought about this?

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

[Frederic Weisbecker]

On Wed, Dec 04, 2013 at 02:57:43PM +0100, Oleg Nesterov wrote:
> > Ideally it would be nice if we drop bp_mask and use extended ranges
> > only when len > 8. How does that sound?
> 
> Again, iirc, this is what the code does. except (in essence) it checks
> mask != 0 instead of len > 8.
> 
> And yes, we can probably drop bp_mask (unless we are going to support
> the contiguous ranges), just I think we can do this later.

Ah wait, I understand now, this is not a user ABI, just an internal state.
So you're right after all.

It's never too late to be unconfused ;)

Ok let me dig into more details on the patchset.

Thanks.

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

[Frederic Weisbecker]

On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpanit@amd.com 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>

Thinking further about this, Oleg convinced me that we can keep the mask eventually.
It actually makes sense internally just for the use of bpext.

But I still have a few comments on this patchset:

> ---
>  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       | 47 ++++++++++++++---------------------
>  6 files changed, 49 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> index d3f5c63..26609bb 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 */

Please comment that it's AMD specific.

>  #define X86_FEATURE_PERFCTR_L2	(6*32+28) /* L2 performance counter extensions */
>  
>  /*
> @@ -332,6 +333,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 bb04650..1f04f6c 100644
> --- a/arch/x86/include/uapi/asm/msr-index.h
> +++ b/arch/x86/include/uapi/asm/msr-index.h
> @@ -205,6 +205,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 903a264..fffc9cb 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -909,3 +909,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:

How about just:

if (dr <= 4 && dr >= 0)
   wrmsr(MSR_F16H_DR1_ADDR_MASK + dr - 1, mask, 0)

> +		break;
> +	}
> +}
> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index f66ff16..3cb1951 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -128,6 +128,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;
>  }
> @@ -163,29 +165,8 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
>  	*dr7 &= ~__encode_dr7(i, info->len, info->type);
>  
>  	set_debugreg(*dr7, 7);
> -}
> -
> -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;
> +	if (info->mask)
> +		set_dr_addr_mask(0, i);
>  }
>  
>  /*
> @@ -198,7 +179,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;

So we can access the attr directly instead of get_hbp_len(). Good, that's a welcome
simplification but it's a change that is meaningful and tricky enough to deserve a seperate patch.

Could you send a seperate one for that?

>  
>  	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
>  }
> @@ -279,7 +260,16 @@ static int arch_build_bp_info(struct perf_event *bp)
>  	}
>  
>  	/* Len */
> +	info->mask = 0;
> +
>  	switch (bp->attr.bp_len) {
> +	default:
> +		if (!is_power_of_2(bp->attr.bp_len))
> +			return -EINVAL;
> +		if (!cpu_has_bpext)
> +			return -EOPNOTSUPP;
> +		info->mask = bp->attr.bp_len - 1;
> +		/* fall through */

So, that's perhaps just personal preference but having the default on
top of the switch makes things very confusing. Can't we put the above
in the end of the switch instead?

>  	case HW_BREAKPOINT_LEN_1:
>  		info->len = X86_BREAKPOINT_LEN_1;
>  		break;
> @@ -294,12 +284,11 @@ static int arch_build_bp_info(struct perf_event *bp)
>  		info->len = X86_BREAKPOINT_LEN_8;
>  		break;
>  #endif
> -	default:
> -		return -EINVAL;
>  	}
>  
>  	return 0;
>  }
> +
>  /*
>   * Validate the arch-specific HW Breakpoint register settings
>   */
> @@ -314,11 +303,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;

Confused, I thought mask is set only when length is above 8?

>  		break;
>  	case X86_BREAKPOINT_LEN_2:
>  		align = 1;
> @@ -332,7 +321,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
>  		break;
>  #endif
>  	default:
> -		return ret;
> +		BUG();

Please use WARN_ON_ONCE() instead.

Thanks.

>  	}
>  
>  	/*
> -- 
> 1.8.1.2
> 
> 

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

[Frederic Weisbecker]

On Wed, Oct 02, 2013 at 11:11:07AM -0500, suravee.suthikulpanit@amd.com 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.
> 
> Signed-off-by: Jacob Shin <jacob.w.shin@gmail.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  tools/perf/Documentation/perf-record.txt |  7 +++++--
>  tools/perf/util/parse-events.c           | 17 +++++++----------
>  tools/perf/util/parse-events.h           |  2 +-
>  tools/perf/util/parse-events.l           |  1 +
>  tools/perf/util/parse-events.y           | 26 ++++++++++++++++++++++++--
>  5 files changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> index e297b74..e07491f 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 9812531..65d81ae 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -515,7 +515,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;
>  
> @@ -525,14 +525,11 @@ 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)
> +		len = attr.bp_type == HW_BREAKPOINT_X ? sizeof(long) :
> +							HW_BREAKPOINT_LEN_4;

Why is LEN_4 affected in the type?

> +	attr.bp_len = len;

Thanks.

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

[Oleg Nesterov]

On 12/10, Frederic Weisbecker wrote:
>
> On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpanit@amd.com wrote:
> > @@ -279,7 +260,16 @@ static int arch_build_bp_info(struct perf_event *bp)
> >  	}
> >
> >  	/* Len */
> > +	info->mask = 0;
> > +
> >  	switch (bp->attr.bp_len) {
> > +	default:
> > +		if (!is_power_of_2(bp->attr.bp_len))
> > +			return -EINVAL;
> > +		if (!cpu_has_bpext)
> > +			return -EOPNOTSUPP;
> > +		info->mask = bp->attr.bp_len - 1;
> > +		/* fall through */
>
> So, that's perhaps just personal preference but having the default on
> top of the switch makes things very confusing. Can't we put the above
> in the end of the switch instead?

Then "fall through" won't work ;)

> > @@ -314,11 +303,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;
>
> Confused, I thought mask is set only when length is above 8?

Yes. But we need the info->len for hw anyway. if mask != 0 then
len == X86_BREAKPOINT_LEN_1 and it is still used by encode_dr7().
Note that it is not the length in bytes, it is the magic x86 code.

->bp_len is the length.

Oleg.

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

[Oleg Nesterov]

On 12/10, Frederic Weisbecker wrote:
>
> On Wed, Oct 02, 2013 at 11:11:07AM -0500, suravee.suthikulpanit@amd.com wrote:
> > @@ -525,14 +525,11 @@ 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)
> > +		len = attr.bp_type == HW_BREAKPOINT_X ? sizeof(long) :
> > +							HW_BREAKPOINT_LEN_4;
>
> Why is LEN_4 affected in the type?

Hmm, what do you mean?

Oleg.

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

[Frederic Weisbecker]

On Tue, Dec 10, 2013 at 05:22:29PM +0100, Oleg Nesterov wrote:
> On 12/10, Frederic Weisbecker wrote:
> >
> > On Wed, Oct 02, 2013 at 11:11:07AM -0500, suravee.suthikulpanit@amd.com wrote:
> > > @@ -525,14 +525,11 @@ 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)
> > > +		len = attr.bp_type == HW_BREAKPOINT_X ? sizeof(long) :
> > > +							HW_BREAKPOINT_LEN_4;
> >
> > Why is LEN_4 affected in the type?
> 
> Hmm, what do you mean?

I got confused (as usual) by the x = y == foo ? bar : stuff;

I suggest we avoid that kind of coding style, at least to spare repeated confused
and annoying emails from me ;-)

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

[Frederic Weisbecker]

On Tue, Dec 10, 2013 at 05:19:45PM +0100, Oleg Nesterov wrote:
> On 12/10, Frederic Weisbecker wrote:
> >
> > On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpanit@amd.com wrote:
> > > @@ -279,7 +260,16 @@ static int arch_build_bp_info(struct perf_event *bp)
> > >  	}
> > >
> > >  	/* Len */
> > > +	info->mask = 0;
> > > +
> > >  	switch (bp->attr.bp_len) {
> > > +	default:
> > > +		if (!is_power_of_2(bp->attr.bp_len))
> > > +			return -EINVAL;
> > > +		if (!cpu_has_bpext)
> > > +			return -EOPNOTSUPP;
> > > +		info->mask = bp->attr.bp_len - 1;
> > > +		/* fall through */
> >
> > So, that's perhaps just personal preference but having the default on
> > top of the switch makes things very confusing. Can't we put the above
> > in the end of the switch instead?
> 
> Then "fall through" won't work ;)

Indeed, now may be that's just me but it's very hard to parse :)

There are other ways to perform the above, it's no big deal if
we duplicate one line of code.

> 
> > > @@ -314,11 +303,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;
> >
> > Confused, I thought mask is set only when length is above 8?
> 
> Yes. But we need the info->len for hw anyway. if mask != 0 then
> len == X86_BREAKPOINT_LEN_1 and it is still used by encode_dr7().
> Note that it is not the length in bytes, it is the magic x86 code.

Good point, and that matches the above fallthrough.

Thanks.

> 
> ->bp_len is the length.
> 
> Oleg.
> 

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

[Suravee Suthikulanit]

On 12/10/2013 9:23 AM, Frederic Weisbecker wrote:
> On Wed, Oct 02, 2013 at 11:11:06AM -0500,suravee.suthikulpanit@amd.com  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>
> Thinking further about this, Oleg convinced me that we can keep the mask eventually.
> It actually makes sense internally just for the use of bpext.
>
> But I still have a few comments on this patchset:
>> ---
>>   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       | 47 ++++++++++++++---------------------
>>   6 files changed, 49 insertions(+), 29 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
>> index d3f5c63..26609bb 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 */
> Please comment that it's AMD specific.

This #define locates in the AMD-specific section within the code,
which already has a comment at the beginning of the section saying
"/* More extended AMD flags: CPUID level 0x80000001, ecx, word 6 */".

>>   #define X86_FEATURE_PERFCTR_L2	(6*32+28) /* L2 performance counter extensions */
>>   
>>   /*
>> @@ -332,6 +333,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 bb04650..1f04f6c 100644
>> --- a/arch/x86/include/uapi/asm/msr-index.h
>> +++ b/arch/x86/include/uapi/asm/msr-index.h
>> @@ -205,6 +205,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 903a264..fffc9cb 100644
>> --- a/arch/x86/kernel/cpu/amd.c
>> +++ b/arch/x86/kernel/cpu/amd.c
>> @@ -909,3 +909,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:
> How about just:
>
> if (dr <= 4 && dr >= 0)
>     wrmsr(MSR_F16H_DR1_ADDR_MASK + dr - 1, mask, 0)

That won't work since the DR0 (MSR 0xc0011027)is at not contiguous
with DR1-3 (MSR 0xc0011019 - 0xc001101b).  If you prefer if/else, we
can do that as well, but I think it might be as cumbersome in the code.

>> +		break;
>> +	}
>> +}
>> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
>> index f66ff16..3cb1951 100644
>> --- a/arch/x86/kernel/hw_breakpoint.c
>> +++ b/arch/x86/kernel/hw_breakpoint.c
>> @@ -128,6 +128,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;
>>   }
>> @@ -163,29 +165,8 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
>>   	*dr7 &= ~__encode_dr7(i, info->len, info->type);
>>   
>>   	set_debugreg(*dr7, 7);
>> -}
>> -
>> -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;
>> +	if (info->mask)
>> +		set_dr_addr_mask(0, i);
>>   }
>>   
>>   /*
>> @@ -198,7 +179,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;
> So we can access the attr directly instead of get_hbp_len(). Good, that's a welcome
> simplification but it's a change that is meaningful and tricky enough to deserve a seperate patch.
>
> Could you send a seperate one for that?

Sure, I can split the patch.

>>   	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
>>   }
>> @@ -279,7 +260,16 @@ static int arch_build_bp_info(struct perf_event *bp)
>>   	}
>>   
>>   	/* Len */
>> +	info->mask = 0;
>> +
>>   	switch (bp->attr.bp_len) {
>> +	default:
>> +		if (!is_power_of_2(bp->attr.bp_len))
>> +			return -EINVAL;
>> +		if (!cpu_has_bpext)
>> +			return -EOPNOTSUPP;
>> +		info->mask = bp->attr.bp_len - 1;
>> +		/* fall through */
> So, that's perhaps just personal preference but having the default on
> top of the switch makes things very confusing. Can't we put the above
> in the end of the switch instead?

Ok. I'll clean this up a bit to your preference.

>
>>   	case HW_BREAKPOINT_LEN_1:
>>   		info->len = X86_BREAKPOINT_LEN_1;
>>   		break;
>> @@ -294,12 +284,11 @@ static int arch_build_bp_info(struct perf_event *bp)
>>   		info->len = X86_BREAKPOINT_LEN_8;
>>   		break;
>>   #endif
>> -	default:
>> -		return -EINVAL;
>>   	}
>>   
>>   	return 0;
>>   }
>> +
>>   /*
>>    * Validate the arch-specific HW Breakpoint register settings
>>    */
>> @@ -314,11 +303,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;
> Confused, I thought mask is set only when length is above 8?
>
>>   		break;
>>   	case X86_BREAKPOINT_LEN_2:
>>   		align = 1;
>> @@ -332,7 +321,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
>>   		break;
>>   #endif
>>   	default:
>> -		return ret;
>> +		BUG();
> Please use WARN_ON_ONCE() instead.
>
> Thanks.

OK.

I'll send out soon.

Suravee

   

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

[Jacob Shin]

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.shin@amd.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       |   47 +++++++++++++--------------------
 6 files changed, 49 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index ac10df7..bee6994 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -168,6 +168,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 */
 
 /*
@@ -317,6 +318,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 b575788..dac1d3b 100644
--- a/arch/x86/include/uapi/asm/msr-index.h
+++ b/arch/x86/include/uapi/asm/msr-index.h
@@ -199,6 +199,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 fa96eb0..2c3f12e 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -910,3 +910,22 @@ bool cpu_has_amd_erratum(const int *erratum)
 }
 
 EXPORT_SYMBOL_GPL(cpu_has_amd_erratum);
+
+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 02f0763..885ce48 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -128,6 +128,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;
 }
@@ -163,29 +165,8 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 	*dr7 &= ~__encode_dr7(i, info->len, info->type);
 
 	set_debugreg(*dr7, 7);
-}
-
-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;
+	if (info->mask)
+		set_dr_addr_mask(0, i);
 }
 
 /*
@@ -198,7 +179,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);
 }
@@ -279,7 +260,16 @@ static int arch_build_bp_info(struct perf_event *bp)
 	}
 
 	/* Len */
+	info->mask = 0;
+
 	switch (bp->attr.bp_len) {
+	default:
+		if (!is_power_of_2(bp->attr.bp_len))
+			return -EINVAL;
+		if (!cpu_has_bpext)
+			return -EOPNOTSUPP;
+		info->mask = bp->attr.bp_len - 1;
+		/* fall through */
 	case HW_BREAKPOINT_LEN_1:
 		info->len = X86_BREAKPOINT_LEN_1;
 		break;
@@ -294,12 +284,11 @@ static int arch_build_bp_info(struct perf_event *bp)
 		info->len = X86_BREAKPOINT_LEN_8;
 		break;
 #endif
-	default:
-		return -EINVAL;
 	}
 
 	return 0;
 }
+
 /*
  * Validate the arch-specific HW Breakpoint register settings
  */
@@ -314,11 +303,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;
@@ -332,7 +321,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 		break;
 #endif
 	default:
-		return ret;
+		BUG();
 	}
 
 	/*
-- 
1.7.9.5

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

[Jacob Shin]

On Sat, Apr 27, 2013 at 06:10:28PM +0200, Oleg Nesterov wrote:
> On 04/27, Jacob Shin wrote:
> >
> > On Sat, Apr 27, 2013 at 05:05:10PM +0200, Oleg Nesterov wrote:
> > > ...
> > > > +	if (info->mask)
> > > > +		set_dr_addr_mask(0, i);
> > >
> > > I agree we should clear addr_mask anyway.
> > >
> > > But I am just curious, what if we do not? I mean what will the hardware
> > > do if this breakpoint was already disabled but the mask wasn't cleared?
> >
> > Oh, it is fine if we don't and we are not using breakpoints, however I
> > was trying to account for per-thread events sharing the same DR
> > register, in that case we don't want previous event's mask value still
> > in the MSR.
> 
> Aha, so CPU "remembers" the non-cleared mask and this can affect the
> next "enable". Thanks.
> 
> > > Suppose that the kernel was compiled without CONFIG_CPU_SUP_AMD.
> > > Then perf_event_open(attr => { .bp_len == 16 }) will succeed, but
> > > this breakpoint won't actually work as expected?
> >
> > Well, on non-AMD, even if we have CONFIG_CPU_SUP_AMD=y or not,
> > cpu_has_bpext (x86 CPUID check) will fail.
> >
> > On AMD, if we don't have CONFIG_CPU_SUP_AMD=y then we can't even boot.
> 
> Heh, I didn't know ;)
> 
> OK, I think the patch is fine then.
> 
> Except... cough, the last nit, I promise ;)
> 
> Currently this doesn't matter, the only caller of modify_user_hw_breakpoint()
> is ptrace, and it can't use bp_len != HW_BREAKPOINT_LEN_*.
> 
> But still, I think your patch needs a small fixlet,
> 
> 	- /* info->len == info->mask == 0 */
> 	+ info->mask = 0;
> 
> Or we can do this later.
> 
> And while this is purely cosmetic (feel free to ignore), perhaps we
> can join the bp_len checks and move cpu_has_bpext from _validate to
> _build, this looks a little bit cleaner imho. IOW,
> 
> 
> 	info->mask == 0;
> 
> 	switch (bp->attr.bp_len) {
> 	default:
> 		if (!is_power_of_2(bp->attr.bp_len))
> 			return -EINVAL;
> 		if (!cpu_has_bpext)
> 			return -EOPNOTSUPP;
> 		info->mask = bp->attr.bp_len - 1;
> 		/* fallthrough */
> 	case HW_BREAKPOINT_LEN_1:
> 		info->len = X86_BREAKPOINT_LEN_1;
> 		break;
> 	case HW_BREAKPOINT_LEN_2:
> 		info->len = X86_BREAKPOINT_LEN_2;
> 		break;
> 	case HW_BREAKPOINT_LEN_4:
> 		info->len = X86_BREAKPOINT_LEN_4;
> 		break;
> #ifdef CONFIG_X86_64
> 	case HW_BREAKPOINT_LEN_8:
> 		info->len = X86_BREAKPOINT_LEN_8;
> 		break;
> #endif
> 	}
> 
> Then arch_validate_hwbkpt_settings() only needs
> 
> 		case X86_BREAKPOINT_LEN_1:
> 			align = 0;
> 	+	if (info->mask)
> 	+		align = mask;
> 
> change.

Okay I have made these changes and did final smoke testing .. I'll
send out the patch bomb in a bit.

Thanks again, and again for taking the time to look this over!

-Jacob

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

[Oleg Nesterov]

On 04/27, Jacob Shin wrote:
>
> On Sat, Apr 27, 2013 at 05:05:10PM +0200, Oleg Nesterov wrote:
> > ...
> > > +	if (info->mask)
> > > +		set_dr_addr_mask(0, i);
> >
> > I agree we should clear addr_mask anyway.
> >
> > But I am just curious, what if we do not? I mean what will the hardware
> > do if this breakpoint was already disabled but the mask wasn't cleared?
>
> Oh, it is fine if we don't and we are not using breakpoints, however I
> was trying to account for per-thread events sharing the same DR
> register, in that case we don't want previous event's mask value still
> in the MSR.

Aha, so CPU "remembers" the non-cleared mask and this can affect the
next "enable". Thanks.

> > Suppose that the kernel was compiled without CONFIG_CPU_SUP_AMD.
> > Then perf_event_open(attr => { .bp_len == 16 }) will succeed, but
> > this breakpoint won't actually work as expected?
>
> Well, on non-AMD, even if we have CONFIG_CPU_SUP_AMD=y or not,
> cpu_has_bpext (x86 CPUID check) will fail.
>
> On AMD, if we don't have CONFIG_CPU_SUP_AMD=y then we can't even boot.

Heh, I didn't know ;)

OK, I think the patch is fine then.

Except... cough, the last nit, I promise ;)

Currently this doesn't matter, the only caller of modify_user_hw_breakpoint()
is ptrace, and it can't use bp_len != HW_BREAKPOINT_LEN_*.

But still, I think your patch needs a small fixlet,

	- /* info->len == info->mask == 0 */
	+ info->mask = 0;

Or we can do this later.

And while this is purely cosmetic (feel free to ignore), perhaps we
can join the bp_len checks and move cpu_has_bpext from _validate to
_build, this looks a little bit cleaner imho. IOW,


	info->mask == 0;

	switch (bp->attr.bp_len) {
	default:
		if (!is_power_of_2(bp->attr.bp_len))
			return -EINVAL;
		if (!cpu_has_bpext)
			return -EOPNOTSUPP;
		info->mask = bp->attr.bp_len - 1;
		/* fallthrough */
	case HW_BREAKPOINT_LEN_1:
		info->len = X86_BREAKPOINT_LEN_1;
		break;
	case HW_BREAKPOINT_LEN_2:
		info->len = X86_BREAKPOINT_LEN_2;
		break;
	case HW_BREAKPOINT_LEN_4:
		info->len = X86_BREAKPOINT_LEN_4;
		break;
#ifdef CONFIG_X86_64
	case HW_BREAKPOINT_LEN_8:
		info->len = X86_BREAKPOINT_LEN_8;
		break;
#endif
	}

Then arch_validate_hwbkpt_settings() only needs

		case X86_BREAKPOINT_LEN_1:
			align = 0;
	+	if (info->mask)
	+		align = mask;

change.

Oleg.

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

[Jacob Shin]

On Sat, Apr 27, 2013 at 05:05:10PM +0200, Oleg Nesterov wrote:
> On 04/26, Jacob Shin wrote:
> >
> > 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.
> 
> Imho, looks good.
> 
> Just one nit and one question below.
> 
> > @@ -163,29 +165,8 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
> >  	*dr7 &= ~__encode_dr7(i, info->len, info->type);
> ...
> > +	if (info->mask)
> > +		set_dr_addr_mask(0, i);
> 
> I agree we should clear addr_mask anyway.
> 
> But I am just curious, what if we do not? I mean what will the hardware
> do if this breakpoint was already disabled but the mask wasn't cleared?

Oh, it is fine if we don't and we are not using breakpoints, however I
was trying to account for per-thread events sharing the same DR
register, in that case we don't want previous event's mask value still
in the MSR.

So it was either clear on uninstall, or unconditionally set (even if
the new event's mask should be 0)

> 
> > @@ -314,11 +300,14 @@ 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) {
> > +			if (!cpu_has_bpext)
> > +				return -EOPNOTSUPP;
> > +			align = info->mask;
> 
> OK. But it seems we need a CONFIG_CPU_SUP_AMD-dependant helper for
> this cpu_has_bpext check? (like we have the nop set_dr_addr_mask()
> variant if !CONFIG_CPU_SUP_AMD).
> 
> Suppose that the kernel was compiled without CONFIG_CPU_SUP_AMD.
> Then perf_event_open(attr => { .bp_len == 16 }) will succeed, but
> this breakpoint won't actually work as expected?

Well, on non-AMD, even if we have CONFIG_CPU_SUP_AMD=y or not,
cpu_has_bpext (x86 CPUID check) will fail.

On AMD, if we don't have CONFIG_CPU_SUP_AMD=y then we can't even boot.
So I think ... its fine.


> 
> Oleg.
> 
> 

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

[Oleg Nesterov]

On 04/27, Oleg Nesterov wrote:
>
> Suppose that the kernel was compiled without CONFIG_CPU_SUP_AMD.
> Then perf_event_open(attr => { .bp_len == 16 }) will succeed,
                                                  ^^^^^^^^^^^^
sorry, I meant "can succeed" if CPU has X86_FEATURE_BPEXT.

Oleg.

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

[Oleg Nesterov]

On 04/26, Jacob Shin wrote:
>
> 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.

Imho, looks good.

Just one nit and one question below.

> @@ -163,29 +165,8 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
>  	*dr7 &= ~__encode_dr7(i, info->len, info->type);
...
> +	if (info->mask)
> +		set_dr_addr_mask(0, i);

I agree we should clear addr_mask anyway.

But I am just curious, what if we do not? I mean what will the hardware
do if this breakpoint was already disabled but the mask wasn't cleared?

> @@ -314,11 +300,14 @@ 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) {
> +			if (!cpu_has_bpext)
> +				return -EOPNOTSUPP;
> +			align = info->mask;

OK. But it seems we need a CONFIG_CPU_SUP_AMD-dependant helper for
this cpu_has_bpext check? (like we have the nop set_dr_addr_mask()
variant if !CONFIG_CPU_SUP_AMD).

Suppose that the kernel was compiled without CONFIG_CPU_SUP_AMD.
Then perf_event_open(attr => { .bp_len == 16 }) will succeed, but
this breakpoint won't actually work as expected?

Oleg.

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

[Jacob Shin]

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.shin@amd.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       |   47 +++++++++++++--------------------
 6 files changed, 49 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index ac10df7..bee6994 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -168,6 +168,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 */
 
 /*
@@ -317,6 +318,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 b575788..dac1d3b 100644
--- a/arch/x86/include/uapi/asm/msr-index.h
+++ b/arch/x86/include/uapi/asm/msr-index.h
@@ -199,6 +199,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 fa96eb0..2c3f12e 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -910,3 +910,22 @@ bool cpu_has_amd_erratum(const int *erratum)
 }
 
 EXPORT_SYMBOL_GPL(cpu_has_amd_erratum);
+
+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 02f0763..b306d0e 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -128,6 +128,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;
 }
@@ -163,29 +165,8 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 	*dr7 &= ~__encode_dr7(i, info->len, info->type);
 
 	set_debugreg(*dr7, 7);
-}
-
-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;
+	if (info->mask)
+		set_dr_addr_mask(0, i);
 }
 
 /*
@@ -198,7 +179,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);
 }
@@ -278,7 +259,8 @@ static int arch_build_bp_info(struct perf_event *bp)
 		return -EINVAL;
 	}
 
-	/* Len */
+	/* info->len == info->mask == 0 */
+
 	switch (bp->attr.bp_len) {
 	case HW_BREAKPOINT_LEN_1:
 		info->len = X86_BREAKPOINT_LEN_1;
@@ -295,11 +277,15 @@ 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;
+		info->mask = bp->attr.bp_len - 1;
+		info->len = X86_BREAKPOINT_LEN_1;
 	}
 
 	return 0;
 }
+
 /*
  * Validate the arch-specific HW Breakpoint register settings
  */
@@ -314,11 +300,14 @@ 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) {
+			if (!cpu_has_bpext)
+				return -EOPNOTSUPP;
+			align = info->mask;
+		}
 		break;
 	case X86_BREAKPOINT_LEN_2:
 		align = 1;
@@ -332,7 +321,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 		break;
 #endif
 	default:
-		return ret;
+		BUG();
 	}
 
 	/*
-- 
1.7.9.5