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