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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2024-07-22 at 13:02 +0200, Jan Beulich wrote:
> 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 <oleksii.kurochko@xxxxxxxxx>
> > ---
> > 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.
Agree, that is is confusing, but BUG_insn is defined as ebreak
instruction ( Linux kernel uses also ebreak ) and it generates
CAUSE_BREAKPOINT.

> 
> > +        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?
Good point. We could drop it.

> And btw, why the "+ 1" in both function arguments?
There is no need for them anymore, just missed to drop +1.

~ Oleksii

> 
> > +            {
> > +                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