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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/15/26 2:12 PM, Jan Beulich wrote:
> 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.
I used before 0x%02lx and after switch to %# don't think that 02 would fully 
consumed
by #.

I am okay to use just %lx.

> > +} while ( 0 )
> Why the do/while wrapping, btw?
Added it automatically, there is no really to have it here. I'll drop.


> > @@ -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).
Agree, with such usage there is no really necessity for it.

> > +    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?
It could be useful when we jump to guest, something wasn't configured
correctly and so lets say an illegal instruction in guest happen and
so it would be useful to at least understand what is this instruction
based on VSEPC or VSTVAL.

All others probaly there is no need to have printed, I don't remember
that I used them during debug.


> > +    printk("\nHypervisor CSRs\n");
> > +
> > +    print_csr(CSR_HSTATUS);
> Above you special-case VSCAUSE, but here you re-read HSTATUS.
Because above I re-used 'cause' then for decode_cause().

> > +    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?
Agree, visually it would be better.

> > +    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?
Maybe, it is a good idea and it would be nice to add them.

I just almost never needed them for debugging so I am not printing
them.

~ Oleksii