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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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