Re: [PATCH v3 03/39] xen/riscv: introduce csr_volatile_read64()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/8/26 4:46 PM, Jan Beulich wrote:
> On 30.09.2026 18:18, Oleksii Kurochko wrote:
> > On RV32 a 64-bit CSR is accessed as two halves, <csr> and <csr>H. Reading
> > the halves with two separate instructions isn't always safe: for a CSR
> > which hardware increments, a carry from the low half into the high one can
> > happen in between, so a plain pair of reads can produce a value the CSR
> > never held. Therefore the high half is re-read and the sequence retried if
> > it changed in the meantime.
> >
> > Only CSRs which change under Xen's feet need that, hence the name. The
> > majority can be read with a plain pair of csr_read()s, so no helper for
> > those is introduced here as there would be no user for it.
> >
> > Use it for CSR_TIME, which is exactly such a counter, and widen cycles_t to
> > uint64_t. Otherwise get_cycles() would still truncate the time counter to
> > 32 bits on RV32.
> >
> > No functional change on RV64.
> >
> > Fixes: a541ddadec0a ("xen/riscv: introduce time.h")
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
Thanks.

> Nevertheless, a few remarks below.
>
> > --- a/xen/arch/riscv/include/asm/csr.h
> > +++ b/xen/arch/riscv/include/asm/csr.h
> > @@ -39,12 +39,39 @@
> >       csr_write(csr, v_);             \
> >       csr_write(csr ## H, v_ >> 32);  \
> >   })
> > +
> > +/*
> > + * The two halves are read by separate instructions, so a CSR which hardware
> > + * increments can carry from the low half into the high one in between,
> > + * yielding a value the CSR never held. Re-read the high half and retry the
> > + * sequence if it changed.
> > + *
> > + * Only needed for CSRs which change under Xen's feet.
> > + */
> > +#define csr_volatile_read64(csr)                    \
> > +({                                                  \
> > +    uint32_t hi_, lo_, old_;                        \
> > +                                                    \
> > +    hi_ = csr_read(csr ## H);                       \
> This could be the initializer of the variable.
>
> > +    do {                                            \
> > +        old_ = hi_;                                 \
> > +        lo_ = csr_read(csr);                        \
> > +    } while ( (hi_ = csr_read(csr ## H)) != old_ ); \
> > +                                                    \
> > +    ((uint64_t)hi_ << 32) | lo_;                    \
> > +})
> >   #else
> >   #define csr_write64(csr, val)       \
> >   ({                                  \
> >       csr_write(csr, val);            \
> >       (void)csr ## H;                 \
> While I notice the same exists here already, I wonder what the purpose of
> this and ...
I can drop it in separate patch. The purpose was just for the symmetry 
with RV32 case.
> >   })
> > +
> > +#define csr_volatile_read64(csr)    \
> > +({                                  \
> > +    (void)csr ## H;                 \
> ... this is (and why their placements differ). Generally a pure 64-bit
> environment would have no need for these ...H constants, and their
> availability looks to be all that's checked for here. As Baptiste did
> point out on patch 11, these uses constitute Misra violations, which we
> then would need to deviate. I think we want to try to limit the number of
> needed deviations.
If I understood MISRA Rule 20.12 correctly (Baptiste pointed me to it on
patch 11), the issue is that the csr parameter is used both as an operand
of ## and in ordinary, macro-expanded context.

I think it would be better to just rewrite these macros so that no
deviation is needed:

#ifdef CONFIG_RISCV_32
#define csr_write64(name, val)                  \
({                                              \
    uint64_t v_ = (val);                        \
    csr_write(CSR_ ## name, v_);                \
    csr_write(CSR_ ## name ## H, v_ >> 32);     \
})

#define csr_read64(name)                        \
({                                              \
    uint64_t v_ = csr_read(CSR_ ## name ## H);  \
                                                \
    (v_ << 32) | csr_read(CSR_ ## name);        \
})
/* csr_volatile_read64() similarly */
#else
#define csr_write64(name, val)      \
({                                  \
    csr_write(CSR_ ## name, val);   \
    (void)CSR_ ## name ## H;        \
})
...
#endif

For RV64, '(void)CSR_ ## name ## H;' could be dropped altogether, and
csr_volatile_read64() would be reworked in the same way.

IIUC, with such definitions there is no violation of MISRA Rule 20.12.

> And then, seeing how patch 11 introduces an exact copy of this as
> csr_read64() - can't we simply have
>
> #define csr_volatile_read64 csr_read64
>
> ?
It could be an option, then I will move csr_read64 to this patch.

Thanks.

~ Oleksii