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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/20/26 4:59 PM, Baptiste Le Duc wrote:


> >   
> >   static void __init __maybe_unused build_assertions(void)
> >   {
> > +/*
> > + * GPR_LIST() puts nothing between its entries, so the separator has to come
> > + * from here. It is emitted ahead of each entry rather than after it, so that
> > + * the expansion doesn't end with one and its use below can be terminated like
> > + * an ordinary statement.
> > + */
> > +#define CHECK_GPR_INDEX(num, name)                          \
> > +    ; BUILD_BUG_ON(offsetof(struct cpu_user_regs, name) !=  \
> > +                   offsetof(struct cpu_user_regs, zero) +   \
> > +                   (num) * sizeof(unsigned long))
> > +
> >       /*
> >        * Enforce the requirement documented in struct cpu_info that
> >        * guest_cpu_user_regs must be the first field.
> >        */
> >       BUILD_BUG_ON(offsetof(struct cpu_info, guest_cpu_user_regs));
> > +
> > +    /*
> > +     * Enforce the requirement documented in struct cpu_user_regs that the
> > +     * x0..x31 fields are contiguous and in architectural register-number
> > +     * order, which is what lets a register number be resolved to its saved
> > +     * value by indexing them as an array anchored at ->zero.
> > +     */
> > +    GPR_LIST(CHECK_GPR_INDEX);
> > +
> > +#undef CHECK_GPR_INDEX
> >   }

> > 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..867ee172c0
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/gpr-num.h
> > @@ -0,0 +1,53 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Based on Linux 6.16 arch/riscv/include/asm/gpr-num.h, with the
> > + * register list turned into GPR_LIST() instead of being open-coded
> > + * with .irp, so that it can also be used outside of assembly.
> > + */
> > +#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; users expand it with
> > + * their own per-register macro. Among them, the layout of struct cpu_user_regs
> > + * is checked against this list at build time (see build_assertions() in
> > + * domain.c), so neither list can 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)
> > +
> > +/* Number of GPRs described by GPR_LIST(). */
> > +#define NR_GPRS 32
> > +
> > +#ifdef __ASSEMBLER__
> > +
> > +/*
> > + * Generate the .L_gpr_num_<name> assembler symbols, used to turn a register
> > + * name emitted by the compiler into a register number.
> > + *
> > + * The separator is emitted ahead of each entry rather than after it, so that
> > + * the expansion doesn't end with one: whatever follows a use of the list has
> > + * to supply its own separator, instead of silently relying on a trailing one.
> > + */
> > +#define GPR_NUM_EQU(num, name)  ; .equ .L_gpr_num_##name, num
> > +GPR_LIST(GPR_NUM_EQU)
> > +#undef GPR_NUM_EQU
> > +
> > +#else /* __ASSEMBLER__ */
> > +
> > +/* See the comment ahead of the __ASSEMBLER__ flavour above. */
> It's hard to understand which comment you're talking about, if you are
> talking about the separator comment above GPR_NUM_EQU, may I
> suggest something like:
>
>      /* See the GPR_NUM_EQU()'s comment about separator use */
Agree, I will apply this suggestion. Maybe a little but more precise:

/* See the comment above the assembler GPR_NUM_EQU() about the separator. */

> > +#define GPR_NUM_EQU(num, name)  "\n.equ .L_gpr_num_" #name ", " #num
> > +#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/riscv_encoding.h 
> > b/xen/arch/riscv/include/asm/riscv_encoding.h
> > index 647268e6d0..599ae3cd60 100644
> > --- a/xen/arch/riscv/include/asm/riscv_encoding.h
> > +++ b/xen/arch/riscv/include/asm/riscv_encoding.h
> > @@ -911,8 +911,13 @@
> >   #define REG_OFFSET(insn, pos)         \
> >         (SHIFT_RIGHT((insn), (pos) - LOG_REGBYTES) & REG_MASK)
> >   
> > +/*
> > + * The x0..x31 block of struct cpu_user_regs is not required to sit at the
> > + * start of the structure, so anchor the offset at ->zero (x0) rather than
> > + * at the structure itself.
> > + */
> >   #define REG_PTR(insn, pos, regs)      \
> > -       (unsigned long *)((unsigned long)(regs) + REG_OFFSET(insn, pos))
> > +       (unsigned long *)((unsigned long)&(regs)->zero + REG_OFFSET(insn, pos))
> I don't see why this hunk is needed here. Nothing in the tree uses REG_PTR()
> (nor GET_RS*() / SET_RD()), and ->zero is the first member of struct
> cpu_user_regs, so this is a no-op today. It is also unrelated to the extable
> change.
>
> If you want REG_PTR() to follow the same layout assumption as regs_gpr_ptr(),
> I'd rather see it built on top of it, e.g. regs_gpr_ptr(regs, REG_OFFSET(insn,
> pos) >> LOG_REGBYTES), in a separate patch together with its first user.
> Otherwise please drop the hunk (and the matching sentence in the commit
> message).
I'd rather keep it here. It isn't a standalone change waiting for a 
user: it follows from this patch relaxing the layout requirement. Until 
now the requirement wasn't written down anywhere, and REG_PTR() 
implicitly relied on the stricter form of it, x0..x31 starting at offset 
0 of struct cpu_user_regs. This patch documents the requirement as 
"contiguous and in register-number order, anchored at ->zero", with the 
BUILD_BUG_ON()s anchored the same way (look at build_assertions() 
above). Leaving REG_PTR() as is would make that comment untrue for the 
one other piece of code in the tree that resolves a register number to 
its saved value. Moving the block would then still build, and REG_PTR() 
would silently compute wrong addresses. It's a no-op today simply 
because the block still sits at the start of the structure.
~ Oleksii