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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.10.2026 16:00, Oleksii Kurochko wrote:
> On 10/8/26 4:46 PM, Jan Beulich wrote:
>> On 30.09.2026 18:18, Oleksii Kurochko wrote:
>>> --- 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.

That's my understanding, too. So afaic - yes please (preferably with the
casts to void of the *H constants also dropped).

Jan