Re: [PATCH v11 4/5] xen/riscv: enable GENERIC_BUG_FRAME

[Jan Beulich <jbeulich@xxxxxxxx>]

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