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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.03.2023 11:38, Oleksii Kurochko wrote:
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -28,6 +28,9 @@ config ALTERNATIVE_CALL
>  config ARCH_MAP_DOMAIN_PAGE
>       bool
>  
> +config GENERIC_BUG_FRAME
> +     bool

With Arm now also going to use the generic logic, do we actually need this
control anymore (provided things have been proven to work on Arm for the
compiler range we support there)? It looks like because of the way the
series is partitioned it may be necessary transiently, but it should be
possible to drop it again in a new 5th patch.

> --- /dev/null
> +++ b/xen/common/bug.c
> @@ -0,0 +1,103 @@
> +#include <xen/bug.h>
> +#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>
> +
> +#include <asm/processor.h>
> +
> +/*
> + * Returns a negative value in case of an error otherwise the bug type.
> + */

Nit: Style (this is a single line comment). But see also below related
comment on the function's declaration.

> +int do_bug_frame(struct cpu_user_regs *regs, unsigned long pc)
> +{
> +    const struct bug_frame *bug = NULL;
> +    const struct virtual_region *region;
> +    const char *prefix = "", *filename, *predicate;
> +    unsigned long fixup;
> +    unsigned int id = BUGFRAME_NR, lineno;
> +
> +    region = find_text_region(pc);
> +    if ( !region )
> +        return -EINVAL;
> +
> +    for ( id = 0; id < BUGFRAME_NR; id++ )
> +    {
> +        const struct bug_frame *b;
> +        size_t i;
> +
> +        for ( i = 0, b = region->frame[id].bugs;
> +                i < region->frame[id].n_bugs; b++, i++ )

Nit: Indentation (the "i" on the 2nd line wants to align with that
on the 1st one).

> +        {
> +            if ( bug_loc(b) == pc )
> +            {
> +                bug = b;
> +                goto found;
> +            }
> +        }
> +    }
> +
> + found:
> +    if ( !bug )
> +        return -EINVAL;

While I'm generally unhappy with many uses of -EINVAL (it's used to
indicate way too many different kinds of errors), can we at least
consider using -ENOENT here instead? (I'm sorry, I should have asked
for this earlier on.)

> --- /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
> +
> +#ifndef BUG_FRAME_STRUCT

This check won't help when it comes ahead of ...

> +#define BUG_DISP_WIDTH    24
> +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> +#endif
> +
> +#include <asm/bug.h>

... this. But is the #ifdef actually necessary? Or can the #define-s
be moved ...

> +#ifndef BUG_DEBUGGER_TRAP_FATAL
> +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0
> +#endif
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <xen/lib.h>
> +
> +#ifndef BUG_FRAME_STRUCT

... here? (I guess having them defined early, but unconditionally is
better. If an arch wants to override them, they can #undef and then
#define.)

Above from here the "#ifndef __ASSEMBLY__" also wants to move up, to
further enclose BUG_DEBUGGER_TRAP_FATAL (which is useless in assembly
code).

> +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])
> +
> +#endif /* BUG_FRAME_STRUCT */
> +
> +/*
> + * Generic implementation has been based on x86 implementation where
> + * '%c' to deal with punctation sign ($, # depending on architecture)
> + * before immediate.
> + *
> + * Not all architecture's compilers have full support of '%c' and not for all
> + * assemblers punctation sign is used before immediate.
> + * Thereby it was decided to introduce BUG_ASM_CONST.
> + */

I have to admit that I'm not really happy with this comment. At the very
least the last sentence imo doesn't belong there. But overall how about
something like

"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
> +
> +#if !defined(_ASM_BUGFRAME_TEXT) || !defined(_ASM_BUGFRAME_INFO)

Please don't make the conditional more complicated than necessary:
Checking for just _ASM_BUGFRAME_TEXT is enough here.

> +#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 || _ASM_BUGFRAME_INFO */
> +
> +#ifndef BUG_FRAME
> +
> +#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)

Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At least
the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use
the generic struct: With how you have things right now
BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would also
be at risk of causing false positives. Perhaps it's appropriate to
have a separate #ifdef (incl the distinct identifier used), but that
first BUILD_BUG_ON() needs abstracting out.

Also nit: Style (see ...

> +#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); \
> +        BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL);             \
> +    } while ( 0 )

... here). Also, considering the boolean nature, I guess we're in the
process of moving such to be "} while ( false );".

Also please be consistent with formatting of the two adjacent macros,
both indentation-wise and where "do {" is placed. Which of the two
forms you use is secondary.

> +#endif /* run_in_exception_handler */
> +
> +#ifndef WARN
> +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
> +#endif
> +
> +#ifndef BUG
> +#define BUG() do {                                              \
> +    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, NULL);      \
> +    unreachable();                                              \
> +} while (0)
> +#endif
> +
> +#ifndef assert_failed
> +#define assert_failed(msg) do {                                 \
> +    BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg);     \
> +    unreachable();                                              \
> +} while (0)
> +#endif
> +
> +#ifdef CONFIG_GENERIC_BUG_FRAME
> +
> +struct cpu_user_regs;
> +
> +/*
> + * Returns a negative value in case of an error otherwise the bug type.
> + */

Perhaps add "(BUGFRAME_*)", which would then also make this a properly
multi-line comment (right now it's a style violation, as is the case
for the function definition as pointed out above)?

Jan