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 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.