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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>> --- a/xen/arch/riscv/include/asm/atomic.h
>>> +++ b/xen/arch/riscv/include/asm/atomic.h
>>> @@ -54,16 +54,16 @@ static always_inline void
>>> read_atomic_size(const volatile void *p,
>>>  })
>>>  
>>>  static always_inline void _write_atomic(volatile void *p,
>>> -                                        unsigned long x,
>>> +                                        void *x,
>>
>> Pointer-to-const please, to further aid in easily recognizing which
>> parameter is what. After all ...
>>
>>>                                          unsigned int size)
>>>  {
>>>      switch ( size )
>>>      {
>>> -    case 1: writeb_cpu(x, p); break;
>>> -    case 2: writew_cpu(x, p); break;
>>> -    case 4: writel_cpu(x, p); break;
>>
>> ... unhelpfully enough parameters are then swapped, just to confuse
>> things.
> If it would be better to keep 'unsigned long' as the type of x, then,
> if I am not mistaken, write_atomic() should be updated in the following
> way:
>    #define write_atomic(p, x)                              \
>    ({                                                      \
>        typeof(*(p)) x_ = (x);                              \
>        _write_atomic(p, *(unsigned long *)&x_, sizeof(*(p)));            
>    \
>    })
> However, I am not sure if it is safe when x is a 2-byte value (for
> example) that it will read more than 2 bytes before passing the value
> to the _write_atomic() function.

No, that's definitely unsafe.

>>> @@ -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.

Jan