Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8
From: Suravee Suthikulanit
Date: Wed Dec 11 2013 - 07:05:23 EST
On 12/10/2013 9:23 AM, Frederic Weisbecker wrote:
On Wed, Oct 02, 2013 at 11:11:06AM -0500,suravee.suthikulpanit@xxxxxxx wrote:
From: Jacob Shin<jacob.w.shin@xxxxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: Jacob Shin<jacob.w.shin@xxxxxxxxx>
Signed-off-by: Suravee Suthikulpanit<suravee.suthikulpanit@xxxxxxx>
Thinking further about this, Oleg convinced me that we can keep the mask eventually.
It actually makes sense internally just for the use of bpext.
But I still have a few comments on this patchset:
---
arch/x86/include/asm/cpufeature.h | 2 ++
arch/x86/include/asm/debugreg.h | 5 ++++
arch/x86/include/asm/hw_breakpoint.h | 1 +
arch/x86/include/uapi/asm/msr-index.h | 4 +++
arch/x86/kernel/cpu/amd.c | 19 ++++++++++++++
arch/x86/kernel/hw_breakpoint.c | 47 ++++++++++++++---------------------
6 files changed, 49 insertions(+), 29 deletions(-)
diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index d3f5c63..26609bb 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -170,6 +170,7 @@
#define X86_FEATURE_TOPOEXT (6*32+22) /* topology extensions CPUID leafs */
#define X86_FEATURE_PERFCTR_CORE (6*32+23) /* core performance counter extensions */
#define X86_FEATURE_PERFCTR_NB (6*32+24) /* NB performance counter extensions */
+#define X86_FEATURE_BPEXT (6*32+26) /* data breakpoint extension */
Please comment that it's AMD specific.
This #define locates in the AMD-specific section within the code,
which already has a comment at the beginning of the section saying
"/* More extended AMD flags: CPUID level 0x80000001, ecx, word 6 */".
#define X86_FEATURE_PERFCTR_L2 (6*32+28) /* L2 performance counter extensions */
/*
@@ -332,6 +333,7 @@ extern const char * const x86_power_flags[32];
#define cpu_has_cx16 boot_cpu_has(X86_FEATURE_CX16)
#define cpu_has_eager_fpu boot_cpu_has(X86_FEATURE_EAGER_FPU)
#define cpu_has_topoext boot_cpu_has(X86_FEATURE_TOPOEXT)
+#define cpu_has_bpext boot_cpu_has(X86_FEATURE_BPEXT)
#ifdef CONFIG_X86_64
diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
index 4b528a9..145b009 100644
--- a/arch/x86/include/asm/debugreg.h
+++ b/arch/x86/include/asm/debugreg.h
@@ -114,5 +114,10 @@ static inline void debug_stack_usage_inc(void) { }
static inline void debug_stack_usage_dec(void) { }
#endif /* X86_64 */
+#ifdef CONFIG_CPU_SUP_AMD
+extern void set_dr_addr_mask(unsigned long mask, int dr);
+#else
+static inline void set_dr_addr_mask(unsigned long mask, int dr) { }
+#endif
#endif /* _ASM_X86_DEBUGREG_H */
diff --git a/arch/x86/include/asm/hw_breakpoint.h b/arch/x86/include/asm/hw_breakpoint.h
index ef1c4d2..6c98be8 100644
--- a/arch/x86/include/asm/hw_breakpoint.h
+++ b/arch/x86/include/asm/hw_breakpoint.h
@@ -12,6 +12,7 @@
*/
struct arch_hw_breakpoint {
unsigned long address;
+ unsigned long mask;
u8 len;
u8 type;
};
diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
index bb04650..1f04f6c 100644
--- a/arch/x86/include/uapi/asm/msr-index.h
+++ b/arch/x86/include/uapi/asm/msr-index.h
@@ -205,6 +205,10 @@
/* Fam 16h MSRs */
#define MSR_F16H_L2I_PERF_CTL 0xc0010230
#define MSR_F16H_L2I_PERF_CTR 0xc0010231
+#define MSR_F16H_DR1_ADDR_MASK 0xc0011019
+#define MSR_F16H_DR2_ADDR_MASK 0xc001101a
+#define MSR_F16H_DR3_ADDR_MASK 0xc001101b
+#define MSR_F16H_DR0_ADDR_MASK 0xc0011027
/* Fam 15h MSRs */
#define MSR_F15H_PERF_CTL 0xc0010200
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 903a264..fffc9cb 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -909,3 +909,22 @@ static bool cpu_has_amd_erratum(struct cpuinfo_x86 *cpu, const int *erratum)
return false;
}
+
+void set_dr_addr_mask(unsigned long mask, int dr)
+{
+ if (!cpu_has_bpext)
+ return;
+
+ switch (dr) {
+ case 0:
+ wrmsr(MSR_F16H_DR0_ADDR_MASK, mask, 0);
+ break;
+ case 1:
+ case 2:
+ case 3:
+ wrmsr(MSR_F16H_DR1_ADDR_MASK - 1 + dr, mask, 0);
+ break;
+ default:
How about just:
if (dr <= 4 && dr >= 0)
wrmsr(MSR_F16H_DR1_ADDR_MASK + dr - 1, mask, 0)
That won't work since the DR0 (MSR 0xc0011027)is at not contiguous
with DR1-3 (MSR 0xc0011019 - 0xc001101b). If you prefer if/else, we
can do that as well, but I think it might be as cumbersome in the code.
+ break;
+ }
+}
diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index f66ff16..3cb1951 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -128,6 +128,8 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
*dr7 |= encode_dr7(i, info->len, info->type);
set_debugreg(*dr7, 7);
+ if (info->mask)
+ set_dr_addr_mask(info->mask, i);
return 0;
}
@@ -163,29 +165,8 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
*dr7 &= ~__encode_dr7(i, info->len, info->type);
set_debugreg(*dr7, 7);
-}
-
-static int get_hbp_len(u8 hbp_len)
-{
- unsigned int len_in_bytes = 0;
-
- switch (hbp_len) {
- case X86_BREAKPOINT_LEN_1:
- len_in_bytes = 1;
- break;
- case X86_BREAKPOINT_LEN_2:
- len_in_bytes = 2;
- break;
- case X86_BREAKPOINT_LEN_4:
- len_in_bytes = 4;
- break;
-#ifdef CONFIG_X86_64
- case X86_BREAKPOINT_LEN_8:
- len_in_bytes = 8;
- break;
-#endif
- }
- return len_in_bytes;
+ if (info->mask)
+ set_dr_addr_mask(0, i);
}
/*
@@ -198,7 +179,7 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
struct arch_hw_breakpoint *info = counter_arch_bp(bp);
va = info->address;
- len = get_hbp_len(info->len);
+ len = bp->attr.bp_len;
So we can access the attr directly instead of get_hbp_len(). Good, that's a welcome
simplification but it's a change that is meaningful and tricky enough to deserve a seperate patch.
Could you send a seperate one for that?
Sure, I can split the patch.
return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
}
@@ -279,7 +260,16 @@ static int arch_build_bp_info(struct perf_event *bp)
}
/* Len */
+ info->mask = 0;
+
switch (bp->attr.bp_len) {
+ default:
+ if (!is_power_of_2(bp->attr.bp_len))
+ return -EINVAL;
+ if (!cpu_has_bpext)
+ return -EOPNOTSUPP;
+ info->mask = bp->attr.bp_len - 1;
+ /* fall through */
So, that's perhaps just personal preference but having the default on
top of the switch makes things very confusing. Can't we put the above
in the end of the switch instead?
Ok. I'll clean this up a bit to your preference.
case HW_BREAKPOINT_LEN_1:
info->len = X86_BREAKPOINT_LEN_1;
break;
@@ -294,12 +284,11 @@ static int arch_build_bp_info(struct perf_event *bp)
info->len = X86_BREAKPOINT_LEN_8;
break;
#endif
- default:
- return -EINVAL;
}
return 0;
}
+
/*
* Validate the arch-specific HW Breakpoint register settings
*/
@@ -314,11 +303,11 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
if (ret)
return ret;
- ret = -EINVAL;
-
switch (info->len) {
case X86_BREAKPOINT_LEN_1:
align = 0;
+ if (info->mask)
+ align = info->mask;
Confused, I thought mask is set only when length is above 8?
break;
case X86_BREAKPOINT_LEN_2:
align = 1;
@@ -332,7 +321,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
break;
#endif
default:
- return ret;
+ BUG();
Please use WARN_ON_ONCE() instead.
Thanks.
OK.
I'll send out soon.
Suravee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/