Re: [PATCH v2 16/39] xen/riscv: extend exception tables with type and data fields

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.09.2026 13:20, Oleksii Kurochko wrote:
> On 9/8/26 3:44 PM, Jan Beulich wrote:
>> On 27.08.2026 17:21, Oleksii Kurochko wrote:
>>> @@ -59,7 +67,49 @@ static void ex_handler_fixup(const struct 
>>> exception_table_entry *ex,
>>>       regs->sepc = ex_fixup(ex);
>>>   }
>>>   
>>> -bool fixup_exception(struct cpu_user_regs *regs)
>>> +#define CHECK_GPR_INDEX(num, name)                      \
>>> +    BUILD_BUG_ON(offsetof(struct cpu_user_regs, name)   \
>>> +                 != (num) * sizeof(unsigned long));
>>
>> Nit: Placement of the !=. Also there should be no semicolon here; it wants
>> to ...
>>
>>> +static unsigned long regs_get_gpr(const struct cpu_user_regs *regs,
>>> +                                  unsigned int num)
>>> +{
>>> +    /*
>>> +     * The GPR number -> struct index mapping below relies on x0..x31 being
>>> +     * laid out at the start of struct cpu_user_regs in architectural 
>>> order,
>>> +     * matching the register numbers GPR_LIST() hands to the assembler.
>>> +     */
>>> +    GPR_LIST(CHECK_GPR_INDEX)
>>
>> ... appear here instead, for this to actually look like a statement.
> 
> I will apply that.
> 
>>
>>> +    ASSERT(num < 32);
>>> +
>>> +    return ((const unsigned long *)regs)[num];
>>
>> What about release builds? You'd happily overrun the array there. Maybe
>> (ab)use array_index_nospec() here?
> 
> num is coming not from guest, not from calculation in runtume, it is 
> generated by assembler at the build time. So it should be always correct.
> 
> So just having the following looks okay to me:
> 
> static unsigned long regs_get_gpr(const struct cpu_user_regs *regs,
>                                    unsigned int num)
> {
> #define CHECK_GPR_INDEX(num, name) \
>      BUILD_BUG_ON(offsetof(struct cpu_user_regs, name) != \
>                   (num) * sizeof(unsigned long))
> 
> #define GPR_CASE(nr, name) case nr: return regs->name;
> 
>      /*
>       * The GPR number -> struct index mapping below relies on x0..x31 being
>       * laid out at the start of struct cpu_user_regs in architectural 
> order,
>       * matching the register numbers GPR_LIST() hands to the assembler.
>       */
>      GPR_LIST(CHECK_GPR_INDEX);
> 
> #undef CHECK_GPR_INDEX
> 
>      switch ( num )
>      {
>      GPR_LIST(GPR_CASE)
>      }
> 
> #undef GPR_CASE
> 
>      ASSERT_UNREACHABLE();
> 
>      return 0;
> }
> 
> Any thoughts on that regard?

Depends very much on how efficiently the compiler translates this (as opposed
to the other variant).

Jan