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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hello Jan,

On Wed, 2023-02-22 at 13:46 +0100, Jan Beulich wrote:
> 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?
BUG_ASM_CONST will be much better.
> 
> 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).
> 
Agree.

Initially, I thought about that too but couldn't estimate how well the
modifier '%c' is supported, deciding that each architecture has to
define it.

But we can make it equal to nothing (at least for 2 architectures ) it
doesn't have the same support as in x86.

> > --- /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?
Yes, it is.

Function show_execution_state() is declared in this header for all
architectures and is used by handle_bug_frame().
> 
> > +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?
> 
It's not static because there is not possible to use do_bug_frame() as
is for x86 as x86 has some additional checks for some cases which
aren't in generic implementation:
[1]
https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1217
[2]
https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1238
[3]
https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1259

Probably to make generic do_bug_frame() more re-usable for x86 we can
implement as stubs fixup_exception_return() and debugger_trap_fatal()
under #ifndef X86 ... #endif inside common/bug.c.

Could you please share your thoughts about that?

> Also, nit: Left most star wants changing places with the adjacent
> blank.
Thanks. I'll update in the next version of the patch series.

> 
> > +{
> > +    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".
Agree, missed that when decided to move that part of the code to
separate function.

> 
> > +}
> > +
> > +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.
Regarding static an answer is the same as with previous one static
question.

> 
> > +{
> > +    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 ...
it is defined by default in Kconfig:
https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/arm/Kconfig#L13
> 
> > +        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?
If I understand Kconfig correctly than there is no significant
difference what check. But probably BUG_FN_REG would be a little bit
better if someone will decide to change a way how pointer to function
will be passed in case of ARM than we will get compilation error and so
won't miss to fix the line in do_bug_frame().

> 
> 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);
Probably this one option will be the most universal and we have to
stick to it.
> 
> > +#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.
Thanks. I'll update it the next version of the patch.
> 
> > +    unsigned int id;
> > +
> > +    bug = find_bug_frame(pc, &id);
> > +    if (!bug)
> 
> Nit: Style (missing blanks).
Thanks. I'll update it the next version of the patch.
> 
> > --- /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?
It should be fine.
Probably I had some issues during the initial stage of making bug.h
more generic...

> 
> > +#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.
Thanks. I'll take it into account.

> 
> > +#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.
Sure. Thanks. I'll take that into account.
> 
> > +    ".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.)
I am not sure that I understand about which one directive we are
talking about... Are we talking about .{push/pop}section and .p2align?
Probably you can show me an example in Xen or other project?

> 
> 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.
You are right. I'll take into account that.
> 
> > +#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().
Thanks.
> 
> Jan

~ Oleksii