On 15.01.2026 13:56, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -17,6 +17,10 @@
> #include <asm/traps.h>
> #include <asm/vsbi.h>
>
> +#define print_csr(csr) do { \
> + printk("\t" #csr ": %#02lx\n", csr_read(csr)); \
Why the 02 in the format? If you wanted to align things, you'd use %016lx. (I
also don't think the 0x prefixes are overly useful in such dumping, but others
may disagree.) As an aside, the width of 2 would be fully consumed by your use
of #, except for zero which would (oddly) be printed as 00 afaict.
> +} while ( 0 )
Why the do/while wrapping, btw?
> @@ -99,12 +103,63 @@ static const char *decode_cause(unsigned long cause)
> return decode_trap_cause(cause);
> }
>
> +static void dump_csrs(unsigned long cause)
> +{
> + unsigned long hstatus;
> + bool gva;
> +
> + 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);
> + gva = !!(hstatus & HSTATUS_GVA);
No need for !! when the lhs type is bool. Question is whether gva is useful
to have as a local in the first place, when ...
> + printk("\tCSR_STVAL: %#02lx%s\n", csr_read(CSR_STVAL),
> + gva ? ", (guest virtual address)" : "");
... this it's sole use (you don't use where you could further down).
> + printk("\tCSR_SCAUSE: %#02lx\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: %#02lx\n", cause);
> + printk("\t\tDescription: %s\n", decode_cause(cause));
> + print_csr(CSR_VSSTATUS);
Everything below I understand wants dumping, but for much of the above
that's less clear to me. Why would guest's values be relevant when we
have a hypervisor problem?
> + printk("\nHypervisor CSRs\n");
> +
> + print_csr(CSR_HSTATUS);
Above you special-case VSCAUSE, but here you re-read HSTATUS.
> + printk("\t\thstatus.VTSR=%d\n", !!(hstatus & HSTATUS_VTSR));
> + printk("\t\thstatus.VTVM=%d\n", !!(hstatus & HSTATUS_VTVM));
> + printk("\t\thstatus.HU=%d\n", !!(hstatus & HSTATUS_HU));
> + printk("\t\thstatus.SPVP=%d\n", !!(hstatus & HSTATUS_SPVP));
> + printk("\t\thstatus.SPV=%d\n", !!(hstatus & HSTATUS_SPV));
> + printk("\t\thstatus.GVA=%d\n", !!(hstatus & HSTATUS_GVA));
Might these better be put on a single line, e.g. in the form
[VTSR SPVP SPV]
i.e. enumerating the (interesting) set bits textually?
> + print_csr(CSR_HGATP);
> + print_csr(CSR_HTVAL);
> + print_csr(CSR_HTINST);
> + print_csr(CSR_HEDELEG);
> + print_csr(CSR_HIDELEG);
> + print_csr(CSR_HSTATEEN0);
> +}
> +
> 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);
> +
> die();
> }
Apart from CSRs, how about also dumping GPRs?
Jan
