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

[Jan Beulich <jbeulich@xxxxxxxx>]

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? (And tangentially,
what's the pregs field there, and what is stack_cpu_regs?)

>>> --- /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.

>>> +#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_*.

Jan