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

[Jan Beulich <jbeulich@suse.com>]

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