[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.