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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/9/26 2:22 PM, Jan Beulich wrote:
> 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).
It is worthier. I will follow then your original suggestion and use 
array_index_nospec().
~ Oleksii