On 26.01.2026 09:38, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/traps.c
> +++ b/xen/arch/riscv/traps.c
> @@ -99,12 +99,81 @@ 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 GPR_LIST \
> + 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, t4, "\n")
> +
> +#define X(regs, name, delim) \
> + printk("\t %-4s: %016lx" delim, #name, (regs)->name);
No semicolon here please; that should be supplied at the use sites of
such a macro.
As to the format string: If past the leading tab you want an extra
padding blank, why not "\t%-5s ..."? Question however is why this deep
indentation is wanted in the first place. I'd suggest to omit the tab
in particular.
> + GPR_LIST
What use is this macro? All it does is hamper readability, by requiring
the line-continuation backslashes in its definition.
> +#undef X
> +#undef GPR_LIST
> +}
> +
> +static void dump_csrs(unsigned long cause)
> +{
> +#define CSR_LIST \
> + 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\n", \
> + decode_cause(v)) \
Anything that can help save space (as indicated, output may go to a limited size
screen) should imo be leveraged. IOW better no double newline here, nor ...
> + 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(htval, CSR_HTVAL, "\n") \
> + X(htinst, CSR_HTINST, "\n") \
> + X(hedeleg, CSR_HEDELEG, "\n") \
> + X(hideleg, CSR_HIDELEG, "\n") \
> + X(hstateen0, CSR_HSTATEEN0, "\n\n")
... here. In this latter case it's questionable anyway, as apparently you
have this here to separate from the GPRs being logged subsequently. Just
that right here you don't know whether your caller actually means to do
so.
As to grouping: How about further putting hedeleg and hideleg on a single
line? Maybe also htval and htinst?
> +#define X(name, csr, fmt, ...) \
> +do { \
> + unsigned long v = csr_read(csr); \
> + printk("\t %-10s: %016lx" fmt, #name, v, ##__VA_ARGS__); \
> +} while (0);
> +
> + CSR_LIST
Same remarks here as above. The issue is actually worse here, as CSR_LIST
uses "v" which it isn't passed.
Jan
