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

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

~ Oleksii