From: Oleksii <oleksii.kurochko@gmail.com>
To: Jan Beulich <jbeulich@suse.com>,
Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
Gianluca Guida <gianluca@rivosinc.com>,
George Dunlap <george.dunlap@citrix.com>,
Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
Date: Wed, 22 Feb 2023 18:16:32 +0200 [thread overview]
Message-ID: <f28da3fa9266f32664280326dca610c67a733195.camel@gmail.com> (raw)
In-Reply-To: <214973b0-5fe7-9208-2cfc-dd2884583157@suse.com>
Hello Jan,
On Wed, 2023-02-22 at 13:46 +0100, Jan Beulich wrote:
> On 20.02.2023 17:40, Oleksii Kurochko wrote:
> > A large part of the content of the bug.h is repeated among all
> > architectures, so it was decided to create a new config
> > CONFIG_GENERIC_BUG_FRAME.
> >
> > The version of <bug.h> from x86 was taken as the base version.
> >
> > The patch introduces the following stuff:
> > * common bug.h header
> > * generic implementation of do_bug_frame()
> > * new config CONFIG_GENERIC_BUG_FRAME
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> >
> > ---
> > Changes in V2:
> > - Switch to x86 implementation as generic as it is more compact
> > ( at least from the point of view of bug frame structure ).
> > - Rename CONFIG_GENERIC_DO_BUG_FRAME to CONFIG_GENERIC_BUG_FRAME.
> > - Change the macro bug_loc(b) to avoid the need for a cast:
> > #define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp)
> > - Rename BUG_FRAME_STUFF to BUG_FRAME_STRUCT
> > - Make macros related to bug frame structure more generic.
> > - Introduce BUG_INSTR and MODIFIER to make _ASM_BUGFRAME_TEXT
> > reusable
> > between x86 and RISC-V.
>
> Hmm, below I see it's really just "MODIFIER". I see two issues with
> this:
> For one the name is too generic for something which cannot be #undef-
> ed
> after use inside the header. And then it also doesn't really say what
> is
> being modified. While ending up longer, how about BUG_ASM_CONST or
> alike?
BUG_ASM_CONST will be much better.
>
> I also think this should default to something if not overridden by an
> arch. Perhaps simply to an expansion to nothing (at which point you
> won't need to define it for RISC-V, aiui).
>
Agree.
Initially, I thought about that too but couldn't estimate how well the
modifier '%c' is supported, deciding that each architecture has to
define it.
But we can make it equal to nothing (at least for 2 architectures ) it
doesn't have the same support as in x86.
> > --- /dev/null
> > +++ b/xen/common/bug.c
> > @@ -0,0 +1,113 @@
> > +#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>
>
> Is this really needed here?
Yes, it is.
Function show_execution_state() is declared in this header for all
architectures and is used by handle_bug_frame().
>
> > +const struct bug_frame* find_bug_frame(unsigned long pc, unsigned
> > int *id)
>
> Is this function going to be needed outside of this CU? IOW why is it
> not
> static?
>
It's not static because there is not possible to use do_bug_frame() as
is for x86 as x86 has some additional checks for some cases which
aren't in generic implementation:
[1]
https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1217
[2]
https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1238
[3]
https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1259
Probably to make generic do_bug_frame() more re-usable for x86 we can
implement as stubs fixup_exception_return() and debugger_trap_fatal()
under #ifndef X86 ... #endif inside common/bug.c.
Could you please share your thoughts about that?
> Also, nit: Left most star wants changing places with the adjacent
> blank.
Thanks. I'll update in the next version of the patch series.
>
> > +{
> > + const struct bug_frame *bug = NULL;
> > + const struct virtual_region *region;
> > +
> > + region = find_text_region(pc);
> > + if ( region )
> > + {
> > + for ( *id = 0; *id < BUGFRAME_NR; (*id)++ )
> > + {
> > + const struct bug_frame *b;
> > + unsigned int i;
> > +
> > + for ( i = 0, b = region->frame[*id].bugs;
> > + i < region->frame[*id].n_bugs; b++, i++ )
> > + {
> > + if ( bug_loc(b) == pc )
> > + {
> > + bug = b;
> > + goto found;
>
> While in the original code the goto is kind of warranted, it isn't
> really
> here imo. You can simply "return b" here and ...
>
> > + }
> > + }
> > + }
> > + }
> > +
> > + found:
> > + return bug;
>
> ... "return NULL" here. That'll allow the function scope "bug" to go
> away,
> at which point the inner scope "b" can become "bug".
Agree, missed that when decided to move that part of the code to
separate function.
>
> > +}
> > +
> > +int handle_bug_frame(const struct cpu_user_regs *regs,
> > + const struct bug_frame *bug,
> > + unsigned int id)
>
> Nit: Indentation is off by one here. Also same question regarding the
> lack
> of static here.
Regarding static an answer is the same as with previous one static
question.
>
> > +{
> > + const char *prefix = "", *filename, *predicate;
> > + unsigned long fixup;
> > + unsigned int lineno;
> > +
> > + if ( id == BUGFRAME_run_fn )
> > + {
> > +#ifdef ARM
>
> Who or what defines ARM? Anyway, seeing ...
it is defined by default in Kconfig:
https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/arm/Kconfig#L13
>
> > + void (*fn)(const struct cpu_user_regs *) = (void *)regs-
> > >BUG_FN_REG;
>
> ... this, wouldn't it be better (and independent of the specific
> arch) if
> you checked for BUG_FN_REG being defined?
If I understand Kconfig correctly than there is no significant
difference what check. But probably BUG_FN_REG would be a little bit
better if someone will decide to change a way how pointer to function
will be passed in case of ARM than we will get compilation error and so
won't miss to fix the line in do_bug_frame().
>
> Another (#ifdef-free) variant would be to have bug_ptr() take a 2nd
> argument
> and then uniformly use ...
>
> > +#else
> > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
>
> ... this, slightly altered to
>
> void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug,
> regs);
Probably this one option will be the most universal and we have to
stick to it.
>
> > +#endif
> > +
> > + fn(regs);
> > + return 0;
> > + }
> > +
> > + /* WARN, BUG or ASSERT: decode the filename pointer and line
> > number. */
> > + filename = bug_ptr(bug);
> > + if ( !is_kernel(filename) && !is_patch(filename) )
> > + return -EINVAL;
> > + fixup = strlen(filename);
> > + if ( fixup > 50 )
> > + {
> > + filename += fixup - 47;
> > + prefix = "...";
> > + }
> > + lineno = bug_line(bug);
> > +
> > + switch ( id )
> > + {
> > + case BUGFRAME_warn:
> > + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
> > + show_execution_state(regs);
> > + return 0;
> > +
> > + case BUGFRAME_bug:
> > + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> > +
> > + show_execution_state(regs);
> > + panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> > +
> > + case BUGFRAME_assert:
> > + /* ASSERT: decode the predicate string pointer. */
> > + predicate = bug_msg(bug);
> > + if ( !is_kernel(predicate) )
> > + predicate = "<unknown>";
> > +
> > + printk("Assertion '%s' failed at %s%s:%d\n",
> > + predicate, prefix, filename, lineno);
> > +
> > + show_execution_state(regs);
> > + panic("Assertion '%s' failed at %s%s:%d\n",
> > + predicate, prefix, filename, lineno);
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long
> > pc)
> > +{
> > + const struct bug_frame *bug = NULL;
>
> Nit: pointless initializer. You could of course have the assignment
> below
> become the initializer here.
Thanks. I'll update it the next version of the patch.
>
> > + unsigned int id;
> > +
> > + bug = find_bug_frame(pc, &id);
> > + if (!bug)
>
> Nit: Style (missing blanks).
Thanks. I'll update it the next version of the patch.
>
> > --- /dev/null
> > +++ b/xen/include/xen/bug.h
> > @@ -0,0 +1,161 @@
> > +#ifndef __XEN_BUG_H__
> > +#define __XEN_BUG_H__
> > +
> > +#define BUG_DISP_WIDTH 24
> > +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> > +
> > +#define BUGFRAME_run_fn 0
> > +#define BUGFRAME_warn 1
> > +#define BUGFRAME_bug 2
> > +#define BUGFRAME_assert 3
> > +
> > +#define BUGFRAME_NR 4
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +#include <xen/errno.h>
> > +#include <xen/lib.h>
> > +#include <xen/stringify.h>
> > +#include <xen/types.h>
> > +
> > +#include <asm/bug.h>
>
> Any reason this cannot live ahead of the #ifndef, eliminating the
> need for
> an #else further down?
It should be fine.
Probably I had some issues during the initial stage of making bug.h
more generic...
>
> > +#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[];
> > +};
> > +
> > +#endif /* BUG_FRAME_STRUCT */
> > +
> > +#ifndef bug_loc
> > +#define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp)
> > +#endif /* bug_loc */
>
> For short #if / #ifdef I don't think such comments are necessary on
> the
> #else or #endif; personally I consider such to hamper readability.
Thanks. I'll take it into account.
>
> > +#ifndef bug_ptr
> > +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
> > +#endif /* bug_ptr */
> > +
> > +#ifndef bug_line
> > +#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)))
> > +#endif /* bug_line */
> > +
> > +#ifndef bug_msg
> > +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
> > +#endif /* bug_msg */
> > +
> > +#ifndef _ASM_BUGFRAME_TEXT
> > +
> > +#define
> > _ASM_BUGFRAME_TEXT(second_frame)
> > \
> > +
> > ".Lbug%=:"BUG_INSTR"\n"
> > \
> > + ".pushsection .bug_frames.%"MODIFIER"[bf_type], \"a\",
> > @progbits\n" \
>
> You may want to use %progbits here right away, as being the more
> portable
> form.
Sure. Thanks. I'll take that into account.
>
> > + ".p2align
> > 2\n" \
> > +
> > ".Lfrm%=:\n"
> > \
> > + ".long (.Lbug%= - .Lfrm%=) +
> > %"MODIFIER"[bf_line_hi]\n" \
> > + ".long (%"MODIFIER"[bf_ptr] - .Lfrm%=) +
> > %"MODIFIER"[bf_line_lo]\n" \
> > + ".if " #second_frame
> > "\n" \
> > + ".long 0, %"MODIFIER"[bf_msg] -
> > .Lfrm%=\n" \
> > +
> > ".endif\n"
> > \
> > + ".popsection\n"
>
> I think I said so in reply to an earlier version already: The moment
> assembly code moves to a common header, it should be adjusted to be
> as
> portable as possible. In particular directives should never start at
> the
> beginning of a line, while labels always should. (Whether .long is
> actually portable is another question, which I'll be happy to leave
> aside for now.)
I am not sure that I understand about which one directive we are
talking about... Are we talking about .{push/pop}section and .p2align?
Probably you can show me an example in Xen or other project?
>
> Also nit (style): The line continuation characters want to all line
> up.
>
> > +#endif /* _ASM_BUGFRAME_TEXT */
> > +
> > +#ifndef _ASM_BUGFRAME_INFO
>
> I don't think these two make sense for an arch to define
> independently.
> INFO absolutely has to match TEXT, so I think an arch should always
> define (or not) both together.
You are right. I'll take into account that.
>
> > +#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_INFO */
> > +
> > +#ifndef BUG_FRAME
> > +
> > +#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)
> > +
> > +#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().
> > + */
>
> I realize you only copy this comment, but I'm having a hard time
> seeing
> the connection to BUILD_BUG_ON() here. Would be nice if the comment
> was
> "generalized" in a form that actually can be understood. Andrew?
>
> > +#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 )
> > +
> > +#endif /* run_in_exception_handler */
> > +
> > +#ifndef WARN
> > +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0,
> > NULL)
> > +#endif /* WARN */
>
> No real need for the comment here; you also have none below for e.g.
> BUG().
Thanks.
>
> Jan
~ Oleksii
next prev parent reply other threads:[~2023-02-22 16:16 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-20 16:40 [PATCH v2 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-02-20 16:40 ` [PATCH v2 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-02-22 12:46 ` Jan Beulich
2023-02-22 16:16 ` Oleksii [this message]
2023-02-23 10:11 ` Jan Beulich
2023-02-23 12:14 ` Oleksii
2023-02-23 13:16 ` Oleksii
2023-02-23 13:25 ` Jan Beulich
2023-02-23 13:32 ` Jan Beulich
2023-02-23 15:09 ` Oleksii
2023-02-20 16:40 ` [PATCH v2 2/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
2023-02-23 13:34 ` Jan Beulich
2023-02-23 15:14 ` Oleksii
2023-02-20 16:40 ` [PATCH v2 3/4] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
2023-02-20 16:41 ` [PATCH v2 4/4] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
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=f28da3fa9266f32664280326dca610c67a733195.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.