From: Julien Grall <julien@xen.org>
To: Oleksii Kurochko <oleksii.kurochko@gmail.com>,
xen-devel@lists.xenproject.org
Cc: Jan Beulich <jbeulich@suse.com>,
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>
Subject: Re: [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
Date: Sat, 25 Feb 2023 16:42:28 +0000 [thread overview]
Message-ID: <8734edd9-70eb-b2bd-8763-b7468d7caba3@xen.org> (raw)
In-Reply-To: <932b0fd2a211d2d837d00e1bda87a84ad680fe7b.1677233393.git.oleksii.kurochko@gmail.com>
Hi Oleksii,
On 24/02/2023 11:31, 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 V3:
> * Add debugger_trap_fatal() to do_bug_frame(). It simplifies usage of
> do_bug_frame() for x86 so making handle_bug_frame() and find_bug_frame()
> not needed anymore.
> * Update do_bug_frame() to return -EINVAL if something goes wrong; otherwise
> id of bug_frame
> * Update _ASM_BUGFRAME_TEXT to make it more portable.
> * Drop unnecessary comments.
> * define stub value for TRAP_invalid_op in case if wasn't defined in
> arch-specific folders.
> ---
> 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.
> - Rework do_bug_frame() and introduce find_bug_frame() and handle_bug_frame()
> functions to make it reusable by x86.
> - code style fixes
> ---
> xen/common/Kconfig | 3 +
> xen/common/Makefile | 1 +
> xen/common/bug.c | 109 ++++++++++++++++++++++++++++++
> xen/include/xen/bug.h | 150 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 263 insertions(+)
> create mode 100644 xen/common/bug.c
> create mode 100644 xen/include/xen/bug.h
>
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index f1ea3199c8..b226323537 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -28,6 +28,9 @@ config ALTERNATIVE_CALL
> config ARCH_MAP_DOMAIN_PAGE
> bool
>
> +config GENERIC_BUG_FRAME
> + bool
> +
> config HAS_ALTERNATIVE
> bool
>
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index bbd75b4be6..46049eac35 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -1,5 +1,6 @@
> obj-$(CONFIG_ARGO) += argo.o
> obj-y += bitmap.o
> +obj-$(CONFIG_GENERIC_BUG_FRAME) += bug.o
> obj-$(CONFIG_HYPFS_CONFIG) += config_data.o
> obj-$(CONFIG_CORE_PARKING) += core_parking.o
> obj-y += cpu.o
> diff --git a/xen/common/bug.c b/xen/common/bug.c
> new file mode 100644
> index 0000000000..f81724fc9b
> --- /dev/null
> +++ b/xen/common/bug.c
> @@ -0,0 +1,109 @@
> +#include <xen/bug.h>
> +#include <xen/debugger.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>
> +
> +/* Set default value for TRAP_invalid_op as it is defined only for X86 now */
> +#ifndef TRAP_invalid_op
> +#define TRAP_invalid_op 0
> +#endif
It feels to me that this value should be defined in the else part in
xen/debugger.h.
> +
> +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc)
I would suggest to document what this function is meant to return.
AFAUI, it would return a negative value in case of an error otherwise
the bug type.
> +{
> + 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;
> +
> + region = find_text_region(pc);
> + if ( region )
NIT: If you invert the condition here, then you can reduce the indention
by one below.
> + {
> + for ( id = 0; id < BUGFRAME_NR; id++ )
> + {
> + const struct bug_frame *b;
> + unsigned int i;
You compare this against n_bugs which is a size_t. So, this wants to be
a size_t.
> +
> + 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;
> + }
> + }
> + }
> + }
> +
> + found:
> + if ( !bug )
> + return -EINVAL;
> +
> + if ( id == BUGFRAME_run_fn )
> + {
> +#ifdef BUG_FN_REG
> + void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;
AFAIU, this is necessary so Arm can use the generic do_bug_frame().
I was under the impression that RISC-V and Arm had the similar issue
with %c. It seems like you managed to resolve it on RISC-V, so can we
fully switch Arm to the generic implementation of bug?
> +#else
> + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> +#endif
> +
> + fn(regs);
> +
> + return id;
> + }
> +
> + /* 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 id;
> +
> + case BUGFRAME_bug:
> + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> +
> + if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
> + return id;
> +
> + 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) && !is_patch(predicate) )
> + predicate = "<unknown>";
> +
> + printk("Assertion '%s' failed at %s%s:%d\n",
> + predicate, prefix, filename, lineno);
> +
> + if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
> + return id;
> +
> + show_execution_state(regs);
> + panic("Assertion '%s' failed at %s%s:%d\n",
> + predicate, prefix, filename, lineno);
> + }
> +
> + return id;
> +}
> diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
> new file mode 100644
> index 0000000000..4b18cfa69c
> --- /dev/null
> +++ b/xen/include/xen/bug.h
> @@ -0,0 +1,150 @@
> +#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
> +
> +#include <asm/bug.h>
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <xen/errno.h>
errno.h doesn't look to be used within this here. So is it necessary to
import it?
> +#include <xen/lib.h>
Why is this necessary to include in the header?
> +#include <xen/stringify.h>
You don't seem to use __stringify in this header. So is this necessary?
> +
> +#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
> +
> +#ifndef bug_ptr
> +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
> +#endif
> +
> +#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
> +
> +#ifndef bug_msg
> +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
> +#endif
For all the macro above, it feels wrong to me to allow an architecture
to override them if the default BUG_FRAME_STRUCT.
It would also feels wrong to me that if the default BUG_FRAME_STRUCT is
not used to still partially rely on the generic version of the helper.\
So I would suggest to move them in the #ifndef BUG_FRAME_STRUCT and drop
the #ifndef <helper>.
> +
> +#ifndef BUG_ASM_CONST
> +#define BUG_ASM_CONST ""
> +#endif
This line is a bit misterious to me. Would you be able to outline why an
architecture would override this?
> +
> +#if !defined(_ASM_BUGFRAME_TEXT) || !defined(_ASM_BUGFRAME_INFO)
> +
> +#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 || _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().
> + */
> +#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
> +
> +#ifndef BUG
> +#define BUG() do { \
> + BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \
> + unreachable(); \
> +} while (0)
> +#endif
> +
> +#ifndef assert_failed
> +#define assert_failed(msg) do { \
> + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
> + unreachable(); \
> +} while (0)
> +#endif
> +
> +#ifdef CONFIG_GENERIC_BUG_FRAME
> +
> +struct cpu_user_regs;
> +
> +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc);
> +
> +#endif /* CONFIG_GENERIC_BUG_FRAME */
> +
> +extern const struct bug_frame __start_bug_frames[],
> + __stop_bug_frames_0[],
> + __stop_bug_frames_1[],
> + __stop_bug_frames_2[],
> + __stop_bug_frames_3[];
> +
> +#endif /* !__ASSEMBLY__ */
> +
> +#endif /* __XEN_BUG_H__ */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2023-02-25 16:42 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 [this message]
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
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=8734edd9-70eb-b2bd-8763-b7468d7caba3@xen.org \
--to=julien@xen.org \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=gianluca@rivosinc.com \
--cc=jbeulich@suse.com \
--cc=oleksii.kurochko@gmail.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).