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<[email protected]>
>>> ---
>>> 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
