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 <[email protected]>Acked-by: Jan Beulich <[email protected]>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. I will update the patch and send new version. Thanks. ~ Oleksii
