On 28.01.2026 12:22, Oleksii Kurochko wrote: > > On 1/28/26 12:08 PM, Jan Beulich wrote: >> On 28.01.2026 12:01, Oleksii Kurochko wrote: >>> 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 <[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. >>> 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? >> It means that it'll be logged twice for an unexpected trap. As said, I can >> only recommend to limit the volume of the output in such situations, as >> sometimes all people may be able to get you is screenshots. > > It will be logged once, basically, mu suggestion is: > > -static void dump_csrs(unsigned long cause) > +static void dump_csrs(void) > { > #define X(name, csr, fmt, ...) \ > v = csr_read(csr); \ > @@ -156,11 +156,7 @@ static void dump_csrs(unsigned long cause) > > static void do_unexpected_trap(const struct cpu_user_regs *regs) > { > - unsigned long cause = csr_read(CSR_SCAUSE); > - > - printk("Unhandled exception: %s\n", decode_cause(cause)); > - > - dump_csrs(cause); > + dump_csrs(); > dump_general_regs(regs);
And the fact that it was an unhandled exception then isn't logged explicitly anymore? Perhaps dump_csrs() then should have a const char * parameter, which here you would pass "Unhandled exception" for. Jan
