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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.03.2023 12:50, 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>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with two further adjustments (which I'd be fine doing while committing):

> --- /dev/null
> +++ b/xen/common/bug.c
> @@ -0,0 +1,126 @@
> +#include <xen/bug.h>
> +/*
> + * Ideally <xen/debugger.h> should be included in <asm/bug.h>
> + * but an issue with compilation can occur as <xen/debugger.h> uses
> + * BUG/ASSERT/etc macros inside but they will be defined later in
> + * <xen/bug.h> after return from inclusion of <asm/bug.h>:
> + * 
> + * <xen/bug.h>:
> + *  ...
> + *   <asm/bug.h>:
> + *     ...
> + *     <xen/debugger.h> -> some of included header in it uses BUG/ASSERT/etc
> + *     ...
> + *  ...
> + *  #define BUG() ...
> + *  ...
> + *  #define ASSERT() ...
> + *  ...
> + */
> +#include <xen/debugger.h>
> +#include <xen/errno.h>
> +#include <xen/kernel.h>
> +#include <xen/livepatch.h>
> +#include <xen/string.h>
> +#include <xen/types.h>
> +#include <xen/virtual_region.h>
> +
> +struct cpu_user_regs;

This doesn't need repeating here, as xen/bug.h necessarily has such a
forward declaration as well, for use in do_bug_frame()'s declaration.

> --- /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 BUG_CHECK_LINE_WIDTH(line) \
> +    BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH))
> +
> +#elif !defined(BUG_CHECK_LINE_WIDTH)
> +
> +#define BUG_CHECK_LINE_WIDTH(line) ((void)(line)

There's a closing parenthesis missing here, as it looks.

Jan