|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 16/39] xen/riscv: extend exception tables with type and data fields
On 2026-09-08 11:19 +0200, Oleksii Kurochko wrote:
>
>
> On 9/7/26 5:57 PM, Baptiste Le Duc wrote:
> >> Extend the RISC-V exception table format to include a type and
> >> auxiliary data field.
> >>
> >> The existing format only supports simple fixups. Some use cases require
> >> additional context from the fault (e.g. capturing trap information),
> >> which cannot be expressed with the current EX_TYPE_FIXUP entries.
> >>
> >> Introduce a generic ASM_EXTABLE_RAW() helper to describe entries with a
> >> handler type and associated data. Reimplement ASM_EXTABLE() in terms of
> >> it using EX_TYPE_FIXUP for compatibility.
> >>
> >> Add EX_TYPE_TRAP_INFO to allow handlers to retrieve trap state
> >> (sepc/scause/stval) and pass it to the fixup path. The data field is
> >> used to encode which GPR contains a pointer to a struct trap_info.
> >>
> >> Provide ASM_EXTABLE_TRAP_INFO() as a convenience wrapper for this case.
> >>
> >> Also add gpr-num.h, providing symbolic GPR numbers for use in assembly
> >> and inline asm. This is derived from Linux 6.16 with minor adjustments such
> >> as using .irp instead of open-coding the same using a set of .equ.
> >>
> >> Update the exception handling code to dispatch based on the entry type.
> >>
> >> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> >>
> >> diff --git a/xen/arch/riscv/extable.c b/xen/arch/riscv/extable.c
> >> index 5b89c4278c..6470198d01 100644
> >> --- a/xen/arch/riscv/extable.c
> >> +++ b/xen/arch/riscv/extable.c
> >> @@ -6,8 +6,10 @@
> >> #include <xen/sort.h>
> >> #include <xen/virtual_region.h>
> >>
> >> +#include <asm/csr.h>
> >> #include <asm/extable.h>
> >> #include <asm/processor.h>
> >> +#include <asm/traps.h>
> >>
> >> #define EX_FIELD(ptr, field) ((unsigned long)&(ptr)->field +
> >> (ptr)->field)
> >>
> >> @@ -32,6 +34,12 @@ static void __init cf_check swap_ex(void *a, void *b)
> >>
> >> x->fixup = y->fixup + delta;
> >> y->fixup = tmp.fixup - delta;
> >> +
> >> + x->type = y->type;
> >> + y->type = tmp.type;
> >> +
> >> + x->data = y->data;
> >> + y->data = tmp.data;
> >> }
> >>
> >> static int cf_check cmp_ex(const void *a, const void *b)
> >> @@ -59,7 +67,49 @@ static void ex_handler_fixup(const struct
> >> exception_table_entry *ex,
> >> regs->sepc = ex_fixup(ex);
> >> }
> >>
> >> -bool fixup_exception(struct cpu_user_regs *regs)
> >> +#define CHECK_GPR_INDEX(num, name) \
> >> + BUILD_BUG_ON(offsetof(struct cpu_user_regs, name) \
> >> + != (num) * sizeof(unsigned long));
> >> +
> >> +static unsigned long regs_get_gpr(const struct cpu_user_regs *regs,
> >> + unsigned int num)
> >> +{
> >> + /*
> >> + * The GPR number -> struct index mapping below relies on x0..x31
> >> being
> >> + * laid out at the start of struct cpu_user_regs in architectural
> >> order,
> >> + * matching the register numbers GPR_LIST() hands to the assembler.
> >> + */
> >> + GPR_LIST(CHECK_GPR_INDEX)
> >> +
> >> + ASSERT(num < 32);
> >> +
> >> + return ((const unsigned long *)regs)[num];
> >> +}
> >> +
> >> +#undef CHECK_GPR_INDEX
> >> +
> >> +static void ex_handler_trap_info(const struct exception_table_entry *ex,
> >> + struct cpu_user_regs *regs,
> >> + unsigned long cause)
> >> +{
> >> + struct trap_info *trap_info =
> >> + (struct trap_info *)regs_get_gpr(regs, ex->data);
> >> +
> >> + BUG_ON(!trap_info);
> >> +
> >> + /*
> >> + * Only stval still needs a CSR read: sepc and scause were already
> >> + * captured by the trap entry path and do_trap() respectively. Latch
> >> + * trap_info->sepc before regs->sepc is pointed at the fixup code.
> >> + */
> >> + trap_info->sepc = regs->sepc;
> >> + trap_info->scause = cause;
> >> + trap_info->stval = csr_read(CSR_STVAL);
> >> +
> >> + regs->sepc = ex_fixup(ex);
> >> +}
> >> +
> >> +bool fixup_exception(struct cpu_user_regs *regs, unsigned long cause)
> >> {
> >> unsigned long pc = regs->sepc;
> >> const struct virtual_region *region = find_text_region(pc);
> >> @@ -77,7 +127,23 @@ bool fixup_exception(struct cpu_user_regs *regs)
> >> if ( !ex )
> >> return false;
> >>
> >> - ex_handler_fixup(ex, regs);
> >> + switch ( ex->type )
> >> + {
> >> + case EX_TYPE_FIXUP:
> >> + ex_handler_fixup(ex, regs);
> >> + break;
> >> +
> >> + case EX_TYPE_TRAP_INFO:
> >> + ex_handler_trap_info(ex, regs, cause);
> >> + break;
> >> +
> >> + default:
> >> + printk(XENLOG_ERR
> >> + "Unsupported exception table entry type %u for pc %#lx\n",
> >> + ex->type, pc);
> >> +
> >> + return false;
> >> + }
> >>
> >> return true;
> >> }
> >> diff --git a/xen/arch/riscv/include/asm/extable.h
> >> b/xen/arch/riscv/include/asm/extable.h
> >> index c0128a9181..7378f86e7e 100644
> >> --- a/xen/arch/riscv/include/asm/extable.h
> >> +++ b/xen/arch/riscv/include/asm/extable.h
> >> @@ -3,17 +3,24 @@
> >> #ifndef ASM__RISCV__ASM_EXTABLE_H
> >> #define ASM__RISCV__ASM_EXTABLE_H
> >>
> >> +#include <asm/gpr-num.h>
> >> +
> >> +#define EX_TYPE_FIXUP 0
> >> +#define EX_TYPE_TRAP_INFO 1
> >> +
> >> #ifdef __ASSEMBLER__
> >>
> >> -#define ASM_EXTABLE(insn, fixup) \
> >> - .pushsection .ex_table, "a"; \
> >> - .balign 4; \
> >> - .word (insn) - .; \
> >> - .word (fixup) - .; \
> >> +#define ASM_EXTABLE_RAW(insn, fixup, type, data) \
> >> + .pushsection .ex_table, "a"; \
> >> + .balign 4; \
> >> + .word (insn) - .; \
> >> + .word (fixup) - .; \
> >> + .half (type); \
> >> + .half (data); \
> >> .popsection
> >>
> >> -.macro asm_extable, insn, fixup
> >> - ASM_EXTABLE(\insn, \fixup)
> >> +.macro _asm_extable, insn, fixup
> >> + ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0)
> >> .endm
> >>
> >> #else /* __ASSEMBLER__ */
> >> @@ -23,20 +30,36 @@
> >>
> >> struct cpu_user_regs;
> >>
> >> -#define ASM_EXTABLE(insn, fixup) \
> >> - ".pushsection .ex_table, \"a\"\n" \
> >> - ".balign 4\n" \
> >> - ".word (" #insn " - .)\n" \
> >> - ".word (" #fixup " - .)\n" \
> >> +#define ASM_EXTABLE_RAW(insn, fixup, type, data) \
> >> + ".pushsection .ex_table, \"a\"\n" \
> >> + ".balign 4\n" \
> >> + ".word (" insn ") - .\n" \
> >> + ".word (" fixup ") - .\n" \
> >> + ".half (" type ")\n" \
> >> + ".half (" data ")\n" \
> >> ".popsection\n"
> >>
> >> +#define ASM_EXTABLE(insn, fixup) \
> >> + ASM_EXTABLE_RAW(#insn, #fixup, __stringify(EX_TYPE_FIXUP), "0")
> >> +
> >> +#define EX_TRAP_INFO_REG(gpr) \
> >> + "(.L_gpr_num_" #gpr ")"
> >> +
> >> +#define ASM_EXTABLE_TRAP_INFO(insn, fixup, data) \
> >> + DEFINE_ASM_GPR_NUMS \
> >> + ASM_EXTABLE_RAW(#insn, #fixup, __stringify(EX_TYPE_TRAP_INFO), \
> >> + EX_TRAP_INFO_REG(data))
> >> +
> >> /*
> >> - * The exception table consists of pairs of relative offsets: the first
> >> - * is the relative offset to an instruction that is allowed to fault,
> >> - * and the second is the relative offset at which the program should
> >> - * continue. No general-purpose registers are modified by the exception
> >> - * handling mechanism itself, so it is up to the fixup code to handle
> >> - * any necessary state cleanup.
> >> + * Each exception table entry consists of two relative offsets and a
> >> + * handler description: `insn` is the relative offset to an instruction
> >> + * that is allowed to fault, `fixup` is the relative offset at which the
> >> + * program should continue, `type` selects how the exception is handled
> >> + * (EX_TYPE_*), and `data` holds auxiliary information for the handler
> >> + * (e.g. for EX_TYPE_TRAP_INFO, the number of the GPR that contains a
> >> + * pointer to a struct trap_info). No general-purpose registers are
> >> + * modified by the exception handling mechanism itself, so it is up to
> >> + * the fixup code to handle any necessary state cleanup.
> >> *
> >> * The exception table and fixup code live out of line with the main
> >> * instruction path. This means when everything is well, we don't even
> >> @@ -45,14 +68,15 @@ struct cpu_user_regs;
> >> */
> >> struct exception_table_entry {
> >> int32_t insn, fixup;
> >> + uint16_t type, data;
> >> };
> >>
> >> extern struct exception_table_entry __start___ex_table[];
> >> extern struct exception_table_entry __stop___ex_table[];
> >>
> >> void sort_exception_tables(void);
> >> -bool fixup_exception(struct cpu_user_regs *regs);
> >> +bool fixup_exception(struct cpu_user_regs *regs, unsigned long cause);
> >>
> >> -#endif /* __ASSEMBLY__ */
> >> +#endif /* __ASSEMBLER__ */
> >>
> >> #endif /* ASM__RISCV__ASM_EXTABLE_H */
> >> diff --git a/xen/arch/riscv/include/asm/gpr-num.h
> >> b/xen/arch/riscv/include/asm/gpr-num.h
> >> new file mode 100644
> >> index 0000000000..3b97a72e6c
> >> --- /dev/null
> >> +++ b/xen/arch/riscv/include/asm/gpr-num.h
> >> @@ -0,0 +1,37 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +#ifndef RISCV_GPR_NUM_H
> >> +#define RISCV_GPR_NUM_H
> > Nit: commit message says this is derived from Linux 6.16. Other
> > imported RISC-V headers here carry an in-file note (bitops.h: "Based on
> > linux/arch/.../bitops.h") but this file doesn't.
> >> +/*
> >> + * GPRs by ABI name, together with their register number (x0 .. x31).
> >> + *
> >> + * This is the single source of truth for the mapping: it generates the
> >> + * .L_gpr_num_<name> assembler symbols used to turn a register name
> >> emitted
> >> + * by the compiler into a register number, and struct cpu_user_regs is
> >> + * checked against it at build time (see regs_get_gpr()). Neither list can
> >> + * therefore be changed without the other.
> >> + */
> >> +#define GPR_LIST(x) \
> >> + x(0, zero) x(1, ra) x(2, sp) x(3, gp) \
> >> + x(4, tp) x(5, t0) x(6, t1) x(7, t2) \
> >> + x(8, s0) x(9, s1) x(10, a0) x(11, a1) \
> >> + x(12, a2) x(13, a3) x(14, a4) x(15, a5) \
> >> + x(16, a6) x(17, a7) x(18, s2) x(19, s3) \
> >> + x(20, s4) x(21, s5) x(22, s6) x(23, s7) \
> >> + x(24, s8) x(25, s9) x(26, s10) x(27, s11) \
> >> + x(28, t3) x(29, t4) x(30, t5) x(31, t6)
> >> +
> >> +#ifdef __ASSEMBLER__
> >> +
> >> +#define GPR_NUM_EQU(num, name) .equ .L_gpr_num_##name, num;
> >> +GPR_LIST(GPR_NUM_EQU)
> >> +#undef GPR_NUM_EQU
> >> +
> >> +#else /* __ASSEMBLER__ */
> >> +
> >> +#define GPR_NUM_EQU(num, name) ".equ .L_gpr_num_" #name ", " #num "\n"
> >> +#define DEFINE_ASM_GPR_NUMS GPR_LIST(GPR_NUM_EQU)
> >> +
> >> +#endif /* __ASSEMBLER__ */
> >> +
> >> +#endif /* RISCV_GPR_NUM_H */
> >> diff --git a/xen/arch/riscv/include/asm/processor.h
> >> b/xen/arch/riscv/include/asm/processor.h
> >> index b1745c1071..e7b0f2321a 100644
> >> --- a/xen/arch/riscv/include/asm/processor.h
> >> +++ b/xen/arch/riscv/include/asm/processor.h
> >> @@ -12,7 +12,19 @@
> >>
> >> #ifndef __ASSEMBLER__
> >>
> >> -/* On stack VCPU state */
> >> +/*
> >> + * On stack VCPU state.
> >> + *
> >> + * x0..x31 must remain at the start of this structure, in architectural
> >> + * register-number order: code which resolves a register number to its
> >> saved
> >> + * value indexes this structure directly (instruction emulation via
> >> + * REG_PTR() from asm/riscv_encoding.h, exception table fixups via
> >> + * regs_get_gpr()). ->zero therefore has to stay at offset 0 and must
> >> always
> >> + * read as 0, since it supplies the value of x0 when x0 is used as a
> >> source
> >> + * operand. The layout is checked against GPR_LIST() at build time; see
> >> + * regs_get_gpr() in extable.c. Do not reorder these fields or insert
> >> + * anything between them.
> >> + */
> >> {
> >> unsigned long zero;
> > Comment claims ->zero "must always read as 0" as it's hard-wired to zero
> > by the HW, but nothing enforces that, it's still a plain writable
> > unsigned long. Maybe a write-side counterpart that special-cases num==0
> > as a no-op, or with a minimum ASSERT(num != 0) / BUG_ON(num == 0) to
> > anticipate any future forbidden writes.
> >
>
> Could you please clarify where do you want me to put this check in this
> patch? In regs_get_gpr()? There is no write-side in this patch. Am i
> missing something?
Honestly, I don't know where it would be most appropriate. But I'd like
to highlight that nothing strictly forbids Xen code from writing a value
!= 0 into regs->zero.
I saw that in emulate_load(), which is introduced later, you used a
branch to avoid an incorrect load into regs->zero:
/*
* A load into x0 discards its result: writing regs->zero would break
* the invariant that it reads as zero when x0 is a source operand
* elsewhere.
*/
One solution could be to make cpu_user_regs private and add get/set
methods, with a check in the setter that forbids this type of write, but
that would imply some big changes, so it may not be the appropriate fix.
>
>
> Thanks in advance.
>
> ~ Oleksii
>
>
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |