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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.08.2026 11:14, Oleksii Kurochko wrote:
> 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.

But SET_RD() will need to avoid touching .zero anyway. Why waste the space,
when something useful can be put there?

Jan