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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> --- /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.

> +#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.

> +
> +#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).

Jan