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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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@xxxxxxxxx>
> 
> 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