Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
On 12/10/2013 9:23 AM, Frederic Weisbecker wrote:
On Wed, Oct 02, 2013 at 11:11:06AM -0500,suravee.suthikulpa...@amd.com wrote:
From: 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
Signed-off-by: Jacob Shin
Signed-off-by: Suravee Suthikulpanit
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 0x8001, 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
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
On 12/10/2013 9:23 AM, Frederic Weisbecker wrote:
On Wed, Oct 02, 2013 at 11:11:06AM -0500,suravee.suthikulpa...@amd.com wrote:
From: Jacob Shinjacob.w.s...@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 Nesterovo...@redhat.com
Signed-off-by: Jacob Shinjacob.w.s...@gmail.com
Signed-off-by: Suravee Suthikulpanitsuravee.suthikulpa...@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 0x8001, 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.
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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.suthikulpa...@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.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
On 12/10, Frederic Weisbecker wrote:
>
> On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpa...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpa...@amd.com wrote:
> From: 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
>
> Signed-off-by: Jacob Shin
> Signed-off-by: Suravee Suthikulpanit
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_fpuboot_cpu_has(X86_FEATURE_EAGER_FPU)
> #define cpu_has_topoext boot_cpu_has(X86_FEATURE_TOPOEXT)
> +#define cpu_has_bpextboot_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_CTL0xc0010230
> #define MSR_F16H_L2I_PERF_CTR0xc0010231
> +#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_CTL0xc0010200
> 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,
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
On Wed, Dec 04, 2013 at 02:57:43PM +0100, Oleg Nesterov wrote: > On 12/03, Frederic Weisbecker wrote: > > > > 2013/11/11 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 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
On Wed, Dec 04, 2013 at 02:57:43PM +0100, Oleg Nesterov wrote: On 12/03, Frederic Weisbecker wrote: 2013/11/11 Oleg Nesterov o...@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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpa...@amd.com wrote:
From: Jacob Shin jacob.w.s...@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 o...@redhat.com
Signed-off-by: Jacob Shin jacob.w.s...@gmail.com
Signed-off-by: Suravee Suthikulpanit suravee.suthikulpa...@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_fpuboot_cpu_has(X86_FEATURE_EAGER_FPU)
#define cpu_has_topoext boot_cpu_has(X86_FEATURE_TOPOEXT)
+#define cpu_has_bpextboot_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_CTL0xc0010230
#define MSR_F16H_L2I_PERF_CTR0xc0010231
+#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_CTL0xc0010200
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);
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
On 12/10, Frederic Weisbecker wrote:
On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpa...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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.suthikulpa...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
On 12/03, Frederic Weisbecker wrote: > > 2013/11/11 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 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
On 12/03, Frederic Weisbecker wrote: 2013/11/11 Oleg Nesterov o...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
2013/11/11 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 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
2013/11/11 Oleg Nesterov o...@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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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.suthikulpa...@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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
On 11/08, Frederic Weisbecker wrote: > > On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpa...@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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
On 11/08, Frederic Weisbecker wrote: On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpa...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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.suthikulpa...@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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpa...@amd.com wrote: > From: 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 > > Signed-off-by: Jacob Shin > Signed-off-by: Suravee Suthikulpanit > --- > 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpa...@amd.com wrote: From: Jacob Shin jacob.w.s...@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 o...@redhat.com Signed-off-by: Jacob Shin jacob.w.s...@gmail.com Signed-off-by: Suravee Suthikulpanit suravee.suthikulpa...@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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
On 10/31, Frederic Weisbecker wrote:
>
> On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpa...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpa...@amd.com wrote:
> From: 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
>
> Signed-off-by: Jacob Shin
> Signed-off-by: Suravee Suthikulpanit
> ---
> 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_fpuboot_cpu_has(X86_FEATURE_EAGER_FPU)
> #define cpu_has_topoext boot_cpu_has(X86_FEATURE_TOPOEXT)
> +#define cpu_has_bpextboot_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_CTL0xc0010230
> #define MSR_F16H_L2I_PERF_CTR0xc0010231
> +#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_CTL0xc0010200
> 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
> +++
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpa...@amd.com wrote:
From: Jacob Shin jacob.w.s...@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 o...@redhat.com
Signed-off-by: Jacob Shin jacob.w.s...@gmail.com
Signed-off-by: Suravee Suthikulpanit suravee.suthikulpa...@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_fpuboot_cpu_has(X86_FEATURE_EAGER_FPU)
#define cpu_has_topoext boot_cpu_has(X86_FEATURE_TOPOEXT)
+#define cpu_has_bpextboot_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_CTL0xc0010230
#define MSR_F16H_L2I_PERF_CTR0xc0010231
+#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_CTL0xc0010200
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
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
On 10/31, Frederic Weisbecker wrote:
On Wed, Oct 02, 2013 at 11:11:06AM -0500, suravee.suthikulpa...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len HW_BREAKPOINT_LEN_8
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
