Re: [PATCH v8 1/5] xen: introduce CONFIG_GENERIC_BUG_FRAME

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.03.2023 18:21, Oleksii Kurochko wrote:
> A large part of the content of the bug.h is repeated among all
> architectures, so it was decided to create a new config
> CONFIG_GENERIC_BUG_FRAME.
> 
> The version of <bug.h> from x86 was taken as the base version.
> 
> The patch introduces the following stuff:
>   * common bug.h header
>   * generic implementation of do_bug_frame
>   * new config CONFIG_GENERIC_BUG_FRAME
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

The patch looks good to me now, but as said in reply to patch 5 I'd
like to ...

> ---
> Changes in V8:
>  * move  BUILD_BUG_ON_LINE_WIDTH(line) to "ifndef BUG_FRAME_STRUCT" and add 
> #elif for
>    "ifndef BUG_FRAME_STRUCT" to define stub for BUILD_BUG_ON_LINE_WIDTH.
>  * move <xen/debuger.h> from <asm/bug.h> to <common/bug.c> to fix compilation 
> issue
>    ( more details in the changes to the patch [xen/x86: switch x86 to use 
> generic
>      implemetation of bug.h] ).

... get away without this, so for the moment I won't offer R-b just yet.

One further note on naming (sorry, could have occurred to me earlier):

> --- /dev/null
> +++ b/xen/include/xen/bug.h
> @@ -0,0 +1,161 @@
> +#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])
> +
> +#define BUILD_BUG_ON_LINE_WIDTH(line) \
> +    BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH))

While this and ...

> +#elif !defined(BUILD_BUG_ON_LINE_WIDTH)
> +
> +#define BUILD_BUG_ON_LINE_WIDTH(line) ((void)(line)

... this look okay-ish here, ...

> +#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 BUG_FRAME
> +
> +#define BUG_FRAME(type, line, ptr, second_frame, msg) do {                   
> \
> +    BUILD_BUG_ON_LINE_WIDTH(line);                                           
> \

... the latest at the use site the naming is somewhat odd. I'm inclined
to suggest something like BUG_CHECK_LINE_WIDTH() or BUG_CHECK_LINE().
Aiui the use of the name is isolated to this header, in which case making
such an adjustment while committing would be feasible. If, of course, as
per above no other need for a change arises (i.e. if in particular the
xen/debugger.h inclusion cannot be moved back where it was).

Jan