Re: [PATCH v1 12/17] xen/riscv: extend exception tables with type and data fields

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.07.2026 18:02, Oleksii Kurochko wrote:
> @@ -60,6 +68,40 @@ static void ex_handler_fixup(const struct 
> exception_table_entry *ex,
>      regs->sepc = ex_fixup(ex);
>  }
>  
> +static inline unsigned long regs_get_gpr(struct cpu_user_regs *regs,
> +                                         unsigned int offset)
> +{
> +    /*
> +     * The GPR number -> offset arithmetic below relies on x0..x31 being
> +     * laid out at the start of struct cpu_user_regs in architectural
> +     * order.
> +     */
> +    BUILD_BUG_ON(offsetof(struct cpu_user_regs, ra) !=
> +                 sizeof(unsigned long));
> +    BUILD_BUG_ON(offsetof(struct cpu_user_regs, t6) !=
> +                 31 * sizeof(unsigned long));
> +
> +    if ( unlikely(!offset || (offset > MAX_REG_OFFSET)) )
> +        return 0;

And an offset not divisible by sizeof(unsigned long) is okay?

Returning 0 as error indicator also feels fragile.

> +    return *(unsigned long *)((unsigned long)regs + offset);
> +}
> +
> +static void ex_handler_trap_info(const struct exception_table_entry *ex,
> +                                 struct cpu_user_regs *regs)
> +{
> +    struct trap_info *trap_info =
> +        (struct trap_info *)regs_get_gpr(regs, ex->data * sizeof(unsigned 
> long));

Related to the earlier comment: Simply pass just ex->data here, leaving the
multiplication to regs_get_gpr()?

> +    BUG_ON(!trap_info);
> +
> +    trap_info->sepc = csr_read(CSR_SEPC);
> +    trap_info->scause = csr_read(CSR_SCAUSE);
> +    trap_info->stval = csr_read(CSR_STVAL);

Do you really need to re-read all three registers here? Didn't you read at least
scause already, in order to make it here in the first place?

> --- 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) - .;     \
> -    .popsection
> +#define ASM_EXTABLE_RAW(insn, fixup, type, data)    \
> +    .pushsection .ex_table, "a";                    \
> +    .balign     4;                                  \
> +    .long       ((insn) - .);                       \
> +    .long       ((fixup) - .);                      \

Why the change from .word to .long? And why the extra pairs of parens?

> +    .short      (type);                             \
> +    .short      (data);                             \

Alongside .word, these then likely want to be .half.

> @@ -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"                                \
> +    ".long      ((" insn ") - .)\n"                 \
> +    ".long      ((" fixup ") - .)\n"                \

Same questions here then.

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/gpr-num.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef RISCV_GPR_NUM_H
> +#define RISCV_GPR_NUM_H
> +
> +/* GPR ABI names, in register-number order (x0 .. x31). */
> +#define GPR_ABI_NAMES                   \
> +    zero, ra, sp, gp, tp, t0, t1, t2,   \
> +    s0, s1, a0, a1, a2, a3, a4, a5,     \
> +    a6, a7, s2, s3, s4, s5, s6, s7,     \
> +    s8, s9, s10, s11, t3, t4, t5, t6
> +
> +#ifdef __ASSEMBLER__
> +
> +    .equ    .L_gpr_num, 0
> +    .irp    name, GPR_ABI_NAMES
> +    .equ    .L_gpr_num_\name, .L_gpr_num
> +    .equ    .L_gpr_num, .L_gpr_num + 1
> +    .endr

So this is emitted no matter whether a .S file actually uses any of the 
constants.
Perhaps okayish, but somewhat wasteful.

Jan