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

[Jan Beulich <jbeulich@xxxxxxxx>]

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