Re: [PATCH v2] xen/riscv: dump GPRS and CSRs on unexpected traps

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/19/26 9:34 AM, Jan Beulich wrote:
> On 16.01.2026 17:16, Oleksii Kurochko wrote:
> > Provide more context on the exception state when an unexpected
> > exception occurs by dumping various Supervisor, Virtual Supervisor
> > and Hypervisor CSRs, and GPRs pertaining to the trap.
> >
> > Signed-off-by: Oleksii Kurochko<oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in v2
> >   - Address coments about print_csr() macros and dump_csrs().
> >   - Add dumping of GPRs pertaining to the trap.
> > ---
> >   xen/arch/riscv/traps.c | 82 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 82 insertions(+)
> >
> > diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
> > index e9c967786312..4e0df698552f 100644
> > --- a/xen/arch/riscv/traps.c
> > +++ b/xen/arch/riscv/traps.c
> > @@ -17,6 +17,13 @@
> >   #include <asm/traps.h>
> >   #include <asm/vsbi.h>
> >   
> > +#define print_csr(csr) \
> > +    printk("\t" #csr ": %016lx\n", csr_read(csr));
> This prints the CSR_ prefix of the internally used constants. Is this useful
> (rather than extra clutter)?
No, the prefix isn't really useful. It was printed so only because all CSRs 
registers
defintions start on CSR_*.

>   Unlike for the GPRs this also prints the register
> names in upper case. That may be fine, or may not be.
I will follow then like it is written in RISC-V spec for consistency.


>   The values printed also
> won't align, which may make reading more difficult.
Do you expect the similar alignment like for GPRs?


> > +#define print_gprs(regs, reg1, reg2) \
> > +    printk("\t%-7s: %016lx %-7s: %016lx\n", \
> > +           #reg1, (regs)->reg1, #reg2, (regs)->reg2)
> Yes, two per line is certainly helpful. Why not also for some of the CSRs.
It is less clear how it would be better to group them. I thought about
CSR_STVEC: ....    CSR_VSTVEC: ....
CSR_STTATUS: ...   CSR_VSSTATUS: ....

So group them in S-mode mode register and VS-mode register.

> > @@ -99,12 +106,87 @@ static const char *decode_cause(unsigned long cause)
> >       return decode_trap_cause(cause);
> >   }
> >   
> > +static void dump_general_regs(const struct cpu_user_regs *regs)
> > +{
> > +    printk("\nDumping GPRs...\n");
> Register names alone will be meaningful enough? Be mindful of serial line
> bandwidth as well as screen resolution.
Agree, we could drop this print. (Then probably the same could be for 
Supervisor CSRs
and Virtual Supervisor CSRs, etc as it is clear based on the name which one CSR 
it
is)

> > +    print_gprs(regs, ra, sp);
> > +    print_gprs(regs, gp, tp);
> > +    print_gprs(regs, t0, t1);
> > +    print_gprs(regs, t2, s0);
> > +    print_gprs(regs, s1, a0);
> > +    print_gprs(regs, a1, a2);
> > +    print_gprs(regs, a3, a4);
> > +    print_gprs(regs, a5, a6);
> > +    print_gprs(regs, a7, s2);
> > +    print_gprs(regs, s3, s4);
> > +    print_gprs(regs, s5, s6);
> > +    print_gprs(regs, s7, s8);
> > +    print_gprs(regs, s9, s10);
> > +    print_gprs(regs, s11, t3);
> > +    print_gprs(regs, t4, t5);
> > +    print_gprs(regs, t6, sepc);
> > +    print_gprs(regs, sstatus, hstatus);
> The last three aren't GPRs. Why would they be logged here? (All three also
> being logged in dump_csrs() would further require some disambiguation in
> the output, for people to not need to go look at the source code every
> time.)
Agree, that it would be better to print it with the CSRs. It was print here
only as they are in the same structure with GPRs.

> > +}
> > +
> > +static void dump_csrs(unsigned long cause)
> > +{
> > +    unsigned long hstatus;
> > +
> > +    printk("\nDumping CSRs...\n");
> > +
> > +    printk("Supervisor CSRs\n");
> > +    print_csr(CSR_STVEC);
> > +    print_csr(CSR_SATP);
> > +    print_csr(CSR_SEPC);
> > +
> > +    hstatus = csr_read(CSR_HSTATUS);
> > +
> > +    printk("\tCSR_STVAL: %016lx%s\n", csr_read(CSR_STVAL),
> > +           (hstatus & HSTATUS_GVA) ? ", (guest virtual address)" : "");
> > +
> > +    printk("\tCSR_SCAUSE: %016lx\n", cause);
> > +    printk("\t\tDescription: %s\n", decode_cause(cause));
> > +    print_csr(CSR_SSTATUS);
> > +
> > +    printk("\nVirtual Supervisor CSRs\n");
> > +    print_csr(CSR_VSTVEC);
> > +    print_csr(CSR_VSATP);
> > +    print_csr(CSR_VSEPC);
> > +    print_csr(CSR_VSTVAL);
> > +    cause = csr_read(CSR_VSCAUSE);
> > +    printk("\tCSR_VSCAUSE: %016lx\n", cause);
> > +    printk("\t\tDescription: %s\n", decode_cause(cause));
> > +    print_csr(CSR_VSSTATUS);
> As before, imo justification is wanted (in the description) for logging
> VS* values.
It is hard to describe in general why they could be needed. The best I can
come up is:
  Dump VS* CSRs to provide full guest exception context. When handling traps 
originating
  from VS-mode, host CSRs alone do not describe the fault; VSCAUSE, VSEPC, 
VSTVAL, and
  related state are required to diagnose guest crashes and hypervisor 
misconfiguration,
  and to correlate host-side handling with guest-visible exceptions.

Does it good enough justification?

~ Oleksii