|
[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
> 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.
--
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |