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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.08.2026 11:40, Oleksii Kurochko wrote:
> On 8/18/26 11:26 AM, Jan Beulich wrote:
>> 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?
> 
> For SET_RD() agree, it doesn't make sense. Bad example. But it could be 
> somehow a "protection" to not clobber something useful if SET_RD 
> arguments won't handled correctly.
> 
> SET_RD() is only half of it. The read accessors matter more: sd x0, 
> 0(a0) to an MMIO address is the normal way a guest writes 0 to a device 
> register, and emulate_store() fetches the operand with GET_RS2(), which 
> is a plain index by register number.

Oh, wow, how fragile. Without a bright comment in struct cpu_user_regs, one
should be permitted to move fields around or insert new ones at arbitrary
positions. That comment would then also help clarify why "zero" is there as
a field.

> If something useful lived at offset 
> 0, that store would hand the device that value instead of zero. So 
> whatever we put there would have to survive being read as a source 
> operand and being clobbered by SET_RD(), which means nothing can go there.

But you can't use it as (reliable zero) source when you allow SET_RD() to
clobber it.

Jan