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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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?
  >> +}
>> +
>> +#undef CHECK_GPR_INDEX
> 
> If the sole use of the macro is in a single function, it wants #define-ing
> (and #undef-ing) there, not outside of it.
> 

I will move inside.

>> +static void ex_handler_trap_info(const struct exception_table_entry *ex,
>> +                                 struct cpu_user_regs *regs,
>> +                                 unsigned long cause)
>> +{
>> +    struct trap_info *trap_info =
>> +        (struct trap_info *)regs_get_gpr(regs, ex->data);
>> +
>> +    BUG_ON(!trap_info);
> 
> This feels extremely weak. As you're fetching from a GPR, the majority of
> possible values stored in GPRs is going to be invalid, not just NULL. And
> while the accesses below would trap on NULL anyway, whether other bogus
> values would trap is pretty hard to predict. If trap_info is expected to
> always live on the stack, why not check for that (perhaps also check that
> the low few bits are clear)?
> 

Considering the nature if how trap_info is filled I think we could just 
drop BUG_ON(), it is guaranteed by compilation that trap_info will be 
correct.

I think it could be also hard to force trap_info be always allocated on 
the stack.

>> @@ -23,20 +30,36 @@
>>   
>>   struct cpu_user_regs;
>>   
>> -#define ASM_EXTABLE(insn, fixup)      \
>> -    ".pushsection .ex_table, \"a\"\n" \
>> -    ".balign    4\n"                  \
>> -    ".word      (" #insn " - .)\n"    \
>> -    ".word      (" #fixup " - .)\n"   \
>> +#define ASM_EXTABLE_RAW(insn, fixup, type, data)    \
>> +    ".pushsection .ex_table, \"a\"\n"               \
>> +    ".balign    4\n"                                \
>> +    ".word      (" insn ") - .\n"                   \
>> +    ".word      (" fixup ") - .\n"                  \
>> +    ".half      (" type ")\n"                       \
>> +    ".half      (" data ")\n"                       \
>>       ".popsection\n"
>>   
>> +#define ASM_EXTABLE(insn, fixup)    \
>> +    ASM_EXTABLE_RAW(#insn, #fixup, __stringify(EX_TYPE_FIXUP), "0")
>> +
>> +#define EX_TRAP_INFO_REG(gpr)   \
>> +    "(.L_gpr_num_" #gpr ")"
> 
> This doesn't need to be wrapped across lines, does it?

Oh, really, it could be one line.

> 
>> +#define ASM_EXTABLE_TRAP_INFO(insn, fixup, data)                    \
>> +    DEFINE_ASM_GPR_NUMS                                             \
>> +    ASM_EXTABLE_RAW(#insn, #fixup, __stringify(EX_TYPE_TRAP_INFO),  \
>> +                    EX_TRAP_INFO_REG(data))
> 
> There's no visible statement separator between DEFINE_ASM_GPR_NUMS and
> ASM_EXTABLE_RAW(), which only works because DEFINE_ASM_GPR_NUMS appends
> a separator also at the very end of its expansion. I think that better
> would be changed, such that at use sites such as this one a separator
> becomes mandatory.

I will do the following then:

  #define ASM_EXTABLE_TRAP_INFO(insn, fixup, data)                    \
-    DEFINE_ASM_GPR_NUMS                                             \
+    DEFINE_ASM_GPR_NUMS "\n"                                        \
      ASM_EXTABLE_RAW(#insn, #fixup, __stringify(EX_TYPE_TRAP_INFO),  \
                      EX_TRAP_INFO_REG(data))

diff --git a/xen/arch/riscv/include/asm/gpr-num.h 
b/xen/arch/riscv/include/asm/gpr-num.h
index 3b97a72e6c30..d497bd501e87 100644
--- a/xen/arch/riscv/include/asm/gpr-num.h
+++ b/xen/arch/riscv/include/asm/gpr-num.h
@@ -23,13 +23,19 @@

  #ifdef __ASSEMBLER__

-#define GPR_NUM_EQU(num, name)  .equ .L_gpr_num_##name, num;
+/*
+ * The separator is emitted ahead of each entry rather than after it, 
so that
+ * the expansion doesn't end with one: whatever follows a use of the 
list has
+ * to supply its own separator, instead of silently relying on a 
trailing one.
+ */
+#define GPR_NUM_EQU(num, name)  ; .equ .L_gpr_num_##name, num
  GPR_LIST(GPR_NUM_EQU)
  #undef GPR_NUM_EQU

  #else /* __ASSEMBLER__ */

-#define GPR_NUM_EQU(num, name)  ".equ .L_gpr_num_" #name ", " #num "\n"
+/* See the comment ahead of the __ASSEMBLER__ flavour above. */
+#define GPR_NUM_EQU(num, name)  "\n.equ .L_gpr_num_" #name ", " #num
  #define DEFINE_ASM_GPR_NUMS     GPR_LIST(GPR_NUM_EQU)

> 
>>   /*
>> - * The exception table consists of pairs of relative offsets: the first
>> - * is the relative offset to an instruction that is allowed to fault,
>> - * and the second is the relative offset at which the program should
>> - * continue. No general-purpose registers are modified by the exception
>> - * handling mechanism itself, so it is up to the fixup code to handle
>> - * any necessary state cleanup.
>> + * Each exception table entry consists of two relative offsets and a
>> + * handler description: `insn` is the relative offset to an instruction
>> + * that is allowed to fault, `fixup` is the relative offset at which the
>> + * program should continue,
> 
> "... in case of a fault, ..."

Applied.

> 
>> --- /dev/null
>> +++ b/xen/arch/riscv/include/asm/gpr-num.h
>> @@ -0,0 +1,37 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef RISCV_GPR_NUM_H
>> +#define RISCV_GPR_NUM_H
>> +
>> +/*
>> + * GPRs by ABI name, together with their register number (x0 .. x31).
>> + *
>> + * This is the single source of truth for the mapping:
> 
> True until here, but ...
> 
>> it generates the
>> + * .L_gpr_num_<name> assembler symbols
> 
> ... no, it doesn't. It's ...

I will update the comment to:
/*
  * GPRs by ABI name, together with their register number (x0 .. x31).
  *
  * This is the single source of truth for the mapping; users expand it with
  * their own per-register macro. Among them, struct cpu_user_regs is 
checked
  * against this list at build time (see regs_get_gpr()), so neither 
list can
  * be changed without the other.
  */
#define GPR_LIST(x)                                 \
     ...

and then ...

> 
>> used to turn a register name emitted
>> + * by the compiler into a register number, and struct cpu_user_regs is
>> + * checked against it at build time (see regs_get_gpr()). Neither list can
>> + * therefore be changed without the other.
>> + */
>> +#define GPR_LIST(x)                                 \
>> +    x(0,  zero) x(1,  ra)  x(2,  sp)  x(3,  gp)     \
>> +    x(4,  tp)   x(5,  t0)  x(6,  t1)  x(7,  t2)     \
>> +    x(8,  s0)   x(9,  s1)  x(10, a0)  x(11, a1)     \
>> +    x(12, a2)   x(13, a3)  x(14, a4)  x(15, a5)     \
>> +    x(16, a6)   x(17, a7)  x(18, s2)  x(19, s3)     \
>> +    x(20, s4)   x(21, s5)  x(22, s6)  x(23, s7)     \
>> +    x(24, s8)   x(25, s9)  x(26, s10) x(27, s11)    \
>> +    x(28, t3)   x(29, t4)  x(30, t5)  x(31, t6)
>> +
>> +#ifdef __ASSEMBLER__
>> +
>> +#define GPR_NUM_EQU(num, name)  .equ .L_gpr_num_##name, num;
>> +GPR_LIST(GPR_NUM_EQU)
>> +#undef GPR_NUM_EQU
> 
> ... this construct which does. This is relevant to separate, since
> GPR_LIST() is also used elsewhere.
> 

... here:

/*
  * Generate the .L_gpr_num_<name> assembler symbols, used to turn a 
register
  * name emitted by the compiler into a register number.
  *
  * The separator is emitted ahead of each entry rather than after it, 
so that


>> --- a/xen/arch/riscv/include/asm/processor.h
>> +++ b/xen/arch/riscv/include/asm/processor.h
>> @@ -12,7 +12,19 @@
>>   
>>   #ifndef __ASSEMBLER__
>>   
>> -/* On stack VCPU state */
>> +/*
>> + * On stack VCPU state.
>> + *
>> + * x0..x31 must remain at the start of this structure, in architectural
>> + * register-number order:
> 
> I understand that the order need retaining. But why would it being at the
> start of the struct be (overly) relevant? You could use the "zero" field
> as the anchor for calculations.

You're right.

The placement requirement comes only from REG_PTR() computing a byte 
offset from the struct base. Anchoring it on ->zero instead removes the 
need for the block to sit at the start:

#define REG_PTR(insn, pos, regs) \
     (&(regs)->zero + (REG_OFFSET(insn, pos) / REGBYTES))

I'll do that and reword the comment to state the actual invariants: the 
x0..x31 fields have to stay contiguous and in ascending register-number 
order, and ->zero has to read as 0.

Thanks.

~ Oleksii