From owner-svn-src-all@FreeBSD.ORG Wed Jul 16 04:11:29 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id B6AE03EE; Wed, 16 Jul 2014 04:11:29 +0000 (UTC) Received: from mail-ie0-x229.google.com (mail-ie0-x229.google.com [IPv6:2607:f8b0:4001:c03::229]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 6DD0E289C; Wed, 16 Jul 2014 04:11:29 +0000 (UTC) Received: by mail-ie0-f169.google.com with SMTP id tp5so318164ieb.28 for ; Tue, 15 Jul 2014 21:11:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=ejV4mA5CUUMoSIqW2UxoQJ9yCLC5YpT+Sv/Oqoz3Jqc=; b=YxrPGv0sNPeNJ0JSrcwTqgkiHY8Ja2bUiWSHS8rA17zyroh51Aei90h8bm3z203shl mAMZHIAJb97l/maRFwO2oT8cyGSpLT+rRX6sUVKVh9iZM6T6emElSnZWjyMsMH2uunfG 6Ene2rkY1GLSXORoALjHtRra/Bg49StONxbt6Mfiebz+MUHZAff3oziDiY6q0BhoVBkr UW1VdURD8Bvw250vEnmUIO6CkYYIZ9dcdBLJ3XH8SZwaOFmZW5FOUWjAvk8r0K5c0BEY 0u8kih3fYw+L5H4+OQtSovog55PqjVt8MyNfXOSt58WYqDcBEEpPh922+a8N24vIyGYK ItBg== X-Received: by 10.50.12.105 with SMTP id x9mr12113952igb.10.1405483888783; Tue, 15 Jul 2014 21:11:28 -0700 (PDT) Received: from raichu (24-212-142-131.cable.teksavvy.com. [24.212.142.131]) by mx.google.com with ESMTPSA id ga11sm3310352igd.8.2014.07.15.21.11.27 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 15 Jul 2014 21:11:28 -0700 (PDT) Sender: Mark Johnston Date: Wed, 16 Jul 2014 00:11:23 -0400 From: Mark Johnston To: Konstantin Belousov Subject: Re: svn commit: r268600 - in head/sys: amd64/amd64 cddl/dev/dtrace/amd64 cddl/dev/dtrace/i386 cddl/dev/dtrace/mips cddl/dev/dtrace/powerpc i386/i386 mips/mips powerpc/aim sys Message-ID: <20140716041123.GA20065@raichu> References: <201407140438.s6E4cHCh016707@svn.freebsd.org> <20140714081050.GE93733@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140714081050.GE93733@kib.kiev.ua> User-Agent: Mutt/1.5.23 (2014-03-12) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Jul 2014 04:11:29 -0000 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 (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. 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. 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 #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); }