The patch introduces macros: BUG(), WARN(), run_in_exception(),
assert_failed.
To be precise, the macros from generic bug implementation (<xen/bug.h>)
will be used.
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>
---
Changes in V7:
- move to this patch the definition of cast_to_bug_frame() from the previous patch.
- update the comment in bug.h.
- update the comment above do_bug_frame().
- fix code style.
- add comment to read_instr func.
- add space for bug_frames in lds.S.
---
Changes in V6:
- Avoid LINK_TO_LOAD() as bug.h functionality expected to be used
after MMU is enabled.
- Change early_printk() to printk()
---
Changes in V5:
- Remove "#include <xen/types.h>" from <asm/bug.h> as there is no any need in it anymore
- Update macros GET_INSN_LENGTH: remove UL and 'unsigned int len;' from it
- Remove " include <xen/bug.h>" from risc/setup.c. it is not needed in the current version of
the patch
- change an argument type from vaddr_t to uint32_t for is_valid_bugaddr and introduce read_instr() to
read instruction properly as the length of qinstruction can be either 32 or 16 bits.
- Code style fixes
- update the comments before do_bug_frame() in riscv/trap.c
- Refactor is_valid_bugaddr() function.
- introduce macros cast_to_bug_frame(addr) to hide casts.
- use LINK_TO_LOAD() for addresses which are linker time relative.
---
Changes in V4:
- Updates in RISC-V's <asm/bug.h>:
* Add explanatory comment about why there is only defined for 32-bits length
instructions and 16/32-bits BUG_INSN_{16,32}.
* Change 'unsigned long' to 'unsigned int' inside GET_INSN_LENGTH().
* Update declaration of is_valid_bugaddr(): switch return type from int to bool
and the argument from 'unsigned int' to 'vaddr'.
- Updates in RISC-V's traps.c:
* replace /xen and /asm includes
* update definition of is_valid_bugaddr():switch return type from int to bool
and the argument from 'unsigned int' to 'vaddr'. Code style inside function
was updated too.
* do_bug_frame() refactoring:
* local variables start and bug became 'const struct bug_frame'
* bug_frames[] array became 'static const struct bug_frame[] = ...'
* remove all casts
* remove unneeded comments and add an explanatory comment that the do_bug_frame()
will be switched to a generic one.
* do_trap() refactoring:
* read 16-bits value instead of 32-bits as compressed instruction can
be used and it might happen than only 16-bits may be accessible.
* code style updates
* re-use instr variable instead of re-reading instruction.
- Updates in setup.c:
* add blank line between xen/ and asm/ includes.
---
Changes in V3:
- Rebase the patch "xen/riscv: introduce an implementation of macros
from <asm/bug.h>" on top of patch series [introduce generic implementation
of macros from bug.h]
---
Changes in V2:
- Remove __ in define namings
- Update run_in_exception_handler() with
register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
- Remove bug_instr_t type and change it's usage to uint32_t
---
xen/arch/riscv/include/asm/bug.h | 27 +++++++
xen/arch/riscv/traps.c | 128 +++++++++++++++++++++++++++++++
xen/arch/riscv/xen.lds.S | 10 +++
3 files changed, 165 insertions(+)
diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h
index e8b1e40823..f5ff96140f 100644
--- a/xen/arch/riscv/include/asm/bug.h
+++ b/xen/arch/riscv/include/asm/bug.h
@@ -7,4 +7,31 @@
#ifndef _ASM_RISCV_BUG_H
#define _ASM_RISCV_BUG_H
+#ifndef __ASSEMBLY__
+
+#define BUG_INSTR "ebreak"
+
+/*
+ * The base instruction set has a fixed length of 32-bit naturally aligned
+ * instructions.
+ *
+ * There are extensions of variable length ( where each instruction can be
+ * any number of 16-bit parcels in length ).
+ *
+ * Compressed ISA is used now where the instruction length is 16 bit and
+ * 'ebreak' instruction, in this case, can be either 16 or 32 bit (
+ * depending on if compressed ISA is used or not )
+ */
+#define INSN_LENGTH_MASK _UL(0x3)
+#define INSN_LENGTH_32 _UL(0x3)
+
+#define BUG_INSN_32 _UL(0x00100073) /* ebreak */
+#define BUG_INSN_16 _UL(0x9002) /* c.ebreak */
+#define COMPRESSED_INSN_MASK _UL(0xffff)
+
+#define GET_INSN_LENGTH(insn) \
+ (((insn) & INSN_LENGTH_MASK) == INSN_LENGTH_32 ? 4 : 2) \
+
+#endif /* !__ASSEMBLY__ */
+
#endif /* _ASM_RISCV_BUG_H */
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 3e08ec44f7..439c098c22 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -5,6 +5,8 @@
* RISC-V Trap handlers
*/
+#include <xen/bug.h>
+#include <xen/errno.h>
#include <xen/lib.h>
#include <asm/csr.h>
@@ -12,6 +14,8 @@
#include <asm/processor.h>
#include <asm/traps.h>
+#define cast_to_bug_frame(addr) ((const struct bug_frame *)(addr))
+
/*
* Initialize the trap handling.
*
@@ -101,7 +105,131 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
die();
}
+void show_execution_state(const struct cpu_user_regs *regs)
+{
+ printk("implement show_execution_state(regs)\n");
+}
+
+/*
+ * TODO: generic do_bug_frame() should be used instead of current
+ * implementation panic(), printk() and find_text_region()
+ * (virtual memory?) will be ready/merged
+ */
+int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
+{
+ const struct bug_frame *start, *end;
+ const struct bug_frame *bug = NULL;
+ unsigned int id = 0;
+ const char *filename, *predicate;
+ int lineno;
+
+ static const struct bug_frame *bug_frames[] = {
+ &__start_bug_frames[0],
+ &__stop_bug_frames_0[0],
+ &__stop_bug_frames_1[0],
+ &__stop_bug_frames_2[0],
+ &__stop_bug_frames_3[0],
+ };
+
+ for ( id = 0; id < BUGFRAME_NR; id++ )
+ {
+ start = cast_to_bug_frame(bug_frames[id]);
+ end = cast_to_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 *) = bug_ptr(bug);
+
+ fn(regs);
+
+ goto end;
+ }
+
+ /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
+ filename = bug_ptr(bug);
+ lineno = bug_line(bug);
+
+ switch ( id )
+ {
+ case BUGFRAME_warn:
+ printk("Xen WARN at %s:%d\n", filename, lineno);
+
+ show_execution_state(regs);
+
+ goto end;
+
+ case BUGFRAME_bug:
+ printk("Xen BUG at %s:%d\n", filename, lineno);
+
+ show_execution_state(regs);
+
+ printk("change wait_for_interrupt to panic() when common is available\n");
+ die();
+
+ case BUGFRAME_assert:
+ /* ASSERT: decode the predicate string pointer. */
+ predicate = bug_msg(bug);
+
+ printk("Assertion %s failed at %s:%d\n", predicate, filename, lineno);
+
+ show_execution_state(regs);
+
+ printk("change wait_for_interrupt to panic() when common is available\n");
+ die();
+ }
+
+ return -EINVAL;
+
+ end:
+ return 0;
+}
+
+static bool is_valid_bugaddr(uint32_t insn)
+{
+ return insn == BUG_INSN_32 ||
+ (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
+}
+
+/* Should be used only in Xen code ? */
+static uint32_t read_instr(unsigned long pc)
+{
+ uint16_t instr16 = *(uint16_t *)pc;
+
+ if ( GET_INSN_LENGTH(instr16) == 2 )
+ return (uint32_t)instr16;
+ else
+ return *(uint32_t *)pc;
+}
+
void do_trap(struct cpu_user_regs *cpu_regs)
{
+ register_t pc = cpu_regs->sepc;
+ uint32_t instr = read_instr(pc);
+
+ if ( is_valid_bugaddr(instr) )
+ {
+ if ( !do_bug_frame(cpu_regs, pc) )
+ {
+ cpu_regs->sepc += GET_INSN_LENGTH(instr);
+ return;
+ }
+ }
+
do_unexpected_trap(cpu_regs);
}
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index 3fa7db3bf9..a10e0ad87c 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -45,6 +45,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.41.0On 03.08.2023 14:05, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -5,6 +5,8 @@
> * RISC-V Trap handlers
> */
>
> +#include <xen/bug.h>
> +#include <xen/errno.h>
> #include <xen/lib.h>
>
> #include <asm/csr.h>
> @@ -12,6 +14,8 @@
> #include <asm/processor.h>
> #include <asm/traps.h>
>
> +#define cast_to_bug_frame(addr) ((const struct bug_frame *)(addr))
> +
> /*
> * Initialize the trap handling.
> *
> @@ -101,7 +105,131 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
> die();
> }
>
> +void show_execution_state(const struct cpu_user_regs *regs)
> +{
> + printk("implement show_execution_state(regs)\n");
> +}
> +
> +/*
> + * TODO: generic do_bug_frame() should be used instead of current
> + * implementation panic(), printk() and find_text_region()
> + * (virtual memory?) will be ready/merged
> + */
> +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
> +{
> + const struct bug_frame *start, *end;
> + const struct bug_frame *bug = NULL;
> + unsigned int id = 0;
Pointless initializer.
> + const char *filename, *predicate;
> + int lineno;
> +
> + static const struct bug_frame *bug_frames[] = {
You likely want another const here.
> + &__start_bug_frames[0],
> + &__stop_bug_frames_0[0],
> + &__stop_bug_frames_1[0],
> + &__stop_bug_frames_2[0],
> + &__stop_bug_frames_3[0],
> + };
> +
> + for ( id = 0; id < BUGFRAME_NR; id++ )
> + {
> + start = cast_to_bug_frame(bug_frames[id]);
> + end = cast_to_bug_frame(bug_frames[id + 1]);
Why these casts (and then even hidden in a macro)? The array elements
look to already be of appropriate type.
> + 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 *) = bug_ptr(bug);
> +
> + fn(regs);
> +
> + goto end;
> + }
> +
> + /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> + filename = bug_ptr(bug);
> + lineno = bug_line(bug);
> +
> + switch ( id )
> + {
> + case BUGFRAME_warn:
> + printk("Xen WARN at %s:%d\n", filename, lineno);
> +
> + show_execution_state(regs);
> +
> + goto end;
> +
> + case BUGFRAME_bug:
> + printk("Xen BUG at %s:%d\n", filename, lineno);
> +
> + show_execution_state(regs);
> +
> + printk("change wait_for_interrupt to panic() when common is available\n");
> + die();
> +
> + case BUGFRAME_assert:
> + /* ASSERT: decode the predicate string pointer. */
> + predicate = bug_msg(bug);
> +
> + printk("Assertion %s failed at %s:%d\n", predicate, filename, lineno);
> +
> + show_execution_state(regs);
> +
> + printk("change wait_for_interrupt to panic() when common is available\n");
> + die();
> + }
> +
> + return -EINVAL;
> +
> + end:
> + return 0;
> +}
> +
> +static bool is_valid_bugaddr(uint32_t insn)
> +{
> + return insn == BUG_INSN_32 ||
> + (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
> +}
Why "addr" in the name when this takes an insn as argument?
> +/* Should be used only in Xen code ? */
What is this question about? With ...
> +static uint32_t read_instr(unsigned long pc)
> +{
> + uint16_t instr16 = *(uint16_t *)pc;
> +
> + if ( GET_INSN_LENGTH(instr16) == 2 )
> + return (uint32_t)instr16;
(I don't think this cast is needed.)
> + else
> + return *(uint32_t *)pc;
> +}
... there still being a double read here, do you perhaps mean to
make a statement (that this code isn't safe to use on guest code)?
> void do_trap(struct cpu_user_regs *cpu_regs)
> {
> + register_t pc = cpu_regs->sepc;
> + uint32_t instr = read_instr(pc);
> +
> + if ( is_valid_bugaddr(instr) )
> + {
> + if ( !do_bug_frame(cpu_regs, pc) )
> + {
> + cpu_regs->sepc += GET_INSN_LENGTH(instr);
> + return;
> + }
> + }
> +
> do_unexpected_trap(cpu_regs);
> }
On Mon, 2023-08-07 at 15:29 +0200, Jan Beulich wrote:
> On 03.08.2023 14:05, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/traps.c
> > +++ b/xen/arch/riscv/traps.c
> > @@ -5,6 +5,8 @@
> > * RISC-V Trap handlers
> > */
> >
> > +#include <xen/bug.h>
> > +#include <xen/errno.h>
> > #include <xen/lib.h>
> >
> > #include <asm/csr.h>
> > @@ -12,6 +14,8 @@
> > #include <asm/processor.h>
> > #include <asm/traps.h>
> >
> > +#define cast_to_bug_frame(addr) ((const struct bug_frame *)(addr))
> > +
> > /*
> > * Initialize the trap handling.
> > *
> > @@ -101,7 +105,131 @@ static void do_unexpected_trap(const struct
> > cpu_user_regs *regs)
> > die();
> > }
> >
> > +void show_execution_state(const struct cpu_user_regs *regs)
> > +{
> > + printk("implement show_execution_state(regs)\n");
> > +}
> > +
> > +/*
> > + * TODO: generic do_bug_frame() should be used instead of current
> > + * implementation panic(), printk() and find_text_region()
> > + * (virtual memory?) will be ready/merged
> > + */
> > +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
> > +{
> > + const struct bug_frame *start, *end;
> > + const struct bug_frame *bug = NULL;
> > + unsigned int id = 0;
>
> Pointless initializer.
Agree. Thanks. I'll remove it.
>
> > + const char *filename, *predicate;
> > + int lineno;
> > +
> > + static const struct bug_frame *bug_frames[] = {
>
> You likely want another const here.
Yes, I will add it.
>
> > + &__start_bug_frames[0],
> > + &__stop_bug_frames_0[0],
> > + &__stop_bug_frames_1[0],
> > + &__stop_bug_frames_2[0],
> > + &__stop_bug_frames_3[0],
> > + };
> > +
> > + for ( id = 0; id < BUGFRAME_NR; id++ )
> > + {
> > + start = cast_to_bug_frame(bug_frames[id]);
> > + end = cast_to_bug_frame(bug_frames[id + 1]);
>
> Why these casts (and then even hidden in a macro)? The array elements
> look to already be of appropriate type.
There is no any sense for these casts. It looks like that before
bug_frames array has a different type.
>
> > + 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 *) = bug_ptr(bug);
> > +
> > + fn(regs);
> > +
> > + goto end;
> > + }
> > +
> > + /* WARN, BUG or ASSERT: decode the filename pointer and line
> > number. */
> > + filename = bug_ptr(bug);
> > + lineno = bug_line(bug);
> > +
> > + switch ( id )
> > + {
> > + case BUGFRAME_warn:
> > + printk("Xen WARN at %s:%d\n", filename, lineno);
> > +
> > + show_execution_state(regs);
> > +
> > + goto end;
> > +
> > + case BUGFRAME_bug:
> > + printk("Xen BUG at %s:%d\n", filename, lineno);
> > +
> > + show_execution_state(regs);
> > +
> > + printk("change wait_for_interrupt to panic() when common
> > is available\n");
> > + die();
> > +
> > + case BUGFRAME_assert:
> > + /* ASSERT: decode the predicate string pointer. */
> > + predicate = bug_msg(bug);
> > +
> > + printk("Assertion %s failed at %s:%d\n", predicate,
> > filename, lineno);
> > +
> > + show_execution_state(regs);
> > +
> > + printk("change wait_for_interrupt to panic() when common
> > is available\n");
> > + die();
> > + }
> > +
> > + return -EINVAL;
> > +
> > + end:
> > + return 0;
> > +}
> > +
> > +static bool is_valid_bugaddr(uint32_t insn)
> > +{
> > + return insn == BUG_INSN_32 ||
> > + (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
> > +}
>
> Why "addr" in the name when this takes an insn as argument?
In the earliest patch series it was an address. But now it should be
changed. Thanks.
>
> > +/* Should be used only in Xen code ? */
>
> What is this question about? With ...
I meant that it's not safe to use in guest code.
>
> > +static uint32_t read_instr(unsigned long pc)
> > +{
> > + uint16_t instr16 = *(uint16_t *)pc;
> > +
> > + if ( GET_INSN_LENGTH(instr16) == 2 )
> > + return (uint32_t)instr16;
>
> (I don't think this cast is needed.)
>
> > + else
> > + return *(uint32_t *)pc;
> > +}
>
> ... there still being a double read here, do you perhaps mean to
> make a statement (that this code isn't safe to use on guest code)?
I wonder if it'll be safe to read 16 bytes at a time then we won't have
double read ( if you meant that first 16 bytes are read twice ):
static uint32_t read_instr(unsigned long pc)
{
uint16_t instr16 = *(uint16_t *)pc;
if ( GET_INSN_LENGTH(instr16) == 2 )
return (uint32_t)instr16;
else{
// return *(uint32_t *)pc;
uint16_t next_16 = *((uint16_t *)pc + 1);
return ((uint32_t)instr16 << sizeof(instr16)) + next_16;
}
}
>
> > void do_trap(struct cpu_user_regs *cpu_regs)
> > {
> > + register_t pc = cpu_regs->sepc;
> > + uint32_t instr = read_instr(pc);
> > +
> > + if ( is_valid_bugaddr(instr) )
> > + {
> > + if ( !do_bug_frame(cpu_regs, pc) )
> > + {
> > + cpu_regs->sepc += GET_INSN_LENGTH(instr);
> > + return;
> > + }
> > + }
> > +
> > do_unexpected_trap(cpu_regs);
> > }
>
~ Oleksii
On 08.08.2023 10:48, Oleksii wrote:
> On Mon, 2023-08-07 at 15:29 +0200, Jan Beulich wrote:
>> On 03.08.2023 14:05, Oleksii Kurochko wrote:
>>> +static uint32_t read_instr(unsigned long pc)
>>> +{
>>> + uint16_t instr16 = *(uint16_t *)pc;
>>> +
>>> + if ( GET_INSN_LENGTH(instr16) == 2 )
>>> + return (uint32_t)instr16;
>>
>> (I don't think this cast is needed.)
>>
>>> + else
>>> + return *(uint32_t *)pc;
>>> +}
>>
>> ... there still being a double read here, do you perhaps mean to
>> make a statement (that this code isn't safe to use on guest code)?
> I wonder if it'll be safe to read 16 bytes at a time then we won't have
> double read ( if you meant that first 16 bytes are read twice ):
>
> static uint32_t read_instr(unsigned long pc)
> {
> uint16_t instr16 = *(uint16_t *)pc;
>
> if ( GET_INSN_LENGTH(instr16) == 2 )
> return (uint32_t)instr16;
> else{
> // return *(uint32_t *)pc;
>
> uint16_t next_16 = *((uint16_t *)pc + 1);
> return ((uint32_t)instr16 << sizeof(instr16)) + next_16;
> }
> }
Whether this is safe for guest code depends further on what underlying
mappings there are. Surely you can't simply cast a guest add (coming
in as "unsigned long pc") to a hypervisor address. So as it stands the
function can only ever be used on Xen code anyway.
Jan
© 2016 - 2026 Red Hat, Inc.