Messages in this thread Patch in this message | | | Date | Tue, 10 Dec 2013 16:23:44 +0100 | From | Frederic Weisbecker <> | Subject | 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.suthikulpanit@amd.com wrote: > From: Jacob Shin <jacob.w.shin@gmail.com> > > Implement hardware breakpoint address mask for AMD Family 16h and > above processors. CPUID feature bit indicates hardware support for > DRn_ADDR_MASK MSRs. These masks further qualify DRn/DR7 hardware > breakpoint addresses to allow matching of larger addresses ranges. > > Valuable advice and pseudo code from Oleg Nesterov <oleg@redhat.com> > > Signed-off-by: Jacob Shin <jacob.w.shin@gmail.com> > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Thinking further about this, Oleg convinced me that we can keep the mask eventually. It actually makes sense internally just for the use of bpext.
But I still have a few comments on this patchset:
> --- > arch/x86/include/asm/cpufeature.h | 2 ++ > arch/x86/include/asm/debugreg.h | 5 ++++ > arch/x86/include/asm/hw_breakpoint.h | 1 + > arch/x86/include/uapi/asm/msr-index.h | 4 +++ > arch/x86/kernel/cpu/amd.c | 19 ++++++++++++++ > arch/x86/kernel/hw_breakpoint.c | 47 ++++++++++++++--------------------- > 6 files changed, 49 insertions(+), 29 deletions(-) > > diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h > index d3f5c63..26609bb 100644 > --- a/arch/x86/include/asm/cpufeature.h > +++ b/arch/x86/include/asm/cpufeature.h > @@ -170,6 +170,7 @@ > #define X86_FEATURE_TOPOEXT (6*32+22) /* topology extensions CPUID leafs */ > #define X86_FEATURE_PERFCTR_CORE (6*32+23) /* core performance counter extensions */ > #define X86_FEATURE_PERFCTR_NB (6*32+24) /* NB performance counter extensions */ > +#define X86_FEATURE_BPEXT (6*32+26) /* data breakpoint extension */
Please comment that it's AMD specific.
> #define X86_FEATURE_PERFCTR_L2 (6*32+28) /* L2 performance counter extensions */ > > /* > @@ -332,6 +333,7 @@ extern const char * const x86_power_flags[32]; > #define cpu_has_cx16 boot_cpu_has(X86_FEATURE_CX16) > #define cpu_has_eager_fpu boot_cpu_has(X86_FEATURE_EAGER_FPU) > #define cpu_has_topoext boot_cpu_has(X86_FEATURE_TOPOEXT) > +#define cpu_has_bpext boot_cpu_has(X86_FEATURE_BPEXT) > > #ifdef CONFIG_X86_64 > > diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h > index 4b528a9..145b009 100644 > --- a/arch/x86/include/asm/debugreg.h > +++ b/arch/x86/include/asm/debugreg.h > @@ -114,5 +114,10 @@ static inline void debug_stack_usage_inc(void) { } > static inline void debug_stack_usage_dec(void) { } > #endif /* X86_64 */ > > +#ifdef CONFIG_CPU_SUP_AMD > +extern void set_dr_addr_mask(unsigned long mask, int dr); > +#else > +static inline void set_dr_addr_mask(unsigned long mask, int dr) { } > +#endif > > #endif /* _ASM_X86_DEBUGREG_H */ > diff --git a/arch/x86/include/asm/hw_breakpoint.h b/arch/x86/include/asm/hw_breakpoint.h > index ef1c4d2..6c98be8 100644 > --- a/arch/x86/include/asm/hw_breakpoint.h > +++ b/arch/x86/include/asm/hw_breakpoint.h > @@ -12,6 +12,7 @@ > */ > struct arch_hw_breakpoint { > unsigned long address; > + unsigned long mask; > u8 len; > u8 type; > }; > diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h > index bb04650..1f04f6c 100644 > --- a/arch/x86/include/uapi/asm/msr-index.h > +++ b/arch/x86/include/uapi/asm/msr-index.h > @@ -205,6 +205,10 @@ > /* Fam 16h MSRs */ > #define MSR_F16H_L2I_PERF_CTL 0xc0010230 > #define MSR_F16H_L2I_PERF_CTR 0xc0010231 > +#define MSR_F16H_DR1_ADDR_MASK 0xc0011019 > +#define MSR_F16H_DR2_ADDR_MASK 0xc001101a > +#define MSR_F16H_DR3_ADDR_MASK 0xc001101b > +#define MSR_F16H_DR0_ADDR_MASK 0xc0011027 > > /* Fam 15h MSRs */ > #define MSR_F15H_PERF_CTL 0xc0010200 > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c > index 903a264..fffc9cb 100644 > --- a/arch/x86/kernel/cpu/amd.c > +++ b/arch/x86/kernel/cpu/amd.c > @@ -909,3 +909,22 @@ static bool cpu_has_amd_erratum(struct cpuinfo_x86 *cpu, const int *erratum) > > return false; > } > + > +void set_dr_addr_mask(unsigned long mask, int dr) > +{ > + if (!cpu_has_bpext) > + return; > + > + switch (dr) { > + case 0: > + wrmsr(MSR_F16H_DR0_ADDR_MASK, mask, 0); > + break; > + case 1: > + case 2: > + case 3: > + wrmsr(MSR_F16H_DR1_ADDR_MASK - 1 + dr, mask, 0); > + break; > + default:
How about just:
if (dr <= 4 && dr >= 0) wrmsr(MSR_F16H_DR1_ADDR_MASK + dr - 1, mask, 0)
> + break; > + } > +} > diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c > index f66ff16..3cb1951 100644 > --- a/arch/x86/kernel/hw_breakpoint.c > +++ b/arch/x86/kernel/hw_breakpoint.c > @@ -128,6 +128,8 @@ int arch_install_hw_breakpoint(struct perf_event *bp) > *dr7 |= encode_dr7(i, info->len, info->type); > > set_debugreg(*dr7, 7); > + if (info->mask) > + set_dr_addr_mask(info->mask, i); > > return 0; > } > @@ -163,29 +165,8 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp) > *dr7 &= ~__encode_dr7(i, info->len, info->type); > > set_debugreg(*dr7, 7); > -} > - > -static int get_hbp_len(u8 hbp_len) > -{ > - unsigned int len_in_bytes = 0; > - > - switch (hbp_len) { > - case X86_BREAKPOINT_LEN_1: > - len_in_bytes = 1; > - break; > - case X86_BREAKPOINT_LEN_2: > - len_in_bytes = 2; > - break; > - case X86_BREAKPOINT_LEN_4: > - len_in_bytes = 4; > - break; > -#ifdef CONFIG_X86_64 > - case X86_BREAKPOINT_LEN_8: > - len_in_bytes = 8; > - break; > -#endif > - } > - return len_in_bytes; > + if (info->mask) > + set_dr_addr_mask(0, i); > } > > /* > @@ -198,7 +179,7 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp) > struct arch_hw_breakpoint *info = counter_arch_bp(bp); > > va = info->address; > - len = get_hbp_len(info->len); > + len = bp->attr.bp_len;
So we can access the attr directly instead of get_hbp_len(). Good, that's a welcome simplification but it's a change that is meaningful and tricky enough to deserve a seperate patch.
Could you send a seperate one for that?
> > return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE); > } > @@ -279,7 +260,16 @@ static int arch_build_bp_info(struct perf_event *bp) > } > > /* Len */ > + info->mask = 0; > + > switch (bp->attr.bp_len) { > + default: > + if (!is_power_of_2(bp->attr.bp_len)) > + return -EINVAL; > + if (!cpu_has_bpext) > + return -EOPNOTSUPP; > + info->mask = bp->attr.bp_len - 1; > + /* fall through */
So, that's perhaps just personal preference but having the default on top of the switch makes things very confusing. Can't we put the above in the end of the switch instead?
> case HW_BREAKPOINT_LEN_1: > info->len = X86_BREAKPOINT_LEN_1; > break; > @@ -294,12 +284,11 @@ static int arch_build_bp_info(struct perf_event *bp) > info->len = X86_BREAKPOINT_LEN_8; > break; > #endif > - default: > - return -EINVAL; > } > > return 0; > } > + > /* > * Validate the arch-specific HW Breakpoint register settings > */ > @@ -314,11 +303,11 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp) > if (ret) > return ret; > > - ret = -EINVAL; > - > switch (info->len) { > case X86_BREAKPOINT_LEN_1: > align = 0; > + if (info->mask) > + align = info->mask;
Confused, I thought mask is set only when length is above 8?
> break; > case X86_BREAKPOINT_LEN_2: > align = 1; > @@ -332,7 +321,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp) > break; > #endif > default: > - return ret; > + BUG();
Please use WARN_ON_ONCE() instead.
Thanks.
> } > > /* > -- > 1.8.1.2 > >
| |