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

[Jan Beulich <jbeulich@xxxxxxxx>]

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)? Unlike for the GPRs this also prints the register
names in upper case. That may be fine, or may not be. The values printed also
won't align, which may make reading more difficult.

> +#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.

> @@ -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.

> +    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.)

> +}
> +
> +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.

Jan