* [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-02-24 11:31 [PATCH v3 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
@ 2023-02-24 11:31 ` Oleksii Kurochko
2023-02-25 16:42 ` Julien Grall
2023-02-27 14:23 ` Jan Beulich
2023-02-24 11:31 ` [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
` (2 subsequent siblings)
3 siblings, 2 replies; 39+ messages in thread
From: Oleksii Kurochko @ 2023-02-24 11:31 UTC (permalink / raw)
To: xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Oleksii Kurochko, George Dunlap, Julien Grall, 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 V3:
* Add debugger_trap_fatal() to do_bug_frame(). It simplifies usage of
do_bug_frame() for x86 so making handle_bug_frame() and find_bug_frame()
not needed anymore.
* Update do_bug_frame() to return -EINVAL if something goes wrong; otherwise
id of bug_frame
* Update _ASM_BUGFRAME_TEXT to make it more portable.
* Drop unnecessary comments.
* define stub value for TRAP_invalid_op in case if wasn't defined in
arch-specific folders.
---
Changes in V2:
- Switch to x86 implementation as generic as it is more compact
( at least from the point of view of bug frame structure ).
- Rename CONFIG_GENERIC_DO_BUG_FRAME to CONFIG_GENERIC_BUG_FRAME.
- Change the macro bug_loc(b) to avoid the need for a cast:
#define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp)
- Rename BUG_FRAME_STUFF to BUG_FRAME_STRUCT
- Make macros related to bug frame structure more generic.
- Introduce BUG_INSTR and MODIFIER to make _ASM_BUGFRAME_TEXT reusable
between x86 and RISC-V.
- Rework do_bug_frame() and introduce find_bug_frame() and handle_bug_frame()
functions to make it reusable by x86.
- code style fixes
---
xen/common/Kconfig | 3 +
xen/common/Makefile | 1 +
xen/common/bug.c | 109 ++++++++++++++++++++++++++++++
xen/include/xen/bug.h | 150 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 263 insertions(+)
create mode 100644 xen/common/bug.c
create mode 100644 xen/include/xen/bug.h
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index f1ea3199c8..b226323537 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -28,6 +28,9 @@ config ALTERNATIVE_CALL
config ARCH_MAP_DOMAIN_PAGE
bool
+config GENERIC_BUG_FRAME
+ bool
+
config HAS_ALTERNATIVE
bool
diff --git a/xen/common/Makefile b/xen/common/Makefile
index bbd75b4be6..46049eac35 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -1,5 +1,6 @@
obj-$(CONFIG_ARGO) += argo.o
obj-y += bitmap.o
+obj-$(CONFIG_GENERIC_BUG_FRAME) += bug.o
obj-$(CONFIG_HYPFS_CONFIG) += config_data.o
obj-$(CONFIG_CORE_PARKING) += core_parking.o
obj-y += cpu.o
diff --git a/xen/common/bug.c b/xen/common/bug.c
new file mode 100644
index 0000000000..f81724fc9b
--- /dev/null
+++ b/xen/common/bug.c
@@ -0,0 +1,109 @@
+#include <xen/bug.h>
+#include <xen/debugger.h>
+#include <xen/errno.h>
+#include <xen/kernel.h>
+#include <xen/livepatch.h>
+#include <xen/string.h>
+#include <xen/types.h>
+#include <xen/virtual_region.h>
+
+#include <asm/processor.h>
+
+/* Set default value for TRAP_invalid_op as it is defined only for X86 now */
+#ifndef TRAP_invalid_op
+#define TRAP_invalid_op 0
+#endif
+
+int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc)
+{
+ const struct bug_frame *bug = NULL;
+ const struct virtual_region *region;
+ const char *prefix = "", *filename, *predicate;
+ unsigned long fixup;
+ unsigned int id = BUGFRAME_NR, lineno;
+
+ region = find_text_region(pc);
+ if ( region )
+ {
+ for ( id = 0; id < BUGFRAME_NR; id++ )
+ {
+ const struct bug_frame *b;
+ unsigned int i;
+
+ for ( i = 0, b = region->frame[id].bugs;
+ i < region->frame[id].n_bugs; b++, i++ )
+ {
+ if ( bug_loc(b) == pc )
+ {
+ bug = b;
+ goto found;
+ }
+ }
+ }
+ }
+
+ found:
+ if ( !bug )
+ return -EINVAL;
+
+ if ( id == BUGFRAME_run_fn )
+ {
+#ifdef BUG_FN_REG
+ void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;
+#else
+ void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
+#endif
+
+ fn(regs);
+
+ return id;
+ }
+
+ /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
+ filename = bug_ptr(bug);
+ if ( !is_kernel(filename) && !is_patch(filename) )
+ return -EINVAL;
+ fixup = strlen(filename);
+ if ( fixup > 50 )
+ {
+ filename += fixup - 47;
+ prefix = "...";
+ }
+ lineno = bug_line(bug);
+
+ switch ( id )
+ {
+ case BUGFRAME_warn:
+ printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
+ show_execution_state(regs);
+
+ return id;
+
+ case BUGFRAME_bug:
+ printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
+
+ if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
+ return id;
+
+ show_execution_state(regs);
+ panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
+
+ case BUGFRAME_assert:
+ /* ASSERT: decode the predicate string pointer. */
+ predicate = bug_msg(bug);
+ if ( !is_kernel(predicate) && !is_patch(predicate) )
+ predicate = "<unknown>";
+
+ printk("Assertion '%s' failed at %s%s:%d\n",
+ predicate, prefix, filename, lineno);
+
+ if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
+ return id;
+
+ show_execution_state(regs);
+ panic("Assertion '%s' failed at %s%s:%d\n",
+ predicate, prefix, filename, lineno);
+ }
+
+ return id;
+}
diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
new file mode 100644
index 0000000000..4b18cfa69c
--- /dev/null
+++ b/xen/include/xen/bug.h
@@ -0,0 +1,150 @@
+#ifndef __XEN_BUG_H__
+#define __XEN_BUG_H__
+
+#define BUG_DISP_WIDTH 24
+#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
+#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
+
+#define BUGFRAME_run_fn 0
+#define BUGFRAME_warn 1
+#define BUGFRAME_bug 2
+#define BUGFRAME_assert 3
+
+#define BUGFRAME_NR 4
+
+#include <asm/bug.h>
+
+#ifndef __ASSEMBLY__
+
+#include <xen/errno.h>
+#include <xen/lib.h>
+#include <xen/stringify.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[];
+};
+
+#endif /* BUG_FRAME_STRUCT */
+
+#ifndef bug_loc
+#define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp)
+#endif
+
+#ifndef bug_ptr
+#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
+#endif
+
+#ifndef bug_line
+#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0)) & \
+ ((1 << BUG_LINE_HI_WIDTH) - 1)) << \
+ BUG_LINE_LO_WIDTH) + \
+ (((b)->line_lo + ((b)->ptr_disp < 0)) & \
+ ((1 << BUG_LINE_LO_WIDTH) - 1)))
+#endif
+
+#ifndef bug_msg
+#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
+#endif
+
+#ifndef BUG_ASM_CONST
+#define BUG_ASM_CONST ""
+#endif
+
+#if !defined(_ASM_BUGFRAME_TEXT) || !defined(_ASM_BUGFRAME_INFO)
+
+#define _ASM_BUGFRAME_TEXT(second_frame) \
+ ".Lbug%=:"BUG_INSTR"\n" \
+ " .pushsection .bug_frames.%"BUG_ASM_CONST"[bf_type], \"a\", %%progbits\n" \
+ " .p2align 2\n" \
+ ".Lfrm%=:\n" \
+ " .long (.Lbug%= - .Lfrm%=) + %"BUG_ASM_CONST"[bf_line_hi]\n" \
+ " .long (%"BUG_ASM_CONST"[bf_ptr] - .Lfrm%=) + %"BUG_ASM_CONST"[bf_line_lo]\n"\
+ " .if " #second_frame "\n" \
+ " .long 0, %"BUG_ASM_CONST"[bf_msg] - .Lfrm%=\n" \
+ " .endif\n" \
+ " .popsection\n"
+
+#define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \
+ [bf_type] "i" (type), \
+ [bf_ptr] "i" (ptr), \
+ [bf_msg] "i" (msg), \
+ [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1)) \
+ << BUG_DISP_WIDTH), \
+ [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
+
+#endif /* _ASM_BUGFRAME_TEXT || _ASM_BUGFRAME_INFO */
+
+#ifndef BUG_FRAME
+
+#define BUG_FRAME(type, line, ptr, second_frame, msg) do { \
+ BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)); \
+ BUILD_BUG_ON((type) >= BUGFRAME_NR); \
+ asm volatile ( _ASM_BUGFRAME_TEXT(second_frame) \
+ :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \
+} while (0)
+
+#endif
+
+#ifndef run_in_exception_handler
+
+/*
+ * TODO: untangle header dependences, break BUILD_BUG_ON() out of xen/lib.h,
+ * and use a real static inline here to get proper type checking of fn().
+ */
+#define run_in_exception_handler(fn) \
+ do { \
+ (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \
+ BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \
+ } while ( 0 )
+
+#endif /* run_in_exception_handler */
+
+#ifndef WARN
+#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
+#endif
+
+#ifndef BUG
+#define BUG() do { \
+ BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \
+ unreachable(); \
+} while (0)
+#endif
+
+#ifndef assert_failed
+#define assert_failed(msg) do { \
+ BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
+ unreachable(); \
+} while (0)
+#endif
+
+#ifdef CONFIG_GENERIC_BUG_FRAME
+
+struct cpu_user_regs;
+
+int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc);
+
+#endif /* CONFIG_GENERIC_BUG_FRAME */
+
+extern const struct bug_frame __start_bug_frames[],
+ __stop_bug_frames_0[],
+ __stop_bug_frames_1[],
+ __stop_bug_frames_2[],
+ __stop_bug_frames_3[];
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* __XEN_BUG_H__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.39.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-02-24 11:31 ` [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
@ 2023-02-25 16:42 ` Julien Grall
2023-02-27 9:48 ` Jan Beulich
2023-02-28 17:21 ` Oleksii
2023-02-27 14:23 ` Jan Beulich
1 sibling, 2 replies; 39+ messages in thread
From: Julien Grall @ 2023-02-25 16:42 UTC (permalink / raw)
To: Oleksii Kurochko, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu
Hi Oleksii,
On 24/02/2023 11:31, Oleksii Kurochko wrote:
> A large part of the content of the bug.h is repeated among all
> architectures, so it was decided to create a new config
> CONFIG_GENERIC_BUG_FRAME.
>
> The version of <bug.h> from x86 was taken as the base version.
>
> The patch introduces the following stuff:
> * common bug.h header
> * generic implementation of do_bug_frame
> * new config CONFIG_GENERIC_BUG_FRAME
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V3:
> * Add debugger_trap_fatal() to do_bug_frame(). It simplifies usage of
> do_bug_frame() for x86 so making handle_bug_frame() and find_bug_frame()
> not needed anymore.
> * Update do_bug_frame() to return -EINVAL if something goes wrong; otherwise
> id of bug_frame
> * Update _ASM_BUGFRAME_TEXT to make it more portable.
> * Drop unnecessary comments.
> * define stub value for TRAP_invalid_op in case if wasn't defined in
> arch-specific folders.
> ---
> Changes in V2:
> - Switch to x86 implementation as generic as it is more compact
> ( at least from the point of view of bug frame structure ).
> - Rename CONFIG_GENERIC_DO_BUG_FRAME to CONFIG_GENERIC_BUG_FRAME.
> - Change the macro bug_loc(b) to avoid the need for a cast:
> #define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp)
> - Rename BUG_FRAME_STUFF to BUG_FRAME_STRUCT
> - Make macros related to bug frame structure more generic.
> - Introduce BUG_INSTR and MODIFIER to make _ASM_BUGFRAME_TEXT reusable
> between x86 and RISC-V.
> - Rework do_bug_frame() and introduce find_bug_frame() and handle_bug_frame()
> functions to make it reusable by x86.
> - code style fixes
> ---
> xen/common/Kconfig | 3 +
> xen/common/Makefile | 1 +
> xen/common/bug.c | 109 ++++++++++++++++++++++++++++++
> xen/include/xen/bug.h | 150 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 263 insertions(+)
> create mode 100644 xen/common/bug.c
> create mode 100644 xen/include/xen/bug.h
>
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index f1ea3199c8..b226323537 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -28,6 +28,9 @@ config ALTERNATIVE_CALL
> config ARCH_MAP_DOMAIN_PAGE
> bool
>
> +config GENERIC_BUG_FRAME
> + bool
> +
> config HAS_ALTERNATIVE
> bool
>
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index bbd75b4be6..46049eac35 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -1,5 +1,6 @@
> obj-$(CONFIG_ARGO) += argo.o
> obj-y += bitmap.o
> +obj-$(CONFIG_GENERIC_BUG_FRAME) += bug.o
> obj-$(CONFIG_HYPFS_CONFIG) += config_data.o
> obj-$(CONFIG_CORE_PARKING) += core_parking.o
> obj-y += cpu.o
> diff --git a/xen/common/bug.c b/xen/common/bug.c
> new file mode 100644
> index 0000000000..f81724fc9b
> --- /dev/null
> +++ b/xen/common/bug.c
> @@ -0,0 +1,109 @@
> +#include <xen/bug.h>
> +#include <xen/debugger.h>
> +#include <xen/errno.h>
> +#include <xen/kernel.h>
> +#include <xen/livepatch.h> > +#include <xen/string.h>
> +#include <xen/types.h>
> +#include <xen/virtual_region.h>
> +
> +#include <asm/processor.h>
> +
> +/* Set default value for TRAP_invalid_op as it is defined only for X86 now */
> +#ifndef TRAP_invalid_op
> +#define TRAP_invalid_op 0
> +#endif
It feels to me that this value should be defined in the else part in
xen/debugger.h.
> +
> +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc)
I would suggest to document what this function is meant to return.
AFAUI, it would return a negative value in case of an error otherwise
the bug type.
> +{
> + const struct bug_frame *bug = NULL;
> + const struct virtual_region *region;
> + const char *prefix = "", *filename, *predicate;
> + unsigned long fixup;
> + unsigned int id = BUGFRAME_NR, lineno;
> +
> + region = find_text_region(pc);
> + if ( region )
NIT: If you invert the condition here, then you can reduce the indention
by one below.
> + {
> + for ( id = 0; id < BUGFRAME_NR; id++ )
> + {
> + const struct bug_frame *b;
> + unsigned int i;
You compare this against n_bugs which is a size_t. So, this wants to be
a size_t.
> +
> + for ( i = 0, b = region->frame[id].bugs;
> + i < region->frame[id].n_bugs; b++, i++ )
> + {
> + if ( bug_loc(b) == pc )
> + {
> + bug = b;
> + goto found;
> + }
> + }
> + }
> + }
> +
> + found:
> + if ( !bug )
> + return -EINVAL;
> +
> + if ( id == BUGFRAME_run_fn )
> + {
> +#ifdef BUG_FN_REG
> + void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;
AFAIU, this is necessary so Arm can use the generic do_bug_frame().
I was under the impression that RISC-V and Arm had the similar issue
with %c. It seems like you managed to resolve it on RISC-V, so can we
fully switch Arm to the generic implementation of bug?
> +#else
> + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> +#endif
> +
> + fn(regs);
> +
> + return id;
> + }
> +
> + /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> + filename = bug_ptr(bug);
> + if ( !is_kernel(filename) && !is_patch(filename) )
> + return -EINVAL;
> + fixup = strlen(filename);
> + if ( fixup > 50 )
> + {
> + filename += fixup - 47;
> + prefix = "...";
> + }
> + lineno = bug_line(bug);
> +
> + switch ( id )
> + {
> + case BUGFRAME_warn:
> + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
> + show_execution_state(regs);
> +
> + return id;
> +
> + case BUGFRAME_bug:
> + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> +
> + if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
> + return id;
> +
> + show_execution_state(regs);
> + panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> +
> + case BUGFRAME_assert:
> + /* ASSERT: decode the predicate string pointer. */
> + predicate = bug_msg(bug);
> + if ( !is_kernel(predicate) && !is_patch(predicate) )
> + predicate = "<unknown>";
> +
> + printk("Assertion '%s' failed at %s%s:%d\n",
> + predicate, prefix, filename, lineno);
> +
> + if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
> + return id;
> +
> + show_execution_state(regs);
> + panic("Assertion '%s' failed at %s%s:%d\n",
> + predicate, prefix, filename, lineno);
> + }
> +
> + return id;
> +}
> diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
> new file mode 100644
> index 0000000000..4b18cfa69c
> --- /dev/null
> +++ b/xen/include/xen/bug.h
> @@ -0,0 +1,150 @@
> +#ifndef __XEN_BUG_H__
> +#define __XEN_BUG_H__
> +
> +#define BUG_DISP_WIDTH 24
> +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> +
> +#define BUGFRAME_run_fn 0
> +#define BUGFRAME_warn 1
> +#define BUGFRAME_bug 2
> +#define BUGFRAME_assert 3
> +
> +#define BUGFRAME_NR 4
> +
> +#include <asm/bug.h>
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <xen/errno.h>
errno.h doesn't look to be used within this here. So is it necessary to
import it?
> +#include <xen/lib.h>
Why is this necessary to include in the header?
> +#include <xen/stringify.h>
You don't seem to use __stringify in this header. So is this necessary?
> +
> +#ifndef BUG_FRAME_STRUCT
> +
> +struct bug_frame {
> + signed int loc_disp:BUG_DISP_WIDTH;
> + unsigned int line_hi:BUG_LINE_HI_WIDTH;
> + signed int ptr_disp:BUG_DISP_WIDTH;
> + unsigned int line_lo:BUG_LINE_LO_WIDTH;
> + signed int msg_disp[];
> +};
> +
> +#endif /* BUG_FRAME_STRUCT */
> +
> +#ifndef bug_loc
> +#define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp)
> +#endif
> +
> +#ifndef bug_ptr
> +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
> +#endif
> +
> +#ifndef bug_line
> +#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0)) & \
> + ((1 << BUG_LINE_HI_WIDTH) - 1)) << \
> + BUG_LINE_LO_WIDTH) + \
> + (((b)->line_lo + ((b)->ptr_disp < 0)) & \
> + ((1 << BUG_LINE_LO_WIDTH) - 1)))
> +#endif
> +
> +#ifndef bug_msg
> +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
> +#endif
For all the macro above, it feels wrong to me to allow an architecture
to override them if the default BUG_FRAME_STRUCT.
It would also feels wrong to me that if the default BUG_FRAME_STRUCT is
not used to still partially rely on the generic version of the helper.\
So I would suggest to move them in the #ifndef BUG_FRAME_STRUCT and drop
the #ifndef <helper>.
> +
> +#ifndef BUG_ASM_CONST
> +#define BUG_ASM_CONST ""
> +#endif
This line is a bit misterious to me. Would you be able to outline why an
architecture would override this?
> +
> +#if !defined(_ASM_BUGFRAME_TEXT) || !defined(_ASM_BUGFRAME_INFO)
> +
> +#define _ASM_BUGFRAME_TEXT(second_frame) \
> + ".Lbug%=:"BUG_INSTR"\n" \
> + " .pushsection .bug_frames.%"BUG_ASM_CONST"[bf_type], \"a\", %%progbits\n" \
> + " .p2align 2\n" \
> + ".Lfrm%=:\n" \
> + " .long (.Lbug%= - .Lfrm%=) + %"BUG_ASM_CONST"[bf_line_hi]\n" \
> + " .long (%"BUG_ASM_CONST"[bf_ptr] - .Lfrm%=) + %"BUG_ASM_CONST"[bf_line_lo]\n"\
> + " .if " #second_frame "\n" \
> + " .long 0, %"BUG_ASM_CONST"[bf_msg] - .Lfrm%=\n" \
> + " .endif\n" \
> + " .popsection\n"
> +
> +#define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \
> + [bf_type] "i" (type), \
> + [bf_ptr] "i" (ptr), \
> + [bf_msg] "i" (msg), \
> + [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1)) \
> + << BUG_DISP_WIDTH), \
> + [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
> +
> +#endif /* _ASM_BUGFRAME_TEXT || _ASM_BUGFRAME_INFO */
> +
> +#ifndef BUG_FRAME
> +
> +#define BUG_FRAME(type, line, ptr, second_frame, msg) do { \
> + BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)); \
> + BUILD_BUG_ON((type) >= BUGFRAME_NR); \
> + asm volatile ( _ASM_BUGFRAME_TEXT(second_frame) \
> + :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \
> +} while (0)
> +
> +#endif
> +
> +#ifndef run_in_exception_handler
> +
> +/*
> + * TODO: untangle header dependences, break BUILD_BUG_ON() out of xen/lib.h,
> + * and use a real static inline here to get proper type checking of fn().
> + */
> +#define run_in_exception_handler(fn) \
> + do { \
> + (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \
> + BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \
> + } while ( 0 )
> +
> +#endif /* run_in_exception_handler */
> +
> +#ifndef WARN
> +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
> +#endif
> +
> +#ifndef BUG
> +#define BUG() do { \
> + BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \
> + unreachable(); \
> +} while (0)
> +#endif
> +
> +#ifndef assert_failed
> +#define assert_failed(msg) do { \
> + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
> + unreachable(); \
> +} while (0)
> +#endif
> +
> +#ifdef CONFIG_GENERIC_BUG_FRAME
> +
> +struct cpu_user_regs;
> +
> +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc);
> +
> +#endif /* CONFIG_GENERIC_BUG_FRAME */
> +
> +extern const struct bug_frame __start_bug_frames[],
> + __stop_bug_frames_0[],
> + __stop_bug_frames_1[],
> + __stop_bug_frames_2[],
> + __stop_bug_frames_3[];
> +
> +#endif /* !__ASSEMBLY__ */
> +
> +#endif /* __XEN_BUG_H__ */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-02-25 16:42 ` Julien Grall
@ 2023-02-27 9:48 ` Jan Beulich
2023-02-28 17:21 ` Oleksii
1 sibling, 0 replies; 39+ messages in thread
From: Jan Beulich @ 2023-02-27 9:48 UTC (permalink / raw)
To: Julien Grall, Oleksii Kurochko
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida, George Dunlap,
Wei Liu, xen-devel
On 25.02.2023 17:42, Julien Grall wrote:
> On 24/02/2023 11:31, Oleksii Kurochko wrote:
>> --- /dev/null
>> +++ b/xen/common/bug.c
>> @@ -0,0 +1,109 @@
>> +#include <xen/bug.h>
>> +#include <xen/debugger.h>
>> +#include <xen/errno.h>
>> +#include <xen/kernel.h>
>> +#include <xen/livepatch.h> > +#include <xen/string.h>
>> +#include <xen/types.h>
>> +#include <xen/virtual_region.h>
>> +
>> +#include <asm/processor.h>
>> +
>> +/* Set default value for TRAP_invalid_op as it is defined only for X86 now */
>> +#ifndef TRAP_invalid_op
>> +#define TRAP_invalid_op 0
>> +#endif
>
> It feels to me that this value should be defined in the else part in
> xen/debugger.h.
I guess with [1] it won't be as straightforward anymore ...
Jan
[1] https://lists.xen.org/archives/html/xen-devel/2023-02/msg01026.html
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-02-25 16:42 ` Julien Grall
2023-02-27 9:48 ` Jan Beulich
@ 2023-02-28 17:21 ` Oleksii
2023-02-28 18:01 ` Julien Grall
1 sibling, 1 reply; 39+ messages in thread
From: Oleksii @ 2023-02-28 17:21 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu
Hi Julien,
On Sat, 2023-02-25 at 16:42 +0000, Julien Grall wrote:
> Hi Oleksii,
>
> On 24/02/2023 11:31, Oleksii Kurochko wrote:
> > A large part of the content of the bug.h is repeated among all
> > architectures, so it was decided to create a new config
> > CONFIG_GENERIC_BUG_FRAME.
> >
> > The version of <bug.h> from x86 was taken as the base version.
> >
> > The patch introduces the following stuff:
> > * common bug.h header
> > * generic implementation of do_bug_frame
> > * new config CONFIG_GENERIC_BUG_FRAME
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in V3:
> > * Add debugger_trap_fatal() to do_bug_frame(). It simplifies
> > usage of
> > do_bug_frame() for x86 so making handle_bug_frame() and
> > find_bug_frame()
> > not needed anymore.
> > * Update do_bug_frame() to return -EINVAL if something goes
> > wrong; otherwise
> > id of bug_frame
> > * Update _ASM_BUGFRAME_TEXT to make it more portable.
> > * Drop unnecessary comments.
> > * define stub value for TRAP_invalid_op in case if wasn't defined
> > in
> > arch-specific folders.
> > ---
> > Changes in V2:
> > - Switch to x86 implementation as generic as it is more compact
> > ( at least from the point of view of bug frame structure ).
> > - Rename CONFIG_GENERIC_DO_BUG_FRAME to
> > CONFIG_GENERIC_BUG_FRAME.
> > - Change the macro bug_loc(b) to avoid the need for a cast:
> > #define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp)
> > - Rename BUG_FRAME_STUFF to BUG_FRAME_STRUCT
> > - Make macros related to bug frame structure more generic.
> > - Introduce BUG_INSTR and MODIFIER to make _ASM_BUGFRAME_TEXT
> > reusable
> > between x86 and RISC-V.
> > - Rework do_bug_frame() and introduce find_bug_frame() and
> > handle_bug_frame()
> > functions to make it reusable by x86.
> > - code style fixes
> > ---
> > xen/common/Kconfig | 3 +
> > xen/common/Makefile | 1 +
> > xen/common/bug.c | 109 ++++++++++++++++++++++++++++++
> > xen/include/xen/bug.h | 150
> > ++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 263 insertions(+)
> > create mode 100644 xen/common/bug.c
> > create mode 100644 xen/include/xen/bug.h
> >
> > diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> > index f1ea3199c8..b226323537 100644
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -28,6 +28,9 @@ config ALTERNATIVE_CALL
> > config ARCH_MAP_DOMAIN_PAGE
> > bool
> >
> > +config GENERIC_BUG_FRAME
> > + bool
> > +
> > config HAS_ALTERNATIVE
> > bool
> >
> > diff --git a/xen/common/Makefile b/xen/common/Makefile
> > index bbd75b4be6..46049eac35 100644
> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -1,5 +1,6 @@
> > obj-$(CONFIG_ARGO) += argo.o
> > obj-y += bitmap.o
> > +obj-$(CONFIG_GENERIC_BUG_FRAME) += bug.o
> > obj-$(CONFIG_HYPFS_CONFIG) += config_data.o
> > obj-$(CONFIG_CORE_PARKING) += core_parking.o
> > obj-y += cpu.o
> > diff --git a/xen/common/bug.c b/xen/common/bug.c
> > new file mode 100644
> > index 0000000000..f81724fc9b
> > --- /dev/null
> > +++ b/xen/common/bug.c
> > @@ -0,0 +1,109 @@
> > +#include <xen/bug.h>
> > +#include <xen/debugger.h>
> > +#include <xen/errno.h>
> > +#include <xen/kernel.h>
> > +#include <xen/livepatch.h> > +#include <xen/string.h>
> > +#include <xen/types.h>
> > +#include <xen/virtual_region.h>
> > +
> > +#include <asm/processor.h>
> > +
> > +/* Set default value for TRAP_invalid_op as it is defined only for
> > X86 now */
> > +#ifndef TRAP_invalid_op
> > +#define TRAP_invalid_op 0
> > +#endif
>
> It feels to me that this value should be defined in the else part in
> xen/debugger.h.
As it was disscussed in the other e-mail [1] the following will be
introduced in <xen/bug.h>
#ifndef BUG_DEBUGGER_TRAP_FATAL
#define BUG_DEBUGGER_TRAP_FATAL(regs) 0
#endif
and re-defined in <asm/bug.h>.
Of course, do_bug_frame() will be updated correspondingly to use
BUG_DEBUGGER_TRAP_FATAL.
[1]
https://lore.kernel.org/xen-devel/9b66ee51-17c3-0f8e-0fc2-4ff083952e9d@suse.com/
>
> > +
> > +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long
> > pc)
>
> I would suggest to document what this function is meant to return.
> AFAUI, it would return a negative value in case of an error otherwise
> the bug type.
sure. i'll added in the next patch version.
>
> > +{
> > + const struct bug_frame *bug = NULL;
> > + const struct virtual_region *region;
> > + const char *prefix = "", *filename, *predicate;
> > + unsigned long fixup;
> > + unsigned int id = BUGFRAME_NR, lineno;
> > +
> > + region = find_text_region(pc);
> > + if ( region )
>
> NIT: If you invert the condition here, then you can reduce the
> indention
> by one below.
Thanks. i'll added in the next patch version.
>
> > + {
> > + for ( id = 0; id < BUGFRAME_NR; id++ )
> > + {
> > + const struct bug_frame *b;
> > + unsigned int i;
>
> You compare this against n_bugs which is a size_t. So, this wants to
> be
> a size_t.
This one will be updated too. Thanks.
>
> > +
> > + for ( i = 0, b = region->frame[id].bugs;
> > + i < region->frame[id].n_bugs; b++, i++ )
> > + {
> > + if ( bug_loc(b) == pc )
> > + {
> > + bug = b;
> > + goto found;
> > + }
> > + }
> > + }
> > + }
> > +
> > + found:
> > + if ( !bug )
> > + return -EINVAL;
> > +
> > + if ( id == BUGFRAME_run_fn )
> > + {
> > +#ifdef BUG_FN_REG
> > + void (*fn)(const struct cpu_user_regs *) = (void *)regs-
> > >BUG_FN_REG;
>
> AFAIU, this is necessary so Arm can use the generic do_bug_frame().
>
> I was under the impression that RISC-V and Arm had the similar issue
> with %c. It seems like you managed to resolve it on RISC-V, so can we
> fully switch Arm to the generic implementation of bug?
I tried to switch ARM to generic implementation.
Here is the patch: [1]
diff --git a/xen/arch/arm/include/asm/bug.h
b/xen/arch/arm/include/asm/bug.h
index e6cc37e1d6..ffb0f569fc 100644
--- a/xen/arch/arm/include/asm/bug.h
+++ b/xen/arch/arm/include/asm/bug.h
@@ -1,8 +1,6 @@
#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)
@@ -11,63 +9,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_ptr(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 BUG_ASM_CONST "c"
#endif /* __ARM_BUG_H__ */
...
(it will be merged with patch 3 if it is OK )
And looks like we can switch ARM to generic implementation as all tests
passed:
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396
The only issue is with yocto-arm:
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396/failures
But I am not sure that it is because of my patch
Is this enough from a verification point of view?
[1]
https://gitlab.com/xen-project/people/olkur/xen/-/commit/5ff7a06e1d354e1e42bde1c203f3cf05a3653ad6https://gitlab.com/xen-project/people/olkur/xen/-/commit/5ff7a06e1d354e1e42bde1c203f3cf05a3653ad6
>
> > +#else
> > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> > +#endif
> > +
> > + fn(regs);
> > +
> > + return id;
> > + }
> > +
> > + /* WARN, BUG or ASSERT: decode the filename pointer and line
> > number. */
> > + filename = bug_ptr(bug);
> > + if ( !is_kernel(filename) && !is_patch(filename) )
> > + return -EINVAL;
> > + fixup = strlen(filename);
> > + if ( fixup > 50 )
> > + {
> > + filename += fixup - 47;
> > + prefix = "...";
> > + }
> > + lineno = bug_line(bug);
> > +
> > + switch ( id )
> > + {
> > + case BUGFRAME_warn:
> > + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
> > + show_execution_state(regs);
> > +
> > + return id;
> > +
> > + case BUGFRAME_bug:
> > + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> > +
> > + if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
> > + return id;
> > +
> > + show_execution_state(regs);
> > + panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> > +
> > + case BUGFRAME_assert:
> > + /* ASSERT: decode the predicate string pointer. */
> > + predicate = bug_msg(bug);
> > + if ( !is_kernel(predicate) && !is_patch(predicate) )
> > + predicate = "<unknown>";
> > +
> > + printk("Assertion '%s' failed at %s%s:%d\n",
> > + predicate, prefix, filename, lineno);
> > +
> > + if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
> > + return id;
> > +
> > + show_execution_state(regs);
> > + panic("Assertion '%s' failed at %s%s:%d\n",
> > + predicate, prefix, filename, lineno);
> > + }
> > +
> > + return id;
> > +}
> > diff --git a/xen/include/xen/bug.h b/xen/include/xen/bug.h
> > new file mode 100644
> > index 0000000000..4b18cfa69c
> > --- /dev/null
> > +++ b/xen/include/xen/bug.h
> > @@ -0,0 +1,150 @@
> > +#ifndef __XEN_BUG_H__
> > +#define __XEN_BUG_H__
> > +
> > +#define BUG_DISP_WIDTH 24
> > +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> > +
> > +#define BUGFRAME_run_fn 0
> > +#define BUGFRAME_warn 1
> > +#define BUGFRAME_bug 2
> > +#define BUGFRAME_assert 3
> > +
> > +#define BUGFRAME_NR 4
> > +
> > +#include <asm/bug.h>
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +#include <xen/errno.h>
>
> errno.h doesn't look to be used within this here. So is it necessary
> to
> import it?
>
> > +#include <xen/lib.h>
>
> Why is this necessary to include in the header?
>
> > +#include <xen/stringify.h>
>
> You don't seem to use __stringify in this header. So is this
> necessary?
The mentioned headers will be removed. They was needed when I tried to
use ARM implementation as generic one.
>
> > +
> > +#ifndef BUG_FRAME_STRUCT
> > +
> > +struct bug_frame {
> > + signed int loc_disp:BUG_DISP_WIDTH;
> > + unsigned int line_hi:BUG_LINE_HI_WIDTH;
> > + signed int ptr_disp:BUG_DISP_WIDTH;
> > + unsigned int line_lo:BUG_LINE_LO_WIDTH;
> > + signed int msg_disp[];
> > +};
> > +
> > +#endif /* BUG_FRAME_STRUCT */
> > +
> > +#ifndef bug_loc
> > +#define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp)
> > +#endif
> > +
> > +#ifndef bug_ptr
> > +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
> > +#endif
> > +
> > +#ifndef bug_line
> > +#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0))
> > & \
> > + ((1 << BUG_LINE_HI_WIDTH) - 1))
> > << \
> > + BUG_LINE_LO_WIDTH)
> > + \
> > + (((b)->line_lo + ((b)->ptr_disp < 0))
> > & \
> > + ((1 << BUG_LINE_LO_WIDTH) - 1)))
> > +#endif
> > +
> > +#ifndef bug_msg
> > +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
> > +#endif
>
> For all the macro above, it feels wrong to me to allow an
> architecture
> to override them if the default BUG_FRAME_STRUCT.
>
> It would also feels wrong to me that if the default BUG_FRAME_STRUCT
> is
> not used to still partially rely on the generic version of the
> helper.\
>
> So I would suggest to move them in the #ifndef BUG_FRAME_STRUCT and
> drop
> the #ifndef <helper>.
Agree. I'll do that in the next version of the patch. Thanks.
>
> > +
> > +#ifndef BUG_ASM_CONST
> > +#define BUG_ASM_CONST ""
> > +#endif
>
> This line is a bit misterious to me. Would you be able to outline why
> an
> architecture would override this?
It is needed in case if compiler for an architecture doesn't have
proper support of '%c' ( it is so for ARM & RISC-V )
>
> > +
> > +#if !defined(_ASM_BUGFRAME_TEXT) || !defined(_ASM_BUGFRAME_INFO)
> > +
> > +#define
> > _ASM_BUGFRAME_TEXT(second_frame)
> > \
> > +
> > ".Lbug%=:"BUG_INSTR"\n"
> > \
> > + " .pushsection .bug_frames.%"BUG_ASM_CONST"[bf_type], \"a\",
> > %%progbits\n" \
> > + " .p2align
> > 2\n"
> > \
> > +
> > ".Lfrm%=:\n"
> > \
> > + " .long (.Lbug%= - .Lfrm%=) +
> > %"BUG_ASM_CONST"[bf_line_hi]\n" \
> > + " .long (%"BUG_ASM_CONST"[bf_ptr] - .Lfrm%=) +
> > %"BUG_ASM_CONST"[bf_line_lo]\n"\
> > + " .if " #second_frame
> > "\n" \
> > + " .long 0, %"BUG_ASM_CONST"[bf_msg] -
> > .Lfrm%=\n" \
> > + "
> > .endif\n"
> > \
> > + " .popsection\n"
> > +
> > +#define _ASM_BUGFRAME_INFO(type, line, ptr,
> > msg) \
> > + [bf_type] "i"
> > (type), \
> > + [bf_ptr] "i"
> > (ptr), \
> > + [bf_msg] "i"
> > (msg), \
> > + [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) -
> > 1)) \
> > + <<
> > BUG_DISP_WIDTH), \
> > + [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) <<
> > BUG_DISP_WIDTH)
> > +
> > +#endif /* _ASM_BUGFRAME_TEXT || _ASM_BUGFRAME_INFO */
> > +
> > +#ifndef BUG_FRAME
> > +
> > +#define BUG_FRAME(type, line, ptr, second_frame, msg) do
> > { \
> > + BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> > BUG_LINE_HI_WIDTH)); \
> > + BUILD_BUG_ON((type) >=
> > BUGFRAME_NR); \
> > + asm volatile (
> > _ASM_BUGFRAME_TEXT(second_frame) \
> > + :: _ASM_BUGFRAME_INFO(type, line, ptr, msg)
> > ); \
> > +} while (0)
> > +
> > +#endif
> > +
> > +#ifndef run_in_exception_handler
> > +
> > +/*
> > + * TODO: untangle header dependences, break BUILD_BUG_ON() out of
> > xen/lib.h,
> > + * and use a real static inline here to get proper type checking
> > of fn().
> > + */
> > +#define run_in_exception_handler(fn) \
> > + do { \
> > + (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \
> > + BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \
> > + } while ( 0 )
> > +
> > +#endif /* run_in_exception_handler */
> > +
> > +#ifndef WARN
> > +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0,
> > NULL)
> > +#endif
> > +
> > +#ifndef BUG
> > +#define BUG() do { \
> > + BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \
> > + unreachable(); \
> > +} while (0)
> > +#endif
> > +
> > +#ifndef assert_failed
> > +#define assert_failed(msg) do { \
> > + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
> > + unreachable(); \
> > +} while (0)
> > +#endif
> > +
> > +#ifdef CONFIG_GENERIC_BUG_FRAME
> > +
> > +struct cpu_user_regs;
> > +
> > +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long
> > pc);
> > +
> > +#endif /* CONFIG_GENERIC_BUG_FRAME */
> > +
> > +extern const struct bug_frame __start_bug_frames[],
> > + __stop_bug_frames_0[],
> > + __stop_bug_frames_1[],
> > + __stop_bug_frames_2[],
> > + __stop_bug_frames_3[];
> > +
> > +#endif /* !__ASSEMBLY__ */
> > +
> > +#endif /* __XEN_BUG_H__ */
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
>
> Cheers,
>
~ Oleksii
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-02-28 17:21 ` Oleksii
@ 2023-02-28 18:01 ` Julien Grall
2023-02-28 20:24 ` Oleksii
0 siblings, 1 reply; 39+ messages in thread
From: Julien Grall @ 2023-02-28 18:01 UTC (permalink / raw)
To: Oleksii, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu
On 28/02/2023 17:21, Oleksii wrote:
> Hi Julien,
Hi Oleksii,
>>> +
>>> + for ( i = 0, b = region->frame[id].bugs;
>>> + i < region->frame[id].n_bugs; b++, i++ )
>>> + {
>>> + if ( bug_loc(b) == pc )
>>> + {
>>> + bug = b;
>>> + goto found;
>>> + }
>>> + }
>>> + }
>>> + }
>>> +
>>> + found:
>>> + if ( !bug )
>>> + return -EINVAL;
>>> +
>>> + if ( id == BUGFRAME_run_fn )
>>> + {
>>> +#ifdef BUG_FN_REG
>>> + void (*fn)(const struct cpu_user_regs *) = (void *)regs-
>>>> BUG_FN_REG;
>>
>> AFAIU, this is necessary so Arm can use the generic do_bug_frame().
>>
>> I was under the impression that RISC-V and Arm had the similar issue
>> with %c. It seems like you managed to resolve it on RISC-V, so can we
>> fully switch Arm to the generic implementation of bug?
> I tried to switch ARM to generic implementation.
>
> Here is the patch: [1]
I have replied on the other thread.
>>> +#ifndef BUG_ASM_CONST
>>> +#define BUG_ASM_CONST ""
>>> +#endif
>>
>> This line is a bit misterious to me. Would you be able to outline why
>> an
>> architecture would override this?
> It is needed in case if compiler for an architecture doesn't have
> proper support of '%c' ( it is so for ARM & RISC-V )
Hmmm.... Why can't x86 use the same version? IOW what's the benefits to
differ on x86?
Anyway, documentation is always good to have because it helps the
reader/reviewer to understand how such decision was made.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-02-28 18:01 ` Julien Grall
@ 2023-02-28 20:24 ` Oleksii
0 siblings, 0 replies; 39+ messages in thread
From: Oleksii @ 2023-02-28 20:24 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
George Dunlap, Wei Liu
On Tue, 2023-02-28 at 18:01 +0000, Julien Grall wrote:
> On 28/02/2023 17:21, Oleksii wrote:
> > Hi Julien,
>
> Hi Oleksii,
> > > > +
> > > > + for ( i = 0, b = region->frame[id].bugs;
> > > > + i < region->frame[id].n_bugs; b++, i++ )
> > > > + {
> > > > + if ( bug_loc(b) == pc )
> > > > + {
> > > > + bug = b;
> > > > + goto found;
> > > > + }
> > > > + }
> > > > + }
> > > > + }
> > > > +
> > > > + found:
> > > > + if ( !bug )
> > > > + return -EINVAL;
> > > > +
> > > > + if ( id == BUGFRAME_run_fn )
> > > > + {
> > > > +#ifdef BUG_FN_REG
> > > > + void (*fn)(const struct cpu_user_regs *) = (void
> > > > *)regs-
> > > > > BUG_FN_REG;
> > >
> > > AFAIU, this is necessary so Arm can use the generic
> > > do_bug_frame().
> > >
> > > I was under the impression that RISC-V and Arm had the similar
> > > issue
> > > with %c. It seems like you managed to resolve it on RISC-V, so
> > > can we
> > > fully switch Arm to the generic implementation of bug?
> > I tried to switch ARM to generic implementation.
> >
> > Here is the patch: [1]
>
> I have replied on the other thread.
> > > > +#ifndef BUG_ASM_CONST
> > > > +#define BUG_ASM_CONST ""
> > > > +#endif
> > >
> > > This line is a bit misterious to me. Would you be able to outline
> > > why
> > > an
> > > architecture would override this?
> > It is needed in case if compiler for an architecture doesn't have
> > proper support of '%c' ( it is so for ARM & RISC-V )
>
> Hmmm.... Why can't x86 use the same version? IOW what's the benefits
> to
> differ on x86?
We can't use '%c' for all architectures because not all compiler
supports '%c' fully for all architectures.
There is no any benefits. In case of x86 it is needed to delete
punctuation before immediate. I mean that immediate is passed as $1 (
or # i always missed with ARM ) and to drop $ it is used %c.
>
> Anyway, documentation is always good to have because it helps the
> reader/reviewer to understand how such decision was made.
I'll add the comment then before define.
>
> Cheers,
>
~ Oleksii
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-02-24 11:31 ` [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-02-25 16:42 ` Julien Grall
@ 2023-02-27 14:23 ` Jan Beulich
2023-02-28 10:30 ` Oleksii
1 sibling, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2023-02-27 14:23 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida, George Dunlap,
Julien Grall, Wei Liu, xen-devel
On 24.02.2023 12:31, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/common/bug.c
> @@ -0,0 +1,109 @@
> +#include <xen/bug.h>
> +#include <xen/debugger.h>
> +#include <xen/errno.h>
> +#include <xen/kernel.h>
> +#include <xen/livepatch.h>
> +#include <xen/string.h>
> +#include <xen/types.h>
> +#include <xen/virtual_region.h>
> +
> +#include <asm/processor.h>
> +
> +/* Set default value for TRAP_invalid_op as it is defined only for X86 now */
> +#ifndef TRAP_invalid_op
> +#define TRAP_invalid_op 0
> +#endif
> +
> +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc)
> +{
> + const struct bug_frame *bug = NULL;
> + const struct virtual_region *region;
> + const char *prefix = "", *filename, *predicate;
> + unsigned long fixup;
> + unsigned int id = BUGFRAME_NR, lineno;
> +
> + region = find_text_region(pc);
> + if ( region )
> + {
> + for ( id = 0; id < BUGFRAME_NR; id++ )
> + {
> + const struct bug_frame *b;
> + unsigned int i;
> +
> + for ( i = 0, b = region->frame[id].bugs;
> + i < region->frame[id].n_bugs; b++, i++ )
> + {
> + if ( bug_loc(b) == pc )
> + {
> + bug = b;
> + goto found;
> + }
> + }
> + }
> + }
> +
> + found:
> + if ( !bug )
> + return -EINVAL;
> +
> + if ( id == BUGFRAME_run_fn )
> + {
> +#ifdef BUG_FN_REG
> + void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;
> +#else
> + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> +#endif
> +
> + fn(regs);
> +
> + return id;
> + }
> +
> + /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> + filename = bug_ptr(bug);
> + if ( !is_kernel(filename) && !is_patch(filename) )
> + return -EINVAL;
> + fixup = strlen(filename);
> + if ( fixup > 50 )
> + {
> + filename += fixup - 47;
> + prefix = "...";
> + }
> + lineno = bug_line(bug);
> +
> + switch ( id )
> + {
> + case BUGFRAME_warn:
> + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
> + show_execution_state(regs);
> +
> + return id;
> +
> + case BUGFRAME_bug:
> + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> +
> + if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
TRAP_invalid_op is, as said, about to disappear on x86 as well. I think
this construct wants abstracting by another asm/bug.h provided macro
(taking just regs).
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-02-27 14:23 ` Jan Beulich
@ 2023-02-28 10:30 ` Oleksii
2023-02-28 10:42 ` Jan Beulich
0 siblings, 1 reply; 39+ messages in thread
From: Oleksii @ 2023-02-28 10:30 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida, George Dunlap,
Julien Grall, Wei Liu, xen-devel
On Mon, 2023-02-27 at 15:23 +0100, Jan Beulich wrote:
> On 24.02.2023 12:31, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/common/bug.c
> > @@ -0,0 +1,109 @@
> > +#include <xen/bug.h>
> > +#include <xen/debugger.h>
> > +#include <xen/errno.h>
> > +#include <xen/kernel.h>
> > +#include <xen/livepatch.h>
> > +#include <xen/string.h>
> > +#include <xen/types.h>
> > +#include <xen/virtual_region.h>
> > +
> > +#include <asm/processor.h>
> > +
> > +/* Set default value for TRAP_invalid_op as it is defined only for
> > X86 now */
> > +#ifndef TRAP_invalid_op
> > +#define TRAP_invalid_op 0
> > +#endif
> > +
> > +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long
> > pc)
> > +{
> > + const struct bug_frame *bug = NULL;
> > + const struct virtual_region *region;
> > + const char *prefix = "", *filename, *predicate;
> > + unsigned long fixup;
> > + unsigned int id = BUGFRAME_NR, lineno;
> > +
> > + region = find_text_region(pc);
> > + if ( region )
> > + {
> > + for ( id = 0; id < BUGFRAME_NR; id++ )
> > + {
> > + const struct bug_frame *b;
> > + unsigned int i;
> > +
> > + for ( i = 0, b = region->frame[id].bugs;
> > + i < region->frame[id].n_bugs; b++, i++ )
> > + {
> > + if ( bug_loc(b) == pc )
> > + {
> > + bug = b;
> > + goto found;
> > + }
> > + }
> > + }
> > + }
> > +
> > + found:
> > + if ( !bug )
> > + return -EINVAL;
> > +
> > + if ( id == BUGFRAME_run_fn )
> > + {
> > +#ifdef BUG_FN_REG
> > + void (*fn)(const struct cpu_user_regs *) = (void *)regs-
> > >BUG_FN_REG;
> > +#else
> > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> > +#endif
> > +
> > + fn(regs);
> > +
> > + return id;
> > + }
> > +
> > + /* WARN, BUG or ASSERT: decode the filename pointer and line
> > number. */
> > + filename = bug_ptr(bug);
> > + if ( !is_kernel(filename) && !is_patch(filename) )
> > + return -EINVAL;
> > + fixup = strlen(filename);
> > + if ( fixup > 50 )
> > + {
> > + filename += fixup - 47;
> > + prefix = "...";
> > + }
> > + lineno = bug_line(bug);
> > +
> > + switch ( id )
> > + {
> > + case BUGFRAME_warn:
> > + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
> > + show_execution_state(regs);
> > +
> > + return id;
> > +
> > + case BUGFRAME_bug:
> > + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
> > +
> > + if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
>
> TRAP_invalid_op is, as said, about to disappear on x86 as well. I
> think
> this construct wants abstracting by another asm/bug.h provided macro
> (taking just regs).
>
Thanks for the link.
Nice idea to abstract 'debugger_trap_fatal(TRAP_invalid_op, regs)'.
Actually we have to options here:
1. As you proposed abstract in <asm/bug.h>:
x86: #define DEBUG_TRAP_FATAL(regs) debugger_trap_fatal(X86_EXC_GP,
regs)
ARM: #define DEBUG_TRAP_FATAL(regs) 0
RISC-V: #define DEBUG_TRAP_FATAL(regs) 0
For ARM and RISC-V it doesn't use so we can skip the check if (
DEBUG_TRAP_FATAL ).
2. Abstract only TRAP_invalid_op in <asm/bug.h>
x86: #define TRAP_invalud_op X86_EXC_GP
RISC-V: #define TRAP_invalid_op 0
ARN: #define TRAP_invalid_op 0
I am not sure if we have to provide real invalid opcodes for RISC-V
and ARM as it looks like debug_trap_fatal() isn't used in ARM&RISC-V
now.
Could you please suggest which one option is better?
~ Oleksii
> Jan
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
2023-02-28 10:30 ` Oleksii
@ 2023-02-28 10:42 ` Jan Beulich
0 siblings, 0 replies; 39+ messages in thread
From: Jan Beulich @ 2023-02-28 10:42 UTC (permalink / raw)
To: Oleksii
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida, George Dunlap,
Julien Grall, Wei Liu, xen-devel
On 28.02.2023 11:30, Oleksii wrote:
> On Mon, 2023-02-27 at 15:23 +0100, Jan Beulich wrote:
>> On 24.02.2023 12:31, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/common/bug.c
>>> @@ -0,0 +1,109 @@
>>> +#include <xen/bug.h>
>>> +#include <xen/debugger.h>
>>> +#include <xen/errno.h>
>>> +#include <xen/kernel.h>
>>> +#include <xen/livepatch.h>
>>> +#include <xen/string.h>
>>> +#include <xen/types.h>
>>> +#include <xen/virtual_region.h>
>>> +
>>> +#include <asm/processor.h>
>>> +
>>> +/* Set default value for TRAP_invalid_op as it is defined only for
>>> X86 now */
>>> +#ifndef TRAP_invalid_op
>>> +#define TRAP_invalid_op 0
>>> +#endif
>>> +
>>> +int do_bug_frame(const struct cpu_user_regs *regs, unsigned long
>>> pc)
>>> +{
>>> + const struct bug_frame *bug = NULL;
>>> + const struct virtual_region *region;
>>> + const char *prefix = "", *filename, *predicate;
>>> + unsigned long fixup;
>>> + unsigned int id = BUGFRAME_NR, lineno;
>>> +
>>> + region = find_text_region(pc);
>>> + if ( region )
>>> + {
>>> + for ( id = 0; id < BUGFRAME_NR; id++ )
>>> + {
>>> + const struct bug_frame *b;
>>> + unsigned int i;
>>> +
>>> + for ( i = 0, b = region->frame[id].bugs;
>>> + i < region->frame[id].n_bugs; b++, i++ )
>>> + {
>>> + if ( bug_loc(b) == pc )
>>> + {
>>> + bug = b;
>>> + goto found;
>>> + }
>>> + }
>>> + }
>>> + }
>>> +
>>> + found:
>>> + if ( !bug )
>>> + return -EINVAL;
>>> +
>>> + if ( id == BUGFRAME_run_fn )
>>> + {
>>> +#ifdef BUG_FN_REG
>>> + void (*fn)(const struct cpu_user_regs *) = (void *)regs-
>>>> BUG_FN_REG;
>>> +#else
>>> + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
>>> +#endif
>>> +
>>> + fn(regs);
>>> +
>>> + return id;
>>> + }
>>> +
>>> + /* WARN, BUG or ASSERT: decode the filename pointer and line
>>> number. */
>>> + filename = bug_ptr(bug);
>>> + if ( !is_kernel(filename) && !is_patch(filename) )
>>> + return -EINVAL;
>>> + fixup = strlen(filename);
>>> + if ( fixup > 50 )
>>> + {
>>> + filename += fixup - 47;
>>> + prefix = "...";
>>> + }
>>> + lineno = bug_line(bug);
>>> +
>>> + switch ( id )
>>> + {
>>> + case BUGFRAME_warn:
>>> + printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
>>> + show_execution_state(regs);
>>> +
>>> + return id;
>>> +
>>> + case BUGFRAME_bug:
>>> + printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
>>> +
>>> + if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
>>
>> TRAP_invalid_op is, as said, about to disappear on x86 as well. I
>> think
>> this construct wants abstracting by another asm/bug.h provided macro
>> (taking just regs).
>>
> Thanks for the link.
>
> Nice idea to abstract 'debugger_trap_fatal(TRAP_invalid_op, regs)'.
> Actually we have to options here:
> 1. As you proposed abstract in <asm/bug.h>:
> x86: #define DEBUG_TRAP_FATAL(regs) debugger_trap_fatal(X86_EXC_GP,
> regs)
> ARM: #define DEBUG_TRAP_FATAL(regs) 0
> RISC-V: #define DEBUG_TRAP_FATAL(regs) 0
> For ARM and RISC-V it doesn't use so we can skip the check if (
> DEBUG_TRAP_FATAL ).
>
> 2. Abstract only TRAP_invalid_op in <asm/bug.h>
> x86: #define TRAP_invalud_op X86_EXC_GP
> RISC-V: #define TRAP_invalid_op 0
> ARN: #define TRAP_invalid_op 0
>
> I am not sure if we have to provide real invalid opcodes for RISC-V
> and ARM as it looks like debug_trap_fatal() isn't used in ARM&RISC-V
> now.
>
> Could you please suggest which one option is better?
I don't view 2 as a viable option. How an arch deals with invalid opcodes
is entirely arch-specific (including the naming). As to 1 - since we want
this solely for bug.c, I'd prefer if the wrapper macro's name would start
with BUG_, e.g. BUG_DEBUGGER_TRAP_FATAL() or BUG_TRAP_FATAL() or just
BUG_FATAL(). Further adding ARCH_ may also be wanted by other maintainers
(I'm neither pro nor con there).
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h>
2023-02-24 11:31 [PATCH v3 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-02-24 11:31 ` [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
@ 2023-02-24 11:31 ` Oleksii Kurochko
2023-02-25 16:47 ` Julien Grall
2023-02-27 14:29 ` Jan Beulich
2023-02-24 11:31 ` [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
2023-02-24 11:31 ` [PATCH v3 4/4] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
3 siblings, 2 replies; 39+ messages in thread
From: Oleksii Kurochko @ 2023-02-24 11:31 UTC (permalink / raw)
To: xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Oleksii Kurochko, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, George Dunlap, Wei Liu, Roger Pau Monné
Since the generic version of bug.h stuff was introduced use <xen/bug.h>
instead of unnecessary <asm/bug.h>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
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 | 19 +++----------------
xen/arch/arm/include/asm/div64.h | 2 +-
xen/arch/arm/vgic/vgic-v2.c | 2 +-
xen/arch/arm/vgic/vgic.c | 2 +-
xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +-
xen/arch/x86/include/asm/asm_defns.h | 2 +-
xen/arch/x86/include/asm/bug.h | 19 ++-----------------
xen/drivers/cpufreq/cpufreq.c | 2 +-
xen/include/xen/lib.h | 2 +-
9 files changed, 12 insertions(+), 40 deletions(-)
diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
index f4088d0913..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,9 +11,7 @@
# error "unknown ARM variant"
#endif
-#define BUG_DISP_WIDTH 24
-#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
-#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
+#define BUG_FRAME_STRUCT
struct bug_frame {
signed int loc_disp; /* Relative address to the bug address */
@@ -26,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
@@ -89,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..18ff2a443b 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -27,6 +27,7 @@
* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
*/
+#include <xen/bug.h>
#include <xen/types.h>
#include <xen/errno.h>
#include <xen/delay.h>
@@ -35,7 +36,6 @@
#include <xen/sched.h>
#include <xen/timer.h>
#include <xen/xmalloc.h>
-#include <asm/bug.h>
#include <asm/msr.h>
#include <asm/io.h>
#include <asm/processor.h>
diff --git a/xen/arch/x86/include/asm/asm_defns.h b/xen/arch/x86/include/asm/asm_defns.h
index d9431180cf..a8526cf36c 100644
--- a/xen/arch/x86/include/asm/asm_defns.h
+++ b/xen/arch/x86/include/asm/asm_defns.h
@@ -6,7 +6,7 @@
/* NB. Auto-generated from arch/.../asm-offsets.c */
#include <asm/asm-offsets.h>
#endif
-#include <asm/bug.h>
+#include <xen/bug.h>
#include <asm/x86-defns.h>
#include <xen/stringify.h>
#include <asm/cpufeature.h>
diff --git a/xen/arch/x86/include/asm/bug.h b/xen/arch/x86/include/asm/bug.h
index b7265bdfbe..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..354f78580b 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -26,6 +26,7 @@
* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
*/
+#include <xen/bug.h>
#include <xen/types.h>
#include <xen/errno.h>
#include <xen/delay.h>
@@ -39,7 +40,6 @@
#include <xen/guest_access.h>
#include <xen/domain.h>
#include <xen/cpu.h>
-#include <asm/bug.h>
#include <asm/io.h>
#include <asm/processor.h>
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 05ee1e18af..e914ccade0 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -24,12 +24,12 @@
#ifndef __ASSEMBLY__
+#include <xen/bug.h>
#include <xen/inttypes.h>
#include <xen/stdarg.h>
#include <xen/types.h>
#include <xen/xmalloc.h>
#include <xen/string.h>
-#include <asm/bug.h>
#define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
#define WARN_ON(p) ({ \
--
2.39.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h>
2023-02-24 11:31 ` [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
@ 2023-02-25 16:47 ` Julien Grall
2023-02-28 12:38 ` Oleksii
2023-02-28 13:07 ` Oleksii
2023-02-27 14:29 ` Jan Beulich
1 sibling, 2 replies; 39+ messages in thread
From: Julien Grall @ 2023-02-25 16:47 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 24/02/2023 11:31, Oleksii Kurochko wrote:
> Since the generic version of bug.h stuff was introduced use <xen/bug.h>
> instead of unnecessary <asm/bug.h>
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> 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 | 19 +++----------------
> xen/arch/arm/include/asm/div64.h | 2 +-
> xen/arch/arm/vgic/vgic-v2.c | 2 +-
> xen/arch/arm/vgic/vgic.c | 2 +-
> xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +-
> xen/arch/x86/include/asm/asm_defns.h | 2 +-
> xen/arch/x86/include/asm/bug.h | 19 ++-----------------
> xen/drivers/cpufreq/cpufreq.c | 2 +-
> xen/include/xen/lib.h | 2 +-
> 9 files changed, 12 insertions(+), 40 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
> index f4088d0913..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>
You are not adding new code in bug.h, so can you explain why this is now
necessary?
> +
> #if defined(CONFIG_ARM_32)
> # include <asm/arm32/bug.h>
> #elif defined(CONFIG_ARM_64)
> @@ -9,9 +11,7 @@
> # error "unknown ARM variant"
> #endif
>
> -#define BUG_DISP_WIDTH 24
> -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
Even if the values are the same as the one you defined in the common
bug.h, it doesn't feel right to remove them as long as...
> +#define BUG_FRAME_STRUCT
the arch is defining BUG_FRAME_STRUCT. So I would say the generic one
should be defined within BUG_FRAME_STRUCT.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h>
2023-02-25 16:47 ` Julien Grall
@ 2023-02-28 12:38 ` Oleksii
2023-02-28 14:04 ` Julien Grall
2023-02-28 13:07 ` Oleksii
1 sibling, 1 reply; 39+ messages in thread
From: Oleksii @ 2023-02-28 12:38 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, George Dunlap, Wei Liu,
Roger Pau Monné
Hi Julien,
On Sat, 2023-02-25 at 16:47 +0000, Julien Grall wrote:
> Hi Oleksii,
>
> On 24/02/2023 11:31, Oleksii Kurochko wrote:
> > Since the generic version of bug.h stuff was introduced use
> > <xen/bug.h>
> > instead of unnecessary <asm/bug.h>
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > 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 | 19 +++----------------
> > xen/arch/arm/include/asm/div64.h | 2 +-
> > xen/arch/arm/vgic/vgic-v2.c | 2 +-
> > xen/arch/arm/vgic/vgic.c | 2 +-
> > xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +-
> > xen/arch/x86/include/asm/asm_defns.h | 2 +-
> > xen/arch/x86/include/asm/bug.h | 19 ++-----------------
> > xen/drivers/cpufreq/cpufreq.c | 2 +-
> > xen/include/xen/lib.h | 2 +-
> > 9 files changed, 12 insertions(+), 40 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/bug.h
> > b/xen/arch/arm/include/asm/bug.h
> > index f4088d0913..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>
>
> You are not adding new code in bug.h, so can you explain why this is
> now
> necessary?
>
> > +
> > #if defined(CONFIG_ARM_32)
> > # include <asm/arm32/bug.h>
> > #elif defined(CONFIG_ARM_64)
> > @@ -9,9 +11,7 @@
> > # error "unknown ARM variant"
> > #endif
> >
> > -#define BUG_DISP_WIDTH 24
> > -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
>
> Even if the values are the same as the one you defined in the common
> bug.h, it doesn't feel right to remove them as long as...
>
> > +#define BUG_FRAME_STRUCT
>
> the arch is defining BUG_FRAME_STRUCT. So I would say the generic one
> should be defined within BUG_FRAME_STRUCT.
>
One of the reason BUG_DISP_WIDTH, BUG_LINE_* were removed is that they
don't use in ARM at all...
But generally I agree that it should be part of "#ifndef
BUG_FRAME_STRUCT" as it is 'struct bug_frame is dependent on it and
these defines look 'struct bug_frame' specific.
I'll update that in new version of the patch.
> Cheers,
>
~ Oleksii
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h>
2023-02-28 12:38 ` Oleksii
@ 2023-02-28 14:04 ` Julien Grall
0 siblings, 0 replies; 39+ messages in thread
From: Julien Grall @ 2023-02-28 14:04 UTC (permalink / raw)
To: Oleksii, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, George Dunlap, Wei Liu,
Roger Pau Monné
On 28/02/2023 12:38, Oleksii wrote:
> Hi Julien,
Hi,
> On Sat, 2023-02-25 at 16:47 +0000, Julien Grall wrote:
>> Hi Oleksii,
>>
>> On 24/02/2023 11:31, Oleksii Kurochko wrote:
>>> Since the generic version of bug.h stuff was introduced use
>>> <xen/bug.h>
>>> instead of unnecessary <asm/bug.h>
>>>
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> ---
>>> 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 | 19 +++----------------
>>> xen/arch/arm/include/asm/div64.h | 2 +-
>>> xen/arch/arm/vgic/vgic-v2.c | 2 +-
>>> xen/arch/arm/vgic/vgic.c | 2 +-
>>> xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +-
>>> xen/arch/x86/include/asm/asm_defns.h | 2 +-
>>> xen/arch/x86/include/asm/bug.h | 19 ++-----------------
>>> xen/drivers/cpufreq/cpufreq.c | 2 +-
>>> xen/include/xen/lib.h | 2 +-
>>> 9 files changed, 12 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/include/asm/bug.h
>>> b/xen/arch/arm/include/asm/bug.h
>>> index f4088d0913..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>
>>
>> You are not adding new code in bug.h, so can you explain why this is
>> now
>> necessary?
>>
>>> +
>>> #if defined(CONFIG_ARM_32)
>>> # include <asm/arm32/bug.h>
>>> #elif defined(CONFIG_ARM_64)
>>> @@ -9,9 +11,7 @@
>>> # error "unknown ARM variant"
>>> #endif
>>>
>>> -#define BUG_DISP_WIDTH 24
>>> -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
>>> -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
>>
>> Even if the values are the same as the one you defined in the common
>> bug.h, it doesn't feel right to remove them as long as...
>>
>>> +#define BUG_FRAME_STRUCT
>>
>> the arch is defining BUG_FRAME_STRUCT. So I would say the generic one
>> should be defined within BUG_FRAME_STRUCT.
>>
> One of the reason BUG_DISP_WIDTH, BUG_LINE_* were removed is that they
> don't use in ARM at all...
Hmmm ok. But this sort of things should have been documented in the
commit message even thought it doesn't feel this is related to what the
patch is doing.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h>
2023-02-25 16:47 ` Julien Grall
2023-02-28 12:38 ` Oleksii
@ 2023-02-28 13:07 ` Oleksii
2023-02-28 13:30 ` Jan Beulich
1 sibling, 1 reply; 39+ messages in thread
From: Oleksii @ 2023-02-28 13:07 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, George Dunlap, Wei Liu,
Roger Pau Monné
On Sat, 2023-02-25 at 16:47 +0000, Julien Grall wrote:
> Hi Oleksii,
>
> On 24/02/2023 11:31, Oleksii Kurochko wrote:
> > Since the generic version of bug.h stuff was introduced use
> > <xen/bug.h>
> > instead of unnecessary <asm/bug.h>
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > 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 | 19 +++----------------
> > xen/arch/arm/include/asm/div64.h | 2 +-
> > xen/arch/arm/vgic/vgic-v2.c | 2 +-
> > xen/arch/arm/vgic/vgic.c | 2 +-
> > xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +-
> > xen/arch/x86/include/asm/asm_defns.h | 2 +-
> > xen/arch/x86/include/asm/bug.h | 19 ++-----------------
> > xen/drivers/cpufreq/cpufreq.c | 2 +-
> > xen/include/xen/lib.h | 2 +-
> > 9 files changed, 12 insertions(+), 40 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/bug.h
> > b/xen/arch/arm/include/asm/bug.h
> > index f4088d0913..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>
>
> You are not adding new code in bug.h, so can you explain why this is
> now
> necessary?
I am not adding new code but inside 'struct bug_frame' there are
uint16_t and uint32_t which are defined in <xen/types.h>.
And after <asm/bug.h> was changed to <xen/bug.h> it started to complain
on these types.
>
> > +
> > #if defined(CONFIG_ARM_32)
> > # include <asm/arm32/bug.h>
> > #elif defined(CONFIG_ARM_64)
> > @@ -9,9 +11,7 @@
> > # error "unknown ARM variant"
> > #endif
> >
> > -#define BUG_DISP_WIDTH 24
> > -#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > -#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
>
> Even if the values are the same as the one you defined in the common
> bug.h, it doesn't feel right to remove them as long as...
>
> > +#define BUG_FRAME_STRUCT
>
> the arch is defining BUG_FRAME_STRUCT. So I would say the generic one
> should be defined within BUG_FRAME_STRUCT.
One of the reason BUG_DISP_WIDTH, BUG_LINE_* were removed is that they
don't use in ARM at all...
But generally I agree that it should be part of "#ifndef
BUG_FRAME_STRUCT" as it is 'struct bug_frame is dependent on it and
these defines look 'struct bug_frame' specific.
I'll update that in new version of the patch.
>
> Cheers,
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h>
2023-02-28 13:07 ` Oleksii
@ 2023-02-28 13:30 ` Jan Beulich
2023-02-28 16:00 ` Oleksii
0 siblings, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2023-02-28 13:30 UTC (permalink / raw)
To: Oleksii
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, George Dunlap, Wei Liu,
Roger Pau Monné,
Julien Grall, xen-devel
On 28.02.2023 14:07, Oleksii wrote:
> On Sat, 2023-02-25 at 16:47 +0000, Julien Grall wrote:
>> On 24/02/2023 11:31, Oleksii Kurochko wrote:
>>> --- 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>
>>
>> You are not adding new code in bug.h, so can you explain why this is
>> now
>> necessary?
> I am not adding new code but inside 'struct bug_frame' there are
> uint16_t and uint32_t which are defined in <xen/types.h>.
>
> And after <asm/bug.h> was changed to <xen/bug.h> it started to complain
> on these types.
Wouldn't xen/bug.h want to include xen/types.h anyway, and then clearly
before including asm/bug.h?
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h>
2023-02-28 13:30 ` Jan Beulich
@ 2023-02-28 16:00 ` Oleksii
0 siblings, 0 replies; 39+ messages in thread
From: Oleksii @ 2023-02-28 16:00 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk, George Dunlap, Wei Liu,
Roger Pau Monné,
Julien Grall, xen-devel
On Tue, 2023-02-28 at 14:30 +0100, Jan Beulich wrote:
> On 28.02.2023 14:07, Oleksii wrote:
> > On Sat, 2023-02-25 at 16:47 +0000, Julien Grall wrote:
> > > On 24/02/2023 11:31, Oleksii Kurochko wrote:
> > > > --- 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>
> > >
> > > You are not adding new code in bug.h, so can you explain why this
> > > is
> > > now
> > > necessary?
> > I am not adding new code but inside 'struct bug_frame' there are
> > uint16_t and uint32_t which are defined in <xen/types.h>.
> >
> > And after <asm/bug.h> was changed to <xen/bug.h> it started to
> > complain
> > on these types.
>
> Wouldn't xen/bug.h want to include xen/types.h anyway, and then
> clearly
> before including asm/bug.h?
<xen/types.h> can be moved to <xen/bug.h> but generic implementation
doesn't use any specific from <xen/types.h> types.
But probably you are right that it would be better move to
<xen/bug.h>...
>
> Jan
~ Oleksii
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h>
2023-02-24 11:31 ` [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
2023-02-25 16:47 ` Julien Grall
@ 2023-02-27 14:29 ` Jan Beulich
2023-02-28 13:14 ` Oleksii
1 sibling, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2023-02-27 14:29 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, George Dunlap, Wei Liu,
Roger Pau Monné,
xen-devel
On 24.02.2023 12:31, Oleksii Kurochko wrote:
> Since the generic version of bug.h stuff was introduced use <xen/bug.h>
> instead of unnecessary <asm/bug.h>
You keep saying "unnecessary" here, but that's not really correct.
Including asm/bug.h alone simply becomes meaningless. So how about
"... instead of now useless (in isolation) <asm/bug.h>"?
> --- 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;
Why would x86 continue to define its own bug_frame (and other items)?
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h>
2023-02-27 14:29 ` Jan Beulich
@ 2023-02-28 13:14 ` Oleksii
0 siblings, 0 replies; 39+ messages in thread
From: Oleksii @ 2023-02-28 13:14 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, George Dunlap, Wei Liu,
Roger Pau Monné,
xen-devel
On Mon, 2023-02-27 at 15:29 +0100, Jan Beulich wrote:
> On 24.02.2023 12:31, Oleksii Kurochko wrote:
> > Since the generic version of bug.h stuff was introduced use
> > <xen/bug.h>
> > instead of unnecessary <asm/bug.h>
>
> You keep saying "unnecessary" here, but that's not really correct.
> Including asm/bug.h alone simply becomes meaningless. So how about
> "... instead of now useless (in isolation) <asm/bug.h>"?
>
> > --- 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;
>
> Why would x86 continue to define its own bug_frame (and other items)?
>
Because x86 will be switched to generic one in the following patches of
the patch series and right now it defines only BUG_FRAME_STRUCT which
means that it will not use generic one implemetation now.
The idea of the patch was to rename <asm/bug.h> to <xen/bug.h> with
minimal required changes to keep Xen compilable.
And I am going to back:
-#define BUG_DISP_WIDTH 24
-#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
-#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
for the same reason as in ARM. these defines are related to 'stuct
bug_frame' so should go with it.
These defines will be removed when an architecture will be switched to
generic implementation.
> Jan
~ Oleksii
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-02-24 11:31 [PATCH v3 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-02-24 11:31 ` [PATCH v3 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-02-24 11:31 ` [PATCH v3 2/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
@ 2023-02-24 11:31 ` Oleksii Kurochko
2023-02-25 16:49 ` Julien Grall
2023-02-24 11:31 ` [PATCH v3 4/4] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
3 siblings, 1 reply; 39+ messages in thread
From: Oleksii Kurochko @ 2023-02-24 11:31 UTC (permalink / raw)
To: xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Oleksii Kurochko, Julien Grall, 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
* Change macros bug_file() to bug_ptr() to align it with generic
implementation.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
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/bug.h | 15 +-----
xen/arch/arm/include/asm/traps.h | 2 -
xen/arch/arm/traps.c | 81 +-------------------------------
5 files changed, 4 insertions(+), 97 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/bug.h b/xen/arch/arm/include/asm/bug.h
index cacaf014ab..e6cc37e1d6 100644
--- a/xen/arch/arm/include/asm/bug.h
+++ b/xen/arch/arm/include/asm/bug.h
@@ -21,8 +21,7 @@ struct bug_frame {
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_ptr(b) ((const void *)(b) + (b)->file_disp)
#define bug_line(b) ((b)->line)
#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
@@ -70,18 +69,6 @@ struct bug_frame {
".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)
-
#endif /* __ARM_BUG_H__ */
/*
* Local variables:
diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
index 883dae368e..c6518008ec 100644
--- a/xen/arch/arm/include/asm/traps.h
+++ b/xen/arch/arm/include/asm/traps.h
@@ -69,8 +69,6 @@ void do_cp(struct cpu_user_regs *regs, const union hsr hsr);
void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
void do_trap_hvc_smccc(struct cpu_user_regs *regs);
-int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc);
-
void noreturn do_unexpected_trap(const char *msg,
const struct cpu_user_regs *regs);
void do_trap_hyp_sync(struct cpu_user_regs *regs);
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 061c92acbd..9c6eb66422 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1197,85 +1197,6 @@ void do_unexpected_trap(const char *msg, const struct cpu_user_regs *regs)
panic("CPU%d: Unexpected Trap: %s\n", smp_processor_id(), msg);
}
-int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
-{
- const struct bug_frame *bug = NULL;
- const char *prefix = "", *filename, *predicate;
- unsigned long fixup;
- int id = -1, lineno;
- const struct virtual_region *region;
-
- region = find_text_region(pc);
- if ( region )
- {
- for ( id = 0; id < BUGFRAME_NR; id++ )
- {
- const struct bug_frame *b;
- unsigned int i;
-
- for ( i = 0, b = region->frame[id].bugs;
- i < region->frame[id].n_bugs; b++, i++ )
- {
- if ( ((vaddr_t)bug_loc(b)) == pc )
- {
- bug = b;
- goto found;
- }
- }
- }
- }
- found:
- if ( !bug )
- return -ENOENT;
-
- if ( id == BUGFRAME_run_fn )
- {
- void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;
-
- fn(regs);
- return 0;
- }
-
- /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
- filename = bug_file(bug);
- if ( !is_kernel(filename) )
- return -EINVAL;
- fixup = strlen(filename);
- if ( fixup > 50 )
- {
- filename += fixup - 47;
- prefix = "...";
- }
- lineno = bug_line(bug);
-
- switch ( id )
- {
- case BUGFRAME_warn:
- printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
- show_execution_state(regs);
- return 0;
-
- case BUGFRAME_bug:
- printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
- show_execution_state(regs);
- panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
-
- case BUGFRAME_assert:
- /* ASSERT: decode the predicate string pointer. */
- predicate = bug_msg(bug);
- if ( !is_kernel(predicate) )
- predicate = "<unknown>";
-
- printk("Assertion '%s' failed at %s%s:%d\n",
- predicate, prefix, filename, lineno);
- show_execution_state(regs);
- panic("Assertion '%s' failed at %s%s:%d\n",
- predicate, prefix, filename, lineno);
- }
-
- return -EINVAL;
-}
-
#ifdef CONFIG_ARM_64
static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr)
{
@@ -1292,7 +1213,7 @@ static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr)
switch ( hsr.brk.comment )
{
case BRK_BUG_FRAME_IMM:
- if ( do_bug_frame(regs, regs->pc) )
+ if ( do_bug_frame(regs, regs->pc) < 0 )
goto die;
regs->pc += 4;
--
2.39.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-02-24 11:31 ` [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
@ 2023-02-25 16:49 ` Julien Grall
2023-02-25 17:05 ` Julien Grall
2023-02-28 15:09 ` Oleksii
0 siblings, 2 replies; 39+ messages in thread
From: Julien Grall @ 2023-02-25 16:49 UTC (permalink / raw)
To: Oleksii Kurochko, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
Hi Oleksii,
On 24/02/2023 11:31, Oleksii Kurochko wrote:
> The following changes were made:
> * make GENERIC_BUG_FRAME mandatory for ARM
I have asked in patch #1 but will ask it again because I think this
should be recorded in the commit message. Can you outline why it is not
possible to completely switch to the generic version?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-02-25 16:49 ` Julien Grall
@ 2023-02-25 17:05 ` Julien Grall
2023-02-28 17:21 ` Oleksii
2023-02-28 15:09 ` Oleksii
1 sibling, 1 reply; 39+ messages in thread
From: Julien Grall @ 2023-02-25 17:05 UTC (permalink / raw)
To: Oleksii Kurochko, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
On 25/02/2023 16:49, Julien Grall wrote:
> Hi Oleksii,
>
> On 24/02/2023 11:31, Oleksii Kurochko wrote:
>> The following changes were made:
>> * make GENERIC_BUG_FRAME mandatory for ARM
>
> I have asked in patch #1 but will ask it again because I think this
> should be recorded in the commit message. Can you outline why it is not
> possible to completely switch to the generic version?
I have just tried to remove it on arm64 and it seems to work. This was
only tested with GCC 10 though. But given the generic version is not not
using the %c modifier, then I wouldn't expect any issue.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-02-25 17:05 ` Julien Grall
@ 2023-02-28 17:21 ` Oleksii
2023-02-28 17:57 ` Julien Grall
0 siblings, 1 reply; 39+ messages in thread
From: Oleksii @ 2023-02-28 17:21 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
On Sat, 2023-02-25 at 17:05 +0000, Julien Grall wrote:
>
>
> On 25/02/2023 16:49, Julien Grall wrote:
> > Hi Oleksii,
> >
> > On 24/02/2023 11:31, Oleksii Kurochko wrote:
> > > The following changes were made:
> > > * make GENERIC_BUG_FRAME mandatory for ARM
> >
> > I have asked in patch #1 but will ask it again because I think this
> > should be recorded in the commit message. Can you outline why it is
> > not
> > possible to completely switch to the generic version?
>
> I have just tried to remove it on arm64 and it seems to work. This
> was
> only tested with GCC 10 though. But given the generic version is not
> not
> using the %c modifier, then I wouldn't expect any issue.
>
> Cheers,
>
I tried to switch ARM to generic implementation.
Here is the patch: [1]
diff --git a/xen/arch/arm/include/asm/bug.h
b/xen/arch/arm/include/asm/bug.h
index e6cc37e1d6..ffb0f569fc 100644
--- a/xen/arch/arm/include/asm/bug.h
+++ b/xen/arch/arm/include/asm/bug.h
@@ -1,8 +1,6 @@
#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)
@@ -11,63 +9,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_ptr(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 BUG_ASM_CONST "c"
#endif /* __ARM_BUG_H__ */
...
(it will be merged with patch 3 if it is OK )
And looks like we can switch ARM to generic implementation as all tests
passed:
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396
The only issue is with yocto-arm:
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396/failures
But I am not sure that it is because of my patch
Is this enough from a verification point of view?
[1]
https://gitlab.com/xen-project/people/olkur/xen/-/commit/5ff7a06e1d354e1e42bde1c203f3cf05a3653ad6
https://gitlab.com/xen-project/people/olkur/xen/-/commit/5ff7a06e1d354e1e42bde1c203f3cf05a3653ad6
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-02-28 17:21 ` Oleksii
@ 2023-02-28 17:57 ` Julien Grall
2023-03-01 12:31 ` Oleksii
0 siblings, 1 reply; 39+ messages in thread
From: Julien Grall @ 2023-02-28 17:57 UTC (permalink / raw)
To: Oleksii, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
On 28/02/2023 17:21, Oleksii wrote:
> Hi Julien,
Hi Oleksii,
> On Sat, 2023-02-25 at 17:05 +0000, Julien Grall wrote:
>>
>>
>> On 25/02/2023 16:49, Julien Grall wrote:
>>> Hi Oleksii,
>>>
>>> On 24/02/2023 11:31, Oleksii Kurochko wrote:
>>>> The following changes were made:
>>>> * make GENERIC_BUG_FRAME mandatory for ARM
>>>
>>> I have asked in patch #1 but will ask it again because I think this
>>> should be recorded in the commit message. Can you outline why it is
>>> not
>>> possible to completely switch to the generic version?
>>
>> I have just tried to remove it on arm64 and it seems to work. This
>> was
>> only tested with GCC 10 though. But given the generic version is not
>> not
>> using the %c modifier, then I wouldn't expect any issue.
>>
>> Cheers,
>>
>
> I tried to switch ARM to generic implementation.
>
> Here is the patch: [1]
This is similar to the patch I wrote to test with generic implementation
on Arm (see my other reply).
[...]
> (it will be merged with patch 3 if it is OK )
>
> And looks like we can switch ARM to generic implementation as all tests
> passed:
> https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396
Thanks for checking with the gitlab CI!
>
> The only issue is with yocto-arm:
> https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396/failures
> But I am not sure that it is because of my patch
This looks unrelated to me. This is happening because there is a data
abort before PSCI (used to reboot the platform) is properly setup. I
think we should consider to only print once the error rather than every
few iterations (not a request for you).
That said, I am a bit puzzled why this issue is only happening in the
Yocto test (the Debian one pass). Do you know if the test is passing in
the normal CI?
If yes, what other modifications did you do?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-02-28 17:57 ` Julien Grall
@ 2023-03-01 12:31 ` Oleksii
2023-03-01 13:58 ` Julien Grall
0 siblings, 1 reply; 39+ messages in thread
From: Oleksii @ 2023-03-01 12:31 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
> > >
> > > > On 24/02/2023 11:31, Oleksii Kurochko wrote:
> > > > > The following changes were made:
> > > > > * make GENERIC_BUG_FRAME mandatory for ARM
> > > >
> > > > I have asked in patch #1 but will ask it again because I think
> > > > this
> > > > should be recorded in the commit message. Can you outline why
> > > > it is
> > > > not
> > > > possible to completely switch to the generic version?
> > >
> > > I have just tried to remove it on arm64 and it seems to work.
> > > This
> > > was
> > > only tested with GCC 10 though. But given the generic version is
> > > not
> > > not
> > > using the %c modifier, then I wouldn't expect any issue.
> > >
> > > Cheers,
> > >
> >
> > I tried to switch ARM to generic implementation.
> >
> > Here is the patch: [1]
>
> This is similar to the patch I wrote to test with generic
> implementation
> on Arm (see my other reply).
>
> [...]
>
> > (it will be merged with patch 3 if it is OK )
> >
> > And looks like we can switch ARM to generic implementation as all
> > tests
> > passed:
> > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396
>
> Thanks for checking with the gitlab CI!
>
> >
> > The only issue is with yocto-arm:
> > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396/failures
> > But I am not sure that it is because of my patch
>
> This looks unrelated to me. This is happening because there is a data
> abort before PSCI (used to reboot the platform) is properly setup. I
> think we should consider to only print once the error rather than
> every
> few iterations (not a request for you).
>
> That said, I am a bit puzzled why this issue is only happening in the
> Yocto test (the Debian one pass). Do you know if the test is passing
> in
> the normal CI?
I checked several pipelines on the normal CI and it is fine.
>
> If yes, what other modifications did you do?
It looks like the issue happens after switch ARM to generic
implementation of bug.h:
-
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792379063/failures
[ failure ]
-
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792381665/failures
[ passed ]
The difference between builds is only in the commit ' check if ARM can
be fully switched to generic implementation '.
For second one it is reverted so it looks like we can't switch ARM to
generic implementation now as it is required addiotional investigation.
There is no other significant changes ( only the changes mentioned in
the current mail thread ).
Could we go ahead without switching ARM to generic implementation to
not block other RISC-V patch series?
~ Oleksii
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-01 12:31 ` Oleksii
@ 2023-03-01 13:58 ` Julien Grall
2023-03-01 15:16 ` Oleksii
0 siblings, 1 reply; 39+ messages in thread
From: Julien Grall @ 2023-03-01 13:58 UTC (permalink / raw)
To: Oleksii, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
On 01/03/2023 12:31, Oleksii wrote:
> Hi Julien,
Hi,
>>>>
>>>>> On 24/02/2023 11:31, Oleksii Kurochko wrote:
>>>>>> The following changes were made:
>>>>>> * make GENERIC_BUG_FRAME mandatory for ARM
>>>>>
>>>>> I have asked in patch #1 but will ask it again because I think
>>>>> this
>>>>> should be recorded in the commit message. Can you outline why
>>>>> it is
>>>>> not
>>>>> possible to completely switch to the generic version?
>>>>
>>>> I have just tried to remove it on arm64 and it seems to work.
>>>> This
>>>> was
>>>> only tested with GCC 10 though. But given the generic version is
>>>> not
>>>> not
>>>> using the %c modifier, then I wouldn't expect any issue.
>>>>
>>>> Cheers,
>>>>
>>>
>>> I tried to switch ARM to generic implementation.
>>>
>>> Here is the patch: [1]
>>
>> This is similar to the patch I wrote to test with generic
>> implementation
>> on Arm (see my other reply).
>>
>> [...]
>>
>>> (it will be merged with patch 3 if it is OK )
>>>
>>> And looks like we can switch ARM to generic implementation as all
>>> tests
>>> passed:
>>> https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396
>>
>> Thanks for checking with the gitlab CI!
>>
>>>
>>> The only issue is with yocto-arm:
>>> https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396/failures
>>> But I am not sure that it is because of my patch
>>
>> This looks unrelated to me. This is happening because there is a data
>> abort before PSCI (used to reboot the platform) is properly setup. I
>> think we should consider to only print once the error rather than
>> every
>> few iterations (not a request for you).
>>
>> That said, I am a bit puzzled why this issue is only happening in the
>> Yocto test (the Debian one pass). Do you know if the test is passing
>> in
>> the normal CI?
> I checked several pipelines on the normal CI and it is fine.
>>
>> If yes, what other modifications did you do?
> It looks like the issue happens after switch ARM to generic
> implementation of bug.h:
> -
> https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792379063/failures
> [ failure ]
> -
> https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792381665/failures
> [ passed ]
>
> The difference between builds is only in the commit ' check if ARM can
> be fully switched to generic implementation '.
> For second one it is reverted so it looks like we can't switch ARM to
> generic implementation now as it is required addiotional investigation.
Thanks. Looking at the error again, it looks like the data abort is
because we are accessing an unaligned address.
From a brief look at arch/arm/xen.lds.S, I can at least see one case of
misalignment (not clear why it would only happen now though). Can you try:
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 3f7ebd19f3ed..fb8155bd729f 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -67,6 +67,7 @@ SECTIONS
*(.data.rel.ro)
*(.data.rel.ro.*)
+ . = ALIGN(4);
__proc_info_start = .;
*(.proc.info)
__proc_info_end = .;
>
> There is no other significant changes ( only the changes mentioned in
> the current mail thread ).
>
> Could we go ahead without switching ARM to generic implementation to
> not block other RISC-V patch series?
Given this is an alignment issue (Arm32 is more sensible to this than
the other architecture, but this is still a potential problem for the
other archs), I would really like to understand whether this is an issue
in the common code or in the Arm linker script.
Cheers,
--
Julien Grall
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-01 13:58 ` Julien Grall
@ 2023-03-01 15:16 ` Oleksii
2023-03-01 15:21 ` Julien Grall
0 siblings, 1 reply; 39+ messages in thread
From: Oleksii @ 2023-03-01 15:16 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
On Wed, 2023-03-01 at 13:58 +0000, Julien Grall wrote:
> On 01/03/2023 12:31, Oleksii wrote:
> > Hi Julien,
>
> Hi,
>
> > > > >
> > > > > > On 24/02/2023 11:31, Oleksii Kurochko wrote:
> > > > > > > The following changes were made:
> > > > > > > * make GENERIC_BUG_FRAME mandatory for ARM
> > > > > >
> > > > > > I have asked in patch #1 but will ask it again because I
> > > > > > think
> > > > > > this
> > > > > > should be recorded in the commit message. Can you outline
> > > > > > why
> > > > > > it is
> > > > > > not
> > > > > > possible to completely switch to the generic version?
> > > > >
> > > > > I have just tried to remove it on arm64 and it seems to work.
> > > > > This
> > > > > was
> > > > > only tested with GCC 10 though. But given the generic version
> > > > > is
> > > > > not
> > > > > not
> > > > > using the %c modifier, then I wouldn't expect any issue.
> > > > >
> > > > > Cheers,
> > > > >
> > > >
> > > > I tried to switch ARM to generic implementation.
> > > >
> > > > Here is the patch: [1]
> > >
> > > This is similar to the patch I wrote to test with generic
> > > implementation
> > > on Arm (see my other reply).
> > >
> > > [...]
> > >
> > > > (it will be merged with patch 3 if it is OK )
> > > >
> > > > And looks like we can switch ARM to generic implementation as
> > > > all
> > > > tests
> > > > passed:
> > > > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396
> > >
> > > Thanks for checking with the gitlab CI!
> > >
> > > >
> > > > The only issue is with yocto-arm:
> > > > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396/failures
> > > > But I am not sure that it is because of my patch
> > >
> > > This looks unrelated to me. This is happening because there is a
> > > data
> > > abort before PSCI (used to reboot the platform) is properly
> > > setup. I
> > > think we should consider to only print once the error rather than
> > > every
> > > few iterations (not a request for you).
> > >
> > > That said, I am a bit puzzled why this issue is only happening in
> > > the
> > > Yocto test (the Debian one pass). Do you know if the test is
> > > passing
> > > in
> > > the normal CI?
> > I checked several pipelines on the normal CI and it is fine.
> > >
> > > If yes, what other modifications did you do?
> > It looks like the issue happens after switch ARM to generic
> > implementation of bug.h:
> > -
> > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792379063/failures
> > [ failure ]
> > -
> > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792381665/failures
> > [ passed ]
> >
> > The difference between builds is only in the commit ' check if ARM
> > can
> > be fully switched to generic implementation '.
> > For second one it is reverted so it looks like we can't switch ARM
> > to
> > generic implementation now as it is required addiotional
> > investigation.
>
> Thanks. Looking at the error again, it looks like the data abort is
> because we are accessing an unaligned address.
>
> From a brief look at arch/arm/xen.lds.S, I can at least see one case
> of
> misalignment (not clear why it would only happen now though). Can you
> try:
>
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 3f7ebd19f3ed..fb8155bd729f 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -67,6 +67,7 @@ SECTIONS
> *(.data.rel.ro)
> *(.data.rel.ro.*)
>
> + . = ALIGN(4);
> __proc_info_start = .;
> *(.proc.info)
> __proc_info_end = .;
>
> >
> > There is no other significant changes ( only the changes mentioned
> > in
> > the current mail thread ).
> >
> > Could we go ahead without switching ARM to generic implementation
> > to
> > not block other RISC-V patch series?
>
> Given this is an alignment issue (Arm32 is more sensible to this than
> the other architecture, but this is still a potential problem for the
> other archs), I would really like to understand whether this is an
> issue
> in the common code or in the Arm linker script.
I have a good news.
Alignment of "*(.proc.info)" helps but I checked only yocto-qemuarm:
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792923264
I ran all tests here:
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792953524
Should I create a separate patch with ALIGN if the tests are passed?
~ Oleksii
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-01 15:16 ` Oleksii
@ 2023-03-01 15:21 ` Julien Grall
2023-03-01 15:28 ` Oleksii
2023-03-01 15:58 ` Oleksii
0 siblings, 2 replies; 39+ messages in thread
From: Julien Grall @ 2023-03-01 15:21 UTC (permalink / raw)
To: Oleksii, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
Hi Oleksii,
On 01/03/2023 15:16, Oleksii wrote:
> On Wed, 2023-03-01 at 13:58 +0000, Julien Grall wrote:
>> On 01/03/2023 12:31, Oleksii wrote:
>> Given this is an alignment issue (Arm32 is more sensible to this than
>> the other architecture, but this is still a potential problem for the
>> other archs), I would really like to understand whether this is an
>> issue
>> in the common code or in the Arm linker script.
> I have a good news.
>
> Alignment of "*(.proc.info)" helps but I checked only yocto-qemuarm:
> https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792923264
>
> I ran all tests here:
> https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792953524
>
> Should I create a separate patch with ALIGN if the tests are passed?
Yes please. But, to be honest, I am not entirely sure what is not
aligned before hand. Do you know if it is possible to download the Xen
binary from gitlab?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-01 15:21 ` Julien Grall
@ 2023-03-01 15:28 ` Oleksii
2023-03-01 15:58 ` Oleksii
1 sibling, 0 replies; 39+ messages in thread
From: Oleksii @ 2023-03-01 15:28 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
> On 01/03/2023 15:16, Oleksii wrote:
> > On Wed, 2023-03-01 at 13:58 +0000, Julien Grall wrote:
> > > On 01/03/2023 12:31, Oleksii wrote:
> > > Given this is an alignment issue (Arm32 is more sensible to this
> > > than
> > > the other architecture, but this is still a potential problem for
> > > the
> > > other archs), I would really like to understand whether this is
> > > an
> > > issue
> > > in the common code or in the Arm linker script.
> > I have a good news.
> >
> > Alignment of "*(.proc.info)" helps but I checked only yocto-
> > qemuarm:
> > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792923264
> >
> > I ran all tests here:
> > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792953524
> >
> > Should I create a separate patch with ALIGN if the tests are
> > passed?
>
> Yes please. But, to be honest, I am not entirely sure what is not
> aligned before hand. Do you know if it is possible to download the
> Xen
> binary from gitlab?
It is possible.
Please go to the link of the job:
https://gitlab.com/xen-project/people/olkur/xen/-/jobs/3856617252
And on the right you will find 'Job artificats' where you can click
'Download'.
Or in case if you need a particular binary can click 'Browse' and go
to Artifcats/Binaries/:
https://gitlab.com/xen-project/people/olkur/xen/-/jobs/3856617252/artifacts/browse/binaries/
~ Oleksii
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-01 15:21 ` Julien Grall
2023-03-01 15:28 ` Oleksii
@ 2023-03-01 15:58 ` Oleksii
1 sibling, 0 replies; 39+ messages in thread
From: Oleksii @ 2023-03-01 15:58 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
On Wed, 2023-03-01 at 15:21 +0000, Julien Grall wrote:
> Hi Oleksii,
>
> On 01/03/2023 15:16, Oleksii wrote:
> > On Wed, 2023-03-01 at 13:58 +0000, Julien Grall wrote:
> > > On 01/03/2023 12:31, Oleksii wrote:
> > > Given this is an alignment issue (Arm32 is more sensible to this
> > > than
> > > the other architecture, but this is still a potential problem for
> > > the
> > > other archs), I would really like to understand whether this is
> > > an
> > > issue
> > > in the common code or in the Arm linker script.
> > I have a good news.
> >
> > Alignment of "*(.proc.info)" helps but I checked only yocto-
> > qemuarm:
> > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792923264
> >
> > I ran all tests here:
> > https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/792953524
> >
> > Should I create a separate patch with ALIGN if the tests are
> > passed?
>
> Yes please. But, to be honest, I am not entirely sure what is not
> aligned before hand. Do you know if it is possible to download the
> Xen
> binary from gitlab?
It is possible.
Please go to the link of the job:
https://gitlab.com/xen-project/people/olkur/xen/-/jobs/3856617252
And on the right you will find 'Job artificats' where you can click
'Download'.
Or in case if you need a particular binary can click 'Browse' and go
to Artifcats/Binaries/:
https://gitlab.com/xen-project/people/olkur/xen/-/jobs/3856617252/artifacts/browse/binaries/
~ Oleksii
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-02-25 16:49 ` Julien Grall
2023-02-25 17:05 ` Julien Grall
@ 2023-02-28 15:09 ` Oleksii
2023-02-28 17:48 ` Julien Grall
1 sibling, 1 reply; 39+ messages in thread
From: Oleksii @ 2023-02-28 15:09 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
On Sat, 2023-02-25 at 16:49 +0000, Julien Grall wrote:
> Hi Oleksii,
>
> On 24/02/2023 11:31, Oleksii Kurochko wrote:
> > The following changes were made:
> > * make GENERIC_BUG_FRAME mandatory for ARM
>
> I have asked in patch #1 but will ask it again because I think this
> should be recorded in the commit message. Can you outline why it is
> not
> possible to completely switch to the generic version?
I haven't tried to switch ARM too because of comment regarding 'i' in
<asm/bug.h>:
/*
* 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 {
\
So I decided to leave ARM implementation as is.
But I'll try to switch it for the test and look at tests. If tests
won't fail that I'll switch ARM to generic implementation too.
>
> Cheers,
>
~ Oleksii
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-02-28 15:09 ` Oleksii
@ 2023-02-28 17:48 ` Julien Grall
2023-03-01 8:58 ` Oleksii
0 siblings, 1 reply; 39+ messages in thread
From: Julien Grall @ 2023-02-28 17:48 UTC (permalink / raw)
To: Oleksii, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
Hi Oleksii,
On 28/02/2023 15:09, Oleksii wrote:
> On Sat, 2023-02-25 at 16:49 +0000, Julien Grall wrote:
>> Hi Oleksii,
>>
>> On 24/02/2023 11:31, Oleksii Kurochko wrote:
>>> The following changes were made:
>>> * make GENERIC_BUG_FRAME mandatory for ARM
>>
>> I have asked in patch #1 but will ask it again because I think this
>> should be recorded in the commit message. Can you outline why it is
>> not
>> possible to completely switch to the generic version?
> I haven't tried to switch ARM too because of comment regarding 'i' in
> <asm/bug.h>:
> /*
> * 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.
> */
I would expect this comment to be valid for any arch. So if there is a
need to deal with PIE, then we would not be able to use "i" in the BUG
frame.
Note that we are now explicitly compiling Xen without PIE (see Config.mk).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-02-28 17:48 ` Julien Grall
@ 2023-03-01 8:58 ` Oleksii
2023-03-01 9:31 ` Julien Grall
0 siblings, 1 reply; 39+ messages in thread
From: Oleksii @ 2023-03-01 8:58 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
On Tue, 2023-02-28 at 17:48 +0000, Julien Grall wrote:
> Hi Oleksii,
>
> On 28/02/2023 15:09, Oleksii wrote:
> > On Sat, 2023-02-25 at 16:49 +0000, Julien Grall wrote:
> > > Hi Oleksii,
> > >
> > > On 24/02/2023 11:31, Oleksii Kurochko wrote:
> > > > The following changes were made:
> > > > * make GENERIC_BUG_FRAME mandatory for ARM
> > >
> > > I have asked in patch #1 but will ask it again because I think
> > > this
> > > should be recorded in the commit message. Can you outline why it
> > > is
> > > not
> > > possible to completely switch to the generic version?
> > I haven't tried to switch ARM too because of comment regarding 'i'
> > in
> > <asm/bug.h>:
> > /*
> > * 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.
> > */
>
> I would expect this comment to be valid for any arch. So if there is
> a
> need to deal with PIE, then we would not be able to use "i" in the
> BUG
> frame.
>
> Note that we are now explicitly compiling Xen without PIE (see
> Config.mk).
Then it looks like some architectures isn't expected to be compiled
with PIE. I mean that x86's bug.h is used 'i' and there is no any
alternative version in case of PIE.
If Xen should be compilable with PIE then we have to use ARM
implementation of bug.h everywhere. ( based on comment about 'i' with
PIE ).
Now I am totally confused...
~ Oleksii
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-01 8:58 ` Oleksii
@ 2023-03-01 9:31 ` Julien Grall
2023-03-01 12:33 ` Oleksii
0 siblings, 1 reply; 39+ messages in thread
From: Julien Grall @ 2023-03-01 9:31 UTC (permalink / raw)
To: Oleksii, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
Hi,
On 01/03/2023 08:58, Oleksii wrote:
> On Tue, 2023-02-28 at 17:48 +0000, Julien Grall wrote:
>> Hi Oleksii,
>>
>> On 28/02/2023 15:09, Oleksii wrote:
>>> On Sat, 2023-02-25 at 16:49 +0000, Julien Grall wrote:
>>>> Hi Oleksii,
>>>>
>>>> On 24/02/2023 11:31, Oleksii Kurochko wrote:
>>>>> The following changes were made:
>>>>> * make GENERIC_BUG_FRAME mandatory for ARM
>>>>
>>>> I have asked in patch #1 but will ask it again because I think
>>>> this
>>>> should be recorded in the commit message. Can you outline why it
>>>> is
>>>> not
>>>> possible to completely switch to the generic version?
>>> I haven't tried to switch ARM too because of comment regarding 'i'
>>> in
>>> <asm/bug.h>:
>>> /*
>>> * 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.
>>> */
>>
>> I would expect this comment to be valid for any arch. So if there is
>> a
>> need to deal with PIE, then we would not be able to use "i" in the
>> BUG
>> frame.
>>
>> Note that we are now explicitly compiling Xen without PIE (see
>> Config.mk).
> Then it looks like some architectures isn't expected to be compiled
> with PIE. I mean that x86's bug.h is used 'i' and there is no any
> alternative version in case of PIE.
>
> If Xen should be compilable with PIE then we have to use ARM
> implementation of bug.h everywhere. ( based on comment about 'i' with
> PIE ).
The comment was added because until commit ecd6b9759919 ("Config.mk:
correct PIE-related option(s) in EMBEDDED_EXTRA_CFLAGS") we would let
the compiler to decide whether PIE should be enabled.
Now we are forcing -fno-pie for Xen on any architecture (the flag is
added at the top-level in Config.mk).
>
> Now I am totally confused...
My point was not about using the Arm implementation everywhere. My point
was that we disable even for Arm and therefore it is fine to use the
common version.
If in the future we need to support PIE in Xen (I am not aware of any
effort yet), then we could decide to update the common BUG framework.
But for now, I don't think this is something you need to care in your
series.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h
2023-03-01 9:31 ` Julien Grall
@ 2023-03-01 12:33 ` Oleksii
0 siblings, 0 replies; 39+ messages in thread
From: Oleksii @ 2023-03-01 12:33 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Bertrand Marquis, Volodymyr Babchuk
On Wed, 2023-03-01 at 09:31 +0000, Julien Grall wrote:
> Hi,
>
> On 01/03/2023 08:58, Oleksii wrote:
> > On Tue, 2023-02-28 at 17:48 +0000, Julien Grall wrote:
> > > Hi Oleksii,
> > >
> > > On 28/02/2023 15:09, Oleksii wrote:
> > > > On Sat, 2023-02-25 at 16:49 +0000, Julien Grall wrote:
> > > > > Hi Oleksii,
> > > > >
> > > > > On 24/02/2023 11:31, Oleksii Kurochko wrote:
> > > > > > The following changes were made:
> > > > > > * make GENERIC_BUG_FRAME mandatory for ARM
> > > > >
> > > > > I have asked in patch #1 but will ask it again because I
> > > > > think
> > > > > this
> > > > > should be recorded in the commit message. Can you outline why
> > > > > it
> > > > > is
> > > > > not
> > > > > possible to completely switch to the generic version?
> > > > I haven't tried to switch ARM too because of comment regarding
> > > > 'i'
> > > > in
> > > > <asm/bug.h>:
> > > > /*
> > > > * 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.
> > > > */
> > >
> > > I would expect this comment to be valid for any arch. So if there
> > > is
> > > a
> > > need to deal with PIE, then we would not be able to use "i" in
> > > the
> > > BUG
> > > frame.
> > >
> > > Note that we are now explicitly compiling Xen without PIE (see
> > > Config.mk).
> > Then it looks like some architectures isn't expected to be compiled
> > with PIE. I mean that x86's bug.h is used 'i' and there is no any
> > alternative version in case of PIE.
> >
> > If Xen should be compilable with PIE then we have to use ARM
> > implementation of bug.h everywhere. ( based on comment about 'i'
> > with
> > PIE ).
>
> The comment was added because until commit ecd6b9759919 ("Config.mk:
> correct PIE-related option(s) in EMBEDDED_EXTRA_CFLAGS") we would let
> the compiler to decide whether PIE should be enabled.
>
> Now we are forcing -fno-pie for Xen on any architecture (the flag is
> added at the top-level in Config.mk).
>
> >
> > Now I am totally confused...
>
> My point was not about using the Arm implementation everywhere. My
> point
> was that we disable even for Arm and therefore it is fine to use the
> common version.
>
> If in the future we need to support PIE in Xen (I am not aware of any
> effort yet), then we could decide to update the common BUG framework.
> But for now, I don't think this is something you need to care in your
> series.
>
Thanks for clarification.
> Cheers,
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v3 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
2023-02-24 11:31 [PATCH v3 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
` (2 preceding siblings ...)
2023-02-24 11:31 ` [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
@ 2023-02-24 11:31 ` Oleksii Kurochko
2023-02-27 14:46 ` Jan Beulich
3 siblings, 1 reply; 39+ messages in thread
From: Oleksii Kurochko @ 2023-02-24 11:31 UTC (permalink / raw)
To: xen-devel
Cc: Jan Beulich, Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Oleksii Kurochko, Roger Pau Monné,
Wei Liu
The following changes were made:
* Make GENERIC_BUG_FRAME mandatory for X86
* Update asm/bug.h using generic implementation in <xen/bug.h>
* Change prototype of debugger_trap_fatal() in asm/debugger.h
to align it with generic prototype in <xen/debugger.h>.
* Update do_invalid_op using generic do_bug_frame()
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V3:
* As prototype and what do_bug_frame() returns was changed so patch 3 and 4
was updated to use a new version of do_bug_frame
* MODIFIER was change to BUG_ASM_CONST to align with generic implementation
---
Changes in V2:
* Remove all unnecessary things from <asm/bug.h> as they were introduced in
<xen/bug.h>.
* Define BUG_INSTR = 'ud2' and MODIFIER = 'c' ( it is needed to skip '$'
when use an imidiate in x86 assembly )
* Update do_invalid_op() to re-use handle_bug_frame() and find_bug_frame()
from generic implemetation of CONFIG_GENERIC_BUG_FRAME
* Code style fixes.
---
xen/arch/x86/Kconfig | 1 +
xen/arch/x86/include/asm/bug.h | 73 ++------------------------
xen/arch/x86/include/asm/debugger.h | 4 +-
xen/arch/x86/traps.c | 81 +++--------------------------
4 files changed, 14 insertions(+), 145 deletions(-)
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 6a7825f4ba..b0ff1f3ee6 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -11,6 +11,7 @@ config X86
select ARCH_MAP_DOMAIN_PAGE
select ARCH_SUPPORTS_INT128
select CORE_PARKING
+ select GENERIC_BUG_FRAME
select HAS_ALTERNATIVE
select HAS_COMPAT
select HAS_CPUFREQ
diff --git a/xen/arch/x86/include/asm/bug.h b/xen/arch/x86/include/asm/bug.h
index 4b3e7b019d..d7eb41baf3 100644
--- a/xen/arch/x86/include/asm/bug.h
+++ b/xen/arch/x86/include/asm/bug.h
@@ -3,75 +3,10 @@
#ifndef __ASSEMBLY__
-#define BUG_FRAME_STRUCT
+#define BUG_INSTR "ud2"
+#define BUG_ASM_CONST "c"
-struct bug_frame {
- signed int loc_disp:BUG_DISP_WIDTH;
- unsigned int line_hi:BUG_LINE_HI_WIDTH;
- signed int ptr_disp:BUG_DISP_WIDTH;
- unsigned int line_lo:BUG_LINE_LO_WIDTH;
- signed int msg_disp[];
-};
-
-#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
-#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
-#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0)) & \
- ((1 << BUG_LINE_HI_WIDTH) - 1)) << \
- BUG_LINE_LO_WIDTH) + \
- (((b)->line_lo + ((b)->ptr_disp < 0)) & \
- ((1 << BUG_LINE_LO_WIDTH) - 1)))
-#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
-
-#define _ASM_BUGFRAME_TEXT(second_frame) \
- ".Lbug%=: ud2\n" \
- ".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n" \
- ".p2align 2\n" \
- ".Lfrm%=:\n" \
- ".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n" \
- ".long (%c[bf_ptr] - .Lfrm%=) + %c[bf_line_lo]\n" \
- ".if " #second_frame "\n" \
- ".long 0, %c[bf_msg] - .Lfrm%=\n" \
- ".endif\n" \
- ".popsection\n" \
-
-#define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \
- [bf_type] "i" (type), \
- [bf_ptr] "i" (ptr), \
- [bf_msg] "i" (msg), \
- [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1)) \
- << BUG_DISP_WIDTH), \
- [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
-
-#define BUG_FRAME(type, line, ptr, second_frame, msg) do { \
- BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)); \
- BUILD_BUG_ON((type) >= BUGFRAME_NR); \
- asm volatile ( _ASM_BUGFRAME_TEXT(second_frame) \
- :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \
-} while (0)
-
-
-#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
-#define BUG() do { \
- BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \
- unreachable(); \
-} while (0)
-
-/*
- * TODO: untangle header dependences, break BUILD_BUG_ON() out of xen/lib.h,
- * and use a real static inline here to get proper type checking of fn().
- */
-#define run_in_exception_handler(fn) \
- do { \
- (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \
- BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \
- } while ( 0 )
-
-#define assert_failed(msg) do { \
- BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
- unreachable(); \
-} while (0)
-
-#else /* !__ASSEMBLY__ */
+#else
/*
* Construct a bugframe, suitable for using in assembly code. Should always
@@ -113,6 +48,6 @@ struct bug_frame {
#define ASSERT_FAILED(msg) \
BUG_FRAME BUGFRAME_assert, __LINE__, __FILE__, 1, msg
-#endif /* !__ASSEMBLY__ */
+#endif /* __ASSEMBLY__ */
#endif /* __X86_BUG_H__ */
diff --git a/xen/arch/x86/include/asm/debugger.h b/xen/arch/x86/include/asm/debugger.h
index a5c299c6c3..1873dc5779 100644
--- a/xen/arch/x86/include/asm/debugger.h
+++ b/xen/arch/x86/include/asm/debugger.h
@@ -14,9 +14,9 @@
/* Returns true if GDB handled the trap, or it is surviveable. */
static inline bool debugger_trap_fatal(
- unsigned int vector, struct cpu_user_regs *regs)
+ unsigned int vector, const struct cpu_user_regs *regs)
{
- int rc = __trap_to_gdb(regs, vector);
+ int rc = __trap_to_gdb((struct cpu_user_regs *)regs, vector);
if ( rc == 0 )
return true;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index cade9e12f8..7c33ebceff 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -24,6 +24,7 @@
* Gareth Hughes <gareth@valinux.com>, May 2000
*/
+#include <xen/bug.h>
#include <xen/init.h>
#include <xen/sched.h>
#include <xen/lib.h>
@@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
void do_invalid_op(struct cpu_user_regs *regs)
{
- const struct bug_frame *bug = NULL;
u8 bug_insn[2];
- const char *prefix = "", *filename, *predicate, *eip = (char *)regs->rip;
- unsigned long fixup;
- int id = -1, lineno;
- const struct virtual_region *region;
+ const char *eip = (char *)regs->rip;
+ int id;
if ( likely(guest_mode(regs)) )
{
@@ -1185,83 +1183,18 @@ void do_invalid_op(struct cpu_user_regs *regs)
memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) )
goto die;
- region = find_text_region(regs->rip);
- if ( region )
- {
- for ( id = 0; id < BUGFRAME_NR; id++ )
- {
- const struct bug_frame *b;
- unsigned int i;
-
- for ( i = 0, b = region->frame[id].bugs;
- i < region->frame[id].n_bugs; b++, i++ )
- {
- if ( bug_loc(b) == eip )
- {
- bug = b;
- goto found;
- }
- }
- }
- }
-
- found:
- if ( !bug )
+ id = do_bug_frame(regs, regs->rip);
+ if ( id < 0 )
goto die;
- eip += sizeof(bug_insn);
- if ( id == BUGFRAME_run_fn )
- {
- void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
-
- fn(regs);
- fixup_exception_return(regs, (unsigned long)eip);
- return;
- }
- /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
- filename = bug_ptr(bug);
- if ( !is_kernel(filename) && !is_patch(filename) )
- goto die;
- fixup = strlen(filename);
- if ( fixup > 50 )
- {
- filename += fixup - 47;
- prefix = "...";
- }
- lineno = bug_line(bug);
+ eip += sizeof(bug_insn);
switch ( id )
{
+ case BUGFRAME_run_fn:
case BUGFRAME_warn:
- printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
- show_execution_state(regs);
fixup_exception_return(regs, (unsigned long)eip);
return;
-
- case BUGFRAME_bug:
- printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
-
- if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
- return;
-
- show_execution_state(regs);
- panic("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
-
- case BUGFRAME_assert:
- /* ASSERT: decode the predicate string pointer. */
- predicate = bug_msg(bug);
- if ( !is_kernel(predicate) && !is_patch(predicate) )
- predicate = "<unknown>";
-
- printk("Assertion '%s' failed at %s%s:%d\n",
- predicate, prefix, filename, lineno);
-
- if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
- return;
-
- show_execution_state(regs);
- panic("Assertion '%s' failed at %s%s:%d\n",
- predicate, prefix, filename, lineno);
}
die:
--
2.39.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v3 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
2023-02-24 11:31 ` [PATCH v3 4/4] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
@ 2023-02-27 14:46 ` Jan Beulich
2023-02-28 16:28 ` Oleksii
0 siblings, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2023-02-27 14:46 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Roger Pau Monné,
Wei Liu, xen-devel
On 24.02.2023 12:31, Oleksii Kurochko wrote:
> The following changes were made:
> * Make GENERIC_BUG_FRAME mandatory for X86
> * Update asm/bug.h using generic implementation in <xen/bug.h>
> * Change prototype of debugger_trap_fatal() in asm/debugger.h
> to align it with generic prototype in <xen/debugger.h>.
> * Update do_invalid_op using generic do_bug_frame()
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Hmm, there's the question of where to draw the boundary between patch 2
and the one here. Personally I'd say the earlier patch should drop
everything that becomes redundant there. I can see the more minimalistic
earlier change as viable, but then the description there needs to say
why things are being kept which - in principle - could be dropped.
> --- a/xen/arch/x86/include/asm/bug.h
> +++ b/xen/arch/x86/include/asm/bug.h
> @@ -3,75 +3,10 @@
>
> #ifndef __ASSEMBLY__
>
> -#define BUG_FRAME_STRUCT
> +#define BUG_INSTR "ud2"
> +#define BUG_ASM_CONST "c"
>
> -struct bug_frame {
> - signed int loc_disp:BUG_DISP_WIDTH;
> - unsigned int line_hi:BUG_LINE_HI_WIDTH;
> - signed int ptr_disp:BUG_DISP_WIDTH;
> - unsigned int line_lo:BUG_LINE_LO_WIDTH;
> - signed int msg_disp[];
> -};
> -
> -#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
> -#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
> -#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0)) & \
> - ((1 << BUG_LINE_HI_WIDTH) - 1)) << \
> - BUG_LINE_LO_WIDTH) + \
> - (((b)->line_lo + ((b)->ptr_disp < 0)) & \
> - ((1 << BUG_LINE_LO_WIDTH) - 1)))
> -#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
> -
> -#define _ASM_BUGFRAME_TEXT(second_frame) \
> - ".Lbug%=: ud2\n" \
> - ".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n" \
> - ".p2align 2\n" \
> - ".Lfrm%=:\n" \
> - ".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n" \
> - ".long (%c[bf_ptr] - .Lfrm%=) + %c[bf_line_lo]\n" \
> - ".if " #second_frame "\n" \
> - ".long 0, %c[bf_msg] - .Lfrm%=\n" \
> - ".endif\n" \
> - ".popsection\n" \
> -
> -#define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \
> - [bf_type] "i" (type), \
> - [bf_ptr] "i" (ptr), \
> - [bf_msg] "i" (msg), \
> - [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1)) \
> - << BUG_DISP_WIDTH), \
> - [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
> -
> -#define BUG_FRAME(type, line, ptr, second_frame, msg) do { \
> - BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH)); \
> - BUILD_BUG_ON((type) >= BUGFRAME_NR); \
> - asm volatile ( _ASM_BUGFRAME_TEXT(second_frame) \
> - :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \
> -} while (0)
> -
> -
> -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
> -#define BUG() do { \
> - BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \
> - unreachable(); \
> -} while (0)
> -
> -/*
> - * TODO: untangle header dependences, break BUILD_BUG_ON() out of xen/lib.h,
> - * and use a real static inline here to get proper type checking of fn().
> - */
> -#define run_in_exception_handler(fn) \
> - do { \
> - (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \
> - BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \
> - } while ( 0 )
> -
> -#define assert_failed(msg) do { \
> - BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
> - unreachable(); \
> -} while (0)
> -
> -#else /* !__ASSEMBLY__ */
> +#else
While for new #ifdef-ary such comments can reasonably be omitted when
the blocks are short, I'd recommend keeping existing comments (except
in extreme cases) in the interest of reduced code churn. In the case
here, considering that ...
> @@ -113,6 +48,6 @@ struct bug_frame {
> #define ASSERT_FAILED(msg) \
> BUG_FRAME BUGFRAME_assert, __LINE__, __FILE__, 1, msg
>
> -#endif /* !__ASSEMBLY__ */
> +#endif /* __ASSEMBLY__ */
... you keep (but alter - please don't) the comment on the #endif,
that's even more so a reason to also keep the comment on #else.
> --- a/xen/arch/x86/include/asm/debugger.h
> +++ b/xen/arch/x86/include/asm/debugger.h
> @@ -14,9 +14,9 @@
>
> /* Returns true if GDB handled the trap, or it is surviveable. */
> static inline bool debugger_trap_fatal(
> - unsigned int vector, struct cpu_user_regs *regs)
> + unsigned int vector, const struct cpu_user_regs *regs)
> {
> - int rc = __trap_to_gdb(regs, vector);
> + int rc = __trap_to_gdb((struct cpu_user_regs *)regs, vector);
I view casts in general as risky and hence preferable to avoid. Casting
away const-ness is particularly problematic. I guess the least bad
option is to drop const from the do_bug_frame()'s parameter again in
patch 1. However, for a fatal trap (assuming that really _is_ fatal,
i.e. execution cannot continue), not allowing register state to be
altered makes kind of sense. So I wonder whether we couldn't push the
casting into the gdbstub functions. But quite likely that's going to
be too intrusive for re-work like you do here.
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
2023-02-27 14:46 ` Jan Beulich
@ 2023-02-28 16:28 ` Oleksii
2023-02-28 16:36 ` Jan Beulich
0 siblings, 1 reply; 39+ messages in thread
From: Oleksii @ 2023-02-28 16:28 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Roger Pau Monné,
Wei Liu, xen-devel
On Mon, 2023-02-27 at 15:46 +0100, Jan Beulich wrote:
> On 24.02.2023 12:31, Oleksii Kurochko wrote:
> > The following changes were made:
> > * Make GENERIC_BUG_FRAME mandatory for X86
> > * Update asm/bug.h using generic implementation in <xen/bug.h>
> > * Change prototype of debugger_trap_fatal() in asm/debugger.h
> > to align it with generic prototype in <xen/debugger.h>.
> > * Update do_invalid_op using generic do_bug_frame()
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>
> Hmm, there's the question of where to draw the boundary between patch
> 2
> and the one here. Personally I'd say the earlier patch should drop
> everything that becomes redundant there. I can see the more
> minimalistic
> earlier change as viable, but then the description there needs to say
> why things are being kept which - in principle - could be dropped.
Then I'll update the comment message for patch 2.
>
> > --- a/xen/arch/x86/include/asm/bug.h
> > +++ b/xen/arch/x86/include/asm/bug.h
> > @@ -3,75 +3,10 @@
> >
> > #ifndef __ASSEMBLY__
> >
> > -#define BUG_FRAME_STRUCT
> > +#define BUG_INSTR "ud2"
> > +#define BUG_ASM_CONST "c"
> >
> > -struct bug_frame {
> > - signed int loc_disp:BUG_DISP_WIDTH;
> > - unsigned int line_hi:BUG_LINE_HI_WIDTH;
> > - signed int ptr_disp:BUG_DISP_WIDTH;
> > - unsigned int line_lo:BUG_LINE_LO_WIDTH;
> > - signed int msg_disp[];
> > -};
> > -
> > -#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
> > -#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
> > -#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0))
> > & \
> > - ((1 << BUG_LINE_HI_WIDTH) - 1))
> > << \
> > - BUG_LINE_LO_WIDTH)
> > + \
> > - (((b)->line_lo + ((b)->ptr_disp < 0))
> > & \
> > - ((1 << BUG_LINE_LO_WIDTH) - 1)))
> > -#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
> > -
> > -#define
> > _ASM_BUGFRAME_TEXT(second_frame)
> > \
> > - ".Lbug%=:
> > ud2\n" \
> > - ".pushsection .bug_frames.%c[bf_type], \"a\",
> > @progbits\n" \
> > - ".p2align
> > 2\n" \
> > -
> > ".Lfrm%=:\n"
> > \
> > - ".long (.Lbug%= - .Lfrm%=) +
> > %c[bf_line_hi]\n" \
> > - ".long (%c[bf_ptr] - .Lfrm%=) +
> > %c[bf_line_lo]\n" \
> > - ".if " #second_frame
> > "\n" \
> > - ".long 0, %c[bf_msg] -
> > .Lfrm%=\n" \
> > -
> > ".endif\n"
> > \
> > -
> > ".popsection\n"
> > \
> > -
> > -#define _ASM_BUGFRAME_INFO(type, line, ptr,
> > msg) \
> > - [bf_type] "i"
> > (type), \
> > - [bf_ptr] "i"
> > (ptr), \
> > - [bf_msg] "i"
> > (msg), \
> > - [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) -
> > 1)) \
> > - <<
> > BUG_DISP_WIDTH), \
> > - [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) <<
> > BUG_DISP_WIDTH)
> > -
> > -#define BUG_FRAME(type, line, ptr, second_frame, msg) do
> > { \
> > - BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> > BUG_LINE_HI_WIDTH)); \
> > - BUILD_BUG_ON((type) >=
> > BUGFRAME_NR); \
> > - asm volatile (
> > _ASM_BUGFRAME_TEXT(second_frame) \
> > - :: _ASM_BUGFRAME_INFO(type, line, ptr, msg)
> > ); \
> > -} while (0)
> > -
> > -
> > -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0,
> > NULL)
> > -#define BUG() do { \
> > - BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \
> > - unreachable(); \
> > -} while (0)
> > -
> > -/*
> > - * TODO: untangle header dependences, break BUILD_BUG_ON() out of
> > xen/lib.h,
> > - * and use a real static inline here to get proper type checking
> > of fn().
> > - */
> > -#define run_in_exception_handler(fn) \
> > - do { \
> > - (void)((fn) == (void (*)(struct cpu_user_regs *))NULL); \
> > - BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL); \
> > - } while ( 0 )
> > -
> > -#define assert_failed(msg) do { \
> > - BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
> > - unreachable(); \
> > -} while (0)
> > -
> > -#else /* !__ASSEMBLY__ */
> > +#else
>
> While for new #ifdef-ary such comments can reasonably be omitted when
> the blocks are short, I'd recommend keeping existing comments (except
> in extreme cases) in the interest of reduced code churn. In the case
> here, considering that ...
Got it.
>
> > @@ -113,6 +48,6 @@ struct bug_frame {
> > #define ASSERT_FAILED(msg) \
> > BUG_FRAME BUGFRAME_assert, __LINE__, __FILE__, 1, msg
> >
> > -#endif /* !__ASSEMBLY__ */
> > +#endif /* __ASSEMBLY__ */
>
> ... you keep (but alter - please don't) the comment on the #endif,
> that's even more so a reason to also keep the comment on #else.
>
> > --- a/xen/arch/x86/include/asm/debugger.h
> > +++ b/xen/arch/x86/include/asm/debugger.h
> > @@ -14,9 +14,9 @@
> >
> > /* Returns true if GDB handled the trap, or it is surviveable. */
> > static inline bool debugger_trap_fatal(
> > - unsigned int vector, struct cpu_user_regs *regs)
> > + unsigned int vector, const struct cpu_user_regs *regs)
> > {
> > - int rc = __trap_to_gdb(regs, vector);
> > + int rc = __trap_to_gdb((struct cpu_user_regs *)regs, vector);
>
> I view casts in general as risky and hence preferable to avoid.
> Casting
> away const-ness is particularly problematic. I guess the least bad
> option is to drop const from the do_bug_frame()'s parameter again in
> patch 1. However, for a fatal trap (assuming that really _is_ fatal,
> i.e. execution cannot continue), not allowing register state to be
> altered makes kind of sense. So I wonder whether we couldn't push the
> casting into the gdbstub functions. But quite likely that's going to
> be too intrusive for re-work like you do here.
I am not sure that casting inside the gdbstub functions will help as
do_bug_frame() calls debugger_trap_fatal() which has define regs
without const. So it will still be compile issue as in do_bug_frame()
regs are declared as const.
So it looks like the best one issue now will be drop const from the
do_bug_frame()...
>
> Jan
~ Oleksii
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v3 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
2023-02-28 16:28 ` Oleksii
@ 2023-02-28 16:36 ` Jan Beulich
0 siblings, 0 replies; 39+ messages in thread
From: Jan Beulich @ 2023-02-28 16:36 UTC (permalink / raw)
To: Oleksii
Cc: Andrew Cooper, Stefano Stabellini, Gianluca Guida,
Roger Pau Monné,
Wei Liu, xen-devel
On 28.02.2023 17:28, Oleksii wrote:
> On Mon, 2023-02-27 at 15:46 +0100, Jan Beulich wrote:
>> On 24.02.2023 12:31, Oleksii Kurochko wrote:
>>> --- a/xen/arch/x86/include/asm/debugger.h
>>> +++ b/xen/arch/x86/include/asm/debugger.h
>>> @@ -14,9 +14,9 @@
>>>
>>> /* Returns true if GDB handled the trap, or it is surviveable. */
>>> static inline bool debugger_trap_fatal(
>>> - unsigned int vector, struct cpu_user_regs *regs)
>>> + unsigned int vector, const struct cpu_user_regs *regs)
>>> {
>>> - int rc = __trap_to_gdb(regs, vector);
>>> + int rc = __trap_to_gdb((struct cpu_user_regs *)regs, vector);
>>
>> I view casts in general as risky and hence preferable to avoid.
>> Casting
>> away const-ness is particularly problematic. I guess the least bad
>> option is to drop const from the do_bug_frame()'s parameter again in
>> patch 1. However, for a fatal trap (assuming that really _is_ fatal,
>> i.e. execution cannot continue), not allowing register state to be
>> altered makes kind of sense. So I wonder whether we couldn't push the
>> casting into the gdbstub functions. But quite likely that's going to
>> be too intrusive for re-work like you do here.
> I am not sure that casting inside the gdbstub functions will help as
> do_bug_frame() calls debugger_trap_fatal() which has define regs
> without const. So it will still be compile issue as in do_bug_frame()
> regs are declared as const.
Well, the idea was of course to propagate const further down, to cast
it away only in restricted cases. But ...
> So it looks like the best one issue now will be drop const from the
> do_bug_frame()...
... looks like we're in agreement then about this being the better
route for the time being.
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread