Re: [PATCH v2 16/39] xen/riscv: extend exception tables with type and data fields

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.08.2026 17:21, Oleksii Kurochko wrote:
> @@ -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));

Nit: Placement of the !=. Also there should be no semicolon here; it wants
to ...

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

... appear here instead, for this to actually look like a statement.

> +    ASSERT(num < 32);
> +
> +    return ((const unsigned long *)regs)[num];

What about release builds? You'd happily overrun the array there. Maybe
(ab)use array_index_nospec() here?

> +}
> +
> +#undef CHECK_GPR_INDEX

If the sole use of the macro is in a single function, it wants #define-ing
(and #undef-ing) there, not outside of it.

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

This feels extremely weak. As you're fetching from a GPR, the majority of
possible values stored in GPRs is going to be invalid, not just NULL. And
while the accesses below would trap on NULL anyway, whether other bogus
values would trap is pretty hard to predict. If trap_info is expected to
always live on the stack, why not check for that (perhaps also check that
the low few bits are clear)?

> @@ -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 ")"

This doesn't need to be wrapped across lines, does it?

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

There's no visible statement separator between DEFINE_ASM_GPR_NUMS and
ASM_EXTABLE_RAW(), which only works because DEFINE_ASM_GPR_NUMS appends
a separator also at the very end of its expansion. I think that better
would be changed, such that at use sites such as this one a separator
becomes mandatory.

>  /*
> - * 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,

"... in case of a fault, ..."

> --- /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
> +
> +/*
> + * GPRs by ABI name, together with their register number (x0 .. x31).
> + *
> + * This is the single source of truth for the mapping:

True until here, but ...

> it generates the
> + * .L_gpr_num_<name> assembler symbols

... no, it doesn't. It's ...

> 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

... this construct which does. This is relevant to separate, since
GPR_LIST() is also used elsewhere.

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

I understand that the order need retaining. But why would it being at the
start of the struct be (overly) relevant? You could use the "zero" field
as the anchor for calculations.

Jan