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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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@gmail.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

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