That's okay; the same thing happens to everyone from time to time.
Well, I can change it easily enough.
At this stage it's a moot point anyway.
Not quite right. The status change in the data structure isn't made until
after all the IPIs have completed, which won't happen until all the CPUs
have responded. A CPU with interrupts disabled will therefore delay the
status change as well as the debug-register update, and so the value of
bp->status would remain valid until interrupts were enabled.
I'm not entirely convinced.
Consider first that the computation involved in encoding .bits is just a
little more complicated than one would like to do at compile time. (At
least, it is on x86_64.) Callers would have to do it themselves
dynamically, or else pass len and type as arguments to the
register_hw_breakpoint routines (which would mean an unused argument on
those other machines). In general it just makes things harder on callers.
The fact the .len would go unused on some architectures shouldn't matter.
It's just a u8; .len and .type together take up no more space than .bits
would.
However, if you insist I can still change things over.
Come to think of it, we don't really need modify_user_hw_breakpoint at
all. It could be replaced by an {unregister(old); register(new);}
sequence. Unless you think there's some pressing reason to keep it, my
inclination is to do away with it.
I'd like to. But with notify_die, the DR6 value whose address is passed
to the notifier chain (args.err) is local to an inlined function.
Modifications to that value made by the callout routines on the chain
would be lost when notify_die returns.
Hmm... Maybe I could store a pointer to the DR6 value in args.err instead
of the value itself...
I was about to disagree, but a second look at the Intel documentation
shows that those bits don't have any real name (other than LE and GE,
which is why I changed the macros).
As I understand it, setting one of those bits is necessary on the 386 but
not necessary for later processors. Should this be controlled by a
runtime (or compile time) check? For that matter, do those bits have any
effect at all on a Pentium?
My Intel manual says that the CPU automatically sets the RF bit in the
EFLAGS image stored on the stack by the debug exception. Hence the
handler doesn't have to worry about it. That's why I removed it from the
existing code.
Setting a callback pointer to NULL generally means that you don't want or
care about callbacks. Trying to make it mean something else will only
confuse people.
If callers want to give up when a kernel breakpoint isn't installed
immediately, all they have to do is check the return value from
register_kernel_hw_breakpoint and call unregister_kernel_hw_breakpoint.
If you really want it, I could add an extra "fail if not installed"
argument flag.
For user breakpoints, the whole notion is almost meaningless. Even if the
breakpoint was allocated a debug register initially, it could get
displaced by the time the debuggee task next runs.
I mentioned that question because I had removed the existing code, which
didn't seem to be necessary. I wanted to make sure this was the correct
thing to do.
Again, this was referring to existing code which I basically copied
without fully understanding. Does the new code in do_debug do the right
thing with regard to TF?
I'll go through the file and see which parts really can be shared. It
might end up being less than you think.
Note that doing this would necessarily create a bunch of new public
symbols. Routines that I now have declared static wouldn't be able to
remain that way.
Not really, since changes to the debug registers on multiple CPUs cannot
be made simultaneously. There will be short periods when different CPUs
have different debug register values. What if a debug exception occurs
during one of those periods? Or what if a task switch occurs?
See how you like the new implementation in the next version (when it's
ready). The local_irq_save can be avoided by extending the size of the
RCU critical section.
Doing it that way would require all the thbi->tdr7 values in all tasks
being debugged to be updated whenever the kdr7_mask value changes. It's
not impossible, but it could have a high cost in terms of cacheline
contention on SMP systems. Maybe you don't think that's a big issue.
For that matter, as long as you're going to update thbi->tdr7 with the
kernel bits masked out, why not go ahead and mask in the kernel's kdr7
bits as well? Then no computation would be needed during task switches
at all.
Alan Stern
-
