On Wed, Jul 16, 2014 at 12:11:23AM -0400, Mark Johnston wrote: > On Mon, Jul 14, 2014 at 11:10:50AM +0300, Konstantin Belousov wrote: > > On Mon, Jul 14, 2014 at 04:38:17AM +0000, Mark Johnston wrote: > > > Author: markj > > > Date: Mon Jul 14 04:38:17 2014 > > > New Revision: 268600 > > > URL: http://svnweb.freebsd.org/changeset/base/268600 > > > > > > Log: > > > Invoke the DTrace trap handler before calling trap() on amd64. This > > > matches > > > the upstream implementation and helps ensure that a trap induced by > > > tracing > > > fbt::trap:entry is handled without recursively generating another trap. > > > > > > This makes it possible to run most (but not all) of the DTrace tests > > > under > > > common/safety/ without triggering a kernel panic. > > > > > > Submitted by: Anton Rang <[email protected]> (original version) > > > Phabric: D95 > > > > > > Modified: > > > head/sys/amd64/amd64/exception.S > > > head/sys/amd64/amd64/trap.c > > > head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c > > > head/sys/cddl/dev/dtrace/i386/dtrace_subr.c > > > head/sys/cddl/dev/dtrace/mips/dtrace_subr.c > > > head/sys/cddl/dev/dtrace/powerpc/dtrace_subr.c > > > head/sys/i386/i386/trap.c > > > head/sys/mips/mips/trap.c > > > head/sys/powerpc/aim/trap.c > > > head/sys/sys/dtrace_bsd.h > > > > > > Modified: head/sys/amd64/amd64/exception.S > > > ============================================================================== > > > --- head/sys/amd64/amd64/exception.S Mon Jul 14 00:16:49 2014 > > > (r268599) > > > +++ head/sys/amd64/amd64/exception.S Mon Jul 14 04:38:17 2014 > > > (r268600) > > > @@ -228,7 +228,24 @@ alltraps_pushregs_no_rdi: > > > .type calltrap,@function > > > calltrap: > > > movq %rsp,%rdi > > > +#ifdef KDTRACE_HOOKS > > > + /* > > > + * Give DTrace a chance to vet this trap and skip the call to trap() if > > > + * it turns out that it was caused by a DTrace probe. > > > + */ > > > + movq dtrace_trap_func,%rax > > > + testq %rax,%rax > > > + je skiphook > > > + call *%rax > > > + testq %rax,%rax > > > + jne skiptrap > > > + movq %rsp,%rdi > > > +skiphook: > > > +#endif > > > call trap > > Why is this fragment done in asm ? I see it relatively easy to > > either move to the start of trap(), or create a new wrapper around > > current trap() which calls dtrace_trap_func conditionally, and then > > resorts to the old trap. > > You're right, it looks like this could be done in C by introducing a > wrapper. I'm not sure how moving the conditional call to > dtrace_trap_func() around within trap() would fix the original problem, > though. Ok.
>
> The diff below adds such a wrapper, and modifies DTrace to explicitly
> ignore it when creating function boundary probes. Additionally, trap()
> is marked __noinline to ensure that it can still be instrumented. I've
> named it "_trap" for now for lack of a better name, but that will need
> to change.
Looks fine. This should also fix the issue of kgdb and ddb not able to
unwind the trap frames.
>
> Thanks,
> -Mark
>
> diff --git a/sys/amd64/amd64/exception.S b/sys/amd64/amd64/exception.S
> index bb5fd56..3d34724 100644
> --- a/sys/amd64/amd64/exception.S
> +++ b/sys/amd64/amd64/exception.S
> @@ -228,24 +228,7 @@ alltraps_pushregs_no_rdi:
> .type calltrap,@function
> calltrap:
> movq %rsp,%rdi
> -#ifdef KDTRACE_HOOKS
> - /*
> - * Give DTrace a chance to vet this trap and skip the call to trap() if
> - * it turns out that it was caused by a DTrace probe.
> - */
> - movq dtrace_trap_func,%rax
> - testq %rax,%rax
> - je skiphook
> - call *%rax
> - testq %rax,%rax
> - jne skiptrap
> - movq %rsp,%rdi
> -skiphook:
> -#endif
> - call trap
> -#ifdef KDTRACE_HOOKS
> -skiptrap:
> -#endif
> + call _trap
> MEXITCOUNT
> jmp doreti /* Handle any pending ASTs */
>
> diff --git a/sys/amd64/amd64/trap.c b/sys/amd64/amd64/trap.c
> index d9203bc..545174a 100644
> --- a/sys/amd64/amd64/trap.c
> +++ b/sys/amd64/amd64/trap.c
> @@ -97,7 +97,8 @@ PMC_SOFT_DEFINE( , , page_fault, write);
> #include <sys/dtrace_bsd.h>
> #endif
>
> -extern void trap(struct trapframe *frame);
> +extern void _trap(struct trapframe *frame);
> +extern void __noinline trap(struct trapframe *frame);
> extern void syscall(struct trapframe *frame);
> void dblfault_handler(struct trapframe *frame);
>
> @@ -604,6 +605,19 @@ out:
> return;
> }
>
> +/*
> + * Ensure that we ignore any DTrace-induced faults. This function cannot
> + * be instrumented, so it cannot generate such faults itself.
> + */
> +void
> +_trap(struct trapframe *frame)
> +{
> +
> + if (dtrace_trap_func != NULL && (*dtrace_trap_func)(frame))
> + return;
> + trap(frame);
> +}
> +
> static int
> trap_pfault(frame, usermode)
> struct trapframe *frame;
> diff --git a/sys/cddl/dev/fbt/fbt.c b/sys/cddl/dev/fbt/fbt.c
> index 7e256e7..0cc2db2 100644
> --- a/sys/cddl/dev/fbt/fbt.c
> +++ b/sys/cddl/dev/fbt/fbt.c
> @@ -232,13 +232,18 @@ fbt_provide_module_function(linker_file_t lf, int
> symindx,
> int size;
> u_int8_t *instr, *limit;
>
> - if (strncmp(name, "dtrace_", 7) == 0 &&
> - strncmp(name, "dtrace_safe_", 12) != 0) {
> + if ((strncmp(name, "dtrace_", 7) == 0 &&
> + strncmp(name, "dtrace_safe_", 12) != 0) ||
> + strcmp(name, "_trap") == 0) {
> /*
> * Anything beginning with "dtrace_" may be called
> * from probe context unless it explicitly indicates
> * that it won't be called from probe context by
> * using the prefix "dtrace_safe_".
> + *
> + * Additionally, we avoid instrumenting _trap() to avoid the
> + * possibility of generating a fault in probe context before
> + * DTrace's fault handler is called.
> */
> return (0);
> }
pgpCFRm_vWdcH.pgp
Description: PGP signature
