On 04/25, Oleg Nesterov wrote:
>
> On 04/25, Frederic Weisbecker wrote:
> >
> > 2013/4/23 Jacob Shin <jacob.s...@amd.com>:
> > > @@ -286,7 +286,10 @@ struct perf_event_attr {
> > > __u64 config1; /* extension of config */
> > > };
> > > union {
> > > - __u64 bp_len;
> > > + struct {
> > > + __u32 bp_len;
> > > + __u32 bp_addr_mask;
> > > + };
> >
> > Do we need len and mask to work at the same time? I can't think of a
> > situation when len and mask mix up together in a useful way to define
> > a range.
>
> And it would be nice (I think) if we could simply turn bp_len into
> bp_mask. It is already the mask actually, bp_addr should be aligned.
>
> But I do not see how we can do this, so I guess we need another field.
>
> Well. Another option is to extend bp_len. Fortunately HW_BREAKPOINT_LEN_*
> match the length, so we can simply allow any 2^n length and amd.c can
> translate it into the mask.
>
> Of course, this doesn't allow to use, say, mask=0xF0. But perhaps this
> is not really useful?
>
> I dunno. I leave this to you and Jacob ;)
IOW, I meant something like the incomplete patch below. But let me
repeat I am not sure this is the good idea.
Hmm. And note the change arch_check_bp_in_kernelspace(). Whatever we
do it should be updated if we have a mask...
And this reminds me that arch_check_bp_in_kernelspace() is buggy but
my trivial fix was ignored ;) see http://marc.info/?t=135248593000001
Oleg.
--- x/arch/x86/include/asm/hw_breakpoint.h
+++ x/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;
};
--- x/arch/x86/kernel/hw_breakpoint.c
+++ x/arch/x86/kernel/hw_breakpoint.c
@@ -128,6 +128,8 @@ int arch_install_hw_breakpoint(struct pe
*dr7 |= encode_dr7(i, info->len, info->type);
set_debugreg(*dr7, 7);
+ if (info->mask)
+ set_dr_addr_mask(...);
return 0;
}
@@ -165,29 +167,6 @@ void arch_uninstall_hw_breakpoint(struct
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;
-}
-
/*
* Check for virtual address in kernel space.
*/
@@ -198,7 +177,7 @@ int arch_check_bp_in_kernelspace(struct
struct arch_hw_breakpoint *info = counter_arch_bp(bp);
va = info->address;
- len = get_hbp_len(info->len);
+ len = bp->attr.bp_len;
return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
}
@@ -278,7 +257,8 @@ static int arch_build_bp_info(struct per
return -EINVAL;
}
- /* Len */
+ /* info->len == info->mask == 0 */
+
switch (bp->attr.bp_len) {
case HW_BREAKPOINT_LEN_1:
info->len = X86_BREAKPOINT_LEN_1;
@@ -295,7 +275,10 @@ static int arch_build_bp_info(struct per
break;
#endif
default:
- return -EINVAL;
+ if (!is_power_of_2(bp->attr.bp_len))
+ return -EINVAL;
+ info->mask = bp->attr.bp_len - 1;
+ info->len = X86_BREAKPOINT_LEN_1;
}
return 0;
@@ -314,11 +297,15 @@ int arch_validate_hwbkpt_settings(struct
if (ret)
return ret;
- ret = -EINVAL;
-
switch (info->len) {
case X86_BREAKPOINT_LEN_1:
align = 0;
+ if (info->mask) {
+ align = info->mask;
+ ret = arch_validate_hwbkpt_addr_mask(...);
+ if (ret)
+ return ret;
+ }
break;
case X86_BREAKPOINT_LEN_2:
align = 1;
@@ -332,7 +319,7 @@ int arch_validate_hwbkpt_settings(struct
break;
#endif
default:
- return ret;
+ BUG();
}
/*
--
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/
