> I really don't understand your point here. What's wrong with bp_show?
That part is fine. The problem is fetching the hw_breakpoint.len field
directly and expecting it to contain the API values. In an implementation
done as I've been referring to, there is no need for any field to contain
the HW_BREAKPOINT_LEN_8 value, and it's a waste to store one. If it were
hw_breakpoint_get_len(bp), that would be fine.
Indeed, that is the natural thing (and all the bits needed) on several.
I hadn't raised this before since I was having so much trouble already
convincing you about storing things in machine-dependent fashion so that
users cannot just use the struct fields directly.
I really think it would be cleanest all around to use just:
struct arch_hw_breakpoint info;
in place of address union, len, type in struct hw_breakpoint. Then each
arch provides hw_breakpoint_get_{kaddr,uaddr,len,type} inlines. For
storing, each arch can define hw_breakpoint_init(addr, len, type) (or
maybe k/u variants). This can be used by callers directly if you want to
keep register_hw_breakpoint to one argument, or could just be internal if
register_hw_breakpoint takes the three more args. If callers use it
directly, there can also be an INIT_ARCH_HW_BREAKPOINT(addr, len, type)
for use in struct hw_breakpoint_init initializers.
On x86 use:
struct arch_hw_breakpoint_info {
union {
const void *kernel;
const void __user *user;
unsigned long va;
} address;
u8 len;
u8 type;
} __attribute__((packed));
and the size of struct hw_breakpoint won't increase.
It's easy for user mode with gdb. kprobes is simple to use, and it
always does a single-step to execute (a copy of) the instruction that
was overwritten with the breakpoint. So, write a module that does:
int testvar=0;
asm(".globl testme; testme: movl $17,testvar; ret");
void testme();
testinit() {
... register kprobe at &testme ...
... register hw_breakpoint at &testvar ...
testme()
}
Your kprobe handlers don't have to actually do anything at all, if you
are just hacking the low-level code so see what %dr6 values you get at
each trap.
I still think it's the proper thing to make it conditional, not always
built in. But it's a pedantic point.
Indeed I think we have come nearly as far as we will before we have a few
arch ports get done and some heavy use to find the rough edges. Thanks
very much for being so accomodating to all my criticism, which I hope has
been constructive.
These need to be static inline. Here you're defining a global function
in every .o file that uses the header.
Some comment here about this conditional clearing, please.
Need #ifdef __KERNEL__ around all these additions to debugreg.h.
For local functions in a source file (not a header), it's standard form
now just to define them static, not static inline. For these trivial
ones, the compiler will always inline them.
Thanks,
Roland
-
