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

[oleksii.kurochko@xxxxxxxxx]

On Mon, 2024-07-29 at 15:00 +0200, Jan Beulich wrote:
> 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.
I am using GDB now and everything working well.

> 
> > @@ -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?
It makes sense to me. Thanks.

~ Oleksii