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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/12/26 4:37 PM, Jan Beulich wrote:
> 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?
No, it isn't okay. I will apply your comment ...

> Returning 0 as error indicator also feels fragile.
With what I suggested below returning could be just dropped.

> > +    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()?
... It would be better to move the multiplication inside regs_get_gpr().

Your comment made me think about whether the multiplication is needed at 
all (regardless of where it is done). In other words, ex->data contains 
the register number, so we could just write:
static unsigned long regs_get_gpr(const struct cpu_user_regs *regs,
                                  unsigned int num)
{
    /*
     * 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));
    ASSERT(num && (num < 32));

    return ((const unsigned long *)regs)[num];
}

Probably, we want to consider this function out of context (for now 
context is that we use it to recieve a pointer to trap_info which can't 
be obviously stored in x0 as it should be always hardwired zero). In 
that case, there is no need to check that num is 0.
So, it probably makes sense to just have:
  ASSERT(num < 32);

ASSERT() is fine here as I don't think that compiler will use incorrect 
number during register allocation.
> > +    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?
Agree, ->scause and ->sepc are already read.


> > --- 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?
I don't see any sense now in changing type and of extra pairs of parens.
This part of changes will be reverted.

> > +    .short      (type);                             \
> > +    .short      (data);                             \
> Alongside .word, these then likely want to be .half.
.half will be better if .word is used.

> > @@ -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.
I will revert these changes too.

> > --- /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.
I can move #include <asm/gpr-num.h> inside "#else /* __ASSEMBLER__ */" 
in asm/extable.h and it will be enough for now. Or just drop declaration 
of .L_gpr_num for assembler code until it will be needed by it.
~ Oleksii