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

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