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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-03-13 at 17:26 +0100, Jan Beulich wrote:
> On 09.03.2023 14:33, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/common/bug.c
> > @@ -0,0 +1,107 @@
> > +#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>
> > +
> > +/*
> > + * Returns a negative value in case of an error otherwise
> > + * BUGFRAME_{run_fn, warn, bug, assert}
> > + */
> > +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;
> 
> Unnecessary initializer; "id" is set ...
> 
> > +    region = find_text_region(pc);
> > +    if ( !region )
> > +        return -EINVAL;
> > +
> > +    for ( id = 0; id < BUGFRAME_NR; id++ )
> 
> ... unconditionally here.
> 
> > --- /dev/null
> > +++ b/xen/include/xen/bug.h
> > @@ -0,0 +1,162 @@
> > +#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])
> > +
> > +#ifndef BUILD_BUG_ON_LINE_WIDTH
> > +#define BUILD_BUG_ON_LINE_WIDTH(line) \
> > +    BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> > BUG_LINE_HI_WIDTH))
> > +#endif
> 
> I still don't see why you have #ifdef here. What I would expect is
> (as
> expressed before)
> 
> #define BUILD_BUG_ON_LINE_WIDTH(line) \
>     BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH))
> 
> #else  /* BUG_FRAME_STRUCT */
> 
> #ifndef BUILD_BUG_ON_LINE_WIDTH
> #define BUILD_BUG_ON_LINE_WIDTH(line) ((void)(line)
> #endif
> 
> (perhaps shortened to
> 
> #elif !defined(BUILD_BUG_ON_LINE_WIDTH)
> #define BUILD_BUG_ON_LINE_WIDTH(line) ((void)(line)
> 
> )
> 
> > +#endif /* BUG_FRAME_STRUCT */
> 
> ... and then the separate conditional further down dropped. Have you
> found anything speaking against this approach?
Both options are fine from compilation point of view.

Lets change it to proposed by you option with '#elif !defined(...)...'

I'll prepare new patch series and sent it to the mailing list.

I would like to add the changes from the [PATCH] xen/cpufreq: Remove
<asm/bug.h> by Jason Andryuk <jandryuk@xxxxxxxxx> but I don't know how
correctly do that. I mean should I added one more Signed-off to the
patch?

Thanks.

~ Oleksii