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@gmail.com>
---
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));
+
+#define print_gprs(regs, reg1, reg2) \
+    printk("\t%-7s: %016lx %-7s: %016lx\n", \
+           #reg1, (regs)->reg1, #reg2, (regs)->reg2)
+
 /*
  * Initialize the trap handling.
  *
@@ -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");
+
+    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);
+}
+
+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);
+
+    printk("\nHypervisor CSRs\n");
+
+    printk("\tCSR_HSTATUS: %016lx [%s%s%s%s%s%s ]\n",
+           hstatus,
+           (hstatus & HSTATUS_VTSR) ? " VTSR" : "",
+           (hstatus & HSTATUS_VTVM) ? " VTVM" : "",
+           (hstatus & HSTATUS_HU)   ? " HU"   : "",
+           (hstatus & HSTATUS_SPVP) ? " SPVP" : "",
+           (hstatus & HSTATUS_SPV)  ? " SPV"  : "",
+           (hstatus & HSTATUS_GVA)  ? " 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);
+    dump_general_regs(regs);
+
     die();
 }
 
-- 
2.52.0

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

On 1/19/26 9:34 AM, Jan Beulich wrote:
> 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@gmail.com>
>> ---
>> 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)?

No, the prefix isn't really useful. It was printed so only because all CSRs registers
defintions start on CSR_*.

>   Unlike for the GPRs this also prints the register
> names in upper case. That may be fine, or may not be.

I will follow then like it is written in RISC-V spec for consistency.


>   The values printed also
> won't align, which may make reading more difficult.

Do you expect the similar alignment like for GPRs?


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

It is less clear how it would be better to group them. I thought about
CSR_STVEC: ....    CSR_VSTVEC: ....
CSR_STTATUS: ...   CSR_VSSTATUS: ....

So group them in S-mode mode register and VS-mode register.

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

Agree, we could drop this print. (Then probably the same could be for Supervisor CSRs
and Virtual Supervisor CSRs, etc as it is clear based on the name which one CSR it
is)

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

Agree, that it would be better to print it with the CSRs. It was print here
only as they are in the same structure with GPRs.

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

It is hard to describe in general why they could be needed. The best I can
come up is:
   Dump VS* CSRs to provide full guest exception context. When handling traps originating
   from VS-mode, host CSRs alone do not describe the fault; VSCAUSE, VSEPC, VSTVAL, and
   related state are required to diagnose guest crashes and hypervisor misconfiguration,
   and to correlate host-side handling with guest-visible exceptions.

Does it good enough justification?

~ Oleksii

On 23.01.2026 10:09, Oleksii Kurochko wrote:
> On 1/19/26 9:34 AM, Jan Beulich wrote:
>> 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@gmail.com>
>>> ---
>>> 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)?
> 
> No, the prefix isn't really useful. It was printed so only because all CSRs registers
> defintions start on CSR_*.
> 
>>   Unlike for the GPRs this also prints the register
>> names in upper case. That may be fine, or may not be.
> 
> I will follow then like it is written in RISC-V spec for consistency.
> 
> 
>>   The values printed also
>> won't align, which may make reading more difficult.
> 
> Do you expect the similar alignment like for GPRs?

"similar" is ambiguous here. I'd expect whatever alignment helps readability.

>>> +#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.
> 
> It is less clear how it would be better to group them. I thought about
> CSR_STVEC: ....    CSR_VSTVEC: ....
> CSR_STTATUS: ...   CSR_VSSTATUS: ....
> 
> So group them in S-mode mode register and VS-mode register.

That's an option. Don't know how this would end looking like all together.

>>> @@ -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.
> 
> Agree, we could drop this print. (Then probably the same could be for Supervisor CSRs
> and Virtual Supervisor CSRs, etc as it is clear based on the name which one CSR it
> is)

Of course I also meant to cover the other, similar ones, yes.

>>> +    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.)
> 
> Agree, that it would be better to print it with the CSRs. It was print here
> only as they are in the same structure with GPRs.
> 
>>> +}
>>> +
>>> +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.
> 
> It is hard to describe in general why they could be needed. The best I can
> come up is:
>    Dump VS* CSRs to provide full guest exception context. When handling traps originating
>    from VS-mode, host CSRs alone do not describe the fault; VSCAUSE, VSEPC, VSTVAL, and
>    related state are required to diagnose guest crashes and hypervisor misconfiguration,
>    and to correlate host-side handling with guest-visible exceptions.
> 
> Does it good enough justification?

I think diagnosing guest crashes doesn't belong here. An unexpected trap is
entirely different from a guest crash. Hypervisor misconfig I might buy, just
that I don't (yet?) see the connection to the three particular registers you
name in the suggested text. (As mentioned before, this may simply be because
of my lack of a proper global picture of RISC-V.)

Jan

On 1/23/26 10:39 AM, Jan Beulich wrote:
>>>> +}
>>>> +
>>>> +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.
>> It is hard to describe in general why they could be needed. The best I can
>> come up is:
>>     Dump VS* CSRs to provide full guest exception context. When handling traps originating
>>     from VS-mode, host CSRs alone do not describe the fault; VSCAUSE, VSEPC, VSTVAL, and
>>     related state are required to diagnose guest crashes and hypervisor misconfiguration,
>>     and to correlate host-side handling with guest-visible exceptions.
>>
>> Does it good enough justification?
> I think diagnosing guest crashes doesn't belong here. An unexpected trap is
> entirely different from a guest crash. Hypervisor misconfig I might buy, just
> that I don't (yet?) see the connection to the three particular registers you
> name in the suggested text. (As mentioned before, this may simply be because
> of my lack of a proper global picture of RISC-V.)

One of the options which could be stored in VSTVAL is a faulty instruction, so
without checking VSEPC we could understand what is the instruction and check
what is in HS-mode register to understand if enough permissions were given to
VS-mode to use this instruction without trap to HS-mode.

Considering that even if a source of trap is faulty instruction it doesn't
guarantee that VSTVAL will contain this faulty instruction, so we should go to
check VSEPC and then use objdump to understand what was faulty instruction.

VSCAUSE could be useful for the case if for some reason guest doesn't dump
VSCAUSE to its console, at least, we could get some extra information what was
wrong during execution of faulty instruction.

Also, there are a cases when from hypervisor some trap redirection to a guest
happens and VSCAUSE is used for this purpose, VSTATUS and VSTVEC are used too,
so it would be nice to check them too if such redirection leads to another trap
because of incorrectly set VSCAUSE, VSTATUS and VSTVEC.

I don't really see any reason to print VSATP, at least, at the moment. So
I can drop it.

~ Oleksii

On 1/23/26 10:39 AM, Jan Beulich wrote:
>>>> +}
>>>> +
>>>> +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.
>> It is hard to describe in general why they could be needed. The best I can
>> come up is:
>>     Dump VS* CSRs to provide full guest exception context. When handling traps originating
>>     from VS-mode, host CSRs alone do not describe the fault; VSCAUSE, VSEPC, VSTVAL, and
>>     related state are required to diagnose guest crashes and hypervisor misconfiguration,
>>     and to correlate host-side handling with guest-visible exceptions.
>>
>> Does it good enough justification?
> I think diagnosing guest crashes doesn't belong here. An unexpected trap is
> entirely different from a guest crash. Hypervisor misconfig I might buy, just
> that I don't (yet?) see the connection to the three particular registers you
> name in the suggested text. (As mentioned before, this may simply be because
> of my lack of a proper global picture of RISC-V.)

One of the options which could be stored in VSTVAL is a faulty instruction, so
without checking VSEPC we could understand what is the instruction and check
what is in HS-mode register to understand if enough permissions were given to
VS-mode to use this instruction without trap to HS-mode.

Considering that even if a source of trap is faulty instruction it doesn't
guarantee that VSTVAL will contain this faulty instruction, so we should go to
check VSEPC and then use objdump to understand what was faulty instruction.

VSCAUSE could be useful for the case if for some reason guest doesn't dump
VSCAUSE to its console, at least, we could get some extra information what was
wrong during execution of faulty instruction.
Also, there are a cases when from hypervisor some trap redirection to a guest
happens and VSCAUSE is used for this purpose, VSTATUS and VSTVEC are used too,
so it would be nice to check them too if such redirection leads to another trap
because of incorrectly set VSCAUSE, VSTATUS and VSTVEC.

I don't really see any reason to print VSATP, at least, at the moment. So
I can drop it.

~ Oleksii