xen/arch/riscv/traps.c | 58 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
Provide additional context when an unexpected exception occurs by dumping
the relevant Supervisor, Virtual Supervisor (VS), and Hypervisor CSRs,
along with the general-purpose registers associated with the trap.
Dumping VS-mode CSRs in addition to host CSRs is beneficial when analysing
VS-mode traps. VSCAUSE, VSEPC, VSTVAL, and related VS state are required to
properly diagnose unexpected guest traps and potential hypervisor
misconfiguration.
For example, on an illegal-instruction exception the hardware may record
the faulting instruction in VSTVAL. If VSTVAL is zero, VSEPC should always
be inspected, and can be used together with objdump to identify the
faulting instruction. Dumping VSCAUSE is also useful when the guest does
not report it, or when the hypervisor redirects a trap to the guest using
VSCAUSE, VSTATUS, and VSTVEC, allowing verification that a subsequent trap
is not caused by incorrect VS state.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v4:
- Drop macros GPR_LIST and CSR_LIST.
- Drop leading tab identation in printk() inside X macros.
- Drop semicolon after printk in X macros.
- Group print of htval and htinst, and HEDELEG and HIDELEG.
- Put printing of hypervisor register first.
- Add printing of missing GPRs: t5, t6, sepc.
---
Changes in v3:
- Refactor the code dumping of CSRs and GPRs:
- Introduce new macros
- Re-group some registers when values are displayed.
- Print all registers name in lower case.
- Drop unnessary "Dumping CSRs", "Dumping GSRs" as it
is clear based on names.
- Update the commit message: add justification of dumping of
some VS* registers.
- Drop printing of VSSATP as I don't know usecase for now
where it could be needed.
---
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 | 58 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index e9c967786312..da74f77ecc90 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -99,12 +99,70 @@ static const char *decode_cause(unsigned long cause)
return decode_trap_cause(cause);
}
+static void dump_general_regs(const struct cpu_user_regs *regs)
+{
+#define X(regs, name, delim) \
+ printk("%-4s: %016lx" delim, #name, (regs)->name)
+
+ X(regs, ra, " "); X(regs, sp, "\n");
+ X(regs, gp, " "); X(regs, tp, "\n");
+ X(regs, t0, " "); X(regs, t1, "\n");
+ X(regs, t2, " "); X(regs, s0, "\n");
+ X(regs, s1, " "); X(regs, a0, "\n");
+ X(regs, a1, " "); X(regs, a2, "\n");
+ X(regs, a3, " "); X(regs, a4, "\n");
+ X(regs, a5, " "); X(regs, a6, "\n");
+ X(regs, a7, " "); X(regs, s2, "\n");
+ X(regs, s3, " "); X(regs, s4, "\n");
+ X(regs, s5, " "); X(regs, s6, "\n");
+ X(regs, s7, " "); X(regs, s8, "\n");
+ X(regs, s9, " "); X(regs, s10, "\n");
+ X(regs, s11, " "); X(regs, t3, "\n");
+ X(regs, t4, " "); X(regs, t5, "\n");
+ X(regs, t6, " "); X(regs, sepc, "\n");
+
+#undef X
+}
+
+static void dump_csrs(unsigned long cause)
+{
+#define X(name, csr, fmt, ...) \
+ v = csr_read(csr); \
+ printk("%-10s: %016lx" fmt, #name, v, ##__VA_ARGS__)
+
+ unsigned long v;
+
+ X(htval, CSR_HTVAL, " "); X(htinst, CSR_HTINST, "\n");
+ X(hedeleg, CSR_HEDELEG, " "); X(hideleg, CSR_HIDELEG, "\n");
+ X(hstatus, CSR_HSTATUS, " [%s%s%s%s%s%s ]\n",
+ (v & HSTATUS_VTSR) ? " VTSR" : "",
+ (v & HSTATUS_VTVM) ? " VTVM" : "",
+ (v & HSTATUS_HU) ? " HU" : "",
+ (v & HSTATUS_SPVP) ? " SPVP" : "",
+ (v & HSTATUS_SPV) ? " SPV" : "",
+ (v & HSTATUS_GVA) ? " GVA" : "");
+ X(hgatp, CSR_HGATP, "\n");
+ X(hstateen0, CSR_HSTATEEN0, "\n");
+ X(stvec, CSR_STVEC, " "); X(vstvec, CSR_VSTVEC, "\n");
+ X(sepc, CSR_SEPC, " "); X(vsepc, CSR_VSEPC, "\n");
+ X(stval, CSR_STVAL, " "); X(vstval, CSR_VSTVAL, "\n");
+ X(status, CSR_SSTATUS, " "); X(vsstatus, CSR_VSSTATUS, "\n");
+ X(satp, CSR_SATP, "\n");
+ X(scause, CSR_SCAUSE, " [%s]\n", decode_cause(v));
+ X(vscause, CSR_VSCAUSE, " [%s]\n", decode_cause(v));
+
+#undef X
+}
+
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.0On 26.01.2026 12:43, Oleksii Kurochko wrote:
> Provide additional context when an unexpected exception occurs by dumping
> the relevant Supervisor, Virtual Supervisor (VS), and Hypervisor CSRs,
> along with the general-purpose registers associated with the trap.
>
> Dumping VS-mode CSRs in addition to host CSRs is beneficial when analysing
> VS-mode traps. VSCAUSE, VSEPC, VSTVAL, and related VS state are required to
> properly diagnose unexpected guest traps and potential hypervisor
> misconfiguration.
> For example, on an illegal-instruction exception the hardware may record
> the faulting instruction in VSTVAL. If VSTVAL is zero, VSEPC should always
> be inspected, and can be used together with objdump to identify the
> faulting instruction. Dumping VSCAUSE is also useful when the guest does
> not report it, or when the hypervisor redirects a trap to the guest using
> VSCAUSE, VSTATUS, and VSTVEC, allowing verification that a subsequent trap
> is not caused by incorrect VS state.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
I still have a question though, which can be addressed incrementally.
> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -99,12 +99,70 @@ static const char *decode_cause(unsigned long cause)
> return decode_trap_cause(cause);
> }
>
> +static void dump_general_regs(const struct cpu_user_regs *regs)
> +{
> +#define X(regs, name, delim) \
> + printk("%-4s: %016lx" delim, #name, (regs)->name)
> +
> + X(regs, ra, " "); X(regs, sp, "\n");
> + X(regs, gp, " "); X(regs, tp, "\n");
> + X(regs, t0, " "); X(regs, t1, "\n");
> + X(regs, t2, " "); X(regs, s0, "\n");
> + X(regs, s1, " "); X(regs, a0, "\n");
> + X(regs, a1, " "); X(regs, a2, "\n");
> + X(regs, a3, " "); X(regs, a4, "\n");
> + X(regs, a5, " "); X(regs, a6, "\n");
> + X(regs, a7, " "); X(regs, s2, "\n");
> + X(regs, s3, " "); X(regs, s4, "\n");
> + X(regs, s5, " "); X(regs, s6, "\n");
> + X(regs, s7, " "); X(regs, s8, "\n");
> + X(regs, s9, " "); X(regs, s10, "\n");
> + X(regs, s11, " "); X(regs, t3, "\n");
> + X(regs, t4, " "); X(regs, t5, "\n");
> + X(regs, t6, " "); X(regs, sepc, "\n");
Does this sepc value differ from ...
> +static void dump_csrs(unsigned long cause)
> +{
> +#define X(name, csr, fmt, ...) \
> + v = csr_read(csr); \
> + printk("%-10s: %016lx" fmt, #name, v, ##__VA_ARGS__)
> +
> + unsigned long v;
> +
> + X(htval, CSR_HTVAL, " "); X(htinst, CSR_HTINST, "\n");
> + X(hedeleg, CSR_HEDELEG, " "); X(hideleg, CSR_HIDELEG, "\n");
> + X(hstatus, CSR_HSTATUS, " [%s%s%s%s%s%s ]\n",
> + (v & HSTATUS_VTSR) ? " VTSR" : "",
> + (v & HSTATUS_VTVM) ? " VTVM" : "",
> + (v & HSTATUS_HU) ? " HU" : "",
> + (v & HSTATUS_SPVP) ? " SPVP" : "",
> + (v & HSTATUS_SPV) ? " SPV" : "",
> + (v & HSTATUS_GVA) ? " GVA" : "");
> + X(hgatp, CSR_HGATP, "\n");
> + X(hstateen0, CSR_HSTATEEN0, "\n");
> + X(stvec, CSR_STVEC, " "); X(vstvec, CSR_VSTVEC, "\n");
> + X(sepc, CSR_SEPC, " "); X(vsepc, CSR_VSEPC, "\n");
... the one logged here? Nothing changes the register between entry
into the hypervisor and coming here?
Jan
On 1/27/26 10:14 AM, Jan Beulich wrote:
> On 26.01.2026 12:43, Oleksii Kurochko wrote:
>> Provide additional context when an unexpected exception occurs by dumping
>> the relevant Supervisor, Virtual Supervisor (VS), and Hypervisor CSRs,
>> along with the general-purpose registers associated with the trap.
>>
>> Dumping VS-mode CSRs in addition to host CSRs is beneficial when analysing
>> VS-mode traps. VSCAUSE, VSEPC, VSTVAL, and related VS state are required to
>> properly diagnose unexpected guest traps and potential hypervisor
>> misconfiguration.
>> For example, on an illegal-instruction exception the hardware may record
>> the faulting instruction in VSTVAL. If VSTVAL is zero, VSEPC should always
>> be inspected, and can be used together with objdump to identify the
>> faulting instruction. Dumping VSCAUSE is also useful when the guest does
>> not report it, or when the hypervisor redirects a trap to the guest using
>> VSCAUSE, VSTATUS, and VSTVEC, allowing verification that a subsequent trap
>> is not caused by incorrect VS state.
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> I still have a question though, which can be addressed incrementally.
>
>> --- a/xen/arch/riscv/traps.c
>> +++ b/xen/arch/riscv/traps.c
>> @@ -99,12 +99,70 @@ static const char *decode_cause(unsigned long cause)
>> return decode_trap_cause(cause);
>> }
>>
>> +static void dump_general_regs(const struct cpu_user_regs *regs)
>> +{
>> +#define X(regs, name, delim) \
>> + printk("%-4s: %016lx" delim, #name, (regs)->name)
>> +
>> + X(regs, ra, " "); X(regs, sp, "\n");
>> + X(regs, gp, " "); X(regs, tp, "\n");
>> + X(regs, t0, " "); X(regs, t1, "\n");
>> + X(regs, t2, " "); X(regs, s0, "\n");
>> + X(regs, s1, " "); X(regs, a0, "\n");
>> + X(regs, a1, " "); X(regs, a2, "\n");
>> + X(regs, a3, " "); X(regs, a4, "\n");
>> + X(regs, a5, " "); X(regs, a6, "\n");
>> + X(regs, a7, " "); X(regs, s2, "\n");
>> + X(regs, s3, " "); X(regs, s4, "\n");
>> + X(regs, s5, " "); X(regs, s6, "\n");
>> + X(regs, s7, " "); X(regs, s8, "\n");
>> + X(regs, s9, " "); X(regs, s10, "\n");
>> + X(regs, s11, " "); X(regs, t3, "\n");
>> + X(regs, t4, " "); X(regs, t5, "\n");
>> + X(regs, t6, " "); X(regs, sepc, "\n");
> Does this sepc value differ from ...
>
>> +static void dump_csrs(unsigned long cause)
>> +{
>> +#define X(name, csr, fmt, ...) \
>> + v = csr_read(csr); \
>> + printk("%-10s: %016lx" fmt, #name, v, ##__VA_ARGS__)
>> +
>> + unsigned long v;
>> +
>> + X(htval, CSR_HTVAL, " "); X(htinst, CSR_HTINST, "\n");
>> + X(hedeleg, CSR_HEDELEG, " "); X(hideleg, CSR_HIDELEG, "\n");
>> + X(hstatus, CSR_HSTATUS, " [%s%s%s%s%s%s ]\n",
>> + (v & HSTATUS_VTSR) ? " VTSR" : "",
>> + (v & HSTATUS_VTVM) ? " VTVM" : "",
>> + (v & HSTATUS_HU) ? " HU" : "",
>> + (v & HSTATUS_SPVP) ? " SPVP" : "",
>> + (v & HSTATUS_SPV) ? " SPV" : "",
>> + (v & HSTATUS_GVA) ? " GVA" : "");
>> + X(hgatp, CSR_HGATP, "\n");
>> + X(hstateen0, CSR_HSTATEEN0, "\n");
>> + X(stvec, CSR_STVEC, " "); X(vstvec, CSR_VSTVEC, "\n");
>> + X(sepc, CSR_SEPC, " "); X(vsepc, CSR_VSEPC, "\n");
> ... the one logged here? Nothing changes the register between entry
> into the hypervisor and coming here?
You are right, the value will be the same. Lets drop printing of SEPC
from GPRS and leave only the printing of SEPC in dump_csrs().
~ Oleksii
On 27.01.2026 10:14, Jan Beulich wrote:
> On 26.01.2026 12:43, Oleksii Kurochko wrote:
>> Provide additional context when an unexpected exception occurs by dumping
>> the relevant Supervisor, Virtual Supervisor (VS), and Hypervisor CSRs,
>> along with the general-purpose registers associated with the trap.
>>
>> Dumping VS-mode CSRs in addition to host CSRs is beneficial when analysing
>> VS-mode traps. VSCAUSE, VSEPC, VSTVAL, and related VS state are required to
>> properly diagnose unexpected guest traps and potential hypervisor
>> misconfiguration.
>> For example, on an illegal-instruction exception the hardware may record
>> the faulting instruction in VSTVAL. If VSTVAL is zero, VSEPC should always
>> be inspected, and can be used together with objdump to identify the
>> faulting instruction. Dumping VSCAUSE is also useful when the guest does
>> not report it, or when the hypervisor redirects a trap to the guest using
>> VSCAUSE, VSTATUS, and VSTVEC, allowing verification that a subsequent trap
>> is not caused by incorrect VS state.
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>
> Acked-by: Jan Beulich <jbeulich@suse.com>
Hmm, wait, there's another anomaly:
> I still have a question though, which can be addressed incrementally.
>
>> --- a/xen/arch/riscv/traps.c
>> +++ b/xen/arch/riscv/traps.c
>> @@ -99,12 +99,70 @@ static const char *decode_cause(unsigned long cause)
>> return decode_trap_cause(cause);
>> }
>>
>> +static void dump_general_regs(const struct cpu_user_regs *regs)
>> +{
>> +#define X(regs, name, delim) \
>> + printk("%-4s: %016lx" delim, #name, (regs)->name)
>> +
>> + X(regs, ra, " "); X(regs, sp, "\n");
>> + X(regs, gp, " "); X(regs, tp, "\n");
>> + X(regs, t0, " "); X(regs, t1, "\n");
>> + X(regs, t2, " "); X(regs, s0, "\n");
>> + X(regs, s1, " "); X(regs, a0, "\n");
>> + X(regs, a1, " "); X(regs, a2, "\n");
>> + X(regs, a3, " "); X(regs, a4, "\n");
>> + X(regs, a5, " "); X(regs, a6, "\n");
>> + X(regs, a7, " "); X(regs, s2, "\n");
>> + X(regs, s3, " "); X(regs, s4, "\n");
>> + X(regs, s5, " "); X(regs, s6, "\n");
>> + X(regs, s7, " "); X(regs, s8, "\n");
>> + X(regs, s9, " "); X(regs, s10, "\n");
>> + X(regs, s11, " "); X(regs, t3, "\n");
>> + X(regs, t4, " "); X(regs, t5, "\n");
>> + X(regs, t6, " "); X(regs, sepc, "\n");
>
> Does this sepc value differ from ...
>
>> +static void dump_csrs(unsigned long cause)
What is this function parameter for?
>> +{
>> +#define X(name, csr, fmt, ...) \
>> + v = csr_read(csr); \
>> + printk("%-10s: %016lx" fmt, #name, v, ##__VA_ARGS__)
>> +
>> + unsigned long v;
>> +
>> + X(htval, CSR_HTVAL, " "); X(htinst, CSR_HTINST, "\n");
>> + X(hedeleg, CSR_HEDELEG, " "); X(hideleg, CSR_HIDELEG, "\n");
>> + X(hstatus, CSR_HSTATUS, " [%s%s%s%s%s%s ]\n",
>> + (v & HSTATUS_VTSR) ? " VTSR" : "",
>> + (v & HSTATUS_VTVM) ? " VTVM" : "",
>> + (v & HSTATUS_HU) ? " HU" : "",
>> + (v & HSTATUS_SPVP) ? " SPVP" : "",
>> + (v & HSTATUS_SPV) ? " SPV" : "",
>> + (v & HSTATUS_GVA) ? " GVA" : "");
>> + X(hgatp, CSR_HGATP, "\n");
>> + X(hstateen0, CSR_HSTATEEN0, "\n");
>> + X(stvec, CSR_STVEC, " "); X(vstvec, CSR_VSTVEC, "\n");
>> + X(sepc, CSR_SEPC, " "); X(vsepc, CSR_VSEPC, "\n");
>
> ... the one logged here? Nothing changes the register between entry
> into the hypervisor and coming here?
Down below here you have
X(scause, CSR_SCAUSE, " [%s]\n", decode_cause(v));
which actually (largely) duplicates what do_unexpected_trap() has already
logged. If dump_csrs() gained other uses, the dumping of scause likely is
wanted, but then likely no scause value would be available to pass in? So
maybe its dumping actually wants to be conditional (and the parameter
wants to be a boolean)?
Jan
On 1/27/26 10:27 AM, Jan Beulich wrote:
> On 27.01.2026 10:14, Jan Beulich wrote:
>> On 26.01.2026 12:43, Oleksii Kurochko wrote:
>>> Provide additional context when an unexpected exception occurs by dumping
>>> the relevant Supervisor, Virtual Supervisor (VS), and Hypervisor CSRs,
>>> along with the general-purpose registers associated with the trap.
>>>
>>> Dumping VS-mode CSRs in addition to host CSRs is beneficial when analysing
>>> VS-mode traps. VSCAUSE, VSEPC, VSTVAL, and related VS state are required to
>>> properly diagnose unexpected guest traps and potential hypervisor
>>> misconfiguration.
>>> For example, on an illegal-instruction exception the hardware may record
>>> the faulting instruction in VSTVAL. If VSTVAL is zero, VSEPC should always
>>> be inspected, and can be used together with objdump to identify the
>>> faulting instruction. Dumping VSCAUSE is also useful when the guest does
>>> not report it, or when the hypervisor redirects a trap to the guest using
>>> VSCAUSE, VSTATUS, and VSTVEC, allowing verification that a subsequent trap
>>> is not caused by incorrect VS state.
>>>
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
> Hmm, wait, there's another anomaly:
>
>> I still have a question though, which can be addressed incrementally.
>>
>>> --- a/xen/arch/riscv/traps.c
>>> +++ b/xen/arch/riscv/traps.c
>>> @@ -99,12 +99,70 @@ static const char *decode_cause(unsigned long cause)
>>> return decode_trap_cause(cause);
>>> }
>>>
>>> +static void dump_general_regs(const struct cpu_user_regs *regs)
>>> +{
>>> +#define X(regs, name, delim) \
>>> + printk("%-4s: %016lx" delim, #name, (regs)->name)
>>> +
>>> + X(regs, ra, " "); X(regs, sp, "\n");
>>> + X(regs, gp, " "); X(regs, tp, "\n");
>>> + X(regs, t0, " "); X(regs, t1, "\n");
>>> + X(regs, t2, " "); X(regs, s0, "\n");
>>> + X(regs, s1, " "); X(regs, a0, "\n");
>>> + X(regs, a1, " "); X(regs, a2, "\n");
>>> + X(regs, a3, " "); X(regs, a4, "\n");
>>> + X(regs, a5, " "); X(regs, a6, "\n");
>>> + X(regs, a7, " "); X(regs, s2, "\n");
>>> + X(regs, s3, " "); X(regs, s4, "\n");
>>> + X(regs, s5, " "); X(regs, s6, "\n");
>>> + X(regs, s7, " "); X(regs, s8, "\n");
>>> + X(regs, s9, " "); X(regs, s10, "\n");
>>> + X(regs, s11, " "); X(regs, t3, "\n");
>>> + X(regs, t4, " "); X(regs, t5, "\n");
>>> + X(regs, t6, " "); X(regs, sepc, "\n");
>> Does this sepc value differ from ...
>>
>>> +static void dump_csrs(unsigned long cause)
> What is this function parameter for?
>
>>> +{
>>> +#define X(name, csr, fmt, ...) \
>>> + v = csr_read(csr); \
>>> + printk("%-10s: %016lx" fmt, #name, v, ##__VA_ARGS__)
>>> +
>>> + unsigned long v;
>>> +
>>> + X(htval, CSR_HTVAL, " "); X(htinst, CSR_HTINST, "\n");
>>> + X(hedeleg, CSR_HEDELEG, " "); X(hideleg, CSR_HIDELEG, "\n");
>>> + X(hstatus, CSR_HSTATUS, " [%s%s%s%s%s%s ]\n",
>>> + (v & HSTATUS_VTSR) ? " VTSR" : "",
>>> + (v & HSTATUS_VTVM) ? " VTVM" : "",
>>> + (v & HSTATUS_HU) ? " HU" : "",
>>> + (v & HSTATUS_SPVP) ? " SPVP" : "",
>>> + (v & HSTATUS_SPV) ? " SPV" : "",
>>> + (v & HSTATUS_GVA) ? " GVA" : "");
>>> + X(hgatp, CSR_HGATP, "\n");
>>> + X(hstateen0, CSR_HSTATEEN0, "\n");
>>> + X(stvec, CSR_STVEC, " "); X(vstvec, CSR_VSTVEC, "\n");
>>> + X(sepc, CSR_SEPC, " "); X(vsepc, CSR_VSEPC, "\n");
>> ... the one logged here? Nothing changes the register between entry
>> into the hypervisor and coming here?
> Down below here you have
>
> X(scause, CSR_SCAUSE, " [%s]\n", decode_cause(v));
>
> which actually (largely) duplicates what do_unexpected_trap() has already
> logged.
Missed that, then it would be better to remove this duplication and leave
only printing of CSR_SCAUSE in dump_csrs().
> If dump_csrs() gained other uses, the dumping of scause likely is
> wanted, but then likely no scause value would be available to pass in? So
> maybe its dumping actually wants to be conditional (and the parameter
> wants to be a boolean)?
Good point. Honestly speaking, I don't know if it will be any other usages
except this one. But to keep things generic I think it is good idea to
add conditional dumping of scause.
I will update the patch and send new version.
Thanks.
~ Oleksii
On 1/28/26 11:34 AM, Oleksii Kurochko wrote:
>
> On 1/27/26 10:27 AM, Jan Beulich wrote:
>> On 27.01.2026 10:14, Jan Beulich wrote:
>>> On 26.01.2026 12:43, Oleksii Kurochko wrote:
>>>> Provide additional context when an unexpected exception occurs by
>>>> dumping
>>>> the relevant Supervisor, Virtual Supervisor (VS), and Hypervisor CSRs,
>>>> along with the general-purpose registers associated with the trap.
>>>>
>>>> Dumping VS-mode CSRs in addition to host CSRs is beneficial when
>>>> analysing
>>>> VS-mode traps. VSCAUSE, VSEPC, VSTVAL, and related VS state are
>>>> required to
>>>> properly diagnose unexpected guest traps and potential hypervisor
>>>> misconfiguration.
>>>> For example, on an illegal-instruction exception the hardware may
>>>> record
>>>> the faulting instruction in VSTVAL. If VSTVAL is zero, VSEPC should
>>>> always
>>>> be inspected, and can be used together with objdump to identify the
>>>> faulting instruction. Dumping VSCAUSE is also useful when the guest
>>>> does
>>>> not report it, or when the hypervisor redirects a trap to the guest
>>>> using
>>>> VSCAUSE, VSTATUS, and VSTVEC, allowing verification that a
>>>> subsequent trap
>>>> is not caused by incorrect VS state.
>>>>
>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> Hmm, wait, there's another anomaly:
>>
>>> I still have a question though, which can be addressed incrementally.
>>>
>>>> --- a/xen/arch/riscv/traps.c
>>>> +++ b/xen/arch/riscv/traps.c
>>>> @@ -99,12 +99,70 @@ static const char *decode_cause(unsigned long
>>>> cause)
>>>> return decode_trap_cause(cause);
>>>> }
>>>> +static void dump_general_regs(const struct cpu_user_regs *regs)
>>>> +{
>>>> +#define X(regs, name, delim) \
>>>> + printk("%-4s: %016lx" delim, #name, (regs)->name)
>>>> +
>>>> + X(regs, ra, " "); X(regs, sp, "\n");
>>>> + X(regs, gp, " "); X(regs, tp, "\n");
>>>> + X(regs, t0, " "); X(regs, t1, "\n");
>>>> + X(regs, t2, " "); X(regs, s0, "\n");
>>>> + X(regs, s1, " "); X(regs, a0, "\n");
>>>> + X(regs, a1, " "); X(regs, a2, "\n");
>>>> + X(regs, a3, " "); X(regs, a4, "\n");
>>>> + X(regs, a5, " "); X(regs, a6, "\n");
>>>> + X(regs, a7, " "); X(regs, s2, "\n");
>>>> + X(regs, s3, " "); X(regs, s4, "\n");
>>>> + X(regs, s5, " "); X(regs, s6, "\n");
>>>> + X(regs, s7, " "); X(regs, s8, "\n");
>>>> + X(regs, s9, " "); X(regs, s10, "\n");
>>>> + X(regs, s11, " "); X(regs, t3, "\n");
>>>> + X(regs, t4, " "); X(regs, t5, "\n");
>>>> + X(regs, t6, " "); X(regs, sepc, "\n");
>>> Does this sepc value differ from ...
>>>
>>>> +static void dump_csrs(unsigned long cause)
>> What is this function parameter for?
>>
>>>> +{
>>>> +#define X(name, csr, fmt, ...) \
>>>> + v = csr_read(csr); \
>>>> + printk("%-10s: %016lx" fmt, #name, v, ##__VA_ARGS__)
>>>> +
>>>> + unsigned long v;
>>>> +
>>>> + X(htval, CSR_HTVAL, " "); X(htinst, CSR_HTINST, "\n");
>>>> + X(hedeleg, CSR_HEDELEG, " "); X(hideleg, CSR_HIDELEG, "\n");
>>>> + X(hstatus, CSR_HSTATUS, " [%s%s%s%s%s%s ]\n",
>>>> + (v & HSTATUS_VTSR) ? " VTSR" : "",
>>>> + (v & HSTATUS_VTVM) ? " VTVM" : "",
>>>> + (v & HSTATUS_HU) ? " HU" : "",
>>>> + (v & HSTATUS_SPVP) ? " SPVP" : "",
>>>> + (v & HSTATUS_SPV) ? " SPV" : "",
>>>> + (v & HSTATUS_GVA) ? " GVA" : "");
>>>> + X(hgatp, CSR_HGATP, "\n");
>>>> + X(hstateen0, CSR_HSTATEEN0, "\n");
>>>> + X(stvec, CSR_STVEC, " "); X(vstvec, CSR_VSTVEC, "\n");
>>>> + X(sepc, CSR_SEPC, " "); X(vsepc, CSR_VSEPC, "\n");
>>> ... the one logged here? Nothing changes the register between entry
>>> into the hypervisor and coming here?
>> Down below here you have
>>
>> X(scause, CSR_SCAUSE, " [%s]\n", decode_cause(v));
>>
>> which actually (largely) duplicates what do_unexpected_trap() has
>> already
>> logged.
>
> Missed that, then it would be better to remove this duplication and leave
> only printing of CSR_SCAUSE in dump_csrs().
>
>> If dump_csrs() gained other uses, the dumping of scause likely is
>> wanted, but then likely no scause value would be available to pass
>> in? So
>> maybe its dumping actually wants to be conditional (and the parameter
>> wants to be a boolean)?
>
> Good point. Honestly speaking, I don't know if it will be any other
> usages
> except this one. But to keep things generic I think it is good idea to
> add conditional dumping of scause.
As an alternative, we could simply remove the dump_csrs() argument and always
print SCAUSE. When running in hypervisor mode, SCAUSE should contain the reason
for the trap. Even it is lets say just hypercall and not something faulty, it
will contain "Environment call from S-mode" what looks okay to be printed.
I tend to prefer this approach slightly. What do you think?
~ Oleksii
On 28.01.2026 12:01, Oleksii Kurochko wrote:
>
> On 1/28/26 11:34 AM, Oleksii Kurochko wrote:
>>
>> On 1/27/26 10:27 AM, Jan Beulich wrote:
>>> On 27.01.2026 10:14, Jan Beulich wrote:
>>>> On 26.01.2026 12:43, Oleksii Kurochko wrote:
>>>>> Provide additional context when an unexpected exception occurs by
>>>>> dumping
>>>>> the relevant Supervisor, Virtual Supervisor (VS), and Hypervisor CSRs,
>>>>> along with the general-purpose registers associated with the trap.
>>>>>
>>>>> Dumping VS-mode CSRs in addition to host CSRs is beneficial when
>>>>> analysing
>>>>> VS-mode traps. VSCAUSE, VSEPC, VSTVAL, and related VS state are
>>>>> required to
>>>>> properly diagnose unexpected guest traps and potential hypervisor
>>>>> misconfiguration.
>>>>> For example, on an illegal-instruction exception the hardware may
>>>>> record
>>>>> the faulting instruction in VSTVAL. If VSTVAL is zero, VSEPC should
>>>>> always
>>>>> be inspected, and can be used together with objdump to identify the
>>>>> faulting instruction. Dumping VSCAUSE is also useful when the guest
>>>>> does
>>>>> not report it, or when the hypervisor redirects a trap to the guest
>>>>> using
>>>>> VSCAUSE, VSTATUS, and VSTVEC, allowing verification that a
>>>>> subsequent trap
>>>>> is not caused by incorrect VS state.
>>>>>
>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>> Hmm, wait, there's another anomaly:
>>>
>>>> I still have a question though, which can be addressed incrementally.
>>>>
>>>>> --- a/xen/arch/riscv/traps.c
>>>>> +++ b/xen/arch/riscv/traps.c
>>>>> @@ -99,12 +99,70 @@ static const char *decode_cause(unsigned long
>>>>> cause)
>>>>> return decode_trap_cause(cause);
>>>>> }
>>>>> +static void dump_general_regs(const struct cpu_user_regs *regs)
>>>>> +{
>>>>> +#define X(regs, name, delim) \
>>>>> + printk("%-4s: %016lx" delim, #name, (regs)->name)
>>>>> +
>>>>> + X(regs, ra, " "); X(regs, sp, "\n");
>>>>> + X(regs, gp, " "); X(regs, tp, "\n");
>>>>> + X(regs, t0, " "); X(regs, t1, "\n");
>>>>> + X(regs, t2, " "); X(regs, s0, "\n");
>>>>> + X(regs, s1, " "); X(regs, a0, "\n");
>>>>> + X(regs, a1, " "); X(regs, a2, "\n");
>>>>> + X(regs, a3, " "); X(regs, a4, "\n");
>>>>> + X(regs, a5, " "); X(regs, a6, "\n");
>>>>> + X(regs, a7, " "); X(regs, s2, "\n");
>>>>> + X(regs, s3, " "); X(regs, s4, "\n");
>>>>> + X(regs, s5, " "); X(regs, s6, "\n");
>>>>> + X(regs, s7, " "); X(regs, s8, "\n");
>>>>> + X(regs, s9, " "); X(regs, s10, "\n");
>>>>> + X(regs, s11, " "); X(regs, t3, "\n");
>>>>> + X(regs, t4, " "); X(regs, t5, "\n");
>>>>> + X(regs, t6, " "); X(regs, sepc, "\n");
>>>> Does this sepc value differ from ...
>>>>
>>>>> +static void dump_csrs(unsigned long cause)
>>> What is this function parameter for?
>>>
>>>>> +{
>>>>> +#define X(name, csr, fmt, ...) \
>>>>> + v = csr_read(csr); \
>>>>> + printk("%-10s: %016lx" fmt, #name, v, ##__VA_ARGS__)
>>>>> +
>>>>> + unsigned long v;
>>>>> +
>>>>> + X(htval, CSR_HTVAL, " "); X(htinst, CSR_HTINST, "\n");
>>>>> + X(hedeleg, CSR_HEDELEG, " "); X(hideleg, CSR_HIDELEG, "\n");
>>>>> + X(hstatus, CSR_HSTATUS, " [%s%s%s%s%s%s ]\n",
>>>>> + (v & HSTATUS_VTSR) ? " VTSR" : "",
>>>>> + (v & HSTATUS_VTVM) ? " VTVM" : "",
>>>>> + (v & HSTATUS_HU) ? " HU" : "",
>>>>> + (v & HSTATUS_SPVP) ? " SPVP" : "",
>>>>> + (v & HSTATUS_SPV) ? " SPV" : "",
>>>>> + (v & HSTATUS_GVA) ? " GVA" : "");
>>>>> + X(hgatp, CSR_HGATP, "\n");
>>>>> + X(hstateen0, CSR_HSTATEEN0, "\n");
>>>>> + X(stvec, CSR_STVEC, " "); X(vstvec, CSR_VSTVEC, "\n");
>>>>> + X(sepc, CSR_SEPC, " "); X(vsepc, CSR_VSEPC, "\n");
>>>> ... the one logged here? Nothing changes the register between entry
>>>> into the hypervisor and coming here?
>>> Down below here you have
>>>
>>> X(scause, CSR_SCAUSE, " [%s]\n", decode_cause(v));
>>>
>>> which actually (largely) duplicates what do_unexpected_trap() has
>>> already
>>> logged.
>>
>> Missed that, then it would be better to remove this duplication and leave
>> only printing of CSR_SCAUSE in dump_csrs().
>>
>>> If dump_csrs() gained other uses, the dumping of scause likely is
>>> wanted, but then likely no scause value would be available to pass
>>> in? So
>>> maybe its dumping actually wants to be conditional (and the parameter
>>> wants to be a boolean)?
>>
>> Good point. Honestly speaking, I don't know if it will be any other
>> usages
>> except this one. But to keep things generic I think it is good idea to
>> add conditional dumping of scause.
>
> As an alternative, we could simply remove the dump_csrs() argument and always
> print SCAUSE. When running in hypervisor mode, SCAUSE should contain the reason
> for the trap. Even it is lets say just hypercall and not something faulty, it
> will contain "Environment call from S-mode" what looks okay to be printed.
>
> I tend to prefer this approach slightly. What do you think?
It means that it'll be logged twice for an unexpected trap. As said, I can
only recommend to limit the volume of the output in such situations, as
sometimes all people may be able to get you is screenshots.
Jan
On 1/28/26 12:08 PM, Jan Beulich wrote:
> On 28.01.2026 12:01, Oleksii Kurochko wrote:
>> On 1/28/26 11:34 AM, Oleksii Kurochko wrote:
>>> On 1/27/26 10:27 AM, Jan Beulich wrote:
>>>> On 27.01.2026 10:14, Jan Beulich wrote:
>>>>> On 26.01.2026 12:43, Oleksii Kurochko wrote:
>>>>>> Provide additional context when an unexpected exception occurs by
>>>>>> dumping
>>>>>> the relevant Supervisor, Virtual Supervisor (VS), and Hypervisor CSRs,
>>>>>> along with the general-purpose registers associated with the trap.
>>>>>>
>>>>>> Dumping VS-mode CSRs in addition to host CSRs is beneficial when
>>>>>> analysing
>>>>>> VS-mode traps. VSCAUSE, VSEPC, VSTVAL, and related VS state are
>>>>>> required to
>>>>>> properly diagnose unexpected guest traps and potential hypervisor
>>>>>> misconfiguration.
>>>>>> For example, on an illegal-instruction exception the hardware may
>>>>>> record
>>>>>> the faulting instruction in VSTVAL. If VSTVAL is zero, VSEPC should
>>>>>> always
>>>>>> be inspected, and can be used together with objdump to identify the
>>>>>> faulting instruction. Dumping VSCAUSE is also useful when the guest
>>>>>> does
>>>>>> not report it, or when the hypervisor redirects a trap to the guest
>>>>>> using
>>>>>> VSCAUSE, VSTATUS, and VSTVEC, allowing verification that a
>>>>>> subsequent trap
>>>>>> is not caused by incorrect VS state.
>>>>>>
>>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>> Hmm, wait, there's another anomaly:
>>>>
>>>>> I still have a question though, which can be addressed incrementally.
>>>>>
>>>>>> --- a/xen/arch/riscv/traps.c
>>>>>> +++ b/xen/arch/riscv/traps.c
>>>>>> @@ -99,12 +99,70 @@ static const char *decode_cause(unsigned long
>>>>>> cause)
>>>>>> return decode_trap_cause(cause);
>>>>>> }
>>>>>> +static void dump_general_regs(const struct cpu_user_regs *regs)
>>>>>> +{
>>>>>> +#define X(regs, name, delim) \
>>>>>> + printk("%-4s: %016lx" delim, #name, (regs)->name)
>>>>>> +
>>>>>> + X(regs, ra, " "); X(regs, sp, "\n");
>>>>>> + X(regs, gp, " "); X(regs, tp, "\n");
>>>>>> + X(regs, t0, " "); X(regs, t1, "\n");
>>>>>> + X(regs, t2, " "); X(regs, s0, "\n");
>>>>>> + X(regs, s1, " "); X(regs, a0, "\n");
>>>>>> + X(regs, a1, " "); X(regs, a2, "\n");
>>>>>> + X(regs, a3, " "); X(regs, a4, "\n");
>>>>>> + X(regs, a5, " "); X(regs, a6, "\n");
>>>>>> + X(regs, a7, " "); X(regs, s2, "\n");
>>>>>> + X(regs, s3, " "); X(regs, s4, "\n");
>>>>>> + X(regs, s5, " "); X(regs, s6, "\n");
>>>>>> + X(regs, s7, " "); X(regs, s8, "\n");
>>>>>> + X(regs, s9, " "); X(regs, s10, "\n");
>>>>>> + X(regs, s11, " "); X(regs, t3, "\n");
>>>>>> + X(regs, t4, " "); X(regs, t5, "\n");
>>>>>> + X(regs, t6, " "); X(regs, sepc, "\n");
>>>>> Does this sepc value differ from ...
>>>>>
>>>>>> +static void dump_csrs(unsigned long cause)
>>>> What is this function parameter for?
>>>>
>>>>>> +{
>>>>>> +#define X(name, csr, fmt, ...) \
>>>>>> + v = csr_read(csr); \
>>>>>> + printk("%-10s: %016lx" fmt, #name, v, ##__VA_ARGS__)
>>>>>> +
>>>>>> + unsigned long v;
>>>>>> +
>>>>>> + X(htval, CSR_HTVAL, " "); X(htinst, CSR_HTINST, "\n");
>>>>>> + X(hedeleg, CSR_HEDELEG, " "); X(hideleg, CSR_HIDELEG, "\n");
>>>>>> + X(hstatus, CSR_HSTATUS, " [%s%s%s%s%s%s ]\n",
>>>>>> + (v & HSTATUS_VTSR) ? " VTSR" : "",
>>>>>> + (v & HSTATUS_VTVM) ? " VTVM" : "",
>>>>>> + (v & HSTATUS_HU) ? " HU" : "",
>>>>>> + (v & HSTATUS_SPVP) ? " SPVP" : "",
>>>>>> + (v & HSTATUS_SPV) ? " SPV" : "",
>>>>>> + (v & HSTATUS_GVA) ? " GVA" : "");
>>>>>> + X(hgatp, CSR_HGATP, "\n");
>>>>>> + X(hstateen0, CSR_HSTATEEN0, "\n");
>>>>>> + X(stvec, CSR_STVEC, " "); X(vstvec, CSR_VSTVEC, "\n");
>>>>>> + X(sepc, CSR_SEPC, " "); X(vsepc, CSR_VSEPC, "\n");
>>>>> ... the one logged here? Nothing changes the register between entry
>>>>> into the hypervisor and coming here?
>>>> Down below here you have
>>>>
>>>> X(scause, CSR_SCAUSE, " [%s]\n", decode_cause(v));
>>>>
>>>> which actually (largely) duplicates what do_unexpected_trap() has
>>>> already
>>>> logged.
>>> Missed that, then it would be better to remove this duplication and leave
>>> only printing of CSR_SCAUSE in dump_csrs().
>>>
>>>> If dump_csrs() gained other uses, the dumping of scause likely is
>>>> wanted, but then likely no scause value would be available to pass
>>>> in? So
>>>> maybe its dumping actually wants to be conditional (and the parameter
>>>> wants to be a boolean)?
>>> Good point. Honestly speaking, I don't know if it will be any other
>>> usages
>>> except this one. But to keep things generic I think it is good idea to
>>> add conditional dumping of scause.
>> As an alternative, we could simply remove the dump_csrs() argument and always
>> print SCAUSE. When running in hypervisor mode, SCAUSE should contain the reason
>> for the trap. Even it is lets say just hypercall and not something faulty, it
>> will contain "Environment call from S-mode" what looks okay to be printed.
>>
>> I tend to prefer this approach slightly. What do you think?
> It means that it'll be logged twice for an unexpected trap. As said, I can
> only recommend to limit the volume of the output in such situations, as
> sometimes all people may be able to get you is screenshots.
It will be logged once, basically, mu suggestion is:
-static void dump_csrs(unsigned long cause)
+static void dump_csrs(void)
{
#define X(name, csr, fmt, ...) \
v = csr_read(csr); \
@@ -156,11 +156,7 @@ static void dump_csrs(unsigned long cause)
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_csrs();
dump_general_regs(regs);
~ Oleksii
On 28.01.2026 12:22, Oleksii Kurochko wrote:
>
> On 1/28/26 12:08 PM, Jan Beulich wrote:
>> On 28.01.2026 12:01, Oleksii Kurochko wrote:
>>> On 1/28/26 11:34 AM, Oleksii Kurochko wrote:
>>>> On 1/27/26 10:27 AM, Jan Beulich wrote:
>>>>> On 27.01.2026 10:14, Jan Beulich wrote:
>>>>>> On 26.01.2026 12:43, Oleksii Kurochko wrote:
>>>>>>> Provide additional context when an unexpected exception occurs by
>>>>>>> dumping
>>>>>>> the relevant Supervisor, Virtual Supervisor (VS), and Hypervisor CSRs,
>>>>>>> along with the general-purpose registers associated with the trap.
>>>>>>>
>>>>>>> Dumping VS-mode CSRs in addition to host CSRs is beneficial when
>>>>>>> analysing
>>>>>>> VS-mode traps. VSCAUSE, VSEPC, VSTVAL, and related VS state are
>>>>>>> required to
>>>>>>> properly diagnose unexpected guest traps and potential hypervisor
>>>>>>> misconfiguration.
>>>>>>> For example, on an illegal-instruction exception the hardware may
>>>>>>> record
>>>>>>> the faulting instruction in VSTVAL. If VSTVAL is zero, VSEPC should
>>>>>>> always
>>>>>>> be inspected, and can be used together with objdump to identify the
>>>>>>> faulting instruction. Dumping VSCAUSE is also useful when the guest
>>>>>>> does
>>>>>>> not report it, or when the hypervisor redirects a trap to the guest
>>>>>>> using
>>>>>>> VSCAUSE, VSTATUS, and VSTVEC, allowing verification that a
>>>>>>> subsequent trap
>>>>>>> is not caused by incorrect VS state.
>>>>>>>
>>>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>> Hmm, wait, there's another anomaly:
>>>>>
>>>>>> I still have a question though, which can be addressed incrementally.
>>>>>>
>>>>>>> --- a/xen/arch/riscv/traps.c
>>>>>>> +++ b/xen/arch/riscv/traps.c
>>>>>>> @@ -99,12 +99,70 @@ static const char *decode_cause(unsigned long
>>>>>>> cause)
>>>>>>> return decode_trap_cause(cause);
>>>>>>> }
>>>>>>> +static void dump_general_regs(const struct cpu_user_regs *regs)
>>>>>>> +{
>>>>>>> +#define X(regs, name, delim) \
>>>>>>> + printk("%-4s: %016lx" delim, #name, (regs)->name)
>>>>>>> +
>>>>>>> + X(regs, ra, " "); X(regs, sp, "\n");
>>>>>>> + X(regs, gp, " "); X(regs, tp, "\n");
>>>>>>> + X(regs, t0, " "); X(regs, t1, "\n");
>>>>>>> + X(regs, t2, " "); X(regs, s0, "\n");
>>>>>>> + X(regs, s1, " "); X(regs, a0, "\n");
>>>>>>> + X(regs, a1, " "); X(regs, a2, "\n");
>>>>>>> + X(regs, a3, " "); X(regs, a4, "\n");
>>>>>>> + X(regs, a5, " "); X(regs, a6, "\n");
>>>>>>> + X(regs, a7, " "); X(regs, s2, "\n");
>>>>>>> + X(regs, s3, " "); X(regs, s4, "\n");
>>>>>>> + X(regs, s5, " "); X(regs, s6, "\n");
>>>>>>> + X(regs, s7, " "); X(regs, s8, "\n");
>>>>>>> + X(regs, s9, " "); X(regs, s10, "\n");
>>>>>>> + X(regs, s11, " "); X(regs, t3, "\n");
>>>>>>> + X(regs, t4, " "); X(regs, t5, "\n");
>>>>>>> + X(regs, t6, " "); X(regs, sepc, "\n");
>>>>>> Does this sepc value differ from ...
>>>>>>
>>>>>>> +static void dump_csrs(unsigned long cause)
>>>>> What is this function parameter for?
>>>>>
>>>>>>> +{
>>>>>>> +#define X(name, csr, fmt, ...) \
>>>>>>> + v = csr_read(csr); \
>>>>>>> + printk("%-10s: %016lx" fmt, #name, v, ##__VA_ARGS__)
>>>>>>> +
>>>>>>> + unsigned long v;
>>>>>>> +
>>>>>>> + X(htval, CSR_HTVAL, " "); X(htinst, CSR_HTINST, "\n");
>>>>>>> + X(hedeleg, CSR_HEDELEG, " "); X(hideleg, CSR_HIDELEG, "\n");
>>>>>>> + X(hstatus, CSR_HSTATUS, " [%s%s%s%s%s%s ]\n",
>>>>>>> + (v & HSTATUS_VTSR) ? " VTSR" : "",
>>>>>>> + (v & HSTATUS_VTVM) ? " VTVM" : "",
>>>>>>> + (v & HSTATUS_HU) ? " HU" : "",
>>>>>>> + (v & HSTATUS_SPVP) ? " SPVP" : "",
>>>>>>> + (v & HSTATUS_SPV) ? " SPV" : "",
>>>>>>> + (v & HSTATUS_GVA) ? " GVA" : "");
>>>>>>> + X(hgatp, CSR_HGATP, "\n");
>>>>>>> + X(hstateen0, CSR_HSTATEEN0, "\n");
>>>>>>> + X(stvec, CSR_STVEC, " "); X(vstvec, CSR_VSTVEC, "\n");
>>>>>>> + X(sepc, CSR_SEPC, " "); X(vsepc, CSR_VSEPC, "\n");
>>>>>> ... the one logged here? Nothing changes the register between entry
>>>>>> into the hypervisor and coming here?
>>>>> Down below here you have
>>>>>
>>>>> X(scause, CSR_SCAUSE, " [%s]\n", decode_cause(v));
>>>>>
>>>>> which actually (largely) duplicates what do_unexpected_trap() has
>>>>> already
>>>>> logged.
>>>> Missed that, then it would be better to remove this duplication and leave
>>>> only printing of CSR_SCAUSE in dump_csrs().
>>>>
>>>>> If dump_csrs() gained other uses, the dumping of scause likely is
>>>>> wanted, but then likely no scause value would be available to pass
>>>>> in? So
>>>>> maybe its dumping actually wants to be conditional (and the parameter
>>>>> wants to be a boolean)?
>>>> Good point. Honestly speaking, I don't know if it will be any other
>>>> usages
>>>> except this one. But to keep things generic I think it is good idea to
>>>> add conditional dumping of scause.
>>> As an alternative, we could simply remove the dump_csrs() argument and always
>>> print SCAUSE. When running in hypervisor mode, SCAUSE should contain the reason
>>> for the trap. Even it is lets say just hypercall and not something faulty, it
>>> will contain "Environment call from S-mode" what looks okay to be printed.
>>>
>>> I tend to prefer this approach slightly. What do you think?
>> It means that it'll be logged twice for an unexpected trap. As said, I can
>> only recommend to limit the volume of the output in such situations, as
>> sometimes all people may be able to get you is screenshots.
>
> It will be logged once, basically, mu suggestion is:
>
> -static void dump_csrs(unsigned long cause)
> +static void dump_csrs(void)
> {
> #define X(name, csr, fmt, ...) \
> v = csr_read(csr); \
> @@ -156,11 +156,7 @@ static void dump_csrs(unsigned long cause)
>
> 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_csrs();
> dump_general_regs(regs);
And the fact that it was an unhandled exception then isn't logged explicitly
anymore? Perhaps dump_csrs() then should have a const char * parameter, which
here you would pass "Unhandled exception" for.
Jan
On 1/28/26 12:27 PM, Jan Beulich wrote:
> On 28.01.2026 12:22, Oleksii Kurochko wrote:
>> On 1/28/26 12:08 PM, Jan Beulich wrote:
>>> On 28.01.2026 12:01, Oleksii Kurochko wrote:
>>>> On 1/28/26 11:34 AM, Oleksii Kurochko wrote:
>>>>> On 1/27/26 10:27 AM, Jan Beulich wrote:
>>>>>> On 27.01.2026 10:14, Jan Beulich wrote:
>>>>>>> On 26.01.2026 12:43, Oleksii Kurochko wrote:
>>>>>>>> Provide additional context when an unexpected exception occurs by
>>>>>>>> dumping
>>>>>>>> the relevant Supervisor, Virtual Supervisor (VS), and Hypervisor CSRs,
>>>>>>>> along with the general-purpose registers associated with the trap.
>>>>>>>>
>>>>>>>> Dumping VS-mode CSRs in addition to host CSRs is beneficial when
>>>>>>>> analysing
>>>>>>>> VS-mode traps. VSCAUSE, VSEPC, VSTVAL, and related VS state are
>>>>>>>> required to
>>>>>>>> properly diagnose unexpected guest traps and potential hypervisor
>>>>>>>> misconfiguration.
>>>>>>>> For example, on an illegal-instruction exception the hardware may
>>>>>>>> record
>>>>>>>> the faulting instruction in VSTVAL. If VSTVAL is zero, VSEPC should
>>>>>>>> always
>>>>>>>> be inspected, and can be used together with objdump to identify the
>>>>>>>> faulting instruction. Dumping VSCAUSE is also useful when the guest
>>>>>>>> does
>>>>>>>> not report it, or when the hypervisor redirects a trap to the guest
>>>>>>>> using
>>>>>>>> VSCAUSE, VSTATUS, and VSTVEC, allowing verification that a
>>>>>>>> subsequent trap
>>>>>>>> is not caused by incorrect VS state.
>>>>>>>>
>>>>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>>> Hmm, wait, there's another anomaly:
>>>>>>
>>>>>>> I still have a question though, which can be addressed incrementally.
>>>>>>>
>>>>>>>> --- a/xen/arch/riscv/traps.c
>>>>>>>> +++ b/xen/arch/riscv/traps.c
>>>>>>>> @@ -99,12 +99,70 @@ static const char *decode_cause(unsigned long
>>>>>>>> cause)
>>>>>>>> return decode_trap_cause(cause);
>>>>>>>> }
>>>>>>>> +static void dump_general_regs(const struct cpu_user_regs *regs)
>>>>>>>> +{
>>>>>>>> +#define X(regs, name, delim) \
>>>>>>>> + printk("%-4s: %016lx" delim, #name, (regs)->name)
>>>>>>>> +
>>>>>>>> + X(regs, ra, " "); X(regs, sp, "\n");
>>>>>>>> + X(regs, gp, " "); X(regs, tp, "\n");
>>>>>>>> + X(regs, t0, " "); X(regs, t1, "\n");
>>>>>>>> + X(regs, t2, " "); X(regs, s0, "\n");
>>>>>>>> + X(regs, s1, " "); X(regs, a0, "\n");
>>>>>>>> + X(regs, a1, " "); X(regs, a2, "\n");
>>>>>>>> + X(regs, a3, " "); X(regs, a4, "\n");
>>>>>>>> + X(regs, a5, " "); X(regs, a6, "\n");
>>>>>>>> + X(regs, a7, " "); X(regs, s2, "\n");
>>>>>>>> + X(regs, s3, " "); X(regs, s4, "\n");
>>>>>>>> + X(regs, s5, " "); X(regs, s6, "\n");
>>>>>>>> + X(regs, s7, " "); X(regs, s8, "\n");
>>>>>>>> + X(regs, s9, " "); X(regs, s10, "\n");
>>>>>>>> + X(regs, s11, " "); X(regs, t3, "\n");
>>>>>>>> + X(regs, t4, " "); X(regs, t5, "\n");
>>>>>>>> + X(regs, t6, " "); X(regs, sepc, "\n");
>>>>>>> Does this sepc value differ from ...
>>>>>>>
>>>>>>>> +static void dump_csrs(unsigned long cause)
>>>>>> What is this function parameter for?
>>>>>>
>>>>>>>> +{
>>>>>>>> +#define X(name, csr, fmt, ...) \
>>>>>>>> + v = csr_read(csr); \
>>>>>>>> + printk("%-10s: %016lx" fmt, #name, v, ##__VA_ARGS__)
>>>>>>>> +
>>>>>>>> + unsigned long v;
>>>>>>>> +
>>>>>>>> + X(htval, CSR_HTVAL, " "); X(htinst, CSR_HTINST, "\n");
>>>>>>>> + X(hedeleg, CSR_HEDELEG, " "); X(hideleg, CSR_HIDELEG, "\n");
>>>>>>>> + X(hstatus, CSR_HSTATUS, " [%s%s%s%s%s%s ]\n",
>>>>>>>> + (v & HSTATUS_VTSR) ? " VTSR" : "",
>>>>>>>> + (v & HSTATUS_VTVM) ? " VTVM" : "",
>>>>>>>> + (v & HSTATUS_HU) ? " HU" : "",
>>>>>>>> + (v & HSTATUS_SPVP) ? " SPVP" : "",
>>>>>>>> + (v & HSTATUS_SPV) ? " SPV" : "",
>>>>>>>> + (v & HSTATUS_GVA) ? " GVA" : "");
>>>>>>>> + X(hgatp, CSR_HGATP, "\n");
>>>>>>>> + X(hstateen0, CSR_HSTATEEN0, "\n");
>>>>>>>> + X(stvec, CSR_STVEC, " "); X(vstvec, CSR_VSTVEC, "\n");
>>>>>>>> + X(sepc, CSR_SEPC, " "); X(vsepc, CSR_VSEPC, "\n");
>>>>>>> ... the one logged here? Nothing changes the register between entry
>>>>>>> into the hypervisor and coming here?
>>>>>> Down below here you have
>>>>>>
>>>>>> X(scause, CSR_SCAUSE, " [%s]\n", decode_cause(v));
>>>>>>
>>>>>> which actually (largely) duplicates what do_unexpected_trap() has
>>>>>> already
>>>>>> logged.
>>>>> Missed that, then it would be better to remove this duplication and leave
>>>>> only printing of CSR_SCAUSE in dump_csrs().
>>>>>
>>>>>> If dump_csrs() gained other uses, the dumping of scause likely is
>>>>>> wanted, but then likely no scause value would be available to pass
>>>>>> in? So
>>>>>> maybe its dumping actually wants to be conditional (and the parameter
>>>>>> wants to be a boolean)?
>>>>> Good point. Honestly speaking, I don't know if it will be any other
>>>>> usages
>>>>> except this one. But to keep things generic I think it is good idea to
>>>>> add conditional dumping of scause.
>>>> As an alternative, we could simply remove the dump_csrs() argument and always
>>>> print SCAUSE. When running in hypervisor mode, SCAUSE should contain the reason
>>>> for the trap. Even it is lets say just hypercall and not something faulty, it
>>>> will contain "Environment call from S-mode" what looks okay to be printed.
>>>>
>>>> I tend to prefer this approach slightly. What do you think?
>>> It means that it'll be logged twice for an unexpected trap. As said, I can
>>> only recommend to limit the volume of the output in such situations, as
>>> sometimes all people may be able to get you is screenshots.
>> It will be logged once, basically, mu suggestion is:
>>
>> -static void dump_csrs(unsigned long cause)
>> +static void dump_csrs(void)
>> {
>> #define X(name, csr, fmt, ...) \
>> v = csr_read(csr); \
>> @@ -156,11 +156,7 @@ static void dump_csrs(unsigned long cause)
>>
>> 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_csrs();
>> dump_general_regs(regs);
> And the fact that it was an unhandled exception then isn't logged explicitly
> anymore? Perhaps dump_csrs() then should have a const char * parameter, which
> here you would pass "Unhandled exception" for.
Yes, it will be lost, I thought it would be enough to see SCAUSE, but agree
it would a little bit more convenient to have message: "Unhandled exception".
~ Oleksii
© 2016 - 2026 Red Hat, Inc.