* [PATCH v9 0/5] introduce generic implementation of macros from bug.h
@ 2023-03-29 10:50 Oleksii Kurochko
2023-03-29 10:50 ` [PATCH v9 1/5] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
` (4 more replies)
0 siblings, 5 replies; 20+ messages in thread
From: Oleksii Kurochko @ 2023-03-29 10:50 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
* Introduce BUG_DEBUGGER_TRAP_FATAL to hide details about TRAP_invalid_op for specific
architecture
* Introduce BUILD_BUG_ON_LINE_WIDTH(line) to make more generic BUG_FRAME macros
* 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 V9:
* rename BUILD_BUG_ON_LINE_WIDTH to BUG_CHECK_LINE_WIDTH
* Remove <asm/processor.h> from <common/bug.c> as the patch were
merged to staging which move all things used in <common/bug.c>
from <asm/processor.h> to more proper headers.
* add "struct cpu_user_regs" to <common/bug.c>.
* add explanation why <xen/debugger.h> is included in <common/bug.c>
* add additional explanation to <asm/bug.h> header
* update descrption.
---
Changes in V8:
* move BUILD_BUG_ON_LINE_WIDTH(line) to "ifndef BUG_FRAME_STRUCT" and add #elif for
"ifndef BUG_FRAME_STRUCT" to define stub for BUILD_BUG_ON_LINE_WIDTH.
* move <xen/debuger.h> from <asm/bug.h> to <common/bug.c> to fix compilation issue
( more details in the changes to the patch [xen/x86: switch x86 to use generic
implemetation of bug.h] ).
* remove <asm/bug.h> from <x86/acpi/cpufreq/cpufreq.c> and
<drivers/cpufreq/cpufreq.c> as nothing from <xen/bug.h> are used in
<*/cpufreq.c>
* Rebase the patch series on the top of the latest staging: it was a merge conflict
inisde x86/Kconfig.
---
Changes in V7:
* Introduce new patch to clean up ARM's <asm/bug.h> from unused defines:
BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH
* fix addressed code style.
* Remove '#include <xen/debugger.h>' from bug.c. it should be a part
of <asm/bug.h>.
* move BUILD_BUG_ON_LINE_WIDTH to '#ifndef BUG_FRAME_STRUCT' and define
dummy BUILD_BUG_ON_LINE_WIDTH when BUG_FRAME_STRUCT is defined.
* remove update prototype of 'void (*fn)(const struct cpu_user_regs *)' to
'void (*fn)(struct cpu_user_regs *)'.
* add code to to make sure the type used in run_in_exception_handler()
matches the one used here.
* Remove #undef {BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH} from
ARM and x86:
- for ARM was created separate patch where the defines are removed as
unused.
- for x86, the defines were removed now not to produce #undef of them to remove
them again in the following-up patch
* make eip 'const void *' in x86's do_invalid_op()
* change [eip = (unsigned char *)eip + sizeof(bug_insn);] to [eip += sizeof(bug_insn);]
* add '#include <xen/debugger.h>' to x86's <asm/bug.h>
---
Changes in V6:
* Update the cover letter message: add information that BUG_DEBUGGER_TRAP_FATAL() and
BUILD_BUG_ON_LINE_WIDTH().
* Introduce BUILD_BUG_ON_LINE_WIDTH(line) to make more generic BUG_FRAME macros.
* fix addressed code style
* change -EINVAL to -ENOENT in case when bug_frame wasn't found in generic do_bug_frame().
* change all 'return id' to 'break' inside switch/case of do_bug_frame().
* move up "#ifndef __ASSEMBLY__" to include BUG_DEBUGGER_TRAP_FATAL.
* update the comment of BUG_ASM_CONST.
* Introduce BUILD_BUG_ON_LINE_WIDTH(line) to make more generic BUG_FRAME macros
* remove #ifndef BUG_FRAME_STRUCT around BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH as it is
required to be defined before <asm/bug.h> as it is used by x86's <asm/bug.h> when
the header is included in assembly code.
* add undef of BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH for ARM & x86
as they were introduced unconditionally in <xen/bug.h>.
* update the type of eip to 'void *' in do_invalid_op() for x86
* fix the logic of do_invalid_op() for x86
* move macros BUG_DEBUGGER_TRAP_FATAL under #ifndef __ASSEMBLY__ as
it is not necessary to be in assembly code for x86.
---
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 (5):
xen: introduce CONFIG_GENERIC_BUG_FRAME
xen/arm: remove unused defines in <asm/bug.h>
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 | 106 ++++--------------
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 | 1 -
xen/arch/x86/include/asm/asm_defns.h | 2 +-
xen/arch/x86/include/asm/bug.h | 100 ++++-------------
xen/arch/x86/traps.c | 81 ++------------
xen/common/Kconfig | 3 +
xen/common/Makefile | 1 +
xen/common/bug.c | 126 +++++++++++++++++++++
xen/drivers/cpufreq/cpufreq.c | 1 -
xen/include/xen/bug.h | 161 +++++++++++++++++++++++++++
xen/include/xen/lib.h | 2 +-
21 files changed, 349 insertions(+), 332 deletions(-)
create mode 100644 xen/common/bug.c
create mode 100644 xen/include/xen/bug.h
--
2.39.2
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v9 1/5] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-03-29 10:50 [PATCH v9 0/5] introduce generic implementation of macros from bug.h Oleksii Kurochko
@ 2023-03-29 10:50 ` Oleksii Kurochko
2023-03-29 11:42 ` Jan Beulich
2023-03-31 20:32 ` Julien Grall
2023-03-29 10:50 ` [PATCH v9 2/5] xen/arm: remove unused defines in <asm/bug.h> Oleksii Kurochko
` (3 subsequent siblings)
4 siblings, 2 replies; 20+ messages in thread
From: Oleksii Kurochko @ 2023-03-29 10:50 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 V9:
* rename BUILD_BUG_ON_LINE_WIDTH to BUG_CHECK_LINE_WIDTH
* Remove <asm/processor.h> from <common/bug.c> as the patch were
merged to staging which move all things used in <common/bug.c>
from <asm/processor.h> to more proper headers.
* add "struct cpu_user_regs" to <common/bug.c>.
* add explanation why <xen/debugger.h> is included in <common/bug.c>
---
Changes in V8:
* move BUILD_BUG_ON_LINE_WIDTH(line) to "ifndef BUG_FRAME_STRUCT" and add #elif for
"ifndef BUG_FRAME_STRUCT" to define stub for BUILD_BUG_ON_LINE_WIDTH.
* move <xen/debuger.h> from <asm/bug.h> to <common/bug.c> to fix compilation issue
( more details in the changes to the patch [xen/x86: switch x86 to use generic
implemetation of bug.h] ).
---
Changes in V7:
* fix code style.
* Remove '#include <xen/debugger.h>' from bug.c. it should be a part
of <asm/bug.h>.
* move BUILD_BUG_ON_LINE_WIDTH to '#ifndef BUG_FRAME_STRUCT' and define
dummy BUILD_BUG_ON_LINE_WIDTH when BUG_FRAME_STRUCT is defined.
* remove update prototype of 'void (*fn)(const struct cpu_user_regs *)' to
'void (*fn)(struct cpu_user_regs *)'.
* add code to to make sure the type used in run_in_exception_handler()
matches the one used here.
---
Changes in V6:
* fix code style.
* change -EINVAL to -ENOENT in case when bug_frame wasn't found in
generic do_bug_frame()
* change all 'return id' to 'break' inside switch/case of generic do_bug_frame()
* move up "#ifndef __ASSEMBLY__" to include BUG_DEBUGGER_TRAP_FATAL
* update the comment of BUG_ASM_CONST
* make the line with 'BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH))' in
BUG_FRAME macros more abstract
* remove #ifndef BUG_FRAME_STRUCT around BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH as it is
required to be defined before <asm/bug.h> as it is used by x86's <asm/bug.h> when
the header is included in assembly code.
---
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 | 126 +++++++++++++++++++++++++++++++++
xen/include/xen/bug.h | 161 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 291 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 855c843113..3d2123a783 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -29,6 +29,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..680269b49a
--- /dev/null
+++ b/xen/common/bug.c
@@ -0,0 +1,126 @@
+#include <xen/bug.h>
+/*
+ * Ideally <xen/debugger.h> should be included in <asm/bug.h>
+ * but an issue with compilation can occur as <xen/debugger.h> uses
+ * BUG/ASSERT/etc macros inside but they will be defined later in
+ * <xen/bug.h> after return from inclusion of <asm/bug.h>:
+ *
+ * <xen/bug.h>:
+ * ...
+ * <asm/bug.h>:
+ * ...
+ * <xen/debugger.h> -> some of included header in it uses BUG/ASSERT/etc
+ * ...
+ * ...
+ * #define BUG() ...
+ * ...
+ * #define ASSERT() ...
+ * ...
+ */
+#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>
+
+struct cpu_user_regs;
+
+/*
+ * Returns a negative value in case of an error otherwise
+ * BUGFRAME_{run_fn, warn, bug, assert}
+ */
+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, 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 -ENOENT;
+
+ if ( id == BUGFRAME_run_fn )
+ {
+ void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
+
+ fn(regs);
+
+ /* Re-enforce consistent types, because of the casts involved. */
+ if ( false )
+ run_in_exception_handler(fn);
+
+ 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);
+
+ break;
+
+ case BUGFRAME_bug:
+ printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
+
+ if ( BUG_DEBUGGER_TRAP_FATAL(regs) )
+ break;
+
+ 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) )
+ break;
+
+ 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..4438b31ee2
--- /dev/null
+++ b/xen/include/xen/bug.h
@@ -0,0 +1,161 @@
+#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
+
+#define BUG_DISP_WIDTH 24
+#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
+#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
+
+#include <asm/bug.h>
+
+#ifndef __ASSEMBLY__
+
+#ifndef BUG_DEBUGGER_TRAP_FATAL
+#define BUG_DEBUGGER_TRAP_FATAL(regs) 0
+#endif
+
+#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])
+
+#define BUG_CHECK_LINE_WIDTH(line) \
+ BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH))
+
+#elif !defined(BUG_CHECK_LINE_WIDTH)
+
+#define BUG_CHECK_LINE_WIDTH(line) ((void)(line)
+
+#endif /* BUG_FRAME_STRUCT */
+
+
+/*
+ * 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
+
+#ifndef _ASM_BUGFRAME_TEXT
+
+#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 */
+
+#ifndef BUG_FRAME
+
+#define BUG_FRAME(type, line, ptr, second_frame, msg) do { \
+ BUG_CHECK_LINE_WIDTH(line); \
+ BUILD_BUG_ON((type) >= BUGFRAME_NR); \
+ asm volatile ( _ASM_BUGFRAME_TEXT(second_frame) \
+ :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \
+} while ( false )
+
+#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 ( false )
+
+#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 ( false )
+#endif
+
+#ifndef assert_failed
+#define assert_failed(msg) do { \
+ BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
+ unreachable(); \
+} while ( false )
+#endif
+
+#ifdef CONFIG_GENERIC_BUG_FRAME
+
+struct cpu_user_regs;
+
+/*
+ * Returns a negative value in case of an error otherwise
+ * BUGFRAME_{run_fn, warn, bug, assert}
+ */
+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.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v9 2/5] xen/arm: remove unused defines in <asm/bug.h>
2023-03-29 10:50 [PATCH v9 0/5] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-03-29 10:50 ` [PATCH v9 1/5] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
@ 2023-03-29 10:50 ` Oleksii Kurochko
2023-03-31 20:32 ` Julien Grall
2023-03-29 10:50 ` [PATCH v9 3/5] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Oleksii Kurochko @ 2023-03-29 10:50 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 defines BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH,
BUG_LINE_HI_WIDTH aren't used in ARM so could be purged
as unused.
Requested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V9:
* Nothing changed
---
Changes in V8:
* Update the commit message with Requested-by and Reviewed-by
---
xen/arch/arm/include/asm/bug.h | 4 ----
1 file changed, 4 deletions(-)
diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
index f4088d0913..d6c98505bf 100644
--- a/xen/arch/arm/include/asm/bug.h
+++ b/xen/arch/arm/include/asm/bug.h
@@ -9,10 +9,6 @@
# 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)
-
struct bug_frame {
signed int loc_disp; /* Relative address to the bug address */
signed int file_disp; /* Relative address to the filename */
--
2.39.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v9 3/5] xen: change <asm/bug.h> to <xen/bug.h>
2023-03-29 10:50 [PATCH v9 0/5] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-03-29 10:50 ` [PATCH v9 1/5] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-03-29 10:50 ` [PATCH v9 2/5] xen/arm: remove unused defines in <asm/bug.h> Oleksii Kurochko
@ 2023-03-29 10:50 ` Oleksii Kurochko
2023-03-31 20:36 ` Julien Grall
2023-03-29 10:50 ` [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
2023-03-29 10:50 ` [PATCH v9 5/5] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
4 siblings, 1 reply; 20+ messages in thread
From: Oleksii Kurochko @ 2023-03-29 10:50 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 ...
5. Remove BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH from
x86's <asm.bug.h> to not to produce #undef for them and #define again
with the same values as in <xen/bug.h>. These #undef and #define will
be anyway removed in the patch [2]
6. Remove <asm/bug.h> from <x86/acpi/cpufreq/cpufreq.c> and
<drivers/cpufreq/cpufreq.c> as nothing from <xen/bug.h> are used in
<*/cpufreq.c>
In the following two patches x86 and ARM archictectures will be
switched fully:
[1] xen/arm: switch ARM to use generic implementation of bug.h
[2] xen/x86: switch x86 to use generic implemetation of bug.h
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V9:
* Nothing changed
---
Changes in V8:
* remove <asm/bug.h> from <x86/acpi/cpufreq/cpufreq.c> and
<drivers/cpufreq/cpufreq.c> as nothing from <xen/bug.h> are used in
<*/cpufreq.c>
* update the commit message
---
Changes in V7:
* Remove #undef {BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH} from
ARM and x86:
* for ARM was created separate patch where the defines are removed as
unused.
* for x86, the defines were removed now not to produce #undef of them to remove
them again in the following-up patch
* Update commit message
* Add Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V6:
- change the inclusion order of <xen/bug.h>.
- add #undef of BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH for ARM & x86
as they were introduced unconditionally in <xen/bug.h>.
- update the commit message
---
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 | 1 -
xen/arch/x86/include/asm/asm_defns.h | 2 +-
xen/arch/x86/include/asm/bug.h | 19 ++-----------------
xen/drivers/cpufreq/cpufreq.c | 1 -
xen/include/xen/lib.h | 2 +-
9 files changed, 11 insertions(+), 37 deletions(-)
diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
index d6c98505bf..cacaf014ab 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)
@@ -9,6 +11,8 @@
# error "unknown ARM variant"
#endif
+#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 */
@@ -22,13 +26,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
@@ -85,12 +82,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..2e0067fbe5 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -35,7 +35,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..baaaccb26e 100644
--- a/xen/arch/x86/include/asm/asm_defns.h
+++ b/xen/arch/x86/include/asm/asm_defns.h
@@ -6,8 +6,8 @@
/* NB. Auto-generated from arch/.../asm-offsets.c */
#include <asm/asm-offsets.h>
#endif
-#include <asm/bug.h>
#include <asm/x86-defns.h>
+#include <xen/bug.h>
#include <xen/stringify.h>
#include <asm/cpufeature.h>
#include <asm/alternative.h>
diff --git a/xen/arch/x86/include/asm/bug.h b/xen/arch/x86/include/asm/bug.h
index b7265bdfbe..4b3e7b019d 100644
--- a/xen/arch/x86/include/asm/bug.h
+++ b/xen/arch/x86/include/asm/bug.h
@@ -1,19 +1,10 @@
#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 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 +71,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..2321c7dd07 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -39,7 +39,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.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-29 10:50 [PATCH v9 0/5] introduce generic implementation of macros from bug.h Oleksii Kurochko
` (2 preceding siblings ...)
2023-03-29 10:50 ` [PATCH v9 3/5] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
@ 2023-03-29 10:50 ` Oleksii Kurochko
2023-03-31 21:05 ` Julien Grall
2023-03-29 10:50 ` [PATCH v9 5/5] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
4 siblings, 1 reply; 20+ messages in thread
From: Oleksii Kurochko @ 2023-03-29 10:50 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 V9:
* add additional explanation to <asm/bug.h> header
---
Changes in V8:
* Nothing changed.
---
Changes in V7:
* Rebase the patch.
---
Changes in V6:
* Update the "changes in v5"
* Rebase on top of the patch [xen: introduce CONFIG_GENERIC_BUG_FRAME] as
there were minor changes.
---
Changes in V5:
* common/bug.c changes were removed after rebase
(the patch [xen: introduce CONFIG_GENERIC_BUG_FRAME] was reworked to make
ARM implementation to use generic do_bug_frame())
---
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 | 89 ++++++----------------------
xen/arch/arm/include/asm/traps.h | 2 -
xen/arch/arm/traps.c | 81 +------------------------
7 files changed, 22 insertions(+), 157 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 cacaf014ab..3fb0471a9b 100644
--- a/xen/arch/arm/include/asm/bug.h
+++ b/xen/arch/arm/include/asm/bug.h
@@ -1,6 +1,24 @@
#ifndef __ARM_BUG_H__
#define __ARM_BUG_H__
+/*
+ * Please do not include in the header any header that might
+ * use BUG/ASSERT/etc maros asthey will be defined later after
+ * the return to <xen/bug.h> from the current header:
+ *
+ * <xen/bug.h>:
+ * ...
+ * <asm/bug.h>:
+ * ...
+ * <any_header_which_uses_BUG/ASSERT/etc macros.h>
+ * ...
+ * ...
+ * #define BUG() ...
+ * ...
+ * #define ASSERT() ...
+ * ...
+ */
+
#include <xen/types.h>
#if defined(CONFIG_ARM_32)
@@ -11,76 +29,7 @@
# error "unknown ARM variant"
#endif
-#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.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v9 5/5] xen/x86: switch x86 to use generic implemetation of bug.h
2023-03-29 10:50 [PATCH v9 0/5] introduce generic implementation of macros from bug.h Oleksii Kurochko
` (3 preceding siblings ...)
2023-03-29 10:50 ` [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
@ 2023-03-29 10:50 ` Oleksii Kurochko
4 siblings, 0 replies; 20+ messages in thread
From: Oleksii Kurochko @ 2023-03-29 10:50 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>
* Update do_invalid_op using generic do_bug_frame()
* Define BUG_DEBUGGER_TRAP_FATAL to debugger_trap_fatal(X86_EXC_GP,regs)
* type of eip variable was changed to 'const void *'
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V9:
* update the commit message
* add additional explanation to <asm/bug.h> header
---
Changes in V8:
* move <xen/debuger.h> from <asm/bug.h> to <common/bug.c> to fix compilation issue.
The following compilation issue occurs:
In file included from ./arch/x86/include/asm/smp.h:10,
from ./include/xen/smp.h:4,
from ./arch/x86/include/asm/processor.h:10,
from ./arch/x86/include/asm/system.h:6,
from ./arch/x86/include/asm/atomic.h:5,
from ./include/xen/gdbstub.h:24,
from ./arch/x86/include/asm/debugger.h:10,
from ./include/xen/debugger.h:24,
from ./arch/x86/include/asm/bug.h:7,
from ./include/xen/bug.h:15,
from ./include/xen/lib.h:27,
from ./include/xen/perfc.h:6,
from arch/x86/x86_64/asm-offsets.c:9:
./include/xen/cpumask.h: In function 'cpumask_check':
./include/xen/cpumask.h:84:9: error: implicit declaration of function 'ASSERT' [-Werror=implicit-function-declaration]
84 | ASSERT(cpu < nr_cpu_ids);
It happens in case when CONFIG_CRASH_DEBUG is enabled and the reason for that is when
<xen/lib.h> is included in <x86_64/asm-offsets.c>:9 the "layout" of <xen/lib.h> would be
the following:
#include <xen/bug.h>:
#include <asm/bug.h>:
#include <xen/debugger.h>:
....
cpumask.h:
....
ASSERT(cpu < nr_cpu_ids);
return cpu;
....
....
#define ASSERT ...
....
Thereby ASSERT is defined after it was used in <xen/cpumask.h>.
* Rebase the patch series on the top of the latest staging: it was a merge conflict
inisde x86/Kconfig.
---
Changes in V7:
* update the commit message
* make eip 'const void *'
* change [eip = (unsigned char *)eip + sizeof(bug_insn);] to [eip += sizeof(bug_insn);]
* add '#include <xen/debugger.h>' to <asm/bug.h>
* add Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V6:
* update the commit message
* update the type of eip to 'void *' in do_invalid_op()
* fix the logic of do_invalid_op()
* move macros BUG_DEBUGGER_TRAP_FATAL under #ifndef __ASSEMBLY__ as
it is not necessary to be in assembly code.
---
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 | 87 ++++++++--------------------------
xen/arch/x86/traps.c | 81 ++++---------------------------
3 files changed, 30 insertions(+), 139 deletions(-)
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 2a5c3304e2..406445a358 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
imply 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 4b3e7b019d..7fe879a8cc 100644
--- a/xen/arch/x86/include/asm/bug.h
+++ b/xen/arch/x86/include/asm/bug.h
@@ -1,75 +1,30 @@
#ifndef __X86_BUG_H__
#define __X86_BUG_H__
-#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().
+ * Please do not include in the header any header that might
+ * use BUG/ASSERT/etc maros asthey will be defined later after
+ * the return to <xen/bug.h> from the current header:
+ *
+ * <xen/bug.h>:
+ * ...
+ * <asm/bug.h>:
+ * ...
+ * <any_header_which_uses_BUG/ASSERT/etc macros.h>
+ * ...
+ * ...
+ * #define BUG() ...
+ * ...
+ * #define ASSERT() ...
+ * ...
*/
-#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)
+#ifndef __ASSEMBLY__
+
+#define BUG_DEBUGGER_TRAP_FATAL(regs) debugger_trap_fatal(X86_EXC_GP,regs)
+
+#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..c36e3f855b 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 void *eip = (const void *)regs->rip;
+ int id;
if ( likely(guest_mode(regs)) )
{
@@ -1185,83 +1183,20 @@ 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);
+ return;
}
die:
--
2.39.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v9 1/5] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-03-29 10:50 ` [PATCH v9 1/5] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
@ 2023-03-29 11:42 ` Jan Beulich
2023-03-31 20:32 ` Julien Grall
1 sibling, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2023-03-29 11:42 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: Julien Grall, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu, xen-devel
On 29.03.2023 12:50, Oleksii Kurochko wrote:
> A large part of the content of the bug.h is repeated among all
> architectures, so it was decided to create a new config
> CONFIG_GENERIC_BUG_FRAME.
>
> The version of <bug.h> from x86 was taken as the base version.
>
> The patch introduces the following stuff:
> * common bug.h header
> * generic implementation of do_bug_frame
> * new config CONFIG_GENERIC_BUG_FRAME
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with two further adjustments (which I'd be fine doing while committing):
> --- /dev/null
> +++ b/xen/common/bug.c
> @@ -0,0 +1,126 @@
> +#include <xen/bug.h>
> +/*
> + * Ideally <xen/debugger.h> should be included in <asm/bug.h>
> + * but an issue with compilation can occur as <xen/debugger.h> uses
> + * BUG/ASSERT/etc macros inside but they will be defined later in
> + * <xen/bug.h> after return from inclusion of <asm/bug.h>:
> + *
> + * <xen/bug.h>:
> + * ...
> + * <asm/bug.h>:
> + * ...
> + * <xen/debugger.h> -> some of included header in it uses BUG/ASSERT/etc
> + * ...
> + * ...
> + * #define BUG() ...
> + * ...
> + * #define ASSERT() ...
> + * ...
> + */
> +#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>
> +
> +struct cpu_user_regs;
This doesn't need repeating here, as xen/bug.h necessarily has such a
forward declaration as well, for use in do_bug_frame()'s declaration.
> --- /dev/null
> +++ b/xen/include/xen/bug.h
> @@ -0,0 +1,161 @@
> +#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
> +
> +#define BUG_DISP_WIDTH 24
> +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> +
> +#include <asm/bug.h>
> +
> +#ifndef __ASSEMBLY__
> +
> +#ifndef BUG_DEBUGGER_TRAP_FATAL
> +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0
> +#endif
> +
> +#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])
> +
> +#define BUG_CHECK_LINE_WIDTH(line) \
> + BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH))
> +
> +#elif !defined(BUG_CHECK_LINE_WIDTH)
> +
> +#define BUG_CHECK_LINE_WIDTH(line) ((void)(line)
There's a closing parenthesis missing here, as it looks.
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v9 1/5] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-03-29 10:50 ` [PATCH v9 1/5] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-03-29 11:42 ` Jan Beulich
@ 2023-03-31 20:32 ` Julien Grall
1 sibling, 0 replies; 20+ messages in thread
From: Julien Grall @ 2023-03-31 20:32 UTC (permalink / raw)
To: Oleksii Kurochko, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu
Hi Oleksii,
On 29/03/2023 11:50, Oleksii Kurochko wrote:
> A large part of the content of the bug.h is repeated among all
> architectures, so it was decided to create a new config
> CONFIG_GENERIC_BUG_FRAME.
>
> The version of <bug.h> from x86 was taken as the base version.
>
> The patch introduces the following stuff:
> * common bug.h header
> * generic implementation of do_bug_frame
> * new config CONFIG_GENERIC_BUG_FRAME
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
I have tested the implementation on Arm. So:
Tested-by: Julien Grall <jgrall@amazon.com>
Acked-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v9 2/5] xen/arm: remove unused defines in <asm/bug.h>
2023-03-29 10:50 ` [PATCH v9 2/5] xen/arm: remove unused defines in <asm/bug.h> Oleksii Kurochko
@ 2023-03-31 20:32 ` Julien Grall
0 siblings, 0 replies; 20+ messages in thread
From: Julien Grall @ 2023-03-31 20:32 UTC (permalink / raw)
To: Oleksii Kurochko, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
Hi Oleksii,
On 29/03/2023 11:50, Oleksii Kurochko wrote:
> The following defines BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH,
> BUG_LINE_HI_WIDTH aren't used in ARM so could be purged
> as unused.
>
> Requested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v9 3/5] xen: change <asm/bug.h> to <xen/bug.h>
2023-03-29 10:50 ` [PATCH v9 3/5] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
@ 2023-03-31 20:36 ` Julien Grall
0 siblings, 0 replies; 20+ messages in thread
From: Julien Grall @ 2023-03-31 20:36 UTC (permalink / raw)
To: Oleksii Kurochko, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, George Dunlap, Wei Liu,
Roger Pau Monné
Hi Oleksii,
On 29/03/2023 11:50, 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 ...
> 5. Remove BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH from
> x86's <asm.bug.h> to not to produce #undef for them and #define again
> with the same values as in <xen/bug.h>. These #undef and #define will
> be anyway removed in the patch [2]
> 6. Remove <asm/bug.h> from <x86/acpi/cpufreq/cpufreq.c> and
> <drivers/cpufreq/cpufreq.c> as nothing from <xen/bug.h> are used in
> <*/cpufreq.c>
>
> In the following two patches x86 and ARM archictectures will be
> switched fully:
> [1] xen/arm: switch ARM to use generic implementation of bug.h
> [2] xen/x86: switch x86 to use generic implemetation of bug.h
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-29 10:50 ` [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
@ 2023-03-31 21:05 ` Julien Grall
2023-04-02 23:15 ` Stefano Stabellini
2023-04-03 18:40 ` Oleksii
0 siblings, 2 replies; 20+ messages in thread
From: Julien Grall @ 2023-03-31 21:05 UTC (permalink / raw)
To: Oleksii Kurochko, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
Hi Oleksii,
I was going to ack the patch but then I spotted something that would
want some clarification.
On 29/03/2023 11:50, Oleksii Kurochko wrote:
> diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
> index cacaf014ab..3fb0471a9b 100644
> --- a/xen/arch/arm/include/asm/bug.h
> +++ b/xen/arch/arm/include/asm/bug.h
> @@ -1,6 +1,24 @@
> #ifndef __ARM_BUG_H__
> #define __ARM_BUG_H__
>
> +/*
> + * Please do not include in the header any header that might
> + * use BUG/ASSERT/etc maros asthey will be defined later after
> + * the return to <xen/bug.h> from the current header:
> + *
> + * <xen/bug.h>:
> + * ...
> + * <asm/bug.h>:
> + * ...
> + * <any_header_which_uses_BUG/ASSERT/etc macros.h>
> + * ...
> + * ...
> + * #define BUG() ...
> + * ...
> + * #define ASSERT() ...
> + * ...
> + */
> +
> #include <xen/types.h>
>
> #if defined(CONFIG_ARM_32)
> @@ -11,76 +29,7 @@
> # error "unknown ARM variant"
> #endif
>
> -#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.
> - */
Given this comment ...
> -#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"
... you should explain in the commit message why this is needed and the
problem described above is not a problem anymore.
For instance, I managed to build it without 'c' on arm64 [1]. But it
does break on arm32 [2]. I know that Arm is also where '%c' was an issue.
Skimming through linux, the reason seems to be that GCC may add '#' when
it should not. That said, I haven't look at the impact on the generic
implementation. Neither I looked at which version may be affected (the
original message was from 2011).
However, without an explanation, I am afraid this can't go in because I
am worry we may break some users (thankfully that might just be a
compilation issues rather than weird behavior).
Bertrand, Stefano, do you know if this is still an issue?
Cheers,
[1] aarch64-linux-gnu-gcc (Linaro GCC 7.5-2019.12) 7.5.0
[2] arm-none-linux-gnueabihf-gcc (GNU Toolchain for the A-profile
Architecture 10.3-2021.07 (arm-10.29)) 10.3.1 20210621
--
Julien Grall
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-31 21:05 ` Julien Grall
@ 2023-04-02 23:15 ` Stefano Stabellini
2023-04-05 16:34 ` Julien Grall
2023-04-03 18:40 ` Oleksii
1 sibling, 1 reply; 20+ messages in thread
From: Stefano Stabellini @ 2023-04-02 23:15 UTC (permalink / raw)
To: Julien Grall
Cc: Oleksii Kurochko, xen-devel, Jan Beulich, Andrew Cooper,
Stefano Stabellini, Gianluca Guida, Bertrand Marquis,
Volodymyr Babchuk
On Fri, 31 Mar 2023, Julien Grall wrote:
> Hi Oleksii,
>
> I was going to ack the patch but then I spotted something that would want some
> clarification.
>
> On 29/03/2023 11:50, Oleksii Kurochko wrote:
> > diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
> > index cacaf014ab..3fb0471a9b 100644
> > --- a/xen/arch/arm/include/asm/bug.h
> > +++ b/xen/arch/arm/include/asm/bug.h
> > @@ -1,6 +1,24 @@
> > #ifndef __ARM_BUG_H__
> > #define __ARM_BUG_H__
> > +/*
> > + * Please do not include in the header any header that might
> > + * use BUG/ASSERT/etc maros asthey will be defined later after
> > + * the return to <xen/bug.h> from the current header:
> > + *
> > + * <xen/bug.h>:
> > + * ...
> > + * <asm/bug.h>:
> > + * ...
> > + * <any_header_which_uses_BUG/ASSERT/etc macros.h>
> > + * ...
> > + * ...
> > + * #define BUG() ...
> > + * ...
> > + * #define ASSERT() ...
> > + * ...
> > + */
> > +
> > #include <xen/types.h>
> > #if defined(CONFIG_ARM_32)
> > @@ -11,76 +29,7 @@
> > # error "unknown ARM variant"
> > #endif
> > -#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.
> > - */
>
> Given this comment ...
>
> > -#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"
>
> ... you should explain in the commit message why this is needed and the
> problem described above is not a problem anymore.
>
> For instance, I managed to build it without 'c' on arm64 [1]. But it does
> break on arm32 [2]. I know that Arm is also where '%c' was an issue.
>
> Skimming through linux, the reason seems to be that GCC may add '#' when it
> should not. That said, I haven't look at the impact on the generic
> implementation. Neither I looked at which version may be affected (the
> original message was from 2011).
>
> However, without an explanation, I am afraid this can't go in because I am
> worry we may break some users (thankfully that might just be a compilation
> issues rather than weird behavior).
>
> Bertrand, Stefano, do you know if this is still an issue?
I don't know, but I confirm your observation.
In my system, both ARM64 and ARM32 compile without problems with "c".
Without "c', only ARM64 compiles without problems, while ARM32 breaks.
My ARM32 compiler is:
arm-linux-gnueabihf-gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
Additing a meaningful explanation to the commit message might be
difficult in this case.
Maybe instead we could run a few tests with different versions of arm64
and arm32 gcc to check that everything still works? If everything checks
out, given that the issue has been unchanged for 10+ years we could just
keep "c" and move forward with it?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-31 21:05 ` Julien Grall
2023-04-02 23:15 ` Stefano Stabellini
@ 2023-04-03 18:40 ` Oleksii
2023-04-04 6:41 ` Jan Beulich
1 sibling, 1 reply; 20+ messages in thread
From: Oleksii @ 2023-04-03 18:40 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
Hello Julien,
On Fri, 2023-03-31 at 22:05 +0100, Julien Grall wrote:
> Hi Oleksii,
>
> I was going to ack the patch but then I spotted something that would
> want some clarification.
>
> On 29/03/2023 11:50, Oleksii Kurochko wrote:
> > diff --git a/xen/arch/arm/include/asm/bug.h
> > b/xen/arch/arm/include/asm/bug.h
> > index cacaf014ab..3fb0471a9b 100644
> > --- a/xen/arch/arm/include/asm/bug.h
> > +++ b/xen/arch/arm/include/asm/bug.h
> > @@ -1,6 +1,24 @@
> > #ifndef __ARM_BUG_H__
> > #define __ARM_BUG_H__
> >
> > +/*
> > + * Please do not include in the header any header that might
> > + * use BUG/ASSERT/etc maros asthey will be defined later after
> > + * the return to <xen/bug.h> from the current header:
> > + *
> > + * <xen/bug.h>:
> > + * ...
> > + * <asm/bug.h>:
> > + * ...
> > + * <any_header_which_uses_BUG/ASSERT/etc macros.h>
> > + * ...
> > + * ...
> > + * #define BUG() ...
> > + * ...
> > + * #define ASSERT() ...
> > + * ...
> > + */
> > +
> > #include <xen/types.h>
> >
> > #if defined(CONFIG_ARM_32)
> > @@ -11,76 +29,7 @@
> > # error "unknown ARM variant"
> > #endif
> >
> > -#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.
> > - */
>
> Given this comment ...
>
> > -#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"
>
> ... you should explain in the commit message why this is needed and
> the
> problem described above is not a problem anymore.
>
> For instance, I managed to build it without 'c' on arm64 [1]. But it
> does break on arm32 [2]. I know that Arm is also where '%c' was an
> issue.
>
> Skimming through linux, the reason seems to be that GCC may add '#'
> when
> it should not. That said, I haven't look at the impact on the generic
> implementation. Neither I looked at which version may be affected
> (the
> original message was from 2011).
You are right that some compilers add '#' when it shouldn't. The same
thing happens with RISC-V.
So I'll update both the commit message and comment.
>
> However, without an explanation, I am afraid this can't go in because
> I
> am worry we may break some users (thankfully that might just be a
> compilation issues rather than weird behavior).
>
> Bertrand, Stefano, do you know if this is still an issue?
>
> Cheers,
>
> [1] aarch64-linux-gnu-gcc (Linaro GCC 7.5-2019.12) 7.5.0
> [2] arm-none-linux-gnueabihf-gcc (GNU Toolchain for the A-profile
> Architecture 10.3-2021.07 (arm-10.29)) 10.3.1 20210621
>
~ Oleksii
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
2023-04-03 18:40 ` Oleksii
@ 2023-04-04 6:41 ` Jan Beulich
2023-04-04 8:09 ` Oleksii
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2023-04-04 6:41 UTC (permalink / raw)
To: Oleksii
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, xen-devel, Julien Grall
On 03.04.2023 20:40, Oleksii wrote:
> Hello Julien,
> On Fri, 2023-03-31 at 22:05 +0100, Julien Grall wrote:
>> Hi Oleksii,
>>
>> I was going to ack the patch but then I spotted something that would
>> want some clarification.
>>
>> On 29/03/2023 11:50, Oleksii Kurochko wrote:
>>> diff --git a/xen/arch/arm/include/asm/bug.h
>>> b/xen/arch/arm/include/asm/bug.h
>>> index cacaf014ab..3fb0471a9b 100644
>>> --- a/xen/arch/arm/include/asm/bug.h
>>> +++ b/xen/arch/arm/include/asm/bug.h
>>> @@ -1,6 +1,24 @@
>>> #ifndef __ARM_BUG_H__
>>> #define __ARM_BUG_H__
>>>
>>> +/*
>>> + * Please do not include in the header any header that might
>>> + * use BUG/ASSERT/etc maros asthey will be defined later after
>>> + * the return to <xen/bug.h> from the current header:
>>> + *
>>> + * <xen/bug.h>:
>>> + * ...
>>> + * <asm/bug.h>:
>>> + * ...
>>> + * <any_header_which_uses_BUG/ASSERT/etc macros.h>
>>> + * ...
>>> + * ...
>>> + * #define BUG() ...
>>> + * ...
>>> + * #define ASSERT() ...
>>> + * ...
>>> + */
>>> +
>>> #include <xen/types.h>
>>>
>>> #if defined(CONFIG_ARM_32)
>>> @@ -11,76 +29,7 @@
>>> # error "unknown ARM variant"
>>> #endif
>>>
>>> -#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.
>>> - */
>>
>> Given this comment ...
>>
>>> -#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"
>>
>> ... you should explain in the commit message why this is needed and
>> the
>> problem described above is not a problem anymore.
>>
>> For instance, I managed to build it without 'c' on arm64 [1]. But it
>> does break on arm32 [2]. I know that Arm is also where '%c' was an
>> issue.
>>
>> Skimming through linux, the reason seems to be that GCC may add '#'
>> when
>> it should not. That said, I haven't look at the impact on the generic
>> implementation. Neither I looked at which version may be affected
>> (the
>> original message was from 2011).
> You are right that some compilers add '#' when it shouldn't. The same
> thing happens with RISC-V.
RISC-V doesn't know of a '#' prefix, does it? '#' is a comment character
there afaik, like for many other architectures.
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
2023-04-04 6:41 ` Jan Beulich
@ 2023-04-04 8:09 ` Oleksii
2023-04-05 16:39 ` Julien Grall
0 siblings, 1 reply; 20+ messages in thread
From: Oleksii @ 2023-04-04 8:09 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, xen-devel, Julien Grall
On Tue, 2023-04-04 at 08:41 +0200, Jan Beulich wrote:
> On 03.04.2023 20:40, Oleksii wrote:
> > Hello Julien,
> > On Fri, 2023-03-31 at 22:05 +0100, Julien Grall wrote:
> > > Hi Oleksii,
> > >
> > > I was going to ack the patch but then I spotted something that
> > > would
> > > want some clarification.
> > >
> > > On 29/03/2023 11:50, Oleksii Kurochko wrote:
> > > > diff --git a/xen/arch/arm/include/asm/bug.h
> > > > b/xen/arch/arm/include/asm/bug.h
> > > > index cacaf014ab..3fb0471a9b 100644
> > > > --- a/xen/arch/arm/include/asm/bug.h
> > > > +++ b/xen/arch/arm/include/asm/bug.h
> > > > @@ -1,6 +1,24 @@
> > > > #ifndef __ARM_BUG_H__
> > > > #define __ARM_BUG_H__
> > > >
> > > > +/*
> > > > + * Please do not include in the header any header that might
> > > > + * use BUG/ASSERT/etc maros asthey will be defined later after
> > > > + * the return to <xen/bug.h> from the current header:
> > > > + *
> > > > + * <xen/bug.h>:
> > > > + * ...
> > > > + * <asm/bug.h>:
> > > > + * ...
> > > > + * <any_header_which_uses_BUG/ASSERT/etc macros.h>
> > > > + * ...
> > > > + * ...
> > > > + * #define BUG() ...
> > > > + * ...
> > > > + * #define ASSERT() ...
> > > > + * ...
> > > > + */
> > > > +
> > > > #include <xen/types.h>
> > > >
> > > > #if defined(CONFIG_ARM_32)
> > > > @@ -11,76 +29,7 @@
> > > > # error "unknown ARM variant"
> > > > #endif
> > > >
> > > > -#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.
> > > > - */
> > >
> > > Given this comment ...
> > >
> > > > -#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"
> > >
> > > ... you should explain in the commit message why this is needed
> > > and
> > > the
> > > problem described above is not a problem anymore.
> > >
> > > For instance, I managed to build it without 'c' on arm64 [1]. But
> > > it
> > > does break on arm32 [2]. I know that Arm is also where '%c' was
> > > an
> > > issue.
> > >
> > > Skimming through linux, the reason seems to be that GCC may add
> > > '#'
> > > when
> > > it should not. That said, I haven't look at the impact on the
> > > generic
> > > implementation. Neither I looked at which version may be affected
> > > (the
> > > original message was from 2011).
> > You are right that some compilers add '#' when it shouldn't. The
> > same
> > thing happens with RISC-V.
>
> RISC-V doesn't know of a '#' prefix, does it? '#' is a comment
> character
> there afaik, like for many other architectures.
It doesn't and for RISC-V it's a comment character.
afaik '%c' is needed to skip prefix('sign' ) (# or $ ( in case of
x86)).
I mean that RISC-V doesn't put anything before immediate so there is no
need to use %c as we don't need to skip prefix/'sign' before immediate
but if start to use '%c' it will cause an compiler issue.
~ Oleksii
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
2023-04-02 23:15 ` Stefano Stabellini
@ 2023-04-05 16:34 ` Julien Grall
2023-04-06 21:03 ` Stefano Stabellini
0 siblings, 1 reply; 20+ messages in thread
From: Julien Grall @ 2023-04-05 16:34 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Oleksii Kurochko, xen-devel, Jan Beulich, Andrew Cooper,
Gianluca Guida, Bertrand Marquis, Volodymyr Babchuk
Hi,
On 03/04/2023 00:15, Stefano Stabellini wrote:
> On Fri, 31 Mar 2023, Julien Grall wrote:
>> Hi Oleksii,
>>
>> I was going to ack the patch but then I spotted something that would want some
>> clarification.
>>
>> On 29/03/2023 11:50, Oleksii Kurochko wrote:
>>> diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
>>> index cacaf014ab..3fb0471a9b 100644
>>> --- a/xen/arch/arm/include/asm/bug.h
>>> +++ b/xen/arch/arm/include/asm/bug.h
>>> @@ -1,6 +1,24 @@
>>> #ifndef __ARM_BUG_H__
>>> #define __ARM_BUG_H__
>>> +/*
>>> + * Please do not include in the header any header that might
>>> + * use BUG/ASSERT/etc maros asthey will be defined later after
>>> + * the return to <xen/bug.h> from the current header:
>>> + *
>>> + * <xen/bug.h>:
>>> + * ...
>>> + * <asm/bug.h>:
>>> + * ...
>>> + * <any_header_which_uses_BUG/ASSERT/etc macros.h>
>>> + * ...
>>> + * ...
>>> + * #define BUG() ...
>>> + * ...
>>> + * #define ASSERT() ...
>>> + * ...
>>> + */
>>> +
>>> #include <xen/types.h>
>>> #if defined(CONFIG_ARM_32)
>>> @@ -11,76 +29,7 @@
>>> # error "unknown ARM variant"
>>> #endif
>>> -#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.
>>> - */
>>
>> Given this comment ...
>>
>>> -#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"
>>
>> ... you should explain in the commit message why this is needed and the
>> problem described above is not a problem anymore.
>>
>> For instance, I managed to build it without 'c' on arm64 [1]. But it does
>> break on arm32 [2]. I know that Arm is also where '%c' was an issue.
>>
>> Skimming through linux, the reason seems to be that GCC may add '#' when it
>> should not. That said, I haven't look at the impact on the generic
>> implementation. Neither I looked at which version may be affected (the
>> original message was from 2011).
>>
>> However, without an explanation, I am afraid this can't go in because I am
>> worry we may break some users (thankfully that might just be a compilation
>> issues rather than weird behavior).
>>
>> Bertrand, Stefano, do you know if this is still an issue?
>
> I don't know, but I confirm your observation.
>
> In my system, both ARM64 and ARM32 compile without problems with "c".
> Without "c', only ARM64 compiles without problems, while ARM32 breaks.
>
> My ARM32 compiler is:
> arm-linux-gnueabihf-gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
>
> Additing a meaningful explanation to the commit message might be
> difficult in this case.
Agree. One would need to look at the compiler code to confirm. We should
at least acknowledge the change in the commit message and also...
>
> Maybe instead we could run a few tests with different versions of arm64
> and arm32 gcc to check that everything still works? If everything checks
> out, given that the issue has been unchanged for 10+ years we could just
> keep "c" and move forward with it?
... confirm that we are still able to compile with GCC 4.9 (arm32) and
GCC 5.1 (arm64).
Do we have those compiler in the CI? (I couldn't easily confirm from the
automation directory).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
2023-04-04 8:09 ` Oleksii
@ 2023-04-05 16:39 ` Julien Grall
2023-04-06 6:35 ` Jan Beulich
0 siblings, 1 reply; 20+ messages in thread
From: Julien Grall @ 2023-04-05 16:39 UTC (permalink / raw)
To: Oleksii, Jan Beulich
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, xen-devel
Hi,
On 04/04/2023 09:09, Oleksii wrote:
> On Tue, 2023-04-04 at 08:41 +0200, Jan Beulich wrote:
>> On 03.04.2023 20:40, Oleksii wrote:
>>> Hello Julien,
>>> On Fri, 2023-03-31 at 22:05 +0100, Julien Grall wrote:
>>>> Hi Oleksii,
>>>>
>>>> I was going to ack the patch but then I spotted something that
>>>> would
>>>> want some clarification.
>>>>
>>>> On 29/03/2023 11:50, Oleksii Kurochko wrote:
>>>>> diff --git a/xen/arch/arm/include/asm/bug.h
>>>>> b/xen/arch/arm/include/asm/bug.h
>>>>> index cacaf014ab..3fb0471a9b 100644
>>>>> --- a/xen/arch/arm/include/asm/bug.h
>>>>> +++ b/xen/arch/arm/include/asm/bug.h
>>>>> @@ -1,6 +1,24 @@
>>>>> #ifndef __ARM_BUG_H__
>>>>> #define __ARM_BUG_H__
>>>>>
>>>>> +/*
>>>>> + * Please do not include in the header any header that might
>>>>> + * use BUG/ASSERT/etc maros asthey will be defined later after
>>>>> + * the return to <xen/bug.h> from the current header:
>>>>> + *
>>>>> + * <xen/bug.h>:
>>>>> + * ...
>>>>> + * <asm/bug.h>:
>>>>> + * ...
>>>>> + * <any_header_which_uses_BUG/ASSERT/etc macros.h>
>>>>> + * ...
>>>>> + * ...
>>>>> + * #define BUG() ...
>>>>> + * ...
>>>>> + * #define ASSERT() ...
>>>>> + * ...
>>>>> + */
>>>>> +
>>>>> #include <xen/types.h>
>>>>>
>>>>> #if defined(CONFIG_ARM_32)
>>>>> @@ -11,76 +29,7 @@
>>>>> # error "unknown ARM variant"
>>>>> #endif
>>>>>
>>>>> -#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.
>>>>> - */
>>>>
>>>> Given this comment ...
>>>>
>>>>> -#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"
>>>>
>>>> ... you should explain in the commit message why this is needed
>>>> and
>>>> the
>>>> problem described above is not a problem anymore.
>>>>
>>>> For instance, I managed to build it without 'c' on arm64 [1]. But
>>>> it
>>>> does break on arm32 [2]. I know that Arm is also where '%c' was
>>>> an
>>>> issue.
>>>>
>>>> Skimming through linux, the reason seems to be that GCC may add
>>>> '#'
>>>> when
>>>> it should not. That said, I haven't look at the impact on the
>>>> generic
>>>> implementation. Neither I looked at which version may be affected
>>>> (the
>>>> original message was from 2011).
>>> You are right that some compilers add '#' when it shouldn't. The
>>> same
>>> thing happens with RISC-V.
I am a bit confused with this answer. My point was that on at least on
Arm 32-bit we need to use 'c' otherwise it breaks.
AFAIU, this is not the same problem on RISC-V...
>>
>> RISC-V doesn't know of a '#' prefix, does it? '#' is a comment
>> character
>> there afaik, like for many other architectures.
> It doesn't and for RISC-V it's a comment character.
>
> afaik '%c' is needed to skip prefix('sign' ) (# or $ ( in case of
> x86)).
>
> I mean that RISC-V doesn't put anything before immediate so there is no
> need to use %c as we don't need to skip prefix/'sign' before immediate
> but if start to use '%c' it will cause an compiler issue.
... because here you say it will break when using 'c'. Did I miss anything?
Anyway, it sounds like to me that the default definition in xen/bug.h
should be using 'c' rather than been empty because this seems to be the
more common approach.
To reduce the amount of patch to resend, I was actually thinking to
merge patch #1-3 and #5 (so leave this patch alone) and modify the
default in a follow-up. Any thoughts?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
2023-04-05 16:39 ` Julien Grall
@ 2023-04-06 6:35 ` Jan Beulich
2023-04-06 8:44 ` Julien Grall
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2023-04-06 6:35 UTC (permalink / raw)
To: Julien Grall
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, xen-devel, Oleksii
On 05.04.2023 18:39, Julien Grall wrote:
> To reduce the amount of patch to resend, I was actually thinking to
> merge patch #1-3 and #5 (so leave this patch alone) and modify the
> default in a follow-up. Any thoughts?
Well, yes, that's what I did a couple of days ago already.
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
2023-04-06 6:35 ` Jan Beulich
@ 2023-04-06 8:44 ` Julien Grall
0 siblings, 0 replies; 20+ messages in thread
From: Julien Grall @ 2023-04-06 8:44 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, xen-devel, Oleksii
Hi,
On 06/04/2023 07:35, Jan Beulich wrote:
> On 05.04.2023 18:39, Julien Grall wrote:
>> To reduce the amount of patch to resend, I was actually thinking to
>> merge patch #1-3 and #5 (so leave this patch alone) and modify the
>> default in a follow-up. Any thoughts?
>
> Well, yes, that's what I did a couple of days ago already.
Ah. I didn't check the tree when replying. So ignore me.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
2023-04-05 16:34 ` Julien Grall
@ 2023-04-06 21:03 ` Stefano Stabellini
0 siblings, 0 replies; 20+ messages in thread
From: Stefano Stabellini @ 2023-04-06 21:03 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Oleksii Kurochko, xen-devel, Jan Beulich,
Andrew Cooper, Gianluca Guida, Bertrand Marquis,
Volodymyr Babchuk
On Wed, 5 Apr 2023, Julien Grall wrote:
> On 03/04/2023 00:15, Stefano Stabellini wrote:
> > On Fri, 31 Mar 2023, Julien Grall wrote:
> > > Hi Oleksii,
> > >
> > > I was going to ack the patch but then I spotted something that would want
> > > some
> > > clarification.
> > >
> > > On 29/03/2023 11:50, Oleksii Kurochko wrote:
> > > > diff --git a/xen/arch/arm/include/asm/bug.h
> > > > b/xen/arch/arm/include/asm/bug.h
> > > > index cacaf014ab..3fb0471a9b 100644
> > > > --- a/xen/arch/arm/include/asm/bug.h
> > > > +++ b/xen/arch/arm/include/asm/bug.h
> > > > @@ -1,6 +1,24 @@
> > > > #ifndef __ARM_BUG_H__
> > > > #define __ARM_BUG_H__
> > > > +/*
> > > > + * Please do not include in the header any header that might
> > > > + * use BUG/ASSERT/etc maros asthey will be defined later after
> > > > + * the return to <xen/bug.h> from the current header:
> > > > + *
> > > > + * <xen/bug.h>:
> > > > + * ...
> > > > + * <asm/bug.h>:
> > > > + * ...
> > > > + * <any_header_which_uses_BUG/ASSERT/etc macros.h>
> > > > + * ...
> > > > + * ...
> > > > + * #define BUG() ...
> > > > + * ...
> > > > + * #define ASSERT() ...
> > > > + * ...
> > > > + */
> > > > +
> > > > #include <xen/types.h>
> > > > #if defined(CONFIG_ARM_32)
> > > > @@ -11,76 +29,7 @@
> > > > # error "unknown ARM variant"
> > > > #endif
> > > > -#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.
> > > > - */
> > >
> > > Given this comment ...
> > >
> > > > -#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"
> > >
> > > ... you should explain in the commit message why this is needed and the
> > > problem described above is not a problem anymore.
> > >
> > > For instance, I managed to build it without 'c' on arm64 [1]. But it does
> > > break on arm32 [2]. I know that Arm is also where '%c' was an issue.
> > >
> > > Skimming through linux, the reason seems to be that GCC may add '#' when
> > > it
> > > should not. That said, I haven't look at the impact on the generic
> > > implementation. Neither I looked at which version may be affected (the
> > > original message was from 2011).
> > >
> > > However, without an explanation, I am afraid this can't go in because I am
> > > worry we may break some users (thankfully that might just be a compilation
> > > issues rather than weird behavior).
> > >
> > > Bertrand, Stefano, do you know if this is still an issue?
> >
> > I don't know, but I confirm your observation.
> >
> > In my system, both ARM64 and ARM32 compile without problems with "c".
> > Without "c', only ARM64 compiles without problems, while ARM32 breaks.
> >
> > My ARM32 compiler is:
> > arm-linux-gnueabihf-gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
> >
> > Additing a meaningful explanation to the commit message might be
> > difficult in this case.
>
> Agree. One would need to look at the compiler code to confirm. We should at
> least acknowledge the change in the commit message and also...
>
> >
> > Maybe instead we could run a few tests with different versions of arm64
> > and arm32 gcc to check that everything still works? If everything checks
> > out, given that the issue has been unchanged for 10+ years we could just
> > keep "c" and move forward with it?
>
> ... confirm that we are still able to compile with GCC 4.9 (arm32) and GCC 5.1
> (arm64).
>
> Do we have those compiler in the CI? (I couldn't easily confirm from the
> automation directory).
In the CI we have:
- arm64v8/alpine:3.12, gcc 9.3.0
- arm64v8/debian:unstable, gcc 12.2.0
- arm64v8/debian:unstable cross arm32, gcc 12.2.0
- yocto, not sure exactly but it is going to be 9.x or newer
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2023-04-06 21:03 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-29 10:50 [PATCH v9 0/5] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-03-29 10:50 ` [PATCH v9 1/5] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-03-29 11:42 ` Jan Beulich
2023-03-31 20:32 ` Julien Grall
2023-03-29 10:50 ` [PATCH v9 2/5] xen/arm: remove unused defines in <asm/bug.h> Oleksii Kurochko
2023-03-31 20:32 ` Julien Grall
2023-03-29 10:50 ` [PATCH v9 3/5] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
2023-03-31 20:36 ` Julien Grall
2023-03-29 10:50 ` [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
2023-03-31 21:05 ` Julien Grall
2023-04-02 23:15 ` Stefano Stabellini
2023-04-05 16:34 ` Julien Grall
2023-04-06 21:03 ` Stefano Stabellini
2023-04-03 18:40 ` Oleksii
2023-04-04 6:41 ` Jan Beulich
2023-04-04 8:09 ` Oleksii
2023-04-05 16:39 ` Julien Grall
2023-04-06 6:35 ` Jan Beulich
2023-04-06 8:44 ` Julien Grall
2023-03-29 10:50 ` [PATCH v9 5/5] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.