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

[Jan Beulich <jbeulich@xxxxxxxx>]

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>

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 ...

>  })
> +
> +#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.

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

?

Jan