On 28/02/16(Sun) 17:02, Mike Belopuhov wrote:
> On 28 February 2016 at 14:38, Martin Pieuchot <[email protected]> wrote:
> > In order to dynamically instrument kernel functions, I plan to add
> > breakpoints where a probe needs to be executed. Trap handlers will
> > be modified to check if the address of the trapping instruction
> > correspond to a registered probe, and if that's the case, the kernel
> > will execute the associated code.
> >
> > While implementing such mechanism for amd64 and i386, I discovered
> > some differences in the existing code that only history seems to justify.
> >
> > So the diff below get rids of the BPTTRAP() and other small differences
> > around 'calltrap' for these architectures. In particular interrupts
> > are now enabled unconditionally on i386 before entering trap(). Any
> > counter indication for doing so?
> >
>
> That 'sti' that you're adding might have side effects, but I can't think of
> any
> bad ones. You'll get some funny 'sti', 'sti' sequences though since some
> of the callers do 'sti' themselves (e.g resume_iret).
>
> > I wonder if I should split i386's locore.s to match amd64's vector.S...
> >
>
> Technically i386 has apicvec.s which is what vectror.S on amd64
> since the arch is specified to always have an APIC. Splitting it into
> trap.S is another possibility, but I'm not sure it's worth it. Possibly
> moving actual APIC vector stubs to C is a better plan and then
> vector.s will be up for the grabs.
>
> > Comments, ok?
> >
>
> Honestly, I'd like the alltraps/sti bit to be handled separately even
> if it's just for the purpose of sync'ing up two implementations.
Something like that first?
Index: amd64/amd64/vector.S
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/vector.S,v
retrieving revision 1.44
diff -u -p -r1.44 vector.S
--- amd64/amd64/vector.S 8 Dec 2015 19:45:55 -0000 1.44
+++ amd64/amd64/vector.S 28 Feb 2016 13:12:56 -0000
@@ -96,24 +96,22 @@
* (possibly the next clock tick). Thus, we disable interrupt before checking,
* and only enable them again on the final `iret' or before calling the AST
* handler.
- */
+ */
/*****************************************************************************/
#define TRAP(a) pushq $(a) ; jmp _C_LABEL(alltraps)
#define ZTRAP(a) pushq $0 ; TRAP(a)
-#define BPTTRAP(a) ZTRAP(a)
-
.text
IDTVEC(trap00)
ZTRAP(T_DIVIDE)
IDTVEC(trap01)
- BPTTRAP(T_TRCTRAP)
+ ZTRAP(T_TRCTRAP)
IDTVEC(trap02)
ZTRAP(T_NMI)
IDTVEC(trap03)
- BPTTRAP(T_BPTFLT)
+ ZTRAP(T_BPTFLT)
IDTVEC(trap04)
ZTRAP(T_OFLOW)
IDTVEC(trap05)
@@ -245,7 +243,6 @@ NENTRY(resume_iret)
NENTRY(alltraps)
INTRENTRY
sti
- .globl calltrap
calltrap:
cld
#ifdef DIAGNOSTIC
Index: i386/i386/locore.s
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/locore.s,v
retrieving revision 1.163
diff -u -p -r1.163 locore.s
--- i386/i386/locore.s 26 Feb 2016 02:25:09 -0000 1.163
+++ i386/i386/locore.s 28 Feb 2016 16:25:34 -0000
@@ -1308,14 +1308,14 @@ ENTRY(savectx)
* handler.
*
* XXX - debugger traps are now interrupt gates so at least bdb doesn't lose
- * control. The sti's give the standard losing behaviour for ddb and kgdb.
+ * control. STI give the standard losing behaviour for ddb and kgdb.
*/
#define IDTVEC(name) ALIGN_TEXT; .globl X##name; X##name:
#define TRAP(a) pushl $(a) ; jmp _C_LABEL(alltraps)
#define ZTRAP(a) pushl $0 ; TRAP(a)
-#define BPTTRAP(a) testb $(PSL_I>>8),13(%esp) ; jz 1f ; sti ; 1: ;
\
- TRAP(a)
+#define STI testb $(PSL_I>>8),13(%esp) ; jz 1f ; sti ; 1: ;
+
.text
IDTVEC(div)
@@ -1328,12 +1328,13 @@ IDTVEC(dbg)
andb $~0xf,%al
movl %eax,%dr6
popl %eax
- BPTTRAP(T_TRCTRAP)
+ STI
+ TRAP(T_TRCTRAP)
IDTVEC(nmi)
ZTRAP(T_NMI)
IDTVEC(bpt)
- pushl $0
- BPTTRAP(T_BPTFLT)
+ STI
+ ZTRAP(T_BPTFLT)
IDTVEC(ofl)
ZTRAP(T_OFLOW)
IDTVEC(bnd)
@@ -1447,6 +1448,10 @@ NENTRY(resume_pop_fs)
sti
jmp calltrap
+/*
+ * All traps go through here. Call the generic trap handler, and
+ * check for ASTs afterwards.
+ */
NENTRY(alltraps)
INTRENTRY
calltrap:
