On 12.07.2024 18:18, Oleksii Kurochko wrote:
> To have working BUG(), WARN(), ASSERT, run_in_exception_handler()
> it is needed to enable GENERIC_BUG_FRAME.
>
> Also, <xen/lib.h> is needed to be included for the reason that panic() and
> printk() are used in common/bug.c and RISC-V fails if it is not included
> with the following errors:
> common/bug.c:69:9: error: implicit declaration of function 'printk'
> [-Werror=implicit-function-declaration]
> 69 | printk("Xen WARN at %s%s:%d\n", prefix, filename,
> lineno);
> | ^~~~~~
> common/bug.c:77:9: error: implicit declaration of function 'panic'
> [-Werror=implicit-function-declaration]
> 77 | panic("Xen BUG at %s%s:%d\n", prefix, filename,
> lineno);
I don't think the diagnostics themselves are needed here.
> Signed-off-by: Oleksii Kurochko <[email protected]>
> ---
> Changes in V10:
> - put 'select GENERIC_BUG_FRAME' in "Config RISCV".
> - rework do_trap() to not fetch an instruction in case when the cause of trap
> is BUG_insn.
It's BUG_insn here, but then ...
> @@ -103,7 +104,29 @@ static void do_unexpected_trap(const struct
> cpu_user_regs *regs)
>
> void do_trap(struct cpu_user_regs *cpu_regs)
> {
> - do_unexpected_trap(cpu_regs);
> + register_t pc = cpu_regs->sepc;
> + unsigned long cause = csr_read(CSR_SCAUSE);
> +
> + switch ( cause )
> + {
> + case CAUSE_BREAKPOINT:
... BREAKPOINT here? Generally I'd deem something named "breakpoint" as
debugging related (and hence continuable). I'd have expected
CAUSE_ILLEGAL_INSTRUCTION here, but likely I'm missing something.
> + if ( do_bug_frame(cpu_regs, pc) >= 0 )
> + {
> + if ( !pc ||
In how far does this really need special casing? Isn't that case covered by
> + !(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1)) )
... these checks anyway? And btw, why the "+ 1" in both function arguments?
> + {
> + printk("Something wrong with PC: 0x%lx\n", pc);
Nit: %#lx please in situations like this.
> + die();
> + }
> +
> + cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
> + return;
This isn't needed, is it? You'd return anyway by ...
> + }
> +
> + break;
.... going through here to ...
> + default:
> + do_unexpected_trap(cpu_regs);
> + }
> }
... here.
Two further nits for the default case: Please have a break statement
there as well, and please have a blank line immediately up from it.
Jan
