diff mbox series

[v3,4/4] xen/x86: switch x86 to use generic implemetation of bug.h

Message ID 38bb75b9add8655288347aa4df1394450e4e10c9.1677233393.git.oleksii.kurochko@gmail.com (mailing list archive)
State Superseded
Headers show
Series introduce generic implementation of macros from bug.h | expand

Commit Message

Oleksii Kurochko Feb. 24, 2023, 11:31 a.m. UTC
The following changes were made:
* Make GENERIC_BUG_FRAME mandatory for X86
* Update asm/bug.h using generic implementation in <xen/bug.h>
* Change prototype of debugger_trap_fatal() in asm/debugger.h
  to align it with generic prototype in <xen/debugger.h>.
* Update do_invalid_op using generic do_bug_frame()

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V3:
 * As prototype and what do_bug_frame() returns was changed so patch 3 and 4
   was updated to use a new version of do_bug_frame
 * MODIFIER was change to BUG_ASM_CONST to align with generic implementation
---
Changes in V2:
  * Remove all unnecessary things from <asm/bug.h> as they were introduced in
    <xen/bug.h>.
  * Define BUG_INSTR = 'ud2' and MODIFIER = 'c' ( it is needed to skip '$'
    when use an imidiate in x86 assembly )
  * Update do_invalid_op() to re-use handle_bug_frame() and find_bug_frame()
    from generic implemetation of CONFIG_GENERIC_BUG_FRAME
  * Code style fixes.
---
 xen/arch/x86/Kconfig                |  1 +
 xen/arch/x86/include/asm/bug.h      | 73 ++------------------------
 xen/arch/x86/include/asm/debugger.h |  4 +-
 xen/arch/x86/traps.c                | 81 +++--------------------------
 4 files changed, 14 insertions(+), 145 deletions(-)

Comments

Jan Beulich Feb. 27, 2023, 2:46 p.m. UTC | #1
On 24.02.2023 12:31, Oleksii Kurochko wrote:
> The following changes were made:
> * Make GENERIC_BUG_FRAME mandatory for X86
> * Update asm/bug.h using generic implementation in <xen/bug.h>
> * Change prototype of debugger_trap_fatal() in asm/debugger.h
>   to align it with generic prototype in <xen/debugger.h>.
> * Update do_invalid_op using generic do_bug_frame()
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Hmm, there's the question of where to draw the boundary between patch 2
and the one here. Personally I'd say the earlier patch should drop
everything that becomes redundant there. I can see the more minimalistic
earlier change as viable, but then the description there needs to say
why things are being kept which - in principle - could be dropped.

> --- a/xen/arch/x86/include/asm/bug.h
> +++ b/xen/arch/x86/include/asm/bug.h
> @@ -3,75 +3,10 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define BUG_FRAME_STRUCT
> +#define BUG_INSTR       "ud2"
> +#define BUG_ASM_CONST   "c"
>  
> -struct bug_frame {
> -    signed int loc_disp:BUG_DISP_WIDTH;
> -    unsigned int line_hi:BUG_LINE_HI_WIDTH;
> -    signed int ptr_disp:BUG_DISP_WIDTH;
> -    unsigned int line_lo:BUG_LINE_LO_WIDTH;
> -    signed int msg_disp[];
> -};
> -
> -#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
> -#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
> -#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0)) &                \
> -                       ((1 << BUG_LINE_HI_WIDTH) - 1)) <<                    \
> -                      BUG_LINE_LO_WIDTH) +                                   \
> -                     (((b)->line_lo + ((b)->ptr_disp < 0)) &                 \
> -                      ((1 << BUG_LINE_LO_WIDTH) - 1)))
> -#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
> -
> -#define _ASM_BUGFRAME_TEXT(second_frame)                                     \
> -    ".Lbug%=: ud2\n"                                                         \
> -    ".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n"               \
> -    ".p2align 2\n"                                                           \
> -    ".Lfrm%=:\n"                                                             \
> -    ".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n"                           \
> -    ".long (%c[bf_ptr] - .Lfrm%=) + %c[bf_line_lo]\n"                        \
> -    ".if " #second_frame "\n"                                                \
> -    ".long 0, %c[bf_msg] - .Lfrm%=\n"                                        \
> -    ".endif\n"                                                               \
> -    ".popsection\n"                                                          \
> -
> -#define _ASM_BUGFRAME_INFO(type, line, ptr, msg)                             \
> -    [bf_type]    "i" (type),                                                 \
> -    [bf_ptr]     "i" (ptr),                                                  \
> -    [bf_msg]     "i" (msg),                                                  \
> -    [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1))                \
> -                      << BUG_DISP_WIDTH),                                    \
> -    [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
> -
> -#define BUG_FRAME(type, line, ptr, second_frame, msg) do {                   \
> -    BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH));         \
> -    BUILD_BUG_ON((type) >= BUGFRAME_NR);                                     \
> -    asm volatile ( _ASM_BUGFRAME_TEXT(second_frame)                          \
> -                   :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) );            \
> -} while (0)
> -
> -
> -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
> -#define BUG() do {                                              \
> -    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, NULL);      \
> -    unreachable();                                              \
> -} while (0)
> -
> -/*
> - * TODO: untangle header dependences, break BUILD_BUG_ON() out of xen/lib.h,
> - * and use a real static inline here to get proper type checking of fn().
> - */
> -#define run_in_exception_handler(fn)                            \
> -    do {                                                        \
> -        (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \
> -        BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL);             \
> -    } while ( 0 )
> -
> -#define assert_failed(msg) do {                                 \
> -    BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg);     \
> -    unreachable();                                              \
> -} while (0)
> -
> -#else  /* !__ASSEMBLY__ */
> +#else

While for new #ifdef-ary such comments can reasonably be omitted when
the blocks are short, I'd recommend keeping existing comments (except
in extreme cases) in the interest of reduced code churn. In the case
here, considering that ...

> @@ -113,6 +48,6 @@ struct bug_frame {
>  #define ASSERT_FAILED(msg)                                      \
>       BUG_FRAME BUGFRAME_assert, __LINE__, __FILE__, 1, msg
>  
> -#endif /* !__ASSEMBLY__ */
> +#endif /* __ASSEMBLY__ */

... you keep (but alter - please don't) the comment on the #endif,
that's even more so a reason to also keep the comment on #else.

> --- a/xen/arch/x86/include/asm/debugger.h
> +++ b/xen/arch/x86/include/asm/debugger.h
> @@ -14,9 +14,9 @@
>  
>  /* Returns true if GDB handled the trap, or it is surviveable. */
>  static inline bool debugger_trap_fatal(
> -    unsigned int vector, struct cpu_user_regs *regs)
> +    unsigned int vector, const struct cpu_user_regs *regs)
>  {
> -    int rc = __trap_to_gdb(regs, vector);
> +    int rc = __trap_to_gdb((struct cpu_user_regs *)regs, vector);

I view casts in general as risky and hence preferable to avoid. Casting
away const-ness is particularly problematic. I guess the least bad
option is to drop const from the do_bug_frame()'s parameter again in
patch 1. However, for a fatal trap (assuming that really _is_ fatal,
i.e. execution cannot continue), not allowing register state to be
altered makes kind of sense. So I wonder whether we couldn't push the
casting into the gdbstub functions. But quite likely that's going to
be too intrusive for re-work like you do here.

Jan
Oleksii Kurochko Feb. 28, 2023, 4:28 p.m. UTC | #2
On Mon, 2023-02-27 at 15:46 +0100, Jan Beulich wrote:
> On 24.02.2023 12:31, Oleksii Kurochko wrote:
> > The following changes were made:
> > * Make GENERIC_BUG_FRAME mandatory for X86
> > * Update asm/bug.h using generic implementation in <xen/bug.h>
> > * Change prototype of debugger_trap_fatal() in asm/debugger.h
> >   to align it with generic prototype in <xen/debugger.h>.
> > * Update do_invalid_op using generic do_bug_frame()
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> 
> Hmm, there's the question of where to draw the boundary between patch
> 2
> and the one here. Personally I'd say the earlier patch should drop
> everything that becomes redundant there. I can see the more
> minimalistic
> earlier change as viable, but then the description there needs to say
> why things are being kept which - in principle - could be dropped.
Then I'll update the comment message for patch 2.
> 
> > --- a/xen/arch/x86/include/asm/bug.h
> > +++ b/xen/arch/x86/include/asm/bug.h
> > @@ -3,75 +3,10 @@
> >  
> >  #ifndef __ASSEMBLY__
> >  
> > -#define BUG_FRAME_STRUCT
> > +#define BUG_INSTR       "ud2"
> > +#define BUG_ASM_CONST   "c"
> >  
> > -struct bug_frame {
> > -    signed int loc_disp:BUG_DISP_WIDTH;
> > -    unsigned int line_hi:BUG_LINE_HI_WIDTH;
> > -    signed int ptr_disp:BUG_DISP_WIDTH;
> > -    unsigned int line_lo:BUG_LINE_LO_WIDTH;
> > -    signed int msg_disp[];
> > -};
> > -
> > -#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
> > -#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
> > -#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0))
> > &                \
> > -                       ((1 << BUG_LINE_HI_WIDTH) - 1))
> > <<                    \
> > -                      BUG_LINE_LO_WIDTH)
> > +                                   \
> > -                     (((b)->line_lo + ((b)->ptr_disp < 0))
> > &                 \
> > -                      ((1 << BUG_LINE_LO_WIDTH) - 1)))
> > -#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
> > -
> > -#define
> > _ASM_BUGFRAME_TEXT(second_frame)                                   
> >   \
> > -    ".Lbug%=:
> > ud2\n"                                                         \
> > -    ".pushsection .bug_frames.%c[bf_type], \"a\",
> > @progbits\n"               \
> > -    ".p2align
> > 2\n"                                                           \
> > -   
> > ".Lfrm%=:\n"                                                       
> >       \
> > -    ".long (.Lbug%= - .Lfrm%=) +
> > %c[bf_line_hi]\n"                           \
> > -    ".long (%c[bf_ptr] - .Lfrm%=) +
> > %c[bf_line_lo]\n"                        \
> > -    ".if " #second_frame
> > "\n"                                                \
> > -    ".long 0, %c[bf_msg] -
> > .Lfrm%=\n"                                        \
> > -   
> > ".endif\n"                                                         
> >       \
> > -   
> > ".popsection\n"                                                    
> >       \
> > -
> > -#define _ASM_BUGFRAME_INFO(type, line, ptr,
> > msg)                             \
> > -    [bf_type]    "i"
> > (type),                                                 \
> > -    [bf_ptr]     "i"
> > (ptr),                                                  \
> > -    [bf_msg]     "i"
> > (msg),                                                  \
> > -    [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) -
> > 1))                \
> > -                      <<
> > BUG_DISP_WIDTH),                                    \
> > -    [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) <<
> > BUG_DISP_WIDTH)
> > -
> > -#define BUG_FRAME(type, line, ptr, second_frame, msg) do
> > {                   \
> > -    BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> > BUG_LINE_HI_WIDTH));         \
> > -    BUILD_BUG_ON((type) >=
> > BUGFRAME_NR);                                     \
> > -    asm volatile (
> > _ASM_BUGFRAME_TEXT(second_frame)                          \
> > -                   :: _ASM_BUGFRAME_INFO(type, line, ptr, msg)
> > );            \
> > -} while (0)
> > -
> > -
> > -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0,
> > NULL)
> > -#define BUG() do {                                              \
> > -    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, NULL);      \
> > -    unreachable();                                              \
> > -} while (0)
> > -
> > -/*
> > - * TODO: untangle header dependences, break BUILD_BUG_ON() out of
> > xen/lib.h,
> > - * and use a real static inline here to get proper type checking
> > of fn().
> > - */
> > -#define run_in_exception_handler(fn)                            \
> > -    do {                                                        \
> > -        (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \
> > -        BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL);             \
> > -    } while ( 0 )
> > -
> > -#define assert_failed(msg) do {                                 \
> > -    BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg);     \
> > -    unreachable();                                              \
> > -} while (0)
> > -
> > -#else  /* !__ASSEMBLY__ */
> > +#else
> 
> While for new #ifdef-ary such comments can reasonably be omitted when
> the blocks are short, I'd recommend keeping existing comments (except
> in extreme cases) in the interest of reduced code churn. In the case
> here, considering that ...
Got it.
> 
> > @@ -113,6 +48,6 @@ struct bug_frame {
> >  #define ASSERT_FAILED(msg)                                      \
> >       BUG_FRAME BUGFRAME_assert, __LINE__, __FILE__, 1, msg
> >  
> > -#endif /* !__ASSEMBLY__ */
> > +#endif /* __ASSEMBLY__ */
> 
> ... you keep (but alter - please don't) the comment on the #endif,
> that's even more so a reason to also keep the comment on #else.
> 
> > --- a/xen/arch/x86/include/asm/debugger.h
> > +++ b/xen/arch/x86/include/asm/debugger.h
> > @@ -14,9 +14,9 @@
> >  
> >  /* Returns true if GDB handled the trap, or it is surviveable. */
> >  static inline bool debugger_trap_fatal(
> > -    unsigned int vector, struct cpu_user_regs *regs)
> > +    unsigned int vector, const struct cpu_user_regs *regs)
> >  {
> > -    int rc = __trap_to_gdb(regs, vector);
> > +    int rc = __trap_to_gdb((struct cpu_user_regs *)regs, vector);
> 
> I view casts in general as risky and hence preferable to avoid.
> Casting
> away const-ness is particularly problematic. I guess the least bad
> option is to drop const from the do_bug_frame()'s parameter again in
> patch 1. However, for a fatal trap (assuming that really _is_ fatal,
> i.e. execution cannot continue), not allowing register state to be
> altered makes kind of sense. So I wonder whether we couldn't push the
> casting into the gdbstub functions. But quite likely that's going to
> be too intrusive for re-work like you do here.
I am not sure that casting inside the gdbstub functions will help as
do_bug_frame() calls debugger_trap_fatal() which has define regs 
without const. So it will still be compile issue as in do_bug_frame()
regs are declared as const.

So it looks like the best one issue now will be drop const from the
do_bug_frame()...
> 
> Jan
~ Oleksii
Jan Beulich Feb. 28, 2023, 4:36 p.m. UTC | #3
On 28.02.2023 17:28, Oleksii wrote:
> On Mon, 2023-02-27 at 15:46 +0100, Jan Beulich wrote:
>> On 24.02.2023 12:31, Oleksii Kurochko wrote:
>>> --- a/xen/arch/x86/include/asm/debugger.h
>>> +++ b/xen/arch/x86/include/asm/debugger.h
>>> @@ -14,9 +14,9 @@
>>>  
>>>  /* Returns true if GDB handled the trap, or it is surviveable. */
>>>  static inline bool debugger_trap_fatal(
>>> -    unsigned int vector, struct cpu_user_regs *regs)
>>> +    unsigned int vector, const struct cpu_user_regs *regs)
>>>  {
>>> -    int rc = __trap_to_gdb(regs, vector);
>>> +    int rc = __trap_to_gdb((struct cpu_user_regs *)regs, vector);
>>
>> I view casts in general as risky and hence preferable to avoid.
>> Casting
>> away const-ness is particularly problematic. I guess the least bad
>> option is to drop const from the do_bug_frame()'s parameter again in
>> patch 1. However, for a fatal trap (assuming that really _is_ fatal,
>> i.e. execution cannot continue), not allowing register state to be
>> altered makes kind of sense. So I wonder whether we couldn't push the
>> casting into the gdbstub functions. But quite likely that's going to
>> be too intrusive for re-work like you do here.
> I am not sure that casting inside the gdbstub functions will help as
> do_bug_frame() calls debugger_trap_fatal() which has define regs 
> without const. So it will still be compile issue as in do_bug_frame()
> regs are declared as const.

Well, the idea was of course to propagate const further down, to cast
it away only in restricted cases. But ...

> So it looks like the best one issue now will be drop const from the
> do_bug_frame()...

... looks like we're in agreement then about this being the better
route for the time being.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 6a7825f4ba..b0ff1f3ee6 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -11,6 +11,7 @@  config X86
 	select ARCH_MAP_DOMAIN_PAGE
 	select ARCH_SUPPORTS_INT128
 	select CORE_PARKING
+	select GENERIC_BUG_FRAME
 	select HAS_ALTERNATIVE
 	select HAS_COMPAT
 	select HAS_CPUFREQ
diff --git a/xen/arch/x86/include/asm/bug.h b/xen/arch/x86/include/asm/bug.h
index 4b3e7b019d..d7eb41baf3 100644
--- a/xen/arch/x86/include/asm/bug.h
+++ b/xen/arch/x86/include/asm/bug.h
@@ -3,75 +3,10 @@ 
 
 #ifndef __ASSEMBLY__
 
-#define BUG_FRAME_STRUCT
+#define BUG_INSTR       "ud2"
+#define BUG_ASM_CONST   "c"
 
-struct bug_frame {
-    signed int loc_disp:BUG_DISP_WIDTH;
-    unsigned int line_hi:BUG_LINE_HI_WIDTH;
-    signed int ptr_disp:BUG_DISP_WIDTH;
-    unsigned int line_lo:BUG_LINE_LO_WIDTH;
-    signed int msg_disp[];
-};
-
-#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
-#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
-#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0)) &                \
-                       ((1 << BUG_LINE_HI_WIDTH) - 1)) <<                    \
-                      BUG_LINE_LO_WIDTH) +                                   \
-                     (((b)->line_lo + ((b)->ptr_disp < 0)) &                 \
-                      ((1 << BUG_LINE_LO_WIDTH) - 1)))
-#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
-
-#define _ASM_BUGFRAME_TEXT(second_frame)                                     \
-    ".Lbug%=: ud2\n"                                                         \
-    ".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n"               \
-    ".p2align 2\n"                                                           \
-    ".Lfrm%=:\n"                                                             \
-    ".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n"                           \
-    ".long (%c[bf_ptr] - .Lfrm%=) + %c[bf_line_lo]\n"                        \
-    ".if " #second_frame "\n"                                                \
-    ".long 0, %c[bf_msg] - .Lfrm%=\n"                                        \
-    ".endif\n"                                                               \
-    ".popsection\n"                                                          \
-
-#define _ASM_BUGFRAME_INFO(type, line, ptr, msg)                             \
-    [bf_type]    "i" (type),                                                 \
-    [bf_ptr]     "i" (ptr),                                                  \
-    [bf_msg]     "i" (msg),                                                  \
-    [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1))                \
-                      << BUG_DISP_WIDTH),                                    \
-    [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
-
-#define BUG_FRAME(type, line, ptr, second_frame, msg) do {                   \
-    BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH));         \
-    BUILD_BUG_ON((type) >= BUGFRAME_NR);                                     \
-    asm volatile ( _ASM_BUGFRAME_TEXT(second_frame)                          \
-                   :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) );            \
-} while (0)
-
-
-#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
-#define BUG() do {                                              \
-    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, NULL);      \
-    unreachable();                                              \
-} while (0)
-
-/*
- * TODO: untangle header dependences, break BUILD_BUG_ON() out of xen/lib.h,
- * and use a real static inline here to get proper type checking of fn().
- */
-#define run_in_exception_handler(fn)                            \
-    do {                                                        \
-        (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \
-        BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL);             \
-    } while ( 0 )
-
-#define assert_failed(msg) do {                                 \
-    BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg);     \
-    unreachable();                                              \
-} while (0)
-
-#else  /* !__ASSEMBLY__ */
+#else
 
 /*
  * Construct a bugframe, suitable for using in assembly code.  Should always
@@ -113,6 +48,6 @@  struct bug_frame {
 #define ASSERT_FAILED(msg)                                      \
      BUG_FRAME BUGFRAME_assert, __LINE__, __FILE__, 1, msg
 
-#endif /* !__ASSEMBLY__ */
+#endif /* __ASSEMBLY__ */
 
 #endif /* __X86_BUG_H__ */
diff --git a/xen/arch/x86/include/asm/debugger.h b/xen/arch/x86/include/asm/debugger.h
index a5c299c6c3..1873dc5779 100644
--- a/xen/arch/x86/include/asm/debugger.h
+++ b/xen/arch/x86/include/asm/debugger.h
@@ -14,9 +14,9 @@ 
 
 /* Returns true if GDB handled the trap, or it is surviveable. */
 static inline bool debugger_trap_fatal(
-    unsigned int vector, struct cpu_user_regs *regs)
+    unsigned int vector, const struct cpu_user_regs *regs)
 {
-    int rc = __trap_to_gdb(regs, vector);
+    int rc = __trap_to_gdb((struct cpu_user_regs *)regs, vector);
 
     if ( rc == 0 )
         return true;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index cade9e12f8..7c33ebceff 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -24,6 +24,7 @@ 
  * Gareth Hughes <gareth@valinux.com>, May 2000
  */
 
+#include <xen/bug.h>
 #include <xen/init.h>
 #include <xen/sched.h>
 #include <xen/lib.h>
@@ -1166,12 +1167,9 @@  void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
 
 void do_invalid_op(struct cpu_user_regs *regs)
 {
-    const struct bug_frame *bug = NULL;
     u8 bug_insn[2];
-    const char *prefix = "", *filename, *predicate, *eip = (char *)regs->rip;
-    unsigned long fixup;
-    int id = -1, lineno;
-    const struct virtual_region *region;
+    const char *eip = (char *)regs->rip;
+    int id;
 
     if ( likely(guest_mode(regs)) )
     {
@@ -1185,83 +1183,18 @@  void do_invalid_op(struct cpu_user_regs *regs)
          memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) )
         goto die;
 
-    region = find_text_region(regs->rip);
-    if ( region )
-    {
-        for ( id = 0; id < BUGFRAME_NR; id++ )
-        {
-            const struct bug_frame *b;
-            unsigned int i;
-
-            for ( i = 0, b = region->frame[id].bugs;
-                  i < region->frame[id].n_bugs; b++, i++ )
-            {
-                if ( bug_loc(b) == eip )
-                {
-                    bug = b;
-                    goto found;
-                }
-            }
-        }
-    }
-
- found:
-    if ( !bug )
+    id = do_bug_frame(regs, regs->rip);
+    if ( id < 0 )
         goto die;
-    eip += sizeof(bug_insn);
-    if ( id == BUGFRAME_run_fn )
-    {
-        void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
-
-        fn(regs);
-        fixup_exception_return(regs, (unsigned long)eip);
-        return;
-    }
 
-    /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
-    filename = bug_ptr(bug);
-    if ( !is_kernel(filename) && !is_patch(filename) )
-        goto die;
-    fixup = strlen(filename);
-    if ( fixup > 50 )
-    {
-        filename += fixup - 47;
-        prefix = "...";
-    }
-    lineno = bug_line(bug);
+    eip += sizeof(bug_insn);
 
     switch ( id )
     {
+    case BUGFRAME_run_fn:
     case BUGFRAME_warn:
-        printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
-        show_execution_state(regs);
         fixup_exception_return(regs, (unsigned long)eip);
         return;
-
-    case BUGFRAME_bug:
-        printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
-
-        if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
-            return;
-
-        show_execution_state(regs);
-        panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
-
-    case BUGFRAME_assert:
-        /* ASSERT: decode the predicate string pointer. */
-        predicate = bug_msg(bug);
-        if ( !is_kernel(predicate) && !is_patch(predicate) )
-            predicate = "<unknown>";
-
-        printk("Assertion '%s' failed at %s%s:%d\n",
-               predicate, prefix, filename, lineno);
-
-        if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
-            return;
-
-        show_execution_state(regs);
-        panic("Assertion '%s' failed at %s%s:%d\n",
-              predicate, prefix, filename, lineno);
     }
 
  die: