From: Oleksii <oleksii.kurochko@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
"Stefano Stabellini" <sstabellini@kernel.org>,
"Gianluca Guida" <gianluca@rivosinc.com>,
"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v3 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
Date: Tue, 28 Feb 2023 18:28:00 +0200 [thread overview]
Message-ID: <48e16f406cd85fea531c736f1f213038f8282613.camel@gmail.com> (raw)
In-Reply-To: <90ec98cc-e416-05c3-f507-5e4b2b7f937d@suse.com>
On Mon, 2023-02-27 at 15:46 +0100, Jan Beulich wrote:
> On 24.02.2023 12:31, Oleksii Kurochko wrote:
> > The following changes were made:
> > * Make GENERIC_BUG_FRAME mandatory for X86
> > * Update asm/bug.h using generic implementation in <xen/bug.h>
> > * Change prototype of debugger_trap_fatal() in asm/debugger.h
> > to align it with generic prototype in <xen/debugger.h>.
> > * Update do_invalid_op using generic do_bug_frame()
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>
> Hmm, there's the question of where to draw the boundary between patch
> 2
> and the one here. Personally I'd say the earlier patch should drop
> everything that becomes redundant there. I can see the more
> minimalistic
> earlier change as viable, but then the description there needs to say
> why things are being kept which - in principle - could be dropped.
Then I'll update the comment message for patch 2.
>
> > --- a/xen/arch/x86/include/asm/bug.h
> > +++ b/xen/arch/x86/include/asm/bug.h
> > @@ -3,75 +3,10 @@
> >
> > #ifndef __ASSEMBLY__
> >
> > -#define BUG_FRAME_STRUCT
> > +#define BUG_INSTR "ud2"
> > +#define BUG_ASM_CONST "c"
> >
> > -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) ((const void *)(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])
> > -
> > -#define
> > _ASM_BUGFRAME_TEXT(second_frame)
> > \
> > - ".Lbug%=:
> > ud2\n" \
> > - ".pushsection .bug_frames.%c[bf_type], \"a\",
> > @progbits\n" \
> > - ".p2align
> > 2\n" \
> > -
> > ".Lfrm%=:\n"
> > \
> > - ".long (.Lbug%= - .Lfrm%=) +
> > %c[bf_line_hi]\n" \
> > - ".long (%c[bf_ptr] - .Lfrm%=) +
> > %c[bf_line_lo]\n" \
> > - ".if " #second_frame
> > "\n" \
> > - ".long 0, %c[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)
> > -
> > -#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)
> > -
> > -
> > -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0,
> > NULL)
> > -#define BUG() do { \
> > - BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \
> > - unreachable(); \
> > -} while (0)
> > -
> > -/*
> > - * 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); \
> > - BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \
> > - } while ( 0 )
> > -
> > -#define assert_failed(msg) do { \
> > - BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
> > - unreachable(); \
> > -} while (0)
> > -
> > -#else /* !__ASSEMBLY__ */
> > +#else
>
> While for new #ifdef-ary such comments can reasonably be omitted when
> the blocks are short, I'd recommend keeping existing comments (except
> in extreme cases) in the interest of reduced code churn. In the case
> here, considering that ...
Got it.
>
> > @@ -113,6 +48,6 @@ struct bug_frame {
> > #define ASSERT_FAILED(msg) \
> > BUG_FRAME BUGFRAME_assert, __LINE__, __FILE__, 1, msg
> >
> > -#endif /* !__ASSEMBLY__ */
> > +#endif /* __ASSEMBLY__ */
>
> ... you keep (but alter - please don't) the comment on the #endif,
> that's even more so a reason to also keep the comment on #else.
>
> > --- a/xen/arch/x86/include/asm/debugger.h
> > +++ b/xen/arch/x86/include/asm/debugger.h
> > @@ -14,9 +14,9 @@
> >
> > /* Returns true if GDB handled the trap, or it is surviveable. */
> > static inline bool debugger_trap_fatal(
> > - unsigned int vector, struct cpu_user_regs *regs)
> > + unsigned int vector, const struct cpu_user_regs *regs)
> > {
> > - int rc = __trap_to_gdb(regs, vector);
> > + int rc = __trap_to_gdb((struct cpu_user_regs *)regs, vector);
>
> I view casts in general as risky and hence preferable to avoid.
> Casting
> away const-ness is particularly problematic. I guess the least bad
> option is to drop const from the do_bug_frame()'s parameter again in
> patch 1. However, for a fatal trap (assuming that really _is_ fatal,
> i.e. execution cannot continue), not allowing register state to be
> altered makes kind of sense. So I wonder whether we couldn't push the
> casting into the gdbstub functions. But quite likely that's going to
> be too intrusive for re-work like you do here.
I am not sure that casting inside the gdbstub functions will help as
do_bug_frame() calls debugger_trap_fatal() which has define regs
without const. So it will still be compile issue as in do_bug_frame()
regs are declared as const.
So it looks like the best one issue now will be drop const from the
do_bug_frame()...
>
> Jan
~ Oleksii
next prev parent reply other threads:[~2023-02-28 16:28 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-24 11:31 [PATCH v3 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-02-24 11:31 ` [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-02-25 16:42 ` Julien Grall
2023-02-27 9:48 ` Jan Beulich
2023-02-28 17:21 ` Oleksii
2023-02-28 18:01 ` Julien Grall
2023-02-28 20:24 ` Oleksii
2023-02-27 14:23 ` Jan Beulich
2023-02-28 10:30 ` Oleksii
2023-02-28 10:42 ` Jan Beulich
2023-02-24 11:31 ` [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
2023-02-25 16:47 ` Julien Grall
2023-02-28 12:38 ` Oleksii
2023-02-28 14:04 ` Julien Grall
2023-02-28 13:07 ` Oleksii
2023-02-28 13:30 ` Jan Beulich
2023-02-28 16:00 ` Oleksii
2023-02-27 14:29 ` Jan Beulich
2023-02-28 13:14 ` Oleksii
2023-02-24 11:31 ` [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
2023-02-25 16:49 ` Julien Grall
2023-02-25 17:05 ` Julien Grall
2023-02-28 17:21 ` Oleksii
2023-02-28 17:57 ` Julien Grall
2023-03-01 12:31 ` Oleksii
2023-03-01 13:58 ` Julien Grall
2023-03-01 15:16 ` Oleksii
2023-03-01 15:21 ` Julien Grall
2023-03-01 15:28 ` Oleksii
2023-03-01 15:58 ` Oleksii
2023-02-28 15:09 ` Oleksii
2023-02-28 17:48 ` Julien Grall
2023-03-01 8:58 ` Oleksii
2023-03-01 9:31 ` Julien Grall
2023-03-01 12:33 ` Oleksii
2023-02-24 11:31 ` [PATCH v3 4/4] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
2023-02-27 14:46 ` Jan Beulich
2023-02-28 16:28 ` Oleksii [this message]
2023-02-28 16:36 ` Jan Beulich
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=48e16f406cd85fea531c736f1f213038f8282613.camel@gmail.com \
--to=oleksii.kurochko@gmail.com \
--cc=andrew.cooper3@citrix.com \
--cc=gianluca@rivosinc.com \
--cc=jbeulich@suse.com \
--cc=roger.pau@citrix.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).