Re: [PATCH 1/2] virtio-ccw: fix virtio_set_ind_atomic

[David Hildenbrand <david@redhat.com>]

On 18.06.20 01:56, Halil Pasic wrote:
> On Tue, 16 Jun 2020 08:33:33 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>>>  #define atomic_cmpxchg__nocheck(ptr, old, new)    ({                    \   
>>>  
>>>      typeof_strip_qual(*ptr) _old = (old);                               \   
>>>  
>>>      (void)__atomic_compare_exchange_n(ptr, &_old, new, false,           \   
>>>  
>>>                                __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);      \   
>>>  
>>>      _old;                                                               \   
>>>  
>>>  })
>>>  
>>> ind_old is copied into _old in the macro. Instead of doing the copy from the
>>> register the compiler reloads the value from memory. The result is that _old
>>> and ind_old end up having different values. _old in r1 with the bits set
>>> already and ind_old in r10 with the bits cleared. _old gets updated by CS
>>> and matches ind_old afterwards - both with the bits being 0. So the !=
>>> compare is false and the loop is left without having set any bits.
>>>
>>>
>>> Paolo (to),
>>> I am asking myself if it would be safer to add a barrier or something like
>>> this in the macros in include/qemu/atomic.h.   
>>
>> I'm also wondering whether this has been seen on other architectures as
>> well? There are also some callers in non-s390x code, and dealing with
>> this in common code would catch them as well.
> 
> Quite a bunch of users use something like old = atomic_read(..), where
> atomic_read is documented as in docs/devel/atomics.rst:
> - ``atomic_read()`` and ``atomic_set()``; these prevent the compiler from
>   optimizing accesses out of existence and creating unsolicited
>   accesses, but do not otherwise impose any ordering on loads and
>   stores: both the compiler and the processor are free to reorder
>   them.
> 
> Maybe I should have used that instead of volatile, but my problem was
> that I didn't fully understand what atomic_read() does, and if it does
> more than we need. I found the documentation just now.

IIRC, atomic_read() is the right way of doing it, at least in the
kernel. I use such a loop in QEMU in

https://lkml.kernel.org/r/20200610115419.51688-2-david@redhat.com

But reading docs/devel/atomics.rst:"Comparison with Linux kernel
primitives" I do wonder if that is sufficient.

Any experts around?

-- 
Thanks,

David / dhildenb