Oleg, Ingo, this is my final push for 3.10. I understand that it might
be too late for that .. if so I'll try again later for 3.11.
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
Will count writes to [0x1000 ~ 0x1010)
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 | 10 +++---
tools/perf/util/parse-events.h | 2 +-
tools/perf/util/parse-events.l | 1 +
tools/perf/util/parse-events.y | 24 +++++++++++--
12 files changed, 137 insertions(+), 40 deletions(-)
--
1.7.9.5
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 <[email protected]>
---
tools/perf/Documentation/perf-record.txt | 7 +++++--
tools/perf/util/parse-events.c | 10 ++++------
tools/perf/util/parse-events.h | 2 +-
tools/perf/util/parse-events.l | 1 +
tools/perf/util/parse-events.y | 24 ++++++++++++++++++++++--
5 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index d4da111..d05973b 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 6c8bb0f..100583e 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -508,7 +508,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;
@@ -518,12 +518,10 @@ 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 if (len)
+ attr.bp_len = len;
else
attr.bp_len = HW_BREAKPOINT_LEN_4;
@@ -1147,7 +1145,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 8a48593..8655b83 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -92,7 +92,7 @@ int parse_events_add_numeric(struct list_head **list, int *idx,
int parse_events_add_cache(struct list_head **list, int *idx,
char *type, char *op_result1, char *op_result2);
int parse_events_add_breakpoint(struct list_head **list, int *idx,
- void *ptr, char *type);
+ void *ptr, char *type, u64 len);
int parse_events_add_pmu(struct list_head **list, int *idx,
char *pmu , struct list_head *head_config);
void parse_events__set_leader(char *name, struct list_head *list);
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index e9d1134..15b3efc 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -191,6 +191,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 afc44c1..1751fb1 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -263,13 +263,33 @@ 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 = NULL;
+
+ 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 = NULL;
+
+ 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;
struct list_head *list = NULL;
ABORT_ON(parse_events_add_breakpoint(&list, &data->idx,
- (void *) $2, $4));
+ (void *) $2, $4, 0));
$$ = list;
}
|
@@ -279,7 +299,7 @@ PE_PREFIX_MEM PE_VALUE sep_dc
struct list_head *list = NULL;
ABORT_ON(parse_events_add_breakpoint(&list, &data->idx,
- (void *) $2, NULL));
+ (void *) $2, NULL, 0));
$$ = list;
}
--
1.7.9.5
Signed-off-by: Jacob Shin <[email protected]>
---
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 88e2f44..4fd0f96 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -971,6 +971,49 @@ static int test__group_gh4(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];
@@ -1187,6 +1230,18 @@ static struct evlist_test test__events[] = {
.name = "{cycles:G,cache-misses:H}:uG",
.check = test__group_gh4,
},
+ [38] = {
+ .name = "mem:0/1",
+ .check = test__checkevent_breakpoint_len,
+ },
+ [39] = {
+ .name = "mem:0/2:w",
+ .check = test__checkevent_breakpoint_len_w,
+ },
+ [40] = {
+ .name = "mem:0/4:rw:u",
+ .check = test__checkevent_breakpoint_len_rw_modifier,
+ },
};
static struct evlist_test test__events_pmu[] = {
--
1.7.9.5
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 <[email protected]>
Signed-off-by: Jacob Shin <[email protected]>
---
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
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.
>
>
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.
On 04/26, Jacob Shin wrote:
>
> @@ -518,12 +518,10 @@ 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 if (len)
> + attr.bp_len = len;
> else
> attr.bp_len = HW_BREAKPOINT_LEN_4;
Stupid question... So X86_FEATURE_BPEXT only works for r/w? I mean, it
doesn't allow to specify the mask for an execute breakpoint?
Otherwise the code above should be probably updated, say,
if (!len) {
len = attr.bp_type == HW_BREAKPOINT_X ?
sizeof(long) : HW_BREAKPOINT_LEN_4;
}
attr.bp_len = len;
Oleg.
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.
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.
On 04/27, Oleg Nesterov wrote:
>
> On 04/26, Jacob Shin wrote:
> >
> > @@ -518,12 +518,10 @@ 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 if (len)
> > + attr.bp_len = len;
> > else
> > attr.bp_len = HW_BREAKPOINT_LEN_4;
>
> Stupid question... So X86_FEATURE_BPEXT only works for r/w? I mean, it
> doesn't allow to specify the mask for an execute breakpoint?
>
> Otherwise the code above should be probably updated, say,
>
> if (!len) {
> len = attr.bp_type == HW_BREAKPOINT_X ?
> sizeof(long) : HW_BREAKPOINT_LEN_4;
> }
> attr.bp_len = len;
OK, we can't do this until we change arch_build_bp_info() which
insists on sizeof(long) and sets X86_BREAKPOINT_LEN_X == 1.
Please ignore...
Oleg.
On Sat, Apr 27, 2013 at 07:34:24PM +0200, Oleg Nesterov wrote:
> On 04/27, Oleg Nesterov wrote:
> >
> > On 04/26, Jacob Shin wrote:
> > >
> > > @@ -518,12 +518,10 @@ 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 if (len)
> > > + attr.bp_len = len;
> > > else
> > > attr.bp_len = HW_BREAKPOINT_LEN_4;
> >
> > Stupid question... So X86_FEATURE_BPEXT only works for r/w? I mean, it
> > doesn't allow to specify the mask for an execute breakpoint?
Yes, it does work with instruction execution breakpoints as well.
> >
> > Otherwise the code above should be probably updated, say,
> >
> > if (!len) {
> > len = attr.bp_type == HW_BREAKPOINT_X ?
> > sizeof(long) : HW_BREAKPOINT_LEN_4;
> > }
> > attr.bp_len = len;
>
> OK, we can't do this until we change arch_build_bp_info() which
> insists on sizeof(long) and sets X86_BREAKPOINT_LEN_X == 1.
Right, for instruction execution breakpoints, it is a x86 requirement
to have length as 1. Which makes sense, because you really want to be
trapping on 1 specifc IP.
Now using this new mask MSR, we should be able to trap on range of IPs,
but yeah since we are naturally extending bp_len .... we'll tackle it
later when we can come up with a good scheme to express masks in arch
neutral/general way.
On 04/27/2013 09:58 AM, Oleg Nesterov wrote:
>
> Stupid question... So X86_FEATURE_BPEXT only works for r/w? I mean, it
> doesn't allow to specify the mask for an execute breakpoint?
>
x86 execute breakpoints in general are only a single byte, which has to
be the first byte of the instruction.
-hpa
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
On 04/27, H. Peter Anvin wrote:
>
> On 04/27/2013 09:58 AM, Oleg Nesterov wrote:
> >
> > Stupid question... So X86_FEATURE_BPEXT only works for r/w? I mean, it
> > doesn't allow to specify the mask for an execute breakpoint?
>
> x86 execute breakpoints in general are only a single byte, which has to
> be the first byte of the instruction.
OK, thanks, but this new X86_FEATURE_BPEXT allows to specify the range
even for HW_BREAKPOINT_X... But lets ignore this series for the moment.
If execute breakpoints are only a single byte, then why
arch_build_bp_info() requires ->bp_len = sizeof(long) but not 1?
And note that it sets info->len = X86_BREAKPOINT_LEN_X. The comment says
x86 inst breakpoints need to have a specific undefined len
but despite its "special" name LEN_X is simply LEN_1, and other code
relies on this fact.
And, otoh, ptrace requires DR_LEN_1. Then arch_bp_generic_fields()
translates this into "gen_len = sizeof(long)" for validate. Which
is translated to LEN_1 later.
This looks confusing, imho. And imho X86_BREAKPOINT_LEN_X should die...
But I guess we can't change arch_build_bp_info() to require bp_len = 1,
this can break userspace...
And it is not clear to me how we can change this code to support a
range for the execute breakpoints, perhaps something like below.
Oleg.
--- x/arch/x86/kernel/hw_breakpoint.c
+++ x/arch/x86/kernel/hw_breakpoint.c
@@ -270,10 +270,11 @@ static int arch_build_bp_info(struct per
* But we still need to check userspace is not trying to setup
* an unsupported length, to get a range breakpoint for example.
*/
- if (bp->attr.bp_len == sizeof(long)) {
- info->len = X86_BREAKPOINT_LEN_X;
- return 0;
- }
+ if (bp->attr.bp_len == sizeof(long))
+ bp->attr.bp_len = HW_BREAKPOINT_LEN_1;
+ else if (!cpu_has_bpext)
+ return -EINVAL;
+ break;
default:
return -EINVAL;
}