xen/riscv: dump CSRs on unexpected traps

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

Posted by Oleksii Kurochko 1 week, 3 days ago

Provide more context on the exception state when an unexpected
exception occurs by dumping various Supervisor, Virtual Supervisor
and Hypervisor CSRs.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/traps.c | 55 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index e9c967786312..d6c92588ea47 100644
--- 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)); \
+} while ( 0 )
+
 /*
  * Initialize the trap handling.
  *
@@ -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);
+
+    printk("\tCSR_STVAL: %#02lx%s\n", csr_read(CSR_STVAL),
+           gva ? ", (guest virtual address)" : "");
+
+    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);
+
+    printk("\nHypervisor CSRs\n");
+
+    print_csr(CSR_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));
+    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();
 }
 
-- 
2.52.0

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

Posted by Jan Beulich 1 week, 3 days ago

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

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

Posted by Oleksii Kurochko 1 week, 3 days ago

On 1/15/26 2:12 PM, Jan Beulich wrote:
>>   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?

Just to double-check, do you mean GPRs which are stired in
regs argument of do_unexpected_trap?

~ Oleksii

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

Posted by Jan Beulich 1 week, 3 days ago

On 15.01.2026 16:41, Oleksii Kurochko wrote:
> On 1/15/26 2:12 PM, Jan Beulich wrote:
>>>   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?
> 
> Just to double-check, do you mean GPRs which are stired in
> regs argument of do_unexpected_trap?

Sure, those are the ones pertaining to the trap.

Jan

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

Posted by Oleksii Kurochko 1 week, 3 days ago

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