Re: [PATCH v6 3/9] xen/riscv: allow write_atomic() to work with non-scalar types

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.09.2024 13:34, oleksii.kurochko@xxxxxxxxx wrote:
> On Tue, 2024-09-10 at 18:05 +0200, Jan Beulich wrote:
>> On 10.09.2024 17:28, oleksii.kurochko@xxxxxxxxx wrote:
>>> On Tue, 2024-09-10 at 11:53 +0200, Jan Beulich wrote:
>>>> On 02.09.2024 19:01, Oleksii Kurochko wrote:
>>>>> @@ -72,7 +72,7 @@ static always_inline void
>>>>> _write_atomic(volatile
>>>>> void *p,
>>>>>  #define write_atomic(p, x)                              \
>>>>>  ({                                                      \
>>>>>      typeof(*(p)) x_ = (x);                              \
>>>>> -    _write_atomic(p, x_, sizeof(*(p)));                 \
>>>>> +    _write_atomic(p, &x_, sizeof(*(p)));                \
>>>>>  })
>>>>>  
>>>>>  static always_inline void _add_sized(volatile void *p,
>>>>> @@ -82,27 +82,23 @@ static always_inline void
>>>>> _add_sized(volatile
>>>>> void *p,
>>>>>      {
>>>>>      case 1:
>>>>>      {
>>>>> -        volatile uint8_t *ptr = p;
>>>>> -        write_atomic(ptr, read_atomic(ptr) + x);
>>>>> +        writeb_cpu(readb_cpu(p) + x, p);
>>>>>          break;
>>>>>      }
>>>>>      case 2:
>>>>>      {
>>>>> -        volatile uint16_t *ptr = p;
>>>>> -        write_atomic(ptr, read_atomic(ptr) + x);
>>>>> +        writew_cpu(readw_cpu(p) + x, p);
>>>>>          break;
>>>>>      }
>>>>>      case 4:
>>>>>      {
>>>>> -        volatile uint32_t *ptr = p;
>>>>> -        write_atomic(ptr, read_atomic(ptr) + x);
>>>>> +        writel_cpu(readl_cpu(p) + x, p);
>>>>>          break;
>>>>>      }
>>>>>  #ifndef CONFIG_RISCV_32
>>>>>      case 8:
>>>>>      {
>>>>> -        volatile uint64_t *ptr = p;
>>>>> -        write_atomic(ptr, read_atomic(ptr) + x);
>>>>> +        writeq_cpu(readw_cpu(p) + x, p);
>>>>>          break;
>>>>>      }
>>>>>  #endif
>>>>
>>>> I'm afraid I don't understand this part, or more specifically the
>>>> respective
>>>> part of the description. It is the first parameter of
>>>> write_atomic()
>>>> which is
>>>> volatile qualified. And it is the first argument that's volatile
>>>> qualified
>>>> here. Isn't the problem entirely unrelated to volatile-ness, and
>>>> instead a
>>>> result of the other parameter changing from scalar to pointer
>>>> type,
>>>> which
>>>> doesn't fit the addition expressions you pass in?
>>> if _add_sized() is defined as it was before:
>>>    static always_inline void _add_sized(volatile void *p,
>>>                                         unsigned long x, unsigned
>>> int
>>>    size)
>>>    {
>>>        switch ( size )
>>>        {
>>>        case 1:
>>>        {
>>>            volatile uint8_t *ptr = p;
>>>            write_atomic(ptr, read_atomic(ptr) + x);
>>>            break;
>>>        }
>>>    ...
>>> Then write_atomic(ptr, read_atomic(ptr) + x) will be be changed to:
>>>    volatile uint8_t x_ = (x);
>>>    
>>> And that will cause a compiler error:
>>>    ./arch/riscv/include/asm/atomic.h:75:22: error: passing argument
>>> 2
>>>    of '_write_atomic' discards 'volatile' qualifier from pointer
>>> target
>>>    type [-Werror=discarded-qualifiers]
>>>       75 |     _write_atomic(p, &x_, sizeof(*(p)));
>>> Because it can't cast 'volatile uint8_t *' to 'void *':
>>>    expected 'void *' but argument is of type 'volatile uint8_t *'
>>> {aka
>>>    'volatile unsigned char *'}
>>
>> Oh, I think I see now. What we'd like write_atomic() to derive is the
>> bare
>> (unqualified) type of *ptr, yet iirc only recent compilers have a way
>> to
>> obtain that.
> I assume that you are speaking about typeof_unqual which requires C23
> (?).

What C version it requires doesn't matter much for our purposes. The
question is as of which gcc / clang version (if any) this is supported
as an extension.

> __auto_type seems to me can also drop volatile quilifier but in the
> docs I don't see that it should (or not) discard qualifier. Could it be
> an option:
>    #define write_atomic(p, x)                              \
>    ({                                                      \
>        __auto_type x_ = (x);                              \
>        _write_atomic(p, &x_, sizeof(*(p)));                 \
>    })

For our purposes __auto_type doesn't provide advantages over typeof().
Plus, more importantly, the use above is wrong, just like typeof(x)
would also be wrong. It needs to be p that the type is derived from.
Otherwise consider what happens when ptr is unsigned long * or
unsigned short * and you write

    write_atomic(ptr, 0);

> And another option could be just drop volatile by casting:
>    #define write_atomic(p, x)                              \
>    ...
>        _write_atomic(p, (const void *)&x_, sizeof(*(p)));                 

See what I said earlier about casts: You shall not cast away
qualifiers. Besides doing so being bad practice, you'll notice the
latest when RISC-V code also becomes subject to Misra compliance.

Jan