Sorry I've been slow in responding to your most recent version.
I fell into a large hole and couldn't get out until I fixed some bugs.
I don't really know what more to say. I like things neatly separated too,
most especially between clearly meaningful and misleading yet apparently
meaningful. Overloading unrelated hardware bits for no material purpose
will never make sense to me.
I checked the ioperm code. As far as I can tell, if you have CAP_SYS_RAWIO
then you can use ioperm to enable any port you want, so this will always be
possible. (That seems a bit nutty to me, hence my earlier reaction.)
But the callback is not per-CPU, it is per-registration. When a new
registration displaces an old one, or a deregistration stops displacing
one, isn't the status change in the data structure and the callback made
right away, by the thread doing the registration/deregistration call?
Another CPU with interrupts disabled won't have its breakpoints actually
change, but the data structure will have changed. Isn't that right?
I think that's perfectly fine.
I think this is a mistake. On other machines, there is no need for more
than one field and .len will go unused. There is no reason not to encode
ahead of time. If it's useful for callers to be able to extract the type
and length from a struct hw_breakpoint, it's easy to define macros or
inlines in asm/hw_breakpoint.h that go the other way.
There's no reason __modify_user_hw_breakpoint can't use an encoded value.
modify_user_hw_breakpoint can do checking and encoding before taking the lock.
It doesn't hurt to add some constant bits to the API values that you just
mask out (after checking) as part of the encoding convsersion. In fact,
it's a good way to make sure people are using the right macros.
I object to this. The purpose of having macros is to give a constrained
set of values that can be used at compile time. Letting people use literal
numbers is just asking for people to write their calls unportably. For
machines that support only LEN8, I'd planned to define it to some magic
bit pattern just so it could be checked and rule out cavalier users who
don't pay attention to the macros.
task->thread.trap_no and task->thread.error_code usually store the values
from the hardware trap frame when a trap is turned into a signal (e.g. see
do_trap). This is the hardware trap number, which is 1 for do_debug. The
error code is a word of the hardware trap frame whose meaning is specified
differently for each trap number. For debug traps, it's always zero.
For other kinds of traps, it is sometimes useful to have the value.
My review comments below are about your patch of 3/22 (described as
"actually works with 2.6.21-rc4").
Put it back using notify_die, just pass dr6 instead of error_code.
Don't mess with these names if there isn't a good reason.
It's unrelated to the work you're doing.
Put all this inside #ifdef __KERNEL__.
This should be more explicit about it being machine-dependent what subset
of the macros will be defined.
We need to say something about the restart behavior. i.e., figure out the
situation with the x86 RF flag and what the story is on other machines that
have instruction breakpoint registers.
I've said before and still maintain that there should be the option to have
a NULL installed callback and fail immediately if it can't be installed
right now. The wording below about installed being NULL is not clear on
what this means.
Zero is right.
Yes, I don't think you'll ever progress if you haven't set it. However,
setting it opens a can of worms. Once you set it, the bit could leak into
a signal context, or be seen via ptrace, or stay set when ptrace changes
the pc, etc. It requires some investigation.
What's the question? I don't think hw_breakpoint has anything to do with
TF or single-step at all.
Using register_cpu_notifier in an __init function seems to be what
everything else does, or you can hack __cpu_up directly. For kexec,
clearing dr7 in machine_kexec seems like a good idea.
Though you've split the header file into generic and i386, I see this is
all still in the i386 file. I'd like to see the shareable code (list
handling, priority stuff, multi-CPU coordination) in a common file. For
x86_64, all the machine-specific code is all but identical (literally just
one bit different), so it might as well actually share the i386 source
file. But I'm keen to do the powerpc64 machine-dependent bits as soon as
your generic code is making it easy enough for me. :-)
I think the ia64 version will be straightforward too, though I won't do
that one myself. A notable difference on these processors (and maybe all
other ones that have breakpoint registers) is that execute breakpoints and
data breakpoints are separate disjoint resources. There may be zero
execute breakpoint registers or a few, and may be one data breakpoint
register or a few, but there is no single HB_NUM of how many breakpoint
slots for any kind whatever.
Surely this should be <asm/percpu.h>.
I have a feeling this is more costly than we want, though I don't really
know. It seems to me that things in struct cpu_hw_breakpoint are not
really per-CPU, except for bp_task. They are "current global state",
right? So I think it fits well to have the per_cpu struct contain just
bp_task and a pointer to the current state struct, and use RCU to replace
that struct when registrations change. Perhaps rather than actually
updating other CPU's pointers in the RCU fashion, that would just be done
by each CPU for itself from the IPI handler that changes the hardware.
Still, it just requires rcu_read_lock+rcu_dereference at context switch.
Doesn't this unnecessarily do all four settings in a thread that only uses
one? The old code did that too, but now it seems easy enough to cache the
number of active thread breakpoints and switch on that.
I'd say it's worth saving the loads here to recompute tdr7 with the
kernel-used bits masked out and cache it that way, so the fast path
looks at only thbi->tdr7|chbi->kdr7.
It does not seem necessary to cache the dr7 value. Save the store.
set_debugreg(chbi->kdr7 | thbi->tdr7, 7);
The only use of the saved value is in hw_breakpoint_handler,
which can juse use chbi->kdr7 | thbi->tdr7.
Thanks,
Roland
-
