Re: [PATCH v12 1/3] xen/riscv: enable GENERIC_BUG_FRAME

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.08.2024 15:54, Oleksii Kurochko wrote:
> Enable GENERIC_BUG_FRAME to support BUG(), WARN(), ASSERT,
> and run_in_exception_handler().
> 
> The 0x0000 opcode is used for BUG_INSTR, which, when macros from
> <xen/bug.h> are used, triggers an exception with the
> ILLEGAL_INSTRUCTION cause.
> This opcode is encoded as a 2-byte instruction and is invalid if
> CONFIG_RISCV_ISA_C is enabled or not.

Yes, but there's a caveat: Without the C extension instructions have
to be aligned on 32-bit boundaries. You can't just go and insert a
16-bit item there. When RISCV_ISA_C is not set, I think you want to
insert two such 16-bit zeroes. Beware of an alignment handling bug
in the assembler - don't think of using an alignment directive here.

> Update the commit above the definition of INS_LENGTH_MASK as ebreak

s/commit/comment/?

> --- a/xen/arch/riscv/include/asm/bug.h
> +++ b/xen/arch/riscv/include/asm/bug.h
> @@ -9,7 +9,11 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define BUG_INSTR "ebreak"
> +#include <xen/stringify.h>
> +
> +#define BUG_OPCODE  0x0000

You don't really use this other than ...

> +#define BUG_INSTR ".hword " __stringify(BUG_OPCODE)

... here - does this really warrant a separate #define _and_ inclusion of
stringify.h?

Furthermore you want to avoid using .hword (or any data generating
directive), to avoid disturbing disassembly. Please use .insn if at all
possible. I understand though that in certain cases you won't be able to
use .insn. Yet for the common case (more recent binutils) you'd still
better avoid .hword or alike, imo.

> @@ -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_ILLEGAL_INSTRUCTION:
> +        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;
> +        }
> +
> +    default:

The falling-through here wants annotating, preferably with the pseudo-
keyword.

Jan

> +        do_unexpected_trap(cpu_regs);
> +        break;
> +    }
>  }