Re: [PATCH v3 02/16] xen/riscv: avoid reading hstateen0 when Smstateen is not implemented

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/11/26 8:22 AM, Jan Beulich wrote:
> On 09.02.2026 17:52, Oleksii Kurochko wrote:
> > If the Smstateen extension is not implemented, the hstateen0 CSR is
> > considered non-existent. Any attempt to access it will raise an
> > illegal-instruction exception.
> >
> > Guard the hstateen0 dump with a runtime check for Smstateen support to
> > avoid triggering traps when dumping CSRs on systems that do not
> > implement this extension.
> >
> > Fixes: 3babc8d2e546 ("xen/riscv: dump GPRs and CSRs on unexpected traps")
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
Thanks.

> It is, aiui, independent of patch 1 and hence can go in right away.
Yes, it is independent.

> > @@ -144,7 +145,12 @@ static void dump_csrs(const char *ctx)
> >         (v & HSTATUS_SPV)  ? " SPV"  : "",
> >         (v & HSTATUS_GVA)  ? " GVA"  : "");
> >       X(hgatp, CSR_HGATP, "\n");
> > -    X(hstateen0, CSR_HSTATEEN0, "\n");
> > +
> > +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
> > +    {
> > +        X(hstateen0, CSR_HSTATEEN0, "\n");
> > +    }
> I was going to ask for the braces to be dropped, but I notice they are
> required as long as X() isn't properly adjusted. This is why even for
> local use macros we should take a little more care when introducing
> them, so they can be used without having to pay too close attention to
> their actual implementation.
It would be better to have do {...} while(0) in the definition of X macros.

I will add do {...} while(0) to one of the next patches where I have the same
case.

I don't mind to update X() in dump_csrs() in the next patch version to:
#define X(name, csr, fmt, ...) do { \
    v = csr_read(csr); \
    printk("%-10s: %016lx" fmt, #name, v, ##__VA_ARGS__); \
while(0)

~ Oleksii