From: Oleksii <oleksii.kurochko@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Stefano Stabellini <sstabellini@kernel.org>,
Gianluca Guida <gianluca@rivosinc.com>,
George Dunlap <george.dunlap@citrix.com>, Wei Liu <wl@xen.org>,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v6 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
Date: Wed, 08 Mar 2023 19:18:52 +0200 [thread overview]
Message-ID: <e2823e985ccd05bad4fcd5ac107d8a9c3db69eb9.camel@gmail.com> (raw)
In-Reply-To: <32a4a1c3-018c-fced-2b04-d1db79f7dcd9@suse.com>
On Wed, 2023-03-08 at 16:06 +0100, Jan Beulich wrote:
> On 07.03.2023 16:50, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/common/bug.c
> > @@ -0,0 +1,104 @@
> > +#include <xen/bug.h>
> > +#include <xen/debugger.h>
>
> Isn't it asm/bug.h now which is to include this header, if needed at
> all?
You are right it will be better to move <xen/debugger.h> to
<asm/bug.h>.
>
> > --- /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
> > +
> > +#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])
>
> As indicated earlier, I think that you want to move here what you
> currently have ...
>
> > +#endif /* BUG_FRAME_STRUCT */
> > +
> > +/*
> > + * 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
> > +
> > +#ifndef _ASM_BUGFRAME_TEXT
> > +
> > +#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 */
> > +
> > +#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
>
> ... here, guarded by a separate #ifdef. The check is specifically
> tied to
> the struct layout chosen here. Instead what you want here is
>
> #ifndef BUILD_BUG_ON_LINE_WIDTH
> #define BUILD_BUG_ON_LINE_WIDTH(line) ((void)(line))
> #endif
>
> covering architectures using a different layout where such a check
> isn't
> needed. Of course this also could move up and simply become "#elif
> ..."
> to the earlier "#if !defined(BUG_FRAME_STRUCT)", which would keep
> related things together.
>
The logic behind this was the following. <xen/bug.h> is the generic
implementation which is based on BUG_LINE_{LO,HI}_WIDTH and if
architecture would like to use another one implementation than it
should re-define BUILD_BUG_ON_LINE_WIDTH.
But it might be better to move my 'ifndef BUILD_BUG_ON_LINE_WIDTH' to
'ifndef BUG_FRAME' and instead of it put dummy BUILD_BUG_ON_LINE_WIDTH.
> > +#ifndef BUG_FRAME
> > +
> > +#define BUG_FRAME(type, line, ptr, second_frame, msg) do
> > { \
> > +
> > BUILD_BUG_ON_LINE_WIDTH(line);
> > \
> > + BUILD_BUG_ON((type) >=
> > BUGFRAME_NR); \
> > + asm volatile (
> > _ASM_BUGFRAME_TEXT(second_frame) \
> > + :: _ASM_BUGFRAME_INFO(type, line, ptr, msg)
> > ); \
> > +} while (false)
>
> Nit: Style.
Oh. It should be changed to ( false ) in each macros.
>
> > +
> > +#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); \
>
> Hmm, there's another const-ness anomaly that has slipped in (and is
> no longer necessary with do_bug_frame() now again taking a pointer to
> non-const): At the point you handle BUGFRAME_run_fn the type used
> (wrongly) is void (*)(const struct cpu_user_regs *).
>
> The disconnect isn't good to leave (as the same issue could be
> introduced later, when not looking closely enough). While not for
> this patch, I wonder if we shouldn't make the use site be something
> along the lines of
>
> if ( id == BUGFRAME_run_fn )
> {
> void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
>
> fn(regs);
>
> /* Re-enforce consistent types, because of the casts
> involved. */
> if ( false )
> run_in_exception_handler(fn);
>
> return id;
> }
>
> just to make sure the type used in run_in_exception_handler()
> matches the one used here (without actually producing any code).
>
It looks like it is really make sense to add this check so I will
take it into account in the next version of the patch series.
Thanks.
~ Oleksii
next prev parent reply other threads:[~2023-03-08 17:19 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-07 15:50 [PATCH v6 0/4] [PATCH v5 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-03-07 15:50 ` [PATCH v6 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-03-08 15:06 ` Jan Beulich
2023-03-08 17:18 ` Oleksii [this message]
2023-03-07 15:50 ` [PATCH v6 2/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
2023-03-08 15:27 ` Jan Beulich
2023-03-08 17:25 ` Oleksii
2023-03-09 9:49 ` Jan Beulich
2023-03-07 15:50 ` [PATCH v6 3/4] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
2023-03-07 15:50 ` [PATCH v6 4/4] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
2023-03-08 15:33 ` Jan Beulich
2023-03-08 17:48 ` Oleksii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e2823e985ccd05bad4fcd5ac107d8a9c3db69eb9.camel@gmail.com \
--to=oleksii.kurochko@gmail.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=gianluca@rivosinc.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=sstabellini@kernel.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.