On 31.08.2026 14:42, Oleksii Kurochko wrote:
>
>
> On 8/27/26 5:36 PM, Andrew Cooper wrote:
>> On 27/08/2026 4:20 pm, Oleksii Kurochko wrote:
>>> diff --git a/xen/arch/riscv/include/asm/csr.h
>>> b/xen/arch/riscv/include/asm/csr.h
>>> index 888d6a2a86d6..a5cdd6f99c8e 100644
>>> --- a/xen/arch/riscv/include/asm/csr.h
>>> +++ b/xen/arch/riscv/include/asm/csr.h
>>> @@ -39,12 +39,36 @@
>>> 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.
>>> + */
>>> +#define csr_read64(csr) \
>>> +({ \
>>> + uint32_t hi_, lo_; \
>>> + \
>>> + do { \
>>> + hi_ = csr_read(csr ## H); \
>>> + lo_ = csr_read(csr); \
>>> + } while ( hi_ != csr_read(csr ## H) ); \
>>> + \
>>> + ((uint64_t)hi_ << 32) | lo_; \
>>> +})
>>
>> This double reads H in the looping case. You want something more like:
>>
>> hi = csr_read();
>> do {
>> old = hi;
>> lo = csr_read();
>> } while ( (hi = csr_read()) != old );
>
> Good point. I'll apply that.
>
>>
>>
>> Still, this only matters for volatile CSRs, and is unnecessary in the
>> general case. I'd suggest naming it csr_volatile_read64().
>
> Yes, that makes sense. I will rename it to csr_volatile_read64().
>
>> Most CSRs
>> can use a simple split access.
>
> I may have misunderstood you here, but wouldn't it still make sense to
> have a macro covering the case where a register is 64-bit on RV32 yet
> accessed through two CSRs? VSIE and VSIEH, for example.
>
> My plan was to use a single csr_read64() (but while loop then really
> isn't needed in this case) call to abstract the access to VSIE, so that
> the code looks the same on RV32 and RV64.
>
> Does that make sense, or would it be better to have separate vsie and
> vsieh fields instead?
For registers which can't change under your feet (or where both halves
are independent of one another) the simpler accessor form may still be
useful. And I really mean "change under your feet", i.e. "which
hardware increments" (as you have it in the comment) is really only a
subset of the cases where csr_volatile_read64() will need using.
Jan
