Re: [PATCH v6 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2023-03-08 at 16:06 +0100, Jan Beulich wrote:
> On 07.03.2023 16:50, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/common/bug.c
> > @@ -0,0 +1,104 @@
> > +#include <xen/bug.h>
> > +#include <xen/debugger.h>
> 
> Isn't it asm/bug.h now which is to include this header, if needed at
> all?
You are right it will be better to move <xen/debugger.h> to
<asm/bug.h>.
> 
> > --- /dev/null
> > +++ b/xen/include/xen/bug.h
> > @@ -0,0 +1,158 @@
> > +#ifndef __XEN_BUG_H__
> > +#define __XEN_BUG_H__
> > +
> > +#define BUGFRAME_run_fn 0
> > +#define BUGFRAME_warn   1
> > +#define BUGFRAME_bug    2
> > +#define BUGFRAME_assert 3
> > +
> > +#define BUGFRAME_NR     4
> > +
> > +#define BUG_DISP_WIDTH    24
> > +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> > +
> > +#include <asm/bug.h>
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +#ifndef BUG_DEBUGGER_TRAP_FATAL
> > +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0
> > +#endif
> > +
> > +#include <xen/lib.h>
> > +
> > +#ifndef BUG_FRAME_STRUCT
> > +
> > +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) ((unsigned long)(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])
> 
> As indicated earlier, I think that you want to move here what you
> currently have ...
> 
> > +#endif /* BUG_FRAME_STRUCT */
> > +
> > +/*
> > + * Some architectures mark immediate instruction operands in a
> > special way.
> > + * In such cases the special marking may need omitting when
> > specifying
> > + * directive operands. Allow architectures to specify a suitable
> > + * modifier.
> > + */
> > +#ifndef BUG_ASM_CONST
> > +#define BUG_ASM_CONST ""
> > +#endif
> > +
> > +#ifndef _ASM_BUGFRAME_TEXT
> > +
> > +#define
> > _ASM_BUGFRAME_TEXT(second_frame)                                   
> >          \
> > +   
> > ".Lbug%=:"BUG_INSTR"\n"                                            
> >              \
> > +    "   .pushsection .bug_frames.%"BUG_ASM_CONST"[bf_type], \"a\",
> > %%progbits\n"    \
> > +    "   .p2align
> > 2\n"                                                              
> > \
> > +   
> > ".Lfrm%=:\n"                                                       
> >              \
> > +    "   .long (.Lbug%= - .Lfrm%=) +
> > %"BUG_ASM_CONST"[bf_line_hi]\n"                 \
> > +    "   .long (%"BUG_ASM_CONST"[bf_ptr] - .Lfrm%=) +
> > %"BUG_ASM_CONST"[bf_line_lo]\n"\
> > +    "   .if " #second_frame
> > "\n"                                                    \
> > +    "   .long 0, %"BUG_ASM_CONST"[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)
> > +
> > +#endif /* _ASM_BUGFRAME_TEXT */
> > +
> > +#ifndef BUILD_BUG_ON_LINE_WIDTH
> > +#define BUILD_BUG_ON_LINE_WIDTH(line) \
> > +    BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> > BUG_LINE_HI_WIDTH))
> > +#endif
> 
> ... here, guarded by a separate #ifdef. The check is specifically
> tied to
> the struct layout chosen here. Instead what you want here is
> 
> #ifndef BUILD_BUG_ON_LINE_WIDTH
> #define BUILD_BUG_ON_LINE_WIDTH(line) ((void)(line))
> #endif
> 
> covering architectures using a different layout where such a check
> isn't
> needed. Of course this also could move up and simply become "#elif
> ..."
> to the earlier "#if !defined(BUG_FRAME_STRUCT)", which would keep
> related things together.
> 
The logic behind this was the following. <xen/bug.h> is the generic
implementation which is based on BUG_LINE_{LO,HI}_WIDTH and if
architecture would like to use another one implementation than it
should re-define BUILD_BUG_ON_LINE_WIDTH.

But it might be better to move my 'ifndef BUILD_BUG_ON_LINE_WIDTH' to
'ifndef BUG_FRAME' and instead of it put dummy BUILD_BUG_ON_LINE_WIDTH.

> > +#ifndef BUG_FRAME
> > +
> > +#define BUG_FRAME(type, line, ptr, second_frame, msg) do
> > {                   \
> > +   
> > BUILD_BUG_ON_LINE_WIDTH(line);                                     
> >       \
> > +    BUILD_BUG_ON((type) >=
> > BUGFRAME_NR);                                     \
> > +    asm volatile (
> > _ASM_BUGFRAME_TEXT(second_frame)                          \
> > +                   :: _ASM_BUGFRAME_INFO(type, line, ptr, msg)
> > );            \
> > +} while (false)
> 
> Nit: Style.
Oh. It should be changed to ( false ) in each macros.

> 
> > +
> > +#endif
> > +
> > +#ifndef run_in_exception_handler
> > +
> > +/*
> > + * 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); \
> 
> Hmm, there's another const-ness anomaly that has slipped in (and is
> no longer necessary with do_bug_frame() now again taking a pointer to
> non-const): At the point you handle BUGFRAME_run_fn the type used
> (wrongly) is void (*)(const struct cpu_user_regs *).
> 
> The disconnect isn't good to leave (as the same issue could be
> introduced later, when not looking closely enough). While not for
> this patch, I wonder if we shouldn't make the use site be something
> along the lines of
> 
>     if ( id == BUGFRAME_run_fn )
>     {
>         void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
> 
>         fn(regs);
> 
>         /* Re-enforce consistent types, because of the casts
> involved. */
>         if ( false )
>             run_in_exception_handler(fn);
> 
>         return id;
>     }
> 
> just to make sure the type used in run_in_exception_handler()
> matches the one used here (without actually producing any code).
> 
It looks like it is really make sense to add this check so I will
take it into account in the next version of the patch series.

Thanks.

~ Oleksii