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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.02.2023 17:40, 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>
> 
> ---
> Changes in V2:
>   - Switch to x86 implementation as generic as it is more compact
>     ( at least from the point of view of bug frame structure ).
>   - Rename CONFIG_GENERIC_DO_BUG_FRAME to CONFIG_GENERIC_BUG_FRAME.
>   - Change the macro bug_loc(b) to avoid the need for a cast:
>     #define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp)
>   - Rename BUG_FRAME_STUFF to BUG_FRAME_STRUCT
>   - Make macros related to bug frame structure more generic.
>   - Introduce BUG_INSTR and MODIFIER to make _ASM_BUGFRAME_TEXT reusable
>     between x86 and RISC-V.

Hmm, below I see it's really just "MODIFIER". I see two issues with this:
For one the name is too generic for something which cannot be #undef-ed
after use inside the header. And then it also doesn't really say what is
being modified. While ending up longer, how about BUG_ASM_CONST or alike?

I also think this should default to something if not overridden by an
arch. Perhaps simply to an expansion to nothing (at which point you
won't need to define it for RISC-V, aiui).

> --- /dev/null
> +++ b/xen/common/bug.c
> @@ -0,0 +1,113 @@
> +#include <xen/bug.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>

Is this really needed here?

> +const struct bug_frame* find_bug_frame(unsigned long pc, unsigned int *id)

Is this function going to be needed outside of this CU? IOW why is it not
static?

Also, nit: Left most star wants changing places with the adjacent blank.

> +{
> +    const struct bug_frame *bug = NULL;
> +    const struct virtual_region *region;
> +
> +    region = find_text_region(pc);
> +    if ( region )
> +    {
> +        for ( *id = 0; *id < BUGFRAME_NR; (*id)++ )
> +        {
> +            const struct bug_frame *b;
> +            unsigned int i;
> +
> +            for ( i = 0, b = region->frame[*id].bugs;
> +                  i < region->frame[*id].n_bugs; b++, i++ )
> +            {
> +                if ( bug_loc(b) == pc )
> +                {
> +                    bug = b;
> +                    goto found;

While in the original code the goto is kind of warranted, it isn't really
here imo. You can simply "return b" here and ...

> +                }
> +            }
> +        }
> +    }
> +
> + found:
> +    return bug;

... "return NULL" here. That'll allow the function scope "bug" to go away,
at which point the inner scope "b" can become "bug".

> +}
> +
> +int handle_bug_frame(const struct cpu_user_regs *regs,
> +                    const struct bug_frame *bug,
> +                    unsigned int id)

Nit: Indentation is off by one here. Also same question regarding the lack
of static here.

> +{
> +    const char *prefix = "", *filename, *predicate;
> +    unsigned long fixup;
> +    unsigned int lineno;
> +
> +    if ( id == BUGFRAME_run_fn )
> +    {
> +#ifdef ARM        

Who or what defines ARM? Anyway, seeing ...

> +        void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;

... this, wouldn't it be better (and independent of the specific arch) if
you checked for BUG_FN_REG being defined?

Another (#ifdef-free) variant would be to have bug_ptr() take a 2nd argument
and then uniformly use ...

> +#else
> +        void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);

... this, slightly altered to

        void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug, regs);

> +#endif
> +
> +        fn(regs);
> +        return 0;
> +    }
> +
> +    /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> +    filename = bug_ptr(bug);
> +    if ( !is_kernel(filename) && !is_patch(filename) )
> +        return -EINVAL;
> +    fixup = strlen(filename);
> +    if ( fixup > 50 )
> +    {
> +        filename += fixup - 47;
> +        prefix = "...";
> +    }
> +    lineno = bug_line(bug);
> +
> +    switch ( id )
> +    {
> +    case BUGFRAME_warn:
> +        printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
> +        show_execution_state(regs);
> +        return 0;
> +
> +    case BUGFRAME_bug:
> +        printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> +
> +        show_execution_state(regs);
> +        panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> +
> +    case BUGFRAME_assert:
> +        /* ASSERT: decode the predicate string pointer. */
> +        predicate = bug_msg(bug);
> +        if ( !is_kernel(predicate) )
> +            predicate = "<unknown>";
> +
> +        printk("Assertion '%s' failed at %s%s:%d\n",
> +               predicate, prefix, filename, lineno);
> +
> +        show_execution_state(regs);
> +        panic("Assertion '%s' failed at %s%s:%d\n",
> +              predicate, prefix, filename, lineno);
> +    }
> +
> +    return -EINVAL;
> +}
> +
> +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc)
> +{
> +    const struct bug_frame *bug = NULL;

Nit: pointless initializer. You could of course have the assignment below
become the initializer here.

> +    unsigned int id;
> +
> +    bug = find_bug_frame(pc, &id);
> +    if (!bug)

Nit: Style (missing blanks).

> --- /dev/null
> +++ b/xen/include/xen/bug.h
> @@ -0,0 +1,161 @@
> +#ifndef __XEN_BUG_H__
> +#define __XEN_BUG_H__
> +
> +#define BUG_DISP_WIDTH    24
> +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> +
> +#define BUGFRAME_run_fn 0
> +#define BUGFRAME_warn   1
> +#define BUGFRAME_bug    2
> +#define BUGFRAME_assert 3
> +
> +#define BUGFRAME_NR     4
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <xen/errno.h>
> +#include <xen/lib.h>
> +#include <xen/stringify.h>
> +#include <xen/types.h>
> +
> +#include <asm/bug.h>

Any reason this cannot live ahead of the #ifndef, eliminating the need for
an #else further down?

> +#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[];
> +};
> +
> +#endif /* BUG_FRAME_STRUCT */
> +
> +#ifndef bug_loc
> +#define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp)
> +#endif /* bug_loc */

For short #if / #ifdef I don't think such comments are necessary on the
#else or #endif; personally I consider such to hamper readability.

> +#ifndef bug_ptr
> +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
> +#endif /* bug_ptr */
> +
> +#ifndef bug_line
> +#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)))
> +#endif /* bug_line */
> +
> +#ifndef bug_msg
> +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
> +#endif /* bug_msg */
> +
> +#ifndef _ASM_BUGFRAME_TEXT
> +
> +#define _ASM_BUGFRAME_TEXT(second_frame)                                    \
> +    ".Lbug%=:"BUG_INSTR"\n"                                                 \
> +    ".pushsection .bug_frames.%"MODIFIER"[bf_type], \"a\", @progbits\n"      
> \

You may want to use %progbits here right away, as being the more portable
form.

> +    ".p2align 2\n"                                                          \
> +    ".Lfrm%=:\n"                                                            \
> +    ".long (.Lbug%= - .Lfrm%=) + %"MODIFIER"[bf_line_hi]\n"                  
> \
> +    ".long (%"MODIFIER"[bf_ptr] - .Lfrm%=) + %"MODIFIER"[bf_line_lo]\n"      
>  \
> +    ".if " #second_frame "\n"                                               \
> +    ".long 0, %"MODIFIER"[bf_msg] - .Lfrm%=\n"                               
> \
> +    ".endif\n"                                                              \
> +    ".popsection\n"

I think I said so in reply to an earlier version already: The moment
assembly code moves to a common header, it should be adjusted to be as
portable as possible. In particular directives should never start at the
beginning of a line, while labels always should. (Whether .long is
actually portable is another question, which I'll be happy to leave
aside for now.)

Also nit (style): The line continuation characters want to all line up.

> +#endif /* _ASM_BUGFRAME_TEXT */
> +
> +#ifndef _ASM_BUGFRAME_INFO

I don't think these two make sense for an arch to define independently.
INFO absolutely has to match TEXT, so I think an arch should always
define (or not) both together.

> +#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_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)
> +
> +#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().
> + */

I realize you only copy this comment, but I'm having a hard time seeing
the connection to BUILD_BUG_ON() here. Would be nice if the comment was
"generalized" in a form that actually can be understood. Andrew?

> +#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 )
> +
> +#endif /* run_in_exception_handler */
> +
> +#ifndef WARN
> +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
> +#endif /* WARN */

No real need for the comment here; you also have none below for e.g.
BUG().

Jan