The patch introduces macros: BUG(), WARN(), run_in_exception(),
assert_failed.
The implementation uses "ebreak" instruction in combination with
diffrent bug frame tables (for each type) which contains useful
information.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
xen/arch/riscv/include/asm/bug.h | 120 +++++++++++++++++++++++++++++++
xen/arch/riscv/traps.c | 116 ++++++++++++++++++++++++++++++
xen/arch/riscv/xen.lds.S | 10 +++
3 files changed, 246 insertions(+)
create mode 100644 xen/arch/riscv/include/asm/bug.h
diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h
new file mode 100644
index 0000000000..d17ffdcc4d
--- /dev/null
+++ b/xen/arch/riscv/include/asm/bug.h
@@ -0,0 +1,120 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2012 Regents of the University of California
+ * Copyright (C) 2021-2023 Vates
+ *
+ */
+
+#ifndef _ASM_RISCV_BUG_H
+#define _ASM_RISCV_BUG_H
+
+#include <xen/stringify.h>
+#include <xen/types.h>
+
+#ifndef __ASSEMBLY__
+
+struct bug_frame {
+ signed int loc_disp; /* Relative address to the bug address */
+ signed int file_disp; /* Relative address to the filename */
+ signed int msg_disp; /* Relative address to the predicate (for ASSERT) */
+ uint16_t line; /* Line number */
+ uint32_t pad0:16; /* Padding for 8-bytes align */
+};
+
+#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
+#define bug_file(b) ((const void *)(b) + (b)->file_disp);
+#define bug_line(b) ((b)->line)
+#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
+
+#define BUGFRAME_run_fn 0
+#define BUGFRAME_warn 1
+#define BUGFRAME_bug 2
+#define BUGFRAME_assert 3
+
+#define BUGFRAME_NR 4
+
+#define __INSN_LENGTH_MASK _UL(0x3)
+#define __INSN_LENGTH_32 _UL(0x3)
+#define __COMPRESSED_INSN_MASK _UL(0xffff)
+
+#define __BUG_INSN_32 _UL(0x00100073) /* ebreak */
+#define __BUG_INSN_16 _UL(0x9002) /* c.ebreak */
+
+#define GET_INSN_LENGTH(insn) \
+({ \
+ unsigned long __len; \
+ __len = ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) ? \
+ 4UL : 2UL; \
+ __len; \
+})
+
+typedef u32 bug_insn_t;
+
+/* These are defined by the architecture */
+int is_valid_bugaddr(bug_insn_t addr);
+
+#define BUG_FN_REG t0
+
+/* 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 { \
+ asm ("1:ebreak\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 ("mv " __stringify(BUG_FN_REG) ", %0\n" \
+ "1:ebreak\n" \
+ ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn) "," \
+ " \"a\", %%progbits\n" \
+ "2:\n" \
+ ".p2align 2\n" \
+ ".long (1b - 2b)\n" \
+ ".long 0, 0, 0\n" \
+ ".popsection" :: "r" (fn) : __stringify(BUG_FN_REG) ); \
+} while (0)
+
+#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")
+
+#define BUG() do { \
+ BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, ""); \
+ unreachable(); \
+} while (0)
+
+#define assert_failed(msg) do { \
+ BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
+ unreachable(); \
+} while (0)
+
+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 /* _ASM_RISCV_BUG_H */
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index fc25138a4b..8b719a5ef5 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -4,6 +4,7 @@
*
* RISC-V Trap handlers
*/
+#include <asm/bug.h>
#include <asm/csr.h>
#include <asm/early_printk.h>
#include <asm/processor.h>
@@ -107,7 +108,122 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
wait_for_interrupt();
}
+void show_execution_state(const struct cpu_user_regs *regs)
+{
+ early_printk("implement show_execution_state(regs)\n");
+}
+
+int do_bug_frame(struct cpu_user_regs *regs, vaddr_t pc)
+{
+ struct bug_frame *start, *end;
+ struct bug_frame *bug = NULL;
+ unsigned int id = 0;
+ const char *filename, *predicate;
+ int lineno;
+
+ unsigned long bug_frames[] = {
+ (unsigned long)&__start_bug_frames[0],
+ (unsigned long)&__stop_bug_frames_0[0],
+ (unsigned long)&__stop_bug_frames_1[0],
+ (unsigned long)&__stop_bug_frames_2[0],
+ (unsigned long)&__stop_bug_frames_3[0],
+ };
+
+ for ( id = 0; id < BUGFRAME_NR; id++ )
+ {
+ start = (struct bug_frame *)bug_frames[id];
+ end = (struct bug_frame *)bug_frames[id + 1];
+
+ while ( start != end )
+ {
+ if ( (vaddr_t)bug_loc(start) == pc )
+ {
+ bug = start;
+ goto found;
+ }
+
+ start++;
+ }
+ }
+
+found:
+ if ( bug == NULL )
+ return -ENOENT;
+
+ if ( id == BUGFRAME_run_fn )
+ {
+ void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;
+
+ fn(regs);
+
+ goto end;
+ }
+
+ /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
+ filename = bug_file(bug);
+ lineno = bug_line(bug);
+
+ switch ( id )
+ {
+ case BUGFRAME_warn:
+ early_printk("Xen WARN at ");
+ early_printk(filename);
+ early_printk(":");
+ early_printk_hnum(lineno);
+
+ show_execution_state(regs);
+
+ goto end;
+
+ case BUGFRAME_bug:
+ early_printk("Xen BUG at ");
+ early_printk(filename);
+ early_printk(":");
+ early_printk_hnum(lineno);
+
+ show_execution_state(regs);
+ early_printk("change wait_for_interrupt to panic() when common is available\n");
+ wait_for_interrupt();
+
+ case BUGFRAME_assert:
+ /* ASSERT: decode the predicate string pointer. */
+ predicate = bug_msg(bug);
+
+ early_printk("Assertion \'");
+ early_printk(predicate);
+ early_printk("\' failed at ");
+ early_printk(filename);
+ early_printk(":");
+ early_printk_hnum(lineno);
+
+ show_execution_state(regs);
+ early_printk("change wait_for_interrupt to panic() when common is available\n");
+ wait_for_interrupt();
+ }
+
+ return -EINVAL;
+end:
+ regs->sepc += GET_INSN_LENGTH(*(bug_insn_t *)pc);
+
+ return 0;
+}
+
+int is_valid_bugaddr(bug_insn_t insn)
+{
+ if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32)
+ return (insn == __BUG_INSN_32);
+ else
+ return ((insn & __COMPRESSED_INSN_MASK) == __BUG_INSN_16);
+}
+
void __handle_exception(struct cpu_user_regs *cpu_regs)
{
+ register_t pc = cpu_regs->sepc;
+ uint32_t instr = *(bug_insn_t *)pc;
+
+ if (is_valid_bugaddr(instr))
+ if (!do_bug_frame(cpu_regs, pc)) return;
+
+// die:
do_unexpected_trap(cpu_regs);
}
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index ca57cce75c..139e2d04cb 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -39,6 +39,16 @@ SECTIONS
. = ALIGN(PAGE_SIZE);
.rodata : {
_srodata = .; /* Read-only data */
+ /* Bug frames table */
+ __start_bug_frames = .;
+ *(.bug_frames.0)
+ __stop_bug_frames_0 = .;
+ *(.bug_frames.1)
+ __stop_bug_frames_1 = .;
+ *(.bug_frames.2)
+ __stop_bug_frames_2 = .;
+ *(.bug_frames.3)
+ __stop_bug_frames_3 = .;
*(.rodata)
*(.rodata.*)
*(.data.rel.ro)
--
2.39.0On 20.01.2023 15:59, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/bug.h
> @@ -0,0 +1,120 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2021-2023 Vates
> + *
> + */
> +
> +#ifndef _ASM_RISCV_BUG_H
> +#define _ASM_RISCV_BUG_H
> +
> +#include <xen/stringify.h>
> +#include <xen/types.h>
> +
> +#ifndef __ASSEMBLY__
> +
> +struct bug_frame {
> + signed int loc_disp; /* Relative address to the bug address */
> + signed int file_disp; /* Relative address to the filename */
> + signed int msg_disp; /* Relative address to the predicate (for ASSERT) */
> + uint16_t line; /* Line number */
> + uint32_t pad0:16; /* Padding for 8-bytes align */
> +};
> +
> +#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
> +#define bug_file(b) ((const void *)(b) + (b)->file_disp);
> +#define bug_line(b) ((b)->line)
> +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
> +
> +#define BUGFRAME_run_fn 0
> +#define BUGFRAME_warn 1
> +#define BUGFRAME_bug 2
> +#define BUGFRAME_assert 3
> +
> +#define BUGFRAME_NR 4
> +
> +#define __INSN_LENGTH_MASK _UL(0x3)
> +#define __INSN_LENGTH_32 _UL(0x3)
> +#define __COMPRESSED_INSN_MASK _UL(0xffff)
> +
> +#define __BUG_INSN_32 _UL(0x00100073) /* ebreak */
> +#define __BUG_INSN_16 _UL(0x9002) /* c.ebreak */
May I suggest that you avoid double-underscore (or other reserved) names
where possible?
> +#define GET_INSN_LENGTH(insn) \
> +({ \
> + unsigned long __len; \
> + __len = ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) ? \
> + 4UL : 2UL; \
> + __len; \
> +})
> +
> +typedef u32 bug_insn_t;
This is problematic beyond the u32 instead of uint32_t. You use it once, ...
> +/* These are defined by the architecture */
> +int is_valid_bugaddr(bug_insn_t addr);
... in a call to this function, but you can't assume that you can access
32 bits when the insn you look at might be a compressed one. Just to be
on the safe side I'd like to suggest to either avoid such a type, or to
introduce two (32- and 16-bit) which then of course need using properly
in respective contexts.
> +#define BUG_FN_REG t0
> +
> +/* 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 { \
> + asm ("1:ebreak\n" \
Something's odd with the padding here; looks like there might be hard tabs.
> + ".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 { \
With
register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
you should be able to avoid ...
> + asm ("mv " __stringify(BUG_FN_REG) ", %0\n" \
... this and simply use ...
> + "1:ebreak\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) ); \
:: "r" (fn_) );
here. See x86's alternative_callN() for similar examples.
> @@ -107,7 +108,122 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
> wait_for_interrupt();
> }
>
> +void show_execution_state(const struct cpu_user_regs *regs)
> +{
> + early_printk("implement show_execution_state(regs)\n");
> +}
> +
> +int do_bug_frame(struct cpu_user_regs *regs, vaddr_t pc)
> +{
> + struct bug_frame *start, *end;
> + struct bug_frame *bug = NULL;
> + unsigned int id = 0;
> + const char *filename, *predicate;
> + int lineno;
> +
> + unsigned long bug_frames[] = {
> + (unsigned long)&__start_bug_frames[0],
> + (unsigned long)&__stop_bug_frames_0[0],
> + (unsigned long)&__stop_bug_frames_1[0],
> + (unsigned long)&__stop_bug_frames_2[0],
> + (unsigned long)&__stop_bug_frames_3[0],
> + };
> +
> + for ( id = 0; id < BUGFRAME_NR; id++ )
> + {
> + start = (struct bug_frame *)bug_frames[id];
> + end = (struct bug_frame *)bug_frames[id + 1];
> +
> + while ( start != end )
> + {
> + if ( (vaddr_t)bug_loc(start) == pc )
> + {
> + bug = start;
> + goto found;
> + }
> +
> + start++;
> + }
> + }
> +
> +found:
Please indent labels by at least one blank; see ./CODING_STYLE.
> + if ( bug == NULL )
> + return -ENOENT;
> +
> + if ( id == BUGFRAME_run_fn )
> + {
> + void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG;
> +
> + fn(regs);
> +
> + goto end;
> + }
> +
> + /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> + filename = bug_file(bug);
> + lineno = bug_line(bug);
> +
> + switch ( id )
> + {
> + case BUGFRAME_warn:
> + early_printk("Xen WARN at ");
> + early_printk(filename);
> + early_printk(":");
> + early_printk_hnum(lineno);
> +
> + show_execution_state(regs);
> +
> + goto end;
> +
> + case BUGFRAME_bug:
> + early_printk("Xen BUG at ");
> + early_printk(filename);
> + early_printk(":");
> + early_printk_hnum(lineno);
> +
> + show_execution_state(regs);
> + early_printk("change wait_for_interrupt to panic() when common is available\n");
> + wait_for_interrupt();
> +
> + case BUGFRAME_assert:
> + /* ASSERT: decode the predicate string pointer. */
> + predicate = bug_msg(bug);
> +
> + early_printk("Assertion \'");
> + early_printk(predicate);
> + early_printk("\' failed at ");
> + early_printk(filename);
> + early_printk(":");
> + early_printk_hnum(lineno);
> +
> + show_execution_state(regs);
> + early_printk("change wait_for_interrupt to panic() when common is available\n");
> + wait_for_interrupt();
> + }
> +
> + return -EINVAL;
> +end:
> + regs->sepc += GET_INSN_LENGTH(*(bug_insn_t *)pc);
> +
> + return 0;
> +}
> +
> +int is_valid_bugaddr(bug_insn_t insn)
> +{
> + if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32)
> + return (insn == __BUG_INSN_32);
> + else
> + return ((insn & __COMPRESSED_INSN_MASK) == __BUG_INSN_16);
> +}
> +
> void __handle_exception(struct cpu_user_regs *cpu_regs)
> {
> + register_t pc = cpu_regs->sepc;
> + uint32_t instr = *(bug_insn_t *)pc;
> +
> + if (is_valid_bugaddr(instr))
> + if (!do_bug_frame(cpu_regs, pc)) return;
> +
> +// die:
Perhaps better to omit the label until it's actually needed? In any
event you shouldn't be using C++-style comments (see ./CODING_STYLE
again).
Jan
© 2016 - 2026 Red Hat, Inc.