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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/18/26 9:56 AM, Jan Beulich wrote:
> On 17.08.2026 13:33, Oleksii Kurochko wrote:
> > 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.
> I agree.
>
> However, the x0 aspect is still odd. Why again is it that struct cpu_user_regs
> has a field for it, when the register value is always 0? 
zero field isn't there to hold a value, it's there so the first 32 slots 
form an x0..x31 array indexed by GPR number. It is useful for SET_RD() 
implementation, for example.
> (And tangentially,
> what's the pregs field there, and what is stack_cpu_regs?)
It is rudiment, I don't use it anymore.

I will drop it in separate patch.

> > > > --- /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
> Having looked at struct cpu_user_regs for the response above: How is this
> macro intended to be kept in sync with struct cpu_user_regs? Yes, the ABI
> isn't going to change, but (a) still and (b) if later another ABI was
> introduced, names here and fields there could still easily diverge.
Yes, this macro should be in sync with struct cpu_user_regs too.

Then it is needed to turn GPR_ABI_NAMES into a numbered X-macro list and 
generate everything from it:

/* asm/gpr-num.h */
/*
 * GPRs in register-number order (x0 .. x31), by ABI name.  Single 
source of
 * truth: generates the .L_gpr_num_* assembler symbols, and is 
cross-checked
 * against struct cpu_user_regs at build time (see regs_get_gpr()).
 */
#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)

#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

and then also:


/* extable.c, replacing the two existing BUILD_BUG_ONs */
#define CHECK_GPR_OFFSET(num, name)                     \
    BUILD_BUG_ON(offsetof(struct cpu_user_regs, name)   \
                 != (num) * sizeof(unsigned long));

static inline unsigned long regs_get_gpr(struct cpu_user_regs *regs,
                                         unsigned int offset)
{
    /* GPR number N must be field N of struct cpu_user_regs. */
    GPR_LIST(CHECK_GPR_OFFSET)
    ...
}

> > > > +#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.
> How would either of these address the remark I made? Not every .S file
> including asm/extable.h will need these constants. Imo this new file wants
> strictly only including by files which actually need .L_gpr_num_*.
Oh, now I got your idea. There is no need to icnlude asm/gpr-num.h 
inside <asm/extable.h>. It seems to me then it will be better to follow 
the way which was intrdouced originally just have asm/gpr-num.h included 
at the top of asm/extable.h, this is not a big price for .S file which 
including asm/extrable.h and doesn't really need asm/gpr-num.h.
~ Oleksii