[3/4] x86/debug: move debugger_trap_entry into debugger.c not inlined

Commit Message

Bobby Eshleman July 13, 2021, 1:59 a.m. UTC

The function debugger_trap_entry() is rather large for an inlined
function.  This commit moves debugger_trap_entry() into debugger.c and
makes it not inlined.

Signed-off-by: Bobby Eshleman <[e-mail]>
---
 xen/arch/x86/Makefile          |  1 +
 xen/arch/x86/debugger.c        | 41 ++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/debugger.h | 29 ++----------------------
 3 files changed, 44 insertions(+), 27 deletions(-)
 create mode 100644 xen/arch/x86/debugger.c

Comments

Jan Beulich July 14, 2021, 9:52 a.m. UTC | #1

On 13.07.2021 03:59, Bobby Eshleman wrote:
> The function debugger_trap_entry() is rather large for an inlined
> function.

Well, yes, perhaps.

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -32,6 +32,7 @@ obj-y += emul-i8254.o
>  obj-y += extable.o
>  obj-y += flushtlb.o
>  obj-$(CONFIG_CRASH_DEBUG) += gdbstub.o
> +obj-$(CONFIG_CRASH_DEBUG) += debugger.o

Please insert at the appropriate location, rather than breaking the
(mostly) alphabetical sorting.

> --- /dev/null
> +++ b/xen/arch/x86/debugger.c
> @@ -0,0 +1,41 @@
> +/******************************************************************************
> + * x86 crash debug hooks
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <asm/debugger.h>
> +#include <xen/domain.h>
> +#include <xen/event.h>
> +#include <xen/sched.h>
> +
> +bool debugger_trap_entry(
> +    unsigned int vector, struct cpu_user_regs *regs)

As you have to touch this anyway, can you please bring it in line
with the stubs introduced in the first patch, by constifying the
2nd parameter?

> +{
> +    /*
> +     * This function is called before any checks are made.  Amongst other
> +     * things, be aware that during early boot, current is not a safe pointer
> +     * to follow.
> +     */
> +    struct vcpu *v = current;

This one can (and hence better would) gain a "const" as well.

Jan

Patch

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 2ec883456e..ba274fb8e6 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -32,6 +32,7 @@  obj-y += emul-i8254.o
 obj-y += extable.o
 obj-y += flushtlb.o
 obj-$(CONFIG_CRASH_DEBUG) += gdbstub.o
+obj-$(CONFIG_CRASH_DEBUG) += debugger.o
 obj-y += hypercall.o
 obj-y += i387.o
 obj-y += i8259.o
diff --git a/xen/arch/x86/debugger.c b/xen/arch/x86/debugger.c
new file mode 100644
index 0000000000..6f33f509ff
--- /dev/null
+++ b/xen/arch/x86/debugger.c
@@ -0,0 +1,41 @@ 
+/******************************************************************************
+ * x86 crash debug hooks
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <asm/debugger.h>
+#include <xen/domain.h>
+#include <xen/event.h>
+#include <xen/sched.h>
+
+bool debugger_trap_entry(
+    unsigned int vector, struct cpu_user_regs *regs)
+{
+    /*
+     * This function is called before any checks are made.  Amongst other
+     * things, be aware that during early boot, current is not a safe pointer
+     * to follow.
+     */
+    struct vcpu *v = current;
+
+    if ( vector != TRAP_int3 && vector != TRAP_debug )
+        return false;
+
+    if ( guest_mode(regs) && guest_kernel_mode(v, regs) &&
+         v->domain->debugger_attached  )
+    {
+        if ( vector != TRAP_debug ) /* domain pause is good enough */
+            current->arch.gdbsx_vcpu_event = vector;
+        domain_pause_for_debugger();
+        return true;
+    }
+
+    return false;
+}
diff --git a/xen/include/asm-x86/debugger.h b/xen/include/asm-x86/debugger.h
index 38359da0a1..0e30d27a4e 100644
--- a/xen/include/asm-x86/debugger.h
+++ b/xen/include/asm-x86/debugger.h
@@ -15,9 +15,6 @@ 
 #include <asm/regs.h>
 #include <asm/processor.h>
 #include <xen/gdbstub.h>
-#include <xen/domain.h>
-#include <xen/event.h>
-#include <xen/sched.h>
 
 void domain_pause_for_debugger(void);
 
@@ -31,29 +28,7 @@  static inline bool debugger_trap_fatal(
 /* Int3 is a trivial way to gather cpu_user_regs context. */
 #define debugger_trap_immediate() __asm__ __volatile__ ( "int3" );
 
-static inline bool debugger_trap_entry(
-    unsigned int vector, struct cpu_user_regs *regs)
-{
-    /*
-     * This function is called before any checks are made.  Amongst other
-     * things, be aware that during early boot, current is not a safe pointer
-     * to follow.
-     */
-    struct vcpu *v = current;
-
-    if ( vector != TRAP_int3 && vector != TRAP_debug )
-        return false;
-
-    if ( guest_mode(regs) && guest_kernel_mode(v, regs) &&
-         v->domain->debugger_attached  )
-    {
-        if ( vector != TRAP_debug ) /* domain pause is good enough */
-            current->arch.gdbsx_vcpu_event = vector;
-        domain_pause_for_debugger();
-        return true;
-    }
-
-    return false;
-}
+bool debugger_trap_entry(
+    unsigned int vector, struct cpu_user_regs *regs);
 
 #endif /* __X86_DEBUGGER_H__ */