* [PATCH v5 0/4] introduce generic implementation of macros from bug.h
@ 2023-03-03 10:38 Oleksii Kurochko
2023-03-03 10:38 ` [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: Oleksii Kurochko @ 2023-03-03 10:38 UTC (permalink / raw
To: xen-devel
Cc: Julien Grall, Jan Beulich, Andrew Cooper, Stefano Stabellini,
Gianluca Guida, Oleksii Kurochko, George Dunlap, Wei Liu,
Bertrand Marquis, Volodymyr Babchuk, Roger Pau Monné
A large part of the content of the bug.h is repeated among all
architectures (especially x86 and RISCV have the same implementation), so it
was created a new config CONFIG_GENERIC_BUG_FRAME which is used to
introduce generic implementation of do_bug_frame() and move x86's <asm/bug.h>
to <xen/common/...> with the following changes:
* Add inclusion of arch-specific header <asm/bug.h>
* Rename the guard and remove x86 specific changes
* Wrap macros BUG_FRAME/run_in_exception_handler/WARN/BUG/assert_failed/etc
into #ifndef "BUG_FRAME/run_in_exception_handler/WARN/BUG/assert_failed/etc"
thereby each architecture can override the generic implementation of macros.
* Add #if{n}def __ASSEMBLY__ ... #endif
* Introduce BUG_FRAME_STRUCTURE define to be able to change the structure of bug
frame
* Introduce BUG_INSTR and BUG_ASM_CONST to make _ASM_BUGFRAME_TEXT reusable between
architectures
* Make macros related to bug frame structure more generic.
RISC-V will be switched to use <xen/bug.h> and in the future, it will use common
the version of do_bug_frame() when xen/common will work for RISC-V.
---
Changes in V5:
* Update the cover letter message as the patch, on which the patch series
is based, has been merged to staging
* Remove "#ifdef BUG_FN_REG..." from generic do_bug_frame() as ARM will
use generic implementation fully.
---
Changes in V4:
* Introduce and use generic BUG_DEBUGGER_TRAP_FATAL(regs) mnacros which is used to deal with
debugger_trap_fatal(TRAP_invalid_op, regs) where TRAP_invalid_op is x86-specific.
* Add comment what do_bug_frame() returns.
* Do refactoring of do_bug_frame():
* invert the condition 'if ( region )' in do_bug_frame() to reduce the indention.
* change type of variable i from 'unsigned int' to 'size_t' as it is compared with
n_bugs which has type 'size_t'
* Remove '#include <xen/stringify.h>' from <xen/bug.h> as it doesn't need any more after switch to
x86 implementation.
* Remove '#include <xen/errno.h>' as it isn't needed any more
* Move bug_*() macros inside '#ifndef BUG_FRAME_STRUCT'
* Add <xen/lib.h> to fix compile issue with BUILD_ON()...
* Add documentation for BUG_ASM_CONST.
* Defines BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH were moved into
"ifndef BUG_FRAME_STRUCT" in <xen/bug.h> as they are specific for 'struct bug_frame' and so should
co-exist together. So the defines were back to <asm/bug.h> until BUG_FRAME_STRUCT will be defined in
<asm/bug.h>.
* Switch ARM implementation to generic one
* Remove BUG_FN_REG from arm{16,32}/bug.h as it isn't needed after switch to generic implementation
* Back comment /* !__ASSEMBLY__ */ for #else case in <asm/bug.h>
* Remove changes related to x86/.../asm/debuger.h as do_bug_frame() prototype was updated and cpu_user_regs
isn't const any more.
---
Changes in V3:
* Nothing was done with the comment in <xen/bug.h> before run_in_exception_handler
but I think it can be changed during the merge.
* 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.
* Update patch 2 not to break compilation: move some parts from patches 3 and 4
to patch 2
* As prototype and what do_bug_frame() returns was changed so patch 3 and 4
was updated to use a new version of do_bug_frame
* Change debugger_trap_fatal() prototype for x86 to align with prototype in
<xen/debugger.h>
---
Changes in V2:
* Update cover letter.
* Switch to x86 implementation as generic as it is more compact ( at least from the point of view of bug frame structure).
* Put [PATCH v1 4/4] xen: change <asm/bug.h> to <xen/bug.h> as second patch,
update the patch to change all <asm/bug.h> to <xen/bug.h> among the whole project
to not break compilation.
* 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.
* Rename bug_file() in ARM implementation to bug_ptr() as generic do_bug_frame() uses
bug_ptr().
* Introduce BUG_INSTR and MODIFIER to make _ASM_BUGFRAME_TEXT reusable between x86 and
RISC-V.
* Rework do_invalid_op() in x86 ( re-use handle_bug_frame() and find_bug_frame() )
---
Oleksii Kurochko (4):
xen: introduce CONFIG_GENERIC_BUG_FRAME
xen: change <asm/bug.h> to <xen/bug.h>
xen/arm: switch ARM to use generic implementation of bug.h
xen/x86: switch x86 to use generic implemetation of bug.h
xen/arch/arm/Kconfig | 1 +
xen/arch/arm/arm32/traps.c | 2 +-
xen/arch/arm/include/asm/arm32/bug.h | 2 -
xen/arch/arm/include/asm/arm64/bug.h | 2 -
xen/arch/arm/include/asm/bug.h | 88 +--------------
xen/arch/arm/include/asm/div64.h | 2 +-
xen/arch/arm/include/asm/traps.h | 2 -
xen/arch/arm/traps.c | 81 +-------------
xen/arch/arm/vgic/vgic-v2.c | 2 +-
xen/arch/arm/vgic/vgic.c | 2 +-
xen/arch/x86/Kconfig | 1 +
xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +-
xen/arch/x86/include/asm/asm_defns.h | 2 +-
xen/arch/x86/include/asm/bug.h | 84 +-------------
xen/arch/x86/traps.c | 81 ++------------
xen/common/Kconfig | 3 +
xen/common/Makefile | 1 +
xen/common/bug.c | 103 +++++++++++++++++
xen/drivers/cpufreq/cpufreq.c | 2 +-
xen/include/xen/bug.h | 158 +++++++++++++++++++++++++++
xen/include/xen/lib.h | 2 +-
21 files changed, 289 insertions(+), 334 deletions(-)
create mode 100644 xen/common/bug.c
create mode 100644 xen/include/xen/bug.h
--
2.39.0
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-03-03 10:38 [PATCH v5 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
@ 2023-03-03 10:38 ` Oleksii Kurochko
2023-03-06 10:17 ` Jan Beulich
2023-03-06 10:41 ` Jan Beulich
2023-03-03 10:38 ` [PATCH v5 2/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
` (2 subsequent siblings)
3 siblings, 2 replies; 20+ messages in thread
From: Oleksii Kurochko @ 2023-03-03 10:38 UTC (permalink / raw
To: xen-devel
Cc: Julien Grall, Jan Beulich, Andrew Cooper, Stefano Stabellini,
Gianluca Guida, Oleksii Kurochko, George Dunlap, Wei Liu
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 V5:
* Remove "#ifdef BUG_FN_REG..." from generic do_bug_frame() as ARM will
use generic implementation fully.
---
Changes in V4:
* common/bug.c:
- Use BUG_DEBUGGER_TRAP_FATAL(regs) mnacros instead of debugger_trap_fatal(TRAP_invalid_op, regs)
in <common/bug.c> as TRAP_invalid_op is x86-specific thereby BUG_DEBUGGER_TRAP_FATAL should
be defined for each architecture.
- add information about what do_bug_frame() returns.
- invert the condition 'if ( region )' in do_bug_frame() to reduce the indention.
- change type of variable i from 'unsigned int' to 'size_t' as it is compared with
n_bugs which has type 'size_t'
* xen/bug.h:
- Introduce generic BUG_DEBUGGER_TRAP_FATAL(regs) mnacros which is used to deal with
debugger_trap_fatal(TRAP_invalid_op, regs) where TRAP_invalid_op is x86-specific
- remove '#include <xen/stringify.h>' as it doesn't need any more after switch to
x86 implementation.
- remove '#include <xen/errno.h>' as it isn't needed any more
- move bug_*() macros inside '#ifndef BUG_FRAME_STRUCT'
- add <xen/lib.h> to fix compile issue with BUILD_ON()...
- Add documentation for BUG_ASM_CONST.
* Update the commit message
---
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 | 103 +++++++++++++++++++++++++++
xen/include/xen/bug.h | 158 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 265 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..e636edd85a
--- /dev/null
+++ b/xen/common/bug.c
@@ -0,0 +1,103 @@
+#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>
+
+/*
+ * Returns a negative value in case of an error otherwise the bug type.
+ */
+int do_bug_frame(struct cpu_user_regs *regs, unsigned long pc)
+{
+ 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 )
+ return -EINVAL;
+
+ for ( id = 0; id < BUGFRAME_NR; id++ )
+ {
+ const struct bug_frame *b;
+ size_t 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;
+ }
+ }
+ }
+
+ found:
+ if ( !bug )
+ return -EINVAL;
+
+ if ( id == BUGFRAME_run_fn )
+ {
+ void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
+
+ 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 ( BUG_DEBUGGER_TRAP_FATAL(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 ( BUG_DEBUGGER_TRAP_FATAL(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..722ec5c23b
--- /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
+
+#ifndef BUG_FRAME_STRUCT
+#define BUG_DISP_WIDTH 24
+#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
+#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
+#endif
+
+#include <asm/bug.h>
+
+#ifndef BUG_DEBUGGER_TRAP_FATAL
+#define BUG_DEBUGGER_TRAP_FATAL(regs) 0
+#endif
+
+#ifndef __ASSEMBLY__
+
+#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])
+
+#endif /* BUG_FRAME_STRUCT */
+
+/*
+ * Generic implementation has been based on x86 implementation where
+ * '%c' to deal with punctation sign ($, # depending on architecture)
+ * before immediate.
+ *
+ * Not all architecture's compilers have full support of '%c' and not for all
+ * assemblers punctation sign is used before immediate.
+ * Thereby it was decided to introduce BUG_ASM_CONST.
+ */
+#ifndef BUG_ASM_CONST
+#define BUG_ASM_CONST ""
+#endif
+
+#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;
+
+/*
+ * Returns a negative value in case of an error otherwise the bug type.
+ */
+int do_bug_frame(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:
+ */
--
2.39.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v5 2/4] xen: change <asm/bug.h> to <xen/bug.h>
2023-03-03 10:38 [PATCH v5 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-03-03 10:38 ` [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
@ 2023-03-03 10:38 ` Oleksii Kurochko
2023-03-06 10:24 ` Jan Beulich
2023-03-03 10:38 ` [PATCH v5 3/4] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
2023-03-03 10:38 ` [PATCH v5 4/4] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
3 siblings, 1 reply; 20+ messages in thread
From: Oleksii Kurochko @ 2023-03-03 10:38 UTC (permalink / raw
To: xen-devel
Cc: Julien Grall, Jan Beulich, Andrew Cooper, Stefano Stabellini,
Gianluca Guida, Oleksii Kurochko, Bertrand Marquis,
Volodymyr Babchuk, George Dunlap, Wei Liu, Roger Pau Monné
The idea of the patch is to change all <asm/bug.h> to <xen/bug.h> and
keep Xen compilable with adding only minimal amount of changes:
1. It was added "#include <xen/types.h>" to ARM's "<asm/bug.h>" as it
uses uint_{16,32}t in 'struct bug_frame'.
2. It was added '#define BUG_FRAME_STRUCT' which means that ARM hasn't
been switched to generic implementation yet.
3. It was added '#define BUG_FRAME_STRUCT' which means that x86 hasn't
been switched to generic implementation yet.
4. BUGFRAME_* and _start_bug_frame[], _stop_bug_frame_*[] were removed
for ARM & x86 to deal with compilation errors such as:
redundant redeclaration of ...
In the following two patches x86 and ARM archictectures will be
switched fully:
* xen/arm: switch ARM to use generic implementation of bug.h
* xen/x86: switch x86 to use generic implemetation of bug.h
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V5:
- Nothing changed
---
Changes in V4:
- defines BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH were moved into
"ifndef BUG_FRAME_STRUCT" in <xen/bug.h> as they are specific for 'struct bug_frame' and so should
co-exist together. So the defines were back to <asm/bug.h> until BUG_FRAME_STRUCT will be defined in
<asm/bug.h>.
- Update the comment message.
---
Changes in V3:
* Update patch 2 not to break compilation: move some parts from patches 3 and 4
to patch 2:
* move some generic parts from <asm/bug.h> to <xen/bug.h>
* add define BUG_FRAME_STRUCT in ARM's <asm/bug.h>
---
Changes in V2:
* Put [PATCH v1 4/4] xen: change <asm/bug.h> to <xen/bug.h> as second patch,
update the patch to change all <asm/bug.h> to <xen/bug.h> among the whole project
to not break build.
* Update the commit message.
---
xen/arch/arm/include/asm/bug.h | 17 ++++-------------
xen/arch/arm/include/asm/div64.h | 2 +-
xen/arch/arm/vgic/vgic-v2.c | 2 +-
xen/arch/arm/vgic/vgic.c | 2 +-
xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +-
xen/arch/x86/include/asm/asm_defns.h | 2 +-
xen/arch/x86/include/asm/bug.h | 15 ++-------------
xen/drivers/cpufreq/cpufreq.c | 2 +-
xen/include/xen/lib.h | 2 +-
9 files changed, 13 insertions(+), 33 deletions(-)
diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
index f4088d0913..9315662c6e 100644
--- a/xen/arch/arm/include/asm/bug.h
+++ b/xen/arch/arm/include/asm/bug.h
@@ -1,6 +1,8 @@
#ifndef __ARM_BUG_H__
#define __ARM_BUG_H__
+#include <xen/types.h>
+
#if defined(CONFIG_ARM_32)
# include <asm/arm32/bug.h>
#elif defined(CONFIG_ARM_64)
@@ -13,6 +15,8 @@
#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
+#define BUG_FRAME_STRUCT
+
struct bug_frame {
signed int loc_disp; /* Relative address to the bug address */
signed int file_disp; /* Relative address to the filename */
@@ -26,13 +30,6 @@ struct bug_frame {
#define bug_line(b) ((b)->line)
#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
-#define BUGFRAME_run_fn 0
-#define BUGFRAME_warn 1
-#define BUGFRAME_bug 2
-#define BUGFRAME_assert 3
-
-#define BUGFRAME_NR 4
-
/* Many versions of GCC doesn't support the asm %c parameter which would
* be preferable to this unpleasantness. We use mergeable string
* sections to avoid multiple copies of the string appearing in the
@@ -89,12 +86,6 @@ struct bug_frame {
unreachable(); \
} while (0)
-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 /* __ARM_BUG_H__ */
/*
* Local variables:
diff --git a/xen/arch/arm/include/asm/div64.h b/xen/arch/arm/include/asm/div64.h
index 1cd58bc51a..fc667a80f9 100644
--- a/xen/arch/arm/include/asm/div64.h
+++ b/xen/arch/arm/include/asm/div64.h
@@ -74,7 +74,7 @@
#elif __GNUC__ >= 4
-#include <asm/bug.h>
+#include <xen/bug.h>
/*
* If the divisor happens to be constant, we determine the appropriate
diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
index 1a99d3a8b4..c90e88fddb 100644
--- a/xen/arch/arm/vgic/vgic-v2.c
+++ b/xen/arch/arm/vgic/vgic-v2.c
@@ -16,8 +16,8 @@
*/
#include <asm/new_vgic.h>
-#include <asm/bug.h>
#include <asm/gic.h>
+#include <xen/bug.h>
#include <xen/sched.h>
#include <xen/sizes.h>
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index f0f2ea5021..b9463a5f27 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -15,9 +15,9 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
+#include <xen/bug.h>
#include <xen/list_sort.h>
#include <xen/sched.h>
-#include <asm/bug.h>
#include <asm/event.h>
#include <asm/new_vgic.h>
diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index c27cbb2304..18ff2a443b 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -27,6 +27,7 @@
* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
*/
+#include <xen/bug.h>
#include <xen/types.h>
#include <xen/errno.h>
#include <xen/delay.h>
@@ -35,7 +36,6 @@
#include <xen/sched.h>
#include <xen/timer.h>
#include <xen/xmalloc.h>
-#include <asm/bug.h>
#include <asm/msr.h>
#include <asm/io.h>
#include <asm/processor.h>
diff --git a/xen/arch/x86/include/asm/asm_defns.h b/xen/arch/x86/include/asm/asm_defns.h
index d9431180cf..a8526cf36c 100644
--- a/xen/arch/x86/include/asm/asm_defns.h
+++ b/xen/arch/x86/include/asm/asm_defns.h
@@ -6,7 +6,7 @@
/* NB. Auto-generated from arch/.../asm-offsets.c */
#include <asm/asm-offsets.h>
#endif
-#include <asm/bug.h>
+#include <xen/bug.h>
#include <asm/x86-defns.h>
#include <xen/stringify.h>
#include <asm/cpufeature.h>
diff --git a/xen/arch/x86/include/asm/bug.h b/xen/arch/x86/include/asm/bug.h
index b7265bdfbe..79133e53e7 100644
--- a/xen/arch/x86/include/asm/bug.h
+++ b/xen/arch/x86/include/asm/bug.h
@@ -5,15 +5,10 @@
#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__
+#define BUG_FRAME_STRUCT
+
struct bug_frame {
signed int loc_disp:BUG_DISP_WIDTH;
unsigned int line_hi:BUG_LINE_HI_WIDTH;
@@ -80,12 +75,6 @@ struct bug_frame {
unreachable(); \
} while (0)
-extern const struct bug_frame __start_bug_frames[],
- __stop_bug_frames_0[],
- __stop_bug_frames_1[],
- __stop_bug_frames_2[],
- __stop_bug_frames_3[];
-
#else /* !__ASSEMBLY__ */
/*
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index a94520ee57..354f78580b 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -26,6 +26,7 @@
* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
*/
+#include <xen/bug.h>
#include <xen/types.h>
#include <xen/errno.h>
#include <xen/delay.h>
@@ -39,7 +40,6 @@
#include <xen/guest_access.h>
#include <xen/domain.h>
#include <xen/cpu.h>
-#include <asm/bug.h>
#include <asm/io.h>
#include <asm/processor.h>
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 05ee1e18af..e914ccade0 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -24,12 +24,12 @@
#ifndef __ASSEMBLY__
+#include <xen/bug.h>
#include <xen/inttypes.h>
#include <xen/stdarg.h>
#include <xen/types.h>
#include <xen/xmalloc.h>
#include <xen/string.h>
-#include <asm/bug.h>
#define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
#define WARN_ON(p) ({ \
--
2.39.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v5 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-03 10:38 [PATCH v5 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-03-03 10:38 ` [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-03-03 10:38 ` [PATCH v5 2/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
@ 2023-03-03 10:38 ` Oleksii Kurochko
2023-03-03 11:29 ` Jan Beulich
2023-03-03 10:38 ` [PATCH v5 4/4] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
3 siblings, 1 reply; 20+ messages in thread
From: Oleksii Kurochko @ 2023-03-03 10:38 UTC (permalink / raw
To: xen-devel
Cc: Julien Grall, Jan Beulich, Andrew Cooper, Stefano Stabellini,
Gianluca Guida, Oleksii Kurochko, Bertrand Marquis,
Volodymyr Babchuk
The following changes were made:
* make GENERIC_BUG_FRAME mandatory for ARM
* As do_bug_frame() returns -EINVAL in case something goes wrong
otherwise id of bug frame. Thereby 'if' cases where do_bug_frame() was
updated to check if the returned value is less than 0
* Switch ARM's implementation of bug.h macros to generic one
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V5:
* Nothing changed
---
Changes in V4:
* Switch ARM implementation to generic one
* Remove BUG_FN_REG from arm{16,32}/bug.h as it isn't needed after switch to generic implementation
* Update commit message
---
Changes in V3:
* As prototype and what do_bug_frame() returns was changed so patch 3 and 4
was updated to use a new version of do_bug_frame
---
Changes in V2:
* Rename bug_file() in ARM implementation to bug_ptr() as
generic do_bug_frame() uses bug_ptr().
* Remove generic parts from bug.h
* Remove declaration of 'int do_bug_frame(...)'
from <asm/traps.h> as it was introduced in <xen/bug.h>
---
xen/arch/arm/Kconfig | 1 +
xen/arch/arm/arm32/traps.c | 2 +-
xen/arch/arm/include/asm/arm32/bug.h | 2 -
xen/arch/arm/include/asm/arm64/bug.h | 2 -
xen/arch/arm/include/asm/bug.h | 75 +-------------------------
xen/arch/arm/include/asm/traps.h | 2 -
xen/arch/arm/traps.c | 81 +---------------------------
7 files changed, 4 insertions(+), 161 deletions(-)
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 239d3aed3c..aad6644a7b 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -12,6 +12,7 @@ config ARM_64
config ARM
def_bool y
+ select GENERIC_BUG_FRAME
select HAS_ALTERNATIVE
select HAS_DEVICE_TREE
select HAS_PASSTHROUGH
diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
index a2fc1c22cb..61c61132c7 100644
--- a/xen/arch/arm/arm32/traps.c
+++ b/xen/arch/arm/arm32/traps.c
@@ -48,7 +48,7 @@ void do_trap_undefined_instruction(struct cpu_user_regs *regs)
if ( instr != BUG_OPCODE )
goto die;
- if ( do_bug_frame(regs, pc) )
+ if ( do_bug_frame(regs, pc) < 0 )
goto die;
regs->pc += 4;
diff --git a/xen/arch/arm/include/asm/arm32/bug.h b/xen/arch/arm/include/asm/arm32/bug.h
index 25cce151dc..3e66f35969 100644
--- a/xen/arch/arm/include/asm/arm32/bug.h
+++ b/xen/arch/arm/include/asm/arm32/bug.h
@@ -10,6 +10,4 @@
#define BUG_INSTR ".word " __stringify(BUG_OPCODE)
-#define BUG_FN_REG r0
-
#endif /* __ARM_ARM32_BUG_H__ */
diff --git a/xen/arch/arm/include/asm/arm64/bug.h b/xen/arch/arm/include/asm/arm64/bug.h
index 5e11c0dfd5..59f664d7de 100644
--- a/xen/arch/arm/include/asm/arm64/bug.h
+++ b/xen/arch/arm/include/asm/arm64/bug.h
@@ -6,6 +6,4 @@
#define BUG_INSTR "brk " __stringify(BRK_BUG_FRAME_IMM)
-#define BUG_FN_REG x0
-
#endif /* __ARM_ARM64_BUG_H__ */
diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
index 9315662c6e..1d87533044 100644
--- a/xen/arch/arm/include/asm/bug.h
+++ b/xen/arch/arm/include/asm/bug.h
@@ -11,80 +11,7 @@
# error "unknown ARM variant"
#endif
-#define BUG_DISP_WIDTH 24
-#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
-#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
-
-#define BUG_FRAME_STRUCT
-
-struct bug_frame {
- signed int loc_disp; /* Relative address to the bug address */
- signed int file_disp; /* Relative address to the filename */
- signed int msg_disp; /* Relative address to the predicate (for ASSERT) */
- uint16_t line; /* Line number */
- uint32_t pad0:16; /* Padding for 8-bytes align */
-};
-
-#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
-#define bug_file(b) ((const void *)(b) + (b)->file_disp);
-#define bug_line(b) ((b)->line)
-#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
-
-/* Many versions of GCC doesn't support the asm %c parameter which would
- * be preferable to this unpleasantness. We use mergeable string
- * sections to avoid multiple copies of the string appearing in the
- * Xen image. BUGFRAME_run_fn needs to be handled separately.
- */
-#define BUG_FRAME(type, line, file, has_msg, msg) do { \
- BUILD_BUG_ON((line) >> 16); \
- BUILD_BUG_ON((type) >= BUGFRAME_NR); \
- asm ("1:"BUG_INSTR"\n" \
- ".pushsection .rodata.str, \"aMS\", %progbits, 1\n" \
- "2:\t.asciz " __stringify(file) "\n" \
- "3:\n" \
- ".if " #has_msg "\n" \
- "\t.asciz " #msg "\n" \
- ".endif\n" \
- ".popsection\n" \
- ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\
- "4:\n" \
- ".p2align 2\n" \
- ".long (1b - 4b)\n" \
- ".long (2b - 4b)\n" \
- ".long (3b - 4b)\n" \
- ".hword " __stringify(line) ", 0\n" \
- ".popsection"); \
-} while (0)
-
-/*
- * GCC will not allow to use "i" when PIE is enabled (Xen doesn't set the
- * flag but instead rely on the default value from the compiler). So the
- * easiest way to implement run_in_exception_handler() is to pass the to
- * be called function in a fixed register.
- */
-#define run_in_exception_handler(fn) do { \
- asm ("mov " __stringify(BUG_FN_REG) ", %0\n" \
- "1:"BUG_INSTR"\n" \
- ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn) "," \
- " \"a\", %%progbits\n" \
- "2:\n" \
- ".p2align 2\n" \
- ".long (1b - 2b)\n" \
- ".long 0, 0, 0\n" \
- ".popsection" :: "r" (fn) : __stringify(BUG_FN_REG) ); \
-} while (0)
-
-#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")
-
-#define BUG() do { \
- BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, ""); \
- unreachable(); \
-} while (0)
-
-#define assert_failed(msg) do { \
- BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
- unreachable(); \
-} while (0)
+#define BUG_ASM_CONST "c"
#endif /* __ARM_BUG_H__ */
/*
diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
index 883dae368e..c6518008ec 100644
--- a/xen/arch/arm/include/asm/traps.h
+++ b/xen/arch/arm/include/asm/traps.h
@@ -69,8 +69,6 @@ void do_cp(struct cpu_user_regs *regs, const union hsr hsr);
void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
void do_trap_hvc_smccc(struct cpu_user_regs *regs);
-int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc);
-
void noreturn do_unexpected_trap(const char *msg,
const struct cpu_user_regs *regs);
void do_trap_hyp_sync(struct cpu_user_regs *regs);
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 061c92acbd..9c6eb66422 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1197,85 +1197,6 @@ void do_unexpected_trap(const char *msg, const struct cpu_user_regs *regs)
panic("CPU%d: Unexpected Trap: %s\n", smp_processor_id(), msg);
}
-int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
-{
- const struct bug_frame *bug = NULL;
- const char *prefix = "", *filename, *predicate;
- unsigned long fixup;
- int id = -1, lineno;
- 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 ( ((vaddr_t)bug_loc(b)) == pc )
- {
- bug = b;
- goto found;
- }
- }
- }
- }
- found:
- if ( !bug )
- return -ENOENT;
-
- if ( id == BUGFRAME_run_fn )
- {
- void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;
-
- fn(regs);
- return 0;
- }
-
- /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
- filename = bug_file(bug);
- if ( !is_kernel(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;
-}
-
#ifdef CONFIG_ARM_64
static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr)
{
@@ -1292,7 +1213,7 @@ static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr)
switch ( hsr.brk.comment )
{
case BRK_BUG_FRAME_IMM:
- if ( do_bug_frame(regs, regs->pc) )
+ if ( do_bug_frame(regs, regs->pc) < 0 )
goto die;
regs->pc += 4;
--
2.39.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v5 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
2023-03-03 10:38 [PATCH v5 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
` (2 preceding siblings ...)
2023-03-03 10:38 ` [PATCH v5 3/4] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
@ 2023-03-03 10:38 ` Oleksii Kurochko
2023-03-06 10:36 ` Jan Beulich
3 siblings, 1 reply; 20+ messages in thread
From: Oleksii Kurochko @ 2023-03-03 10:38 UTC (permalink / raw
To: xen-devel
Cc: Julien Grall, Jan Beulich, Andrew Cooper, Stefano Stabellini,
Gianluca Guida, Oleksii Kurochko, Roger Pau Monné, Wei Liu
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>
---
Changes in V5:
* Nothing changed
---
Changes in V4:
* Back comment /* !__ASSEMBLY__ */ for #else case in <asm/bug.h>
* Remove changes related to x86/.../asm/debuger.h as do_bug_frame() prototype
was updated and cpu_user_regs isn't const any more.
---
Changes in V3:
* As prototype and what do_bug_frame() returns was changed so patch 3 and 4
was updated to use a new version of do_bug_frame
* MODIFIER was change to BUG_ASM_CONST to align with generic implementation
---
Changes in V2:
* Remove all unnecessary things from <asm/bug.h> as they were introduced in
<xen/bug.h>.
* Define BUG_INSTR = 'ud2' and MODIFIER = 'c' ( it is needed to skip '$'
when use an imidiate in x86 assembly )
* Update do_invalid_op() to re-use handle_bug_frame() and find_bug_frame()
from generic implemetation of CONFIG_GENERIC_BUG_FRAME
* Code style fixes.
---
xen/arch/x86/Kconfig | 1 +
xen/arch/x86/include/asm/bug.h | 73 ++----------------------------
xen/arch/x86/traps.c | 81 +++-------------------------------
3 files changed, 11 insertions(+), 144 deletions(-)
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 6a7825f4ba..b0ff1f3ee6 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -11,6 +11,7 @@ config X86
select ARCH_MAP_DOMAIN_PAGE
select ARCH_SUPPORTS_INT128
select CORE_PARKING
+ select GENERIC_BUG_FRAME
select HAS_ALTERNATIVE
select HAS_COMPAT
select HAS_CPUFREQ
diff --git a/xen/arch/x86/include/asm/bug.h b/xen/arch/x86/include/asm/bug.h
index 79133e53e7..2a8d3f84b8 100644
--- a/xen/arch/x86/include/asm/bug.h
+++ b/xen/arch/x86/include/asm/bug.h
@@ -1,79 +1,12 @@
#ifndef __X86_BUG_H__
#define __X86_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 BUG_DEBUGGER_TRAP_FATAL(regs) debugger_trap_fatal(X86_EXC_GP,regs)
#ifndef __ASSEMBLY__
-#define 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) ((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)
+#define BUG_INSTR "ud2"
+#define BUG_ASM_CONST "c"
#else /* !__ASSEMBLY__ */
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index cade9e12f8..7c33ebceff 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -24,6 +24,7 @@
* Gareth Hughes <gareth@valinux.com>, May 2000
*/
+#include <xen/bug.h>
#include <xen/init.h>
#include <xen/sched.h>
#include <xen/lib.h>
@@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
void do_invalid_op(struct cpu_user_regs *regs)
{
- const struct bug_frame *bug = NULL;
u8 bug_insn[2];
- const char *prefix = "", *filename, *predicate, *eip = (char *)regs->rip;
- unsigned long fixup;
- int id = -1, lineno;
- const struct virtual_region *region;
+ const char *eip = (char *)regs->rip;
+ int id;
if ( likely(guest_mode(regs)) )
{
@@ -1185,83 +1183,18 @@ void do_invalid_op(struct cpu_user_regs *regs)
memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) )
goto die;
- region = find_text_region(regs->rip);
- 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) == eip )
- {
- bug = b;
- goto found;
- }
- }
- }
- }
-
- found:
- if ( !bug )
+ id = do_bug_frame(regs, regs->rip);
+ if ( id < 0 )
goto die;
- eip += sizeof(bug_insn);
- if ( id == BUGFRAME_run_fn )
- {
- void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
-
- fn(regs);
- fixup_exception_return(regs, (unsigned long)eip);
- return;
- }
- /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
- filename = bug_ptr(bug);
- if ( !is_kernel(filename) && !is_patch(filename) )
- goto die;
- fixup = strlen(filename);
- if ( fixup > 50 )
- {
- filename += fixup - 47;
- prefix = "...";
- }
- lineno = bug_line(bug);
+ eip += sizeof(bug_insn);
switch ( id )
{
+ case BUGFRAME_run_fn:
case BUGFRAME_warn:
- printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
- show_execution_state(regs);
fixup_exception_return(regs, (unsigned long)eip);
return;
-
- case BUGFRAME_bug:
- printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
-
- if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
- return;
-
- 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;
-
- show_execution_state(regs);
- panic("Assertion '%s' failed at %s%s:%d\n",
- predicate, prefix, filename, lineno);
}
die:
--
2.39.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v5 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-03 10:38 ` [PATCH v5 3/4] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
@ 2023-03-03 11:29 ` Jan Beulich
2023-03-03 11:55 ` Oleksii
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2023-03-03 11:29 UTC (permalink / raw
To: Oleksii Kurochko
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, xen-devel
On 03.03.2023 11:38, Oleksii Kurochko wrote:
> The following changes were made:
> * make GENERIC_BUG_FRAME mandatory for ARM
> * As do_bug_frame() returns -EINVAL in case something goes wrong
> otherwise id of bug frame. Thereby 'if' cases where do_bug_frame() was
> updated to check if the returned value is less than 0
> * Switch ARM's implementation of bug.h macros to generic one
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V5:
> * Nothing changed
I'm glad this isn't true, and the change to common/bug.c ...
> ---
> xen/arch/arm/Kconfig | 1 +
> xen/arch/arm/arm32/traps.c | 2 +-
> xen/arch/arm/include/asm/arm32/bug.h | 2 -
> xen/arch/arm/include/asm/arm64/bug.h | 2 -
> xen/arch/arm/include/asm/bug.h | 75 +-------------------------
> xen/arch/arm/include/asm/traps.h | 2 -
> xen/arch/arm/traps.c | 81 +---------------------------
> 7 files changed, 4 insertions(+), 161 deletions(-)
... is gone.
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-03 11:29 ` Jan Beulich
@ 2023-03-03 11:55 ` Oleksii
0 siblings, 0 replies; 20+ messages in thread
From: Oleksii @ 2023-03-03 11:55 UTC (permalink / raw
To: Jan Beulich
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, xen-devel
On Fri, 2023-03-03 at 12:29 +0100, Jan Beulich wrote:
> On 03.03.2023 11:38, Oleksii Kurochko wrote:
> > The following changes were made:
> > * make GENERIC_BUG_FRAME mandatory for ARM
> > * As do_bug_frame() returns -EINVAL in case something goes wrong
> > otherwise id of bug frame. Thereby 'if' cases where
> > do_bug_frame() was
> > updated to check if the returned value is less than 0
> > * Switch ARM's implementation of bug.h macros to generic one
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in V5:
> > * Nothing changed
>
> I'm glad this isn't true, and the change to common/bug.c ...
>
> > ---
> > xen/arch/arm/Kconfig | 1 +
> > xen/arch/arm/arm32/traps.c | 2 +-
> > xen/arch/arm/include/asm/arm32/bug.h | 2 -
> > xen/arch/arm/include/asm/arm64/bug.h | 2 -
> > xen/arch/arm/include/asm/bug.h | 75 +-----------------------
> > --
> > xen/arch/arm/include/asm/traps.h | 2 -
> > xen/arch/arm/traps.c | 81 +-----------------------
> > ----
> > 7 files changed, 4 insertions(+), 161 deletions(-)
>
> ... is gone.
Thanks for clarification.
I had to add this information.
It is gone because the first patch of the patch series was
updated and the changes from common/bug.c which were in
the previous version of the patch has been removed so after
rebase they disappered in the current patch.
~ Oleksii
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-03-03 10:38 ` [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
@ 2023-03-06 10:17 ` Jan Beulich
2023-03-07 11:32 ` Oleksii
2023-03-06 10:41 ` Jan Beulich
1 sibling, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2023-03-06 10:17 UTC (permalink / raw
To: Oleksii Kurochko
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu, xen-devel
On 03.03.2023 11:38, Oleksii Kurochko wrote:
> --- 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
With Arm now also going to use the generic logic, do we actually need this
control anymore (provided things have been proven to work on Arm for the
compiler range we support there)? It looks like because of the way the
series is partitioned it may be necessary transiently, but it should be
possible to drop it again in a new 5th patch.
> --- /dev/null
> +++ b/xen/common/bug.c
> @@ -0,0 +1,103 @@
> +#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>
> +
> +/*
> + * Returns a negative value in case of an error otherwise the bug type.
> + */
Nit: Style (this is a single line comment). But see also below related
comment on the function's declaration.
> +int do_bug_frame(struct cpu_user_regs *regs, unsigned long pc)
> +{
> + 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 )
> + return -EINVAL;
> +
> + for ( id = 0; id < BUGFRAME_NR; id++ )
> + {
> + const struct bug_frame *b;
> + size_t i;
> +
> + for ( i = 0, b = region->frame[id].bugs;
> + i < region->frame[id].n_bugs; b++, i++ )
Nit: Indentation (the "i" on the 2nd line wants to align with that
on the 1st one).
> + {
> + if ( bug_loc(b) == pc )
> + {
> + bug = b;
> + goto found;
> + }
> + }
> + }
> +
> + found:
> + if ( !bug )
> + return -EINVAL;
While I'm generally unhappy with many uses of -EINVAL (it's used to
indicate way too many different kinds of errors), can we at least
consider using -ENOENT here instead? (I'm sorry, I should have asked
for this earlier on.)
> --- /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
> +
> +#ifndef BUG_FRAME_STRUCT
This check won't help when it comes ahead of ...
> +#define BUG_DISP_WIDTH 24
> +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> +#endif
> +
> +#include <asm/bug.h>
... this. But is the #ifdef actually necessary? Or can the #define-s
be moved ...
> +#ifndef BUG_DEBUGGER_TRAP_FATAL
> +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0
> +#endif
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <xen/lib.h>
> +
> +#ifndef BUG_FRAME_STRUCT
... here? (I guess having them defined early, but unconditionally is
better. If an arch wants to override them, they can #undef and then
#define.)
Above from here the "#ifndef __ASSEMBLY__" also wants to move up, to
further enclose BUG_DEBUGGER_TRAP_FATAL (which is useless in assembly
code).
> +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])
> +
> +#endif /* BUG_FRAME_STRUCT */
> +
> +/*
> + * Generic implementation has been based on x86 implementation where
> + * '%c' to deal with punctation sign ($, # depending on architecture)
> + * before immediate.
> + *
> + * Not all architecture's compilers have full support of '%c' and not for all
> + * assemblers punctation sign is used before immediate.
> + * Thereby it was decided to introduce BUG_ASM_CONST.
> + */
I have to admit that I'm not really happy with this comment. At the very
least the last sentence imo doesn't belong there. But overall how about
something like
"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
> +
> +#if !defined(_ASM_BUGFRAME_TEXT) || !defined(_ASM_BUGFRAME_INFO)
Please don't make the conditional more complicated than necessary:
Checking for just _ASM_BUGFRAME_TEXT is enough here.
> +#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)
Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At least
the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use
the generic struct: With how you have things right now
BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would also
be at risk of causing false positives. Perhaps it's appropriate to
have a separate #ifdef (incl the distinct identifier used), but that
first BUILD_BUG_ON() needs abstracting out.
Also nit: Style (see ...
> +#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 )
... here). Also, considering the boolean nature, I guess we're in the
process of moving such to be "} while ( false );".
Also please be consistent with formatting of the two adjacent macros,
both indentation-wise and where "do {" is placed. Which of the two
forms you use is secondary.
> +#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;
> +
> +/*
> + * Returns a negative value in case of an error otherwise the bug type.
> + */
Perhaps add "(BUGFRAME_*)", which would then also make this a properly
multi-line comment (right now it's a style violation, as is the case
for the function definition as pointed out above)?
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 2/4] xen: change <asm/bug.h> to <xen/bug.h>
2023-03-03 10:38 ` [PATCH v5 2/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
@ 2023-03-06 10:24 ` Jan Beulich
0 siblings, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2023-03-06 10:24 UTC (permalink / raw
To: Oleksii Kurochko
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, George Dunlap, Wei Liu,
Roger Pau Monné, xen-devel
On 03.03.2023 11:38, Oleksii Kurochko wrote:
> The idea of the patch is to change all <asm/bug.h> to <xen/bug.h> and
> keep Xen compilable with adding only minimal amount of changes:
> 1. It was added "#include <xen/types.h>" to ARM's "<asm/bug.h>" as it
> uses uint_{16,32}t in 'struct bug_frame'.
> 2. It was added '#define BUG_FRAME_STRUCT' which means that ARM hasn't
> been switched to generic implementation yet.
> 3. It was added '#define BUG_FRAME_STRUCT' which means that x86 hasn't
> been switched to generic implementation yet.
> 4. BUGFRAME_* and _start_bug_frame[], _stop_bug_frame_*[] were removed
> for ARM & x86 to deal with compilation errors such as:
> redundant redeclaration of ...
>
> In the following two patches x86 and ARM archictectures will be
> switched fully:
> * xen/arm: switch ARM to use generic implementation of bug.h
> * xen/x86: switch x86 to use generic implemetation of bug.h
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V5:
> - Nothing changed
> ---
> Changes in V4:
> - defines BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH were moved into
> "ifndef BUG_FRAME_STRUCT" in <xen/bug.h> as they are specific for 'struct bug_frame' and so should
> co-exist together. So the defines were back to <asm/bug.h> until BUG_FRAME_STRUCT will be defined in
> <asm/bug.h>.
> - Update the comment message.
> ---
> Changes in V3:
> * Update patch 2 not to break compilation: move some parts from patches 3 and 4
> to patch 2:
> * move some generic parts from <asm/bug.h> to <xen/bug.h>
> * add define BUG_FRAME_STRUCT in ARM's <asm/bug.h>
> ---
> Changes in V2:
> * Put [PATCH v1 4/4] xen: change <asm/bug.h> to <xen/bug.h> as second patch,
> update the patch to change all <asm/bug.h> to <xen/bug.h> among the whole project
> to not break build.
> * Update the commit message.
> ---
> xen/arch/arm/include/asm/bug.h | 17 ++++-------------
> xen/arch/arm/include/asm/div64.h | 2 +-
> xen/arch/arm/vgic/vgic-v2.c | 2 +-
> xen/arch/arm/vgic/vgic.c | 2 +-
> xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +-
> xen/arch/x86/include/asm/asm_defns.h | 2 +-
> xen/arch/x86/include/asm/bug.h | 15 ++-------------
> xen/drivers/cpufreq/cpufreq.c | 2 +-
> xen/include/xen/lib.h | 2 +-
> 9 files changed, 13 insertions(+), 33 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
> index f4088d0913..9315662c6e 100644
> --- a/xen/arch/arm/include/asm/bug.h
> +++ b/xen/arch/arm/include/asm/bug.h
> @@ -1,6 +1,8 @@
> #ifndef __ARM_BUG_H__
> #define __ARM_BUG_H__
>
> +#include <xen/types.h>
> +
> #if defined(CONFIG_ARM_32)
> # include <asm/arm32/bug.h>
> #elif defined(CONFIG_ARM_64)
> @@ -13,6 +15,8 @@
> #define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> #define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
>
> +#define BUG_FRAME_STRUCT
> +
> struct bug_frame {
> signed int loc_disp; /* Relative address to the bug address */
> signed int file_disp; /* Relative address to the filename */
> @@ -26,13 +30,6 @@ struct bug_frame {
> #define bug_line(b) ((b)->line)
> #define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
>
> -#define BUGFRAME_run_fn 0
> -#define BUGFRAME_warn 1
> -#define BUGFRAME_bug 2
> -#define BUGFRAME_assert 3
> -
> -#define BUGFRAME_NR 4
> -
> /* Many versions of GCC doesn't support the asm %c parameter which would
> * be preferable to this unpleasantness. We use mergeable string
> * sections to avoid multiple copies of the string appearing in the
> @@ -89,12 +86,6 @@ struct bug_frame {
> unreachable(); \
> } while (0)
>
> -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 /* __ARM_BUG_H__ */
> /*
> * Local variables:
> diff --git a/xen/arch/arm/include/asm/div64.h b/xen/arch/arm/include/asm/div64.h
> index 1cd58bc51a..fc667a80f9 100644
> --- a/xen/arch/arm/include/asm/div64.h
> +++ b/xen/arch/arm/include/asm/div64.h
> @@ -74,7 +74,7 @@
>
> #elif __GNUC__ >= 4
>
> -#include <asm/bug.h>
> +#include <xen/bug.h>
>
> /*
> * If the divisor happens to be constant, we determine the appropriate
> diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
> index 1a99d3a8b4..c90e88fddb 100644
> --- a/xen/arch/arm/vgic/vgic-v2.c
> +++ b/xen/arch/arm/vgic/vgic-v2.c
> @@ -16,8 +16,8 @@
> */
>
> #include <asm/new_vgic.h>
> -#include <asm/bug.h>
> #include <asm/gic.h>
> +#include <xen/bug.h>
> #include <xen/sched.h>
> #include <xen/sizes.h>
>
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index f0f2ea5021..b9463a5f27 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -15,9 +15,9 @@
> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include <xen/bug.h>
> #include <xen/list_sort.h>
> #include <xen/sched.h>
> -#include <asm/bug.h>
> #include <asm/event.h>
> #include <asm/new_vgic.h>
>
> diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> index c27cbb2304..18ff2a443b 100644
> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> @@ -27,6 +27,7 @@
> * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> */
>
> +#include <xen/bug.h>
> #include <xen/types.h>
> #include <xen/errno.h>
> #include <xen/delay.h>
> @@ -35,7 +36,6 @@
> #include <xen/sched.h>
> #include <xen/timer.h>
> #include <xen/xmalloc.h>
> -#include <asm/bug.h>
> #include <asm/msr.h>
> #include <asm/io.h>
> #include <asm/processor.h>
> diff --git a/xen/arch/x86/include/asm/asm_defns.h b/xen/arch/x86/include/asm/asm_defns.h
> index d9431180cf..a8526cf36c 100644
> --- a/xen/arch/x86/include/asm/asm_defns.h
> +++ b/xen/arch/x86/include/asm/asm_defns.h
> @@ -6,7 +6,7 @@
> /* NB. Auto-generated from arch/.../asm-offsets.c */
> #include <asm/asm-offsets.h>
> #endif
> -#include <asm/bug.h>
> +#include <xen/bug.h>
> #include <asm/x86-defns.h>
> #include <xen/stringify.h>
> #include <asm/cpufeature.h>
While there's an unhelpful mix of asm/ and xen/ here already, may I ask
that you try to avoid making things yet worse: Unless there's a reason
not to, please move the added line past asm/x86-defns.h, adjacent to
xen/stringify.h. Then non-Arm parts
Acked-by: Jan Beulich <jbeulich@suse.com>
(assuming of course no further large rework is necessary because of the
comments on patch 1).
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
2023-03-03 10:38 ` [PATCH v5 4/4] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
@ 2023-03-06 10:36 ` Jan Beulich
2023-03-07 12:52 ` Oleksii
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2023-03-06 10:36 UTC (permalink / raw
To: Oleksii Kurochko
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Roger Pau Monné, Wei Liu, xen-devel
On 03.03.2023 11:38, 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>.
Is this part of the description stale? The patch ...
> ---
> xen/arch/x86/Kconfig | 1 +
> xen/arch/x86/include/asm/bug.h | 73 ++----------------------------
> xen/arch/x86/traps.c | 81 +++-------------------------------
> 3 files changed, 11 insertions(+), 144 deletions(-)
... doesn't touch asm/debugger.h at all.
> --- a/xen/arch/x86/include/asm/bug.h
> +++ b/xen/arch/x86/include/asm/bug.h
> @@ -1,79 +1,12 @@
> #ifndef __X86_BUG_H__
> #define __X86_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 BUG_DEBUGGER_TRAP_FATAL(regs) debugger_trap_fatal(X86_EXC_GP,regs)
Along the lines of a comment on an earlier patch, please move this ...
> #ifndef __ASSEMBLY__
... into the thus guarded section.
> @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
>
> void do_invalid_op(struct cpu_user_regs *regs)
> {
> - const struct bug_frame *bug = NULL;
> u8 bug_insn[2];
> - const char *prefix = "", *filename, *predicate, *eip = (char *)regs->rip;
> - unsigned long fixup;
> - int id = -1, lineno;
> - const struct virtual_region *region;
> + const char *eip = (char *)regs->rip;
I realize "char" was used before, but now that this is all on its own,
can this at least become "unsigned char", but yet better "void"?
> @@ -1185,83 +1183,18 @@ void do_invalid_op(struct cpu_user_regs *regs)
> memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) )
> goto die;
>
> - region = find_text_region(regs->rip);
> - 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) == eip )
> - {
> - bug = b;
> - goto found;
> - }
> - }
> - }
> - }
> -
> - found:
> - if ( !bug )
> + id = do_bug_frame(regs, regs->rip);
> + if ( id < 0 )
> goto die;
> - eip += sizeof(bug_insn);
> - if ( id == BUGFRAME_run_fn )
> - {
> - void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
> -
> - fn(regs);
> - fixup_exception_return(regs, (unsigned long)eip);
> - return;
> - }
>
> - /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> - filename = bug_ptr(bug);
> - if ( !is_kernel(filename) && !is_patch(filename) )
> - goto die;
> - fixup = strlen(filename);
> - if ( fixup > 50 )
> - {
> - filename += fixup - 47;
> - prefix = "...";
> - }
> - lineno = bug_line(bug);
> + eip += sizeof(bug_insn);
>
> switch ( id )
> {
> + case BUGFRAME_run_fn:
> case BUGFRAME_warn:
> - printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
> - show_execution_state(regs);
> fixup_exception_return(regs, (unsigned long)eip);
> return;
> -
> - case BUGFRAME_bug:
> - printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> -
> - if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
> - return;
This and ...
> - 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;
... this return look to have no proper representation in the new
logic; both cases fall through to ...
> - show_execution_state(regs);
> - panic("Assertion '%s' failed at %s%s:%d\n",
> - predicate, prefix, filename, lineno);
> }
>
> die:
... here now, which is an unwanted functional change.
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-03-03 10:38 ` [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-03-06 10:17 ` Jan Beulich
@ 2023-03-06 10:41 ` Jan Beulich
2023-03-07 11:34 ` Oleksii
1 sibling, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2023-03-06 10:41 UTC (permalink / raw
To: Oleksii Kurochko
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu, xen-devel
On 03.03.2023 11:38, Oleksii Kurochko wrote:
> +int do_bug_frame(struct cpu_user_regs *regs, unsigned long pc)
> +{
> + 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 )
> + return -EINVAL;
> +
> + for ( id = 0; id < BUGFRAME_NR; id++ )
> + {
> + const struct bug_frame *b;
> + size_t 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;
> + }
> + }
> + }
> +
> + found:
> + if ( !bug )
> + return -EINVAL;
> +
> + if ( id == BUGFRAME_run_fn )
> + {
> + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> +
> + 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 ( BUG_DEBUGGER_TRAP_FATAL(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 ( BUG_DEBUGGER_TRAP_FATAL(regs) )
> + return id;
> +
> + show_execution_state(regs);
> + panic("Assertion '%s' failed at %s%s:%d\n",
> + predicate, prefix, filename, lineno);
> + }
> +
> + return id;
> +}
This final "return" looks like almost dead code (it isn't when an
unrecognized id is found). May I suggest to switch all "return id"
inside the switch() block to just "break", to make this final
"return" be (a) the central one and (b) more obviously a used path?
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-03-06 10:17 ` Jan Beulich
@ 2023-03-07 11:32 ` Oleksii
2023-03-07 12:54 ` Jan Beulich
2023-03-07 13:13 ` Oleksii
0 siblings, 2 replies; 20+ messages in thread
From: Oleksii @ 2023-03-07 11:32 UTC (permalink / raw
To: Jan Beulich
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu, xen-devel
On Mon, 2023-03-06 at 11:17 +0100, Jan Beulich wrote:
> > On 03.03.2023 11:38, Oleksii Kurochko wrote:
> > > > --- 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
> >
> > With Arm now also going to use the generic logic, do we actually
> > need
> > this
> > control anymore (provided things have been proven to work on Arm
> > for
> > the
> > compiler range we support there)? It looks like because of the way
> > the
> > series is partitioned it may be necessary transiently, but it
> > should
> > be
> > possible to drop it again in a new 5th patch.
We still need it because RISC-V doesn't ready yet to use do_bug_frame()
from xen/common/bug.c as it requires find_text_region(),
virtual_region() and panic().
The mentioned functionality isn't ready now for RISC-V so RISC-V will
use only generic things from <xen/bug.h> only for some time.
Another one reason I would like to leave the config it is probably the
same situation will be for future architectures.
This is not something mandatory if the config isn't really necessary
than it should be removed after RISC-V will be ready to migrate full on
generic implementation.
> >
> > > > --- /dev/null
> > > > +++ b/xen/common/bug.c
> > > > @@ -0,0 +1,103 @@
> > > > +#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>
> > > > +
> > > > +/*
> > > > + * Returns a negative value in case of an error otherwise the
> > > > bug
> > > > type.
> > > > + */
> >
> > Nit: Style (this is a single line comment). But see also below
> > related
> > comment on the function's declaration.
Thanks. I'll fix that.
> >
> > > > +int do_bug_frame(struct cpu_user_regs *regs, unsigned long pc)
> > > > +{
> > > > + 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 )
> > > > + return -EINVAL;
> > > > +
> > > > + for ( id = 0; id < BUGFRAME_NR; id++ )
> > > > + {
> > > > + const struct bug_frame *b;
> > > > + size_t i;
> > > > +
> > > > + for ( i = 0, b = region->frame[id].bugs;
> > > > + i < region->frame[id].n_bugs; b++, i++ )
> >
> > Nit: Indentation (the "i" on the 2nd line wants to align with that
> > on the 1st one).
Thanks. I'll update the indentation.
> >
> > > > + {
> > > > + if ( bug_loc(b) == pc )
> > > > + {
> > > > + bug = b;
> > > > + goto found;
> > > > + }
> > > > + }
> > > > + }
> > > > +
> > > > + found:
> > > > + if ( !bug )
> > > > + return -EINVAL;
> >
> > While I'm generally unhappy with many uses of -EINVAL (it's used to
> > indicate way too many different kinds of errors), can we at least
> > consider using -ENOENT here instead? (I'm sorry, I should have
> > asked
> > for this earlier on.)
Sure. Done.
> >
> > > > --- /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
> > > > +
> > > > +#ifndef BUG_FRAME_STRUCT
> >
> > This check won't help when it comes ahead of ...
> >
> > > > +#define BUG_DISP_WIDTH 24
> > > > +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > > > +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> > > > +#endif
> > > > +
> > > > +#include <asm/bug.h>
> >
> > ... this. But is the #ifdef actually necessary? Or can the #define-
> > s
> > be moved ...
> >
> > > > +#ifndef BUG_DEBUGGER_TRAP_FATAL
> > > > +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0
> > > > +#endif
> > > > +
> > > > +#ifndef __ASSEMBLY__
> > > > +
> > > > +#include <xen/lib.h>
> > > > +
> > > > +#ifndef BUG_FRAME_STRUCT
> >
> > ... here? (I guess having them defined early, but unconditionally
> > is
> > better. If an arch wants to override them, they can #undef and then
> > #define.)
We can't move it to #indef __ASSEMBLY__ because in this case x86
compilation will fail as in x86's <asm/bug.h> BUG_DISP_WIDTH,
BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH are used in case when the header
is included in assembly code.
I agree that there is no any sense to have the defines under "#ifndef
BUG_FRAME_STUCT" before <asm/bug.h> but it is necessary to define them
before <asm/bug.h>. The defines were put under "#ifndef
BUG_FRAME_STUCT" because it seems to me that logically the defines
should go only with definition of 'struct bug_frame'.
So it looks like the only one way we have. It is define them
unconditionally before <asm/bug.h> and #undef if it will be necessary
for specific architecture.
As an option we can move the defines to #ifndef BUG_FRAME_STRUCT under
#ifndef __ASSEMBLY__ and then define them explicitly in x86's
<asm/bug.h> for case when the header is included some where in assembly
code. But this option looks really weird.
> >
> > Above from here the "#ifndef __ASSEMBLY__" also wants to move up,
> > to
> > further enclose BUG_DEBUGGER_TRAP_FATAL (which is useless in
> > assembly
> > code).
Yeah, sure. Will move up "#ifndef __ASSEMBLY__"
> >
> > > > +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])
> > > > +
> > > > +#endif /* BUG_FRAME_STRUCT */
> > > > +
> > > > +/*
> > > > + * Generic implementation has been based on x86 implementation
> > > > where
> > > > + * '%c' to deal with punctation sign ($, # depending on
> > > > architecture)
> > > > + * before immediate.
> > > > + *
> > > > + * Not all architecture's compilers have full support of '%c'
> > > > and
> > > > not for all
> > > > + * assemblers punctation sign is used before immediate.
> > > > + * Thereby it was decided to introduce BUG_ASM_CONST.
> > > > + */
> >
> > I have to admit that I'm not really happy with this comment. At the
> > very
> > least the last sentence imo doesn't belong there. But overall how
> > about
> > something like
> >
> > "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."
Your comment looks really better. So I'll use it.
> >
> > > > +#ifndef BUG_ASM_CONST
> > > > +#define BUG_ASM_CONST ""
> > > > +#endif
> > > > +
> > > > +#if !defined(_ASM_BUGFRAME_TEXT) ||
> > > > !defined(_ASM_BUGFRAME_INFO)
> >
> > Please don't make the conditional more complicated than necessary:
> > Checking for just _ASM_BUGFRAME_TEXT is enough here.
Sure. I'll update that.
> >
> > > > +#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)
> >
> > Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At
> > least
> > the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use
> > the generic struct: With how you have things right now
> > BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would also
> > be at risk of causing false positives. Perhaps it's appropriate to
> > have a separate #ifdef (incl the distinct identifier used), but
> > that
> > first BUILD_BUG_ON() needs abstracting out.
Missed that. Thanks.
I'll introduce that a separate #ifdef before BUG_FRAME:
#ifndef BUILD_BUG_ON_LINE_WIDTH
#define BUILD_BUG_ON_LINE_WIDTH \
BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH))
#endif
> >
> > Also nit: Style (see ...
> >
> > > > +#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 )
> >
> > ... here). Also, considering the boolean nature, I guess we're in
> > the
> > process of moving such to be "} while ( false );".
> >
> > Also please be consistent with formatting of the two adjacent
> > macros,
> > both indentation-wise and where "do {" is placed. Which of the two
> > forms you use is secondary.
Thanks. Done.
> >
> > > > +#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;
> > > > +
> > > > +/*
> > > > + * Returns a negative value in case of an error otherwise the
> > > > bug
> > > > type.
> > > > + */
> >
> > Perhaps add "(BUGFRAME_*)", which would then also make this a
> > properly
> > multi-line comment (right now it's a style violation, as is the
> > case
> > for the function definition as pointed out above)?
> >
Updated. Thanks.
~ Oleksii
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-03-06 10:41 ` Jan Beulich
@ 2023-03-07 11:34 ` Oleksii
0 siblings, 0 replies; 20+ messages in thread
From: Oleksii @ 2023-03-07 11:34 UTC (permalink / raw
To: Jan Beulich
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu, xen-devel
On Mon, 2023-03-06 at 11:41 +0100, Jan Beulich wrote:
> On 03.03.2023 11:38, Oleksii Kurochko wrote:
> > +int do_bug_frame(struct cpu_user_regs *regs, unsigned long pc)
> > +{
> > + 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 )
> > + return -EINVAL;
> > +
> > + for ( id = 0; id < BUGFRAME_NR; id++ )
> > + {
> > + const struct bug_frame *b;
> > + size_t 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;
> > + }
> > + }
> > + }
> > +
> > + found:
> > + if ( !bug )
> > + return -EINVAL;
> > +
> > + if ( id == BUGFRAME_run_fn )
> > + {
> > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> > +
> > + 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 ( BUG_DEBUGGER_TRAP_FATAL(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 ( BUG_DEBUGGER_TRAP_FATAL(regs) )
> > + return id;
> > +
> > + show_execution_state(regs);
> > + panic("Assertion '%s' failed at %s%s:%d\n",
> > + predicate, prefix, filename, lineno);
> > + }
> > +
> > + return id;
> > +}
>
> This final "return" looks like almost dead code (it isn't when an
> unrecognized id is found). May I suggest to switch all "return id"
> inside the switch() block to just "break", to make this final
> "return" be (a) the central one and (b) more obviously a used path?
>
Sure It will be much better.
~ Oleksii
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
2023-03-06 10:36 ` Jan Beulich
@ 2023-03-07 12:52 ` Oleksii
2023-03-07 12:59 ` Jan Beulich
0 siblings, 1 reply; 20+ messages in thread
From: Oleksii @ 2023-03-07 12:52 UTC (permalink / raw
To: Jan Beulich
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Roger Pau Monné, Wei Liu, xen-devel
On Mon, 2023-03-06 at 11:36 +0100, Jan Beulich wrote:
> On 03.03.2023 11:38, 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>.
>
> Is this part of the description stale? The patch ...
It is stale. Updated now.
>
> > ---
> > xen/arch/x86/Kconfig | 1 +
> > xen/arch/x86/include/asm/bug.h | 73 ++----------------------------
> > xen/arch/x86/traps.c | 81 +++---------------------------
> > ----
> > 3 files changed, 11 insertions(+), 144 deletions(-)
>
> ... doesn't touch asm/debugger.h at all.
>
> > --- a/xen/arch/x86/include/asm/bug.h
> > +++ b/xen/arch/x86/include/asm/bug.h
> > @@ -1,79 +1,12 @@
> > #ifndef __X86_BUG_H__
> > #define __X86_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 BUG_DEBUGGER_TRAP_FATAL(regs)
> > debugger_trap_fatal(X86_EXC_GP,regs)
>
> Along the lines of a comment on an earlier patch, please move this
> ...
>
> > #ifndef __ASSEMBLY__
>
> ... into the thus guarded section.
But it was defined there before the patch series and these definies are
used in "#else /* !__ASSEMBLY__ */"
>
> > @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct
> > vcpu *v, uint32_t leaf,
> >
> > void do_invalid_op(struct cpu_user_regs *regs)
> > {
> > - const struct bug_frame *bug = NULL;
> > u8 bug_insn[2];
> > - const char *prefix = "", *filename, *predicate, *eip = (char
> > *)regs->rip;
> > - unsigned long fixup;
> > - int id = -1, lineno;
> > - const struct virtual_region *region;
> > + const char *eip = (char *)regs->rip;
>
> I realize "char" was used before, but now that this is all on its
> own,
> can this at least become "unsigned char", but yet better "void"?
If we change it to "void" shouldn't it require additional casts here (
which is not nice ):
eip += sizeof(bug_insn);
Probably 'unsgined char' will be better.
>
> > @@ -1185,83 +1183,18 @@ void do_invalid_op(struct cpu_user_regs
> > *regs)
> > memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) )
> > goto die;
> >
> > - region = find_text_region(regs->rip);
> > - 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) == eip )
> > - {
> > - bug = b;
> > - goto found;
> > - }
> > - }
> > - }
> > - }
> > -
> > - found:
> > - if ( !bug )
> > + id = do_bug_frame(regs, regs->rip);
> > + if ( id < 0 )
> > goto die;
> > - eip += sizeof(bug_insn);
> > - if ( id == BUGFRAME_run_fn )
> > - {
> > - void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
> > -
> > - fn(regs);
> > - fixup_exception_return(regs, (unsigned long)eip);
> > - return;
> > - }
> >
> > - /* WARN, BUG or ASSERT: decode the filename pointer and line
> > number. */
> > - filename = bug_ptr(bug);
> > - if ( !is_kernel(filename) && !is_patch(filename) )
> > - goto die;
> > - fixup = strlen(filename);
> > - if ( fixup > 50 )
> > - {
> > - filename += fixup - 47;
> > - prefix = "...";
> > - }
> > - lineno = bug_line(bug);
> > + eip += sizeof(bug_insn);
> >
> > switch ( id )
> > {
> > + case BUGFRAME_run_fn:
> > case BUGFRAME_warn:
> > - printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
> > - show_execution_state(regs);
> > fixup_exception_return(regs, (unsigned long)eip);
> > return;
> > -
> > - case BUGFRAME_bug:
> > - printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> > -
> > - if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
> > - return;
>
> This and ...
>
> > - 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;
>
> ... this return look to have no proper representation in the new
> logic; both cases fall through to ...
>
> > - show_execution_state(regs);
> > - panic("Assertion '%s' failed at %s%s:%d\n",
> > - predicate, prefix, filename, lineno);
> > }
> >
> > die:
>
> ... here now, which is an unwanted functional change.
Oh, you are right. So then in case we have correct id it is needed to
do only return:
switch ( id )
{
case BUGFRAME_run_fn:
case BUGFRAME_warn:
fixup_exception_return(regs, (unsigned long)eip);
break;
}
return;
Thanks for finding that.
~ Oleksii
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-03-07 11:32 ` Oleksii
@ 2023-03-07 12:54 ` Jan Beulich
2023-03-07 13:13 ` Oleksii
1 sibling, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2023-03-07 12:54 UTC (permalink / raw
To: Oleksii
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu, xen-devel
On 07.03.2023 12:32, Oleksii wrote:
> On Mon, 2023-03-06 at 11:17 +0100, Jan Beulich wrote:
>>> On 03.03.2023 11:38, Oleksii Kurochko wrote:
>>>>> --- 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
>>>
>>> With Arm now also going to use the generic logic, do we actually
>>> need
>>> this
>>> control anymore (provided things have been proven to work on Arm
>>> for
>>> the
>>> compiler range we support there)? It looks like because of the way
>>> the
>>> series is partitioned it may be necessary transiently, but it
>>> should
>>> be
>>> possible to drop it again in a new 5th patch.
> We still need it because RISC-V doesn't ready yet to use do_bug_frame()
> from xen/common/bug.c as it requires find_text_region(),
> virtual_region() and panic().
> The mentioned functionality isn't ready now for RISC-V so RISC-V will
> use only generic things from <xen/bug.h> only for some time.
Oh, right. So let's leave it for the time being, but consider dropping
it once RISC-V is more complete.
>>>>> --- /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
>>>>> +
>>>>> +#ifndef BUG_FRAME_STRUCT
>>>
>>> This check won't help when it comes ahead of ...
>>>
>>>>> +#define BUG_DISP_WIDTH 24
>>>>> +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
>>>>> +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
>>>>> +#endif
>>>>> +
>>>>> +#include <asm/bug.h>
>>>
>>> ... this. But is the #ifdef actually necessary? Or can the #define-
>>> s
>>> be moved ...
>>>
>>>>> +#ifndef BUG_DEBUGGER_TRAP_FATAL
>>>>> +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0
>>>>> +#endif
>>>>> +
>>>>> +#ifndef __ASSEMBLY__
>>>>> +
>>>>> +#include <xen/lib.h>
>>>>> +
>>>>> +#ifndef BUG_FRAME_STRUCT
>>>
>>> ... here? (I guess having them defined early, but unconditionally
>>> is
>>> better. If an arch wants to override them, they can #undef and then
>>> #define.)
> We can't move it to #indef __ASSEMBLY__ because in this case x86
> compilation will fail as in x86's <asm/bug.h> BUG_DISP_WIDTH,
> BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH are used in case when the header
> is included in assembly code.
Oh, of course.
> I agree that there is no any sense to have the defines under "#ifndef
> BUG_FRAME_STUCT" before <asm/bug.h> but it is necessary to define them
> before <asm/bug.h>. The defines were put under "#ifndef
> BUG_FRAME_STUCT" because it seems to me that logically the defines
> should go only with definition of 'struct bug_frame'.
>
> So it looks like the only one way we have. It is define them
> unconditionally before <asm/bug.h> and #undef if it will be necessary
> for specific architecture.
> As an option we can move the defines to #ifndef BUG_FRAME_STRUCT under
> #ifndef __ASSEMBLY__ and then define them explicitly in x86's
> <asm/bug.h> for case when the header is included some where in assembly
> code. But this option looks really weird.
Indeed.
>>>>> +#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)
>>>
>>> Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At
>>> least
>>> the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use
>>> the generic struct: With how you have things right now
>>> BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would also
>>> be at risk of causing false positives. Perhaps it's appropriate to
>>> have a separate #ifdef (incl the distinct identifier used), but
>>> that
>>> first BUILD_BUG_ON() needs abstracting out.
> Missed that. Thanks.
> I'll introduce that a separate #ifdef before BUG_FRAME:
>
> #ifndef BUILD_BUG_ON_LINE_WIDTH
> #define BUILD_BUG_ON_LINE_WIDTH \
> BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH))
> #endif
But then please make this a function-like macro.
I'm also not convinced of the #ifndef - an arch using an entirely
different layout should have no need for this check. So I'd rather
attach the #define to what's inside #ifndef BUG_FRAME_STRUCT and
have an #else there to supply a #define to <empty> alternatively.
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
2023-03-07 12:52 ` Oleksii
@ 2023-03-07 12:59 ` Jan Beulich
2023-03-07 13:37 ` Oleksii
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2023-03-07 12:59 UTC (permalink / raw
To: Oleksii
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Roger Pau Monné, Wei Liu, xen-devel
On 07.03.2023 13:52, Oleksii wrote:
> On Mon, 2023-03-06 at 11:36 +0100, Jan Beulich wrote:
>> On 03.03.2023 11:38, Oleksii Kurochko wrote:
>>> --- a/xen/arch/x86/include/asm/bug.h
>>> +++ b/xen/arch/x86/include/asm/bug.h
>>> @@ -1,79 +1,12 @@
>>> #ifndef __X86_BUG_H__
>>> #define __X86_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 BUG_DEBUGGER_TRAP_FATAL(regs)
>>> debugger_trap_fatal(X86_EXC_GP,regs)
>>
>> Along the lines of a comment on an earlier patch, please move this
>> ...
>>
>>> #ifndef __ASSEMBLY__
>>
>> ... into the thus guarded section.
> But it was defined there before the patch series and these definies are
> used in "#else /* !__ASSEMBLY__ */"
Since you use plural, maybe a misunderstanding? My comment was solely
on the line you add; the removed lines are there merely as context.
>>> @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct
>>> vcpu *v, uint32_t leaf,
>>>
>>> void do_invalid_op(struct cpu_user_regs *regs)
>>> {
>>> - const struct bug_frame *bug = NULL;
>>> u8 bug_insn[2];
>>> - const char *prefix = "", *filename, *predicate, *eip = (char
>>> *)regs->rip;
>>> - unsigned long fixup;
>>> - int id = -1, lineno;
>>> - const struct virtual_region *region;
>>> + const char *eip = (char *)regs->rip;
>>
>> I realize "char" was used before, but now that this is all on its
>> own,
>> can this at least become "unsigned char", but yet better "void"?
> If we change it to "void" shouldn't it require additional casts here (
> which is not nice ):
> eip += sizeof(bug_insn);
Arithmetic on pointers to void is a GNU extension which we make
heavy use of all over the hypervisor.
>>> switch ( id )
>>> {
>>> + case BUGFRAME_run_fn:
>>> case BUGFRAME_warn:
>>> - printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
>>> - show_execution_state(regs);
>>> fixup_exception_return(regs, (unsigned long)eip);
>>> return;
>>> -
>>> - case BUGFRAME_bug:
>>> - printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
>>> -
>>> - if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
>>> - return;
>>
>> This and ...
>>
>>> - 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;
>>
>> ... this return look to have no proper representation in the new
>> logic; both cases fall through to ...
>>
>>> - show_execution_state(regs);
>>> - panic("Assertion '%s' failed at %s%s:%d\n",
>>> - predicate, prefix, filename, lineno);
>>> }
>>>
>>> die:
>>
>> ... here now, which is an unwanted functional change.
> Oh, you are right. So then in case we have correct id it is needed to
> do only return:
> switch ( id )
> {
> case BUGFRAME_run_fn:
> case BUGFRAME_warn:
> fixup_exception_return(regs, (unsigned long)eip);
> break;
> }
>
> return;
Except the "return" needs to remain inside the switch; unrecognized id
values should still fall through to the "die" label.
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-03-07 11:32 ` Oleksii
2023-03-07 12:54 ` Jan Beulich
@ 2023-03-07 13:13 ` Oleksii
2023-03-07 13:16 ` Jan Beulich
1 sibling, 1 reply; 20+ messages in thread
From: Oleksii @ 2023-03-07 13:13 UTC (permalink / raw
To: Jan Beulich
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu, xen-devel
> > > > > +
> > > > > +#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)
> > >
> > > Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At
> > > least
> > > the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use
> > > the generic struct: With how you have things right now
> > > BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would
> > > also
> > > be at risk of causing false positives. Perhaps it's appropriate
> > > to
> > > have a separate #ifdef (incl the distinct identifier used), but
> > > that
> > > first BUILD_BUG_ON() needs abstracting out.
> Missed that. Thanks.
> I'll introduce that a separate #ifdef before BUG_FRAME:
>
> #ifndef BUILD_BUG_ON_LINE_WIDTH
> #define BUILD_BUG_ON_LINE_WIDTH \
> BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> BUG_LINE_HI_WIDTH))
> #endif
I think even better will be to do in the following way:
#ifndef LINE_WIDTH
#define LINE_WIDTH (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)
#endif
#define BUG_FRAME(type, line, ptr, second_frame, msg) do {
\
BUILD_BUG_ON((line) >> LINE_WIDTH);
\
BUILD_BUG_ON((type) >= BUGFRAME_NR);
\
asm volatile ( _ASM_BUGFRAME_TEXT(second_frame)
\
:: _ASM_BUGFRAME_INFO(type, line, ptr, msg) );
\
} while (false)
~ Oleksii
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-03-07 13:13 ` Oleksii
@ 2023-03-07 13:16 ` Jan Beulich
2023-03-07 13:31 ` Oleksii
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2023-03-07 13:16 UTC (permalink / raw
To: Oleksii
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu, xen-devel
On 07.03.2023 14:13, Oleksii wrote:
>>>>>> +
>>>>>> +#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)
>>>>
>>>> Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At
>>>> least
>>>> the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use
>>>> the generic struct: With how you have things right now
>>>> BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would
>>>> also
>>>> be at risk of causing false positives. Perhaps it's appropriate
>>>> to
>>>> have a separate #ifdef (incl the distinct identifier used), but
>>>> that
>>>> first BUILD_BUG_ON() needs abstracting out.
>> Missed that. Thanks.
>> I'll introduce that a separate #ifdef before BUG_FRAME:
>>
>> #ifndef BUILD_BUG_ON_LINE_WIDTH
>> #define BUILD_BUG_ON_LINE_WIDTH \
>> BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
>> BUG_LINE_HI_WIDTH))
>> #endif
> I think even better will be to do in the following way:
>
> #ifndef LINE_WIDTH
> #define LINE_WIDTH (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)
> #endif
>
> #define BUG_FRAME(type, line, ptr, second_frame, msg) do {
> \
> BUILD_BUG_ON((line) >> LINE_WIDTH);
> \
> BUILD_BUG_ON((type) >= BUGFRAME_NR);
> \
> asm volatile ( _ASM_BUGFRAME_TEXT(second_frame)
> \
> :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) );
> \
> } while (false)
Not sure - that'll still require arches to define LINE_WIDTH. What
if they store the line number in a 32-bit field? Then defining
LINE_WIDTH to 32 would yield undefined behavior here.
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-03-07 13:16 ` Jan Beulich
@ 2023-03-07 13:31 ` Oleksii
0 siblings, 0 replies; 20+ messages in thread
From: Oleksii @ 2023-03-07 13:31 UTC (permalink / raw
To: Jan Beulich
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu, xen-devel
On Tue, 2023-03-07 at 14:16 +0100, Jan Beulich wrote:
> On 07.03.2023 14:13, Oleksii wrote:
> > > > > > > +
> > > > > > > +#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)
> > > > >
> > > > > Isn't this tied to BUG_FRAME_STRUCT being defined (or not)?
> > > > > At
> > > > > least
> > > > > the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to
> > > > > use
> > > > > the generic struct: With how you have things right now
> > > > > BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check
> > > > > would
> > > > > also
> > > > > be at risk of causing false positives. Perhaps it's
> > > > > appropriate
> > > > > to
> > > > > have a separate #ifdef (incl the distinct identifier used),
> > > > > but
> > > > > that
> > > > > first BUILD_BUG_ON() needs abstracting out.
> > > Missed that. Thanks.
> > > I'll introduce that a separate #ifdef before BUG_FRAME:
> > >
> > > #ifndef BUILD_BUG_ON_LINE_WIDTH
> > > #define BUILD_BUG_ON_LINE_WIDTH \
> > > BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> > > BUG_LINE_HI_WIDTH))
> > > #endif
> > I think even better will be to do in the following way:
> >
> > #ifndef LINE_WIDTH
> > #define LINE_WIDTH (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)
> > #endif
> >
> > #define BUG_FRAME(type, line, ptr, second_frame, msg) do
> > {
> > \
> > BUILD_BUG_ON((line) >>
> > LINE_WIDTH);
> > \
> > BUILD_BUG_ON((type) >=
> > BUGFRAME_NR);
> > \
> > asm volatile (
> > _ASM_BUGFRAME_TEXT(second_frame)
> > \
> > :: _ASM_BUGFRAME_INFO(type, line, ptr, msg)
> > );
> > \
> > } while (false)
>
> Not sure - that'll still require arches to define LINE_WIDTH. What
> if they store the line number in a 32-bit field? Then defining
> LINE_WIDTH to 32 would yield undefined behavior here.
>
It might be an issue.
Than it will be better to have function-like macros mentioned in
previous e-mail.
Thanks
~ Oleksii
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
2023-03-07 12:59 ` Jan Beulich
@ 2023-03-07 13:37 ` Oleksii
0 siblings, 0 replies; 20+ messages in thread
From: Oleksii @ 2023-03-07 13:37 UTC (permalink / raw
To: Jan Beulich
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Roger Pau Monné, Wei Liu, xen-devel
On Tue, 2023-03-07 at 13:59 +0100, Jan Beulich wrote:
> On 07.03.2023 13:52, Oleksii wrote:
> > On Mon, 2023-03-06 at 11:36 +0100, Jan Beulich wrote:
> > > On 03.03.2023 11:38, Oleksii Kurochko wrote:
> > > > --- a/xen/arch/x86/include/asm/bug.h
> > > > +++ b/xen/arch/x86/include/asm/bug.h
> > > > @@ -1,79 +1,12 @@
> > > > #ifndef __X86_BUG_H__
> > > > #define __X86_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 BUG_DEBUGGER_TRAP_FATAL(regs)
> > > > debugger_trap_fatal(X86_EXC_GP,regs)
> > >
> > > Along the lines of a comment on an earlier patch, please move
> > > this
> > > ...
> > >
> > > > #ifndef __ASSEMBLY__
> > >
> > > ... into the thus guarded section.
> > But it was defined there before the patch series and these definies
> > are
> > used in "#else /* !__ASSEMBLY__ */"
>
> Since you use plural, maybe a misunderstanding? My comment was solely
> on the line you add; the removed lines are there merely as context.
Really. I misunderstood you.
>
> > > > @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const
> > > > struct
> > > > vcpu *v, uint32_t leaf,
> > > >
> > > > void do_invalid_op(struct cpu_user_regs *regs)
> > > > {
> > > > - const struct bug_frame *bug = NULL;
> > > > u8 bug_insn[2];
> > > > - const char *prefix = "", *filename, *predicate, *eip =
> > > > (char
> > > > *)regs->rip;
> > > > - unsigned long fixup;
> > > > - int id = -1, lineno;
> > > > - const struct virtual_region *region;
> > > > + const char *eip = (char *)regs->rip;
> > >
> > > I realize "char" was used before, but now that this is all on its
> > > own,
> > > can this at least become "unsigned char", but yet better "void"?
> > If we change it to "void" shouldn't it require additional casts
> > here (
> > which is not nice ):
> > eip += sizeof(bug_insn);
>
> Arithmetic on pointers to void is a GNU extension which we make
> heavy use of all over the hypervisor.
Got it. Than I'll rework it with 'void *'.
>
> > > > switch ( id )
> > > > {
> > > > + case BUGFRAME_run_fn:
> > > > case BUGFRAME_warn:
> > > > - printk("Xen WARN at %s%s:%d\n", prefix, filename,
> > > > lineno);
> > > > - show_execution_state(regs);
> > > > fixup_exception_return(regs, (unsigned long)eip);
> > > > return;
> > > > -
> > > > - case BUGFRAME_bug:
> > > > - printk("Xen BUG at %s%s:%d\n", prefix, filename,
> > > > lineno);
> > > > -
> > > > - if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
> > > > - return;
> > >
> > > This and ...
> > >
> > > > - 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;
> > >
> > > ... this return look to have no proper representation in the new
> > > logic; both cases fall through to ...
> > >
> > > > - show_execution_state(regs);
> > > > - panic("Assertion '%s' failed at %s%s:%d\n",
> > > > - predicate, prefix, filename, lineno);
> > > > }
> > > >
> > > > die:
> > >
> > > ... here now, which is an unwanted functional change.
> > Oh, you are right. So then in case we have correct id it is needed
> > to
> > do only return:
> > switch ( id )
> > {
> > case BUGFRAME_run_fn:
> > case BUGFRAME_warn:
> > fixup_exception_return(regs, (unsigned long)eip);
> > break;
> > }
> >
> > return;
>
> Except the "return" needs to remain inside the switch; unrecognized
> id
> values should still fall through to the "die" label.
Yeah, it is still possible to get unrecognized id.
Thanks.
>
~ Oleksii
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2023-03-07 13:38 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-03 10:38 [PATCH v5 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-03-03 10:38 ` [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-03-06 10:17 ` Jan Beulich
2023-03-07 11:32 ` Oleksii
2023-03-07 12:54 ` Jan Beulich
2023-03-07 13:13 ` Oleksii
2023-03-07 13:16 ` Jan Beulich
2023-03-07 13:31 ` Oleksii
2023-03-06 10:41 ` Jan Beulich
2023-03-07 11:34 ` Oleksii
2023-03-03 10:38 ` [PATCH v5 2/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
2023-03-06 10:24 ` Jan Beulich
2023-03-03 10:38 ` [PATCH v5 3/4] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
2023-03-03 11:29 ` Jan Beulich
2023-03-03 11:55 ` Oleksii
2023-03-03 10:38 ` [PATCH v5 4/4] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
2023-03-06 10:36 ` Jan Beulich
2023-03-07 12:52 ` Oleksii
2023-03-07 12:59 ` Jan Beulich
2023-03-07 13:37 ` Oleksii
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 read-only IMAP folder(s) and NNTP newsgroup(s).