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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.03.2023 20:12, Oleksii wrote:
> 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?

As said in my reply to Jason, I really view his patch as kind of an odd
way to comment on your patch. So no, I don't think you'd need another
S-o-b in this case.

Jan