Re: [PATCH v4] xen/riscv: dump GPRs and CSRs on unexpected traps

[Oleksii Kurochko]

On 1/28/26 11:34 AM, Oleksii Kurochko wrote:

On 1/27/26 10:27 AM, Jan Beulich wrote:

On 27.01.2026 10:14, Jan Beulich wrote:

On 26.01.2026 12:43, Oleksii Kurochko wrote:

Provide additional context when an unexpected exception occurs by dumping

the relevant Supervisor, Virtual Supervisor (VS), and Hypervisor CSRs,
along with the general-purpose registers associated with the trap.

Dumping VS-mode CSRs in addition to host CSRs is beneficial when analysing VS-mode traps. VSCAUSE, VSEPC, VSTVAL, and related VS state are required to

properly diagnose unexpected guest traps and potential hypervisor
misconfiguration.

For example, on an illegal-instruction exception the hardware may record the faulting instruction in VSTVAL. If VSTVAL is zero, VSEPC should always

be inspected, and can be used together with objdump to identify the

faulting instruction. Dumping VSCAUSE is also useful when the guest does not report it, or when the hypervisor redirects a trap to the guest using VSCAUSE, VSTATUS, and VSTVEC, allowing verification that a subsequent trap

is not caused by incorrect VS state.

Signed-off-by: Oleksii Kurochko <oleksii.kuroc...@gmail.com>

Acked-by: Jan Beulich <jbeul...@suse.com>

Hmm, wait, there's another anomaly:

I still have a question though, which can be addressed incrementally.

--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c

@@ -99,12 +99,70 @@ static const char *decode_cause(unsigned long cause)

      return decode_trap_cause(cause);
  }
  +static void dump_general_regs(const struct cpu_user_regs *regs)
+{
+#define X(regs, name, delim) \
+    printk("%-4s: %016lx" delim, #name, (regs)->name)
+
+    X(regs, ra, " "); X(regs, sp, "\n");
+    X(regs, gp, " "); X(regs, tp, "\n");
+    X(regs, t0, " "); X(regs, t1, "\n");
+    X(regs, t2, " "); X(regs, s0, "\n");
+    X(regs, s1, " "); X(regs, a0, "\n");
+    X(regs, a1, " "); X(regs, a2, "\n");
+    X(regs, a3, " "); X(regs, a4, "\n");
+    X(regs, a5, " "); X(regs, a6, "\n");
+    X(regs, a7, " "); X(regs, s2, "\n");
+    X(regs, s3, " "); X(regs, s4, "\n");
+    X(regs, s5, " "); X(regs, s6, "\n");
+    X(regs, s7, " "); X(regs, s8, "\n");
+    X(regs, s9, " "); X(regs, s10, "\n");
+    X(regs, s11, " "); X(regs, t3, "\n");
+    X(regs, t4, " "); X(regs, t5, "\n");
+    X(regs, t6, " "); X(regs, sepc, "\n");

Does this sepc value differ from ...

+static void dump_csrs(unsigned long cause)

What is this function parameter for?

+{
+#define X(name, csr, fmt, ...) \
+    v = csr_read(csr); \
+    printk("%-10s: %016lx" fmt, #name, v, ##__VA_ARGS__)
+
+    unsigned long v;
+
+    X(htval, CSR_HTVAL, " ");  X(htinst, CSR_HTINST, "\n");
+    X(hedeleg, CSR_HEDELEG, " "); X(hideleg, CSR_HIDELEG, "\n");
+    X(hstatus, CSR_HSTATUS, " [%s%s%s%s%s%s ]\n",
+      (v & HSTATUS_VTSR) ? " VTSR" : "",
+      (v & HSTATUS_VTVM) ? " VTVM" : "",
+      (v & HSTATUS_HU)   ? " HU"   : "",
+      (v & HSTATUS_SPVP) ? " SPVP" : "",
+      (v & HSTATUS_SPV)  ? " SPV"  : "",
+      (v & HSTATUS_GVA)  ? " GVA"  : "");
+    X(hgatp, CSR_HGATP, "\n");
+    X(hstateen0, CSR_HSTATEEN0, "\n");
+    X(stvec, CSR_STVEC, " "); X(vstvec, CSR_VSTVEC, "\n");
+    X(sepc, CSR_SEPC, " "); X(vsepc, CSR_VSEPC, "\n");

... the one logged here? Nothing changes the register between entry
into the hypervisor and coming here?

Down below here you have

     X(scause, CSR_SCAUSE, " [%s]\n", decode_cause(v));

which actually (largely) duplicates what do_unexpected_trap() has already

logged.

Missed that, then it would be better to remove this duplication and leave
only printing of CSR_SCAUSE in dump_csrs().

  If dump_csrs() gained other uses, the dumping of scause likely is

wanted, but then likely no scause value would be available to pass in? So

maybe its dumping actually wants to be conditional (and the parameter
wants to be a boolean)?

Good point. Honestly speaking, I don't know if it will be any other usages

except this one. But to keep things generic I think it is good idea to
add conditional dumping of scause.

As an alternative, we could simply remove the dump_csrs() argument and always
print SCAUSE. When running in hypervisor mode, SCAUSE should contain the reason
for the trap. Even it is lets say just hypercall and not something faulty, it
will contain "Environment call from S-mode" what looks okay to be printed.

I tend to prefer this approach slightly. What do you think?

~ Oleksii