Re: [PATCH v2 04/39] xen/riscv: introduce csr_read64()

[Jan Beulich <jbeulich@xxxxxxxx>]

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