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.
> + 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);
> }
