On 24.07.2024 17:31, Oleksii Kurochko wrote:
> To have working BUG(), WARN(), ASSERT, run_in_exception_handler()
> it is needed to enable GENERIC_BUG_FRAME.
>
> ebreak is used as BUG_insn so when macros from <xen/bug.h> are
> used, an exception with BREAKPOINT cause will be generated.
>
> ebreak triggers a debug trap, which starts in debug mode and is
> then filtered by every mode. A debugger will first handle the
> debug trap and check if ebreak was set by it or not. If not,
> CAUSE_BREAKPOINT will be generated for the mode where the ebreak
> instruction was executed.
Here and in the similar code comment you talk about an external
debugger, requiring debug mode, which is an optional feature. In
my earlier comments what I was referring to was pure software
debugging. I continue to fail to see how properly distinguishing
ebreak uses for breakpoints from ebreak uses for e.g. BUG() and
WARN() is going to be feasible.
> @@ -103,7 +104,39 @@ 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 )
> + {
> + /*
> + * ebreak is used as BUG_insn so when macros from <xen/bug.h> are
> + * used, an exception with BREAKPOINT cause will be generated.
> + *
> + * ebreak triggers a debug trap, which starts in debug mode and is
> + * then filtered by every mode. A debugger will first handle the
> + * debug trap and check if ebreak was set by it or not. If not,
> + * CAUSE_BREAKPOINT will be generated for the mode where the ebreak
> + * instruction was executed.
> + */
> + case CAUSE_BREAKPOINT:
> + if ( do_bug_frame(cpu_regs, pc) >= 0 )
> + {
> + if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) )
> + {
> + printk("Something wrong with PC: %#lx\n", pc);
> + die();
> + }
> +
> + cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
> + }
> +
> + break;
Wouldn't you better fall through here, with the "break" moved into
the if()'s body?
Jan
> + default:
> + do_unexpected_trap(cpu_regs);
> + break;
> + }
> }
