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