[PATCH 0/2] two atomic_cmpxchg() related fixes

[PATCH 0/2] two atomic_cmpxchg() related fixes

[Halil Pasic]

The story short: compiler can generate code that does two
distinct fetches of *ind_addr for old and _old. If that happens we can
not figure out if we had the desired xchg or not.

Halil Pasic (2):
  virtio-ccw: fix virtio_set_ind_atomic
  s390x/pci: fix set_ind_atomic

 hw/s390x/s390-pci-bus.c | 16 +++++++++-------
 hw/s390x/virtio-ccw.c   | 18 ++++++++++--------
 2 files changed, 19 insertions(+), 15 deletions(-)


base-commit: 7d3660e79830a069f1848bb4fa1cdf8f666424fb
-- 
2.17.1

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

[Halil Pasic]

The atomic_cmpxchg() loop is broken because we occasionally end up with
old and _old having different values (a legit compiler can generate code
that accessed *ind_addr again to pick up a value for _old instead of
using the value of old that was already fetched according to the
rules of the abstract machine). This means the underlying CS instruction
may use a different old (_old) than the one we intended to use if
atomic_cmpxchg() performed the xchg part.

Let us use volatile to force the rules of the abstract machine for
accesses to *ind_addr. Let us also rewrite the loop so, we that the
new old is used to compute the new desired value if the xchg part
is not performed.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Reported-by: Andre Wild <Andre.Wild1@ibm.com>
Fixes: 7e7494627f ("s390x/virtio-ccw: Adapter interrupt support.")
---
 hw/s390x/virtio-ccw.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index c1f4bb1d33..3c988a000b 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -786,9 +786,10 @@ static inline VirtioCcwDevice *to_virtio_ccw_dev_fast(DeviceState *d)
 static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
                                      uint8_t to_be_set)
 {
-    uint8_t ind_old, ind_new;
+    uint8_t expected, actual;
     hwaddr len = 1;
-    uint8_t *ind_addr;
+    /* avoid  multiple fetches */
+    uint8_t volatile *ind_addr;
 
     ind_addr = cpu_physical_memory_map(ind_loc, &len, true);
     if (!ind_addr) {
@@ -796,14 +797,15 @@ static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
                      __func__, sch->cssid, sch->ssid, sch->schid);
         return -1;
     }
+    actual = *ind_addr;
     do {
-        ind_old = *ind_addr;
-        ind_new = ind_old | to_be_set;
-    } while (atomic_cmpxchg(ind_addr, ind_old, ind_new) != ind_old);
-    trace_virtio_ccw_set_ind(ind_loc, ind_old, ind_new);
-    cpu_physical_memory_unmap(ind_addr, len, 1, len);
+        expected = actual;
+        actual = atomic_cmpxchg(ind_addr, expected, expected | to_be_set);
+    } while (actual != expected);
+    trace_virtio_ccw_set_ind(ind_loc, actual, actual | to_be_set);
+    cpu_physical_memory_unmap((void *)ind_addr, len, 1, len);
 
-    return ind_old;
+    return actual;
 }
 
 static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
-- 
2.17.1

[PATCH 2/2] s390x/pci: fix set_ind_atomic

[Halil Pasic]

The atomic_cmpxchg() loop is broken because we occasionally end up with
old and _old having different values (a legit compiler can generate code
that accessed *ind_addr again to pick up a value for _old instead of
using the value of old that was already fetched according to the
rules of the abstract machine). This means the underlying CS instruction
may use a different old (_old) than the one we intended to use if
atomic_cmpxchg() performed the xchg part.

Let us use volatile to force the rules of the abstract machine for
accesses to *ind_addr. Let us also rewrite the loop so, we that the
new old is used to compute the new desired value if the xchg part
is not performed.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Fixes: 8cba80c3a0 ("s390: Add PCI bus support")
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/s390-pci-bus.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index c4a4259f0c..80eb957a91 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -637,22 +637,24 @@ static AddressSpace *s390_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
 
 static uint8_t set_ind_atomic(uint64_t ind_loc, uint8_t to_be_set)
 {
-    uint8_t ind_old, ind_new;
+    uint8_t expected, actual;
     hwaddr len = 1;
-    uint8_t *ind_addr;
+    /* avoid  multiple fetches */
+    uint8_t volatile *ind_addr;
 
     ind_addr = cpu_physical_memory_map(ind_loc, &len, true);
     if (!ind_addr) {
         s390_pci_generate_error_event(ERR_EVENT_AIRERR, 0, 0, 0, 0);
         return -1;
     }
+    actual = *ind_addr;
     do {
-        ind_old = *ind_addr;
-        ind_new = ind_old | to_be_set;
-    } while (atomic_cmpxchg(ind_addr, ind_old, ind_new) != ind_old);
-    cpu_physical_memory_unmap(ind_addr, len, 1, len);
+        expected = actual;
+        actual = atomic_cmpxchg(ind_addr, expected, expected | to_be_set);
+    } while (actual != expected);
+    cpu_physical_memory_unmap((void *)ind_addr, len, 1, len);
 
-    return ind_old;
+    return actual;
 }
 
 static void s390_msi_ctrl_write(void *opaque, hwaddr addr, uint64_t data,
-- 
2.17.1

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

[Christian Borntraeger]

On 16.06.20 06:50, Halil Pasic wrote:
> The atomic_cmpxchg() loop is broken because we occasionally end up with
> old and _old having different values (a legit compiler can generate code
> that accessed *ind_addr again to pick up a value for _old instead of
> using the value of old that was already fetched according to the
> rules of the abstract machine). This means the underlying CS instruction
> may use a different old (_old) than the one we intended to use if
> atomic_cmpxchg() performed the xchg part.
> 
> Let us use volatile to force the rules of the abstract machine for
> accesses to *ind_addr. Let us also rewrite the loop so, we that the
> new old is used to compute the new desired value if the xchg part
> is not performed.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Reported-by: Andre Wild <Andre.Wild1@ibm.com>
> Fixes: 7e7494627f ("s390x/virtio-ccw: Adapter interrupt support.")
> ---
>  hw/s390x/virtio-ccw.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index c1f4bb1d33..3c988a000b 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -786,9 +786,10 @@ static inline VirtioCcwDevice *to_virtio_ccw_dev_fast(DeviceState *d)
>  static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
>                                       uint8_t to_be_set)
>  {
> -    uint8_t ind_old, ind_new;
> +    uint8_t expected, actual;
>      hwaddr len = 1;
> -    uint8_t *ind_addr;
> +    /* avoid  multiple fetches */
> +    uint8_t volatile *ind_addr;
>  
>      ind_addr = cpu_physical_memory_map(ind_loc, &len, true);
>      if (!ind_addr) {
> @@ -796,14 +797,15 @@ static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
>                       __func__, sch->cssid, sch->ssid, sch->schid);
>          return -1;
>      }
> +    actual = *ind_addr;
>      do {
> -        ind_old = *ind_addr;

to make things easier to understand. Adding a barrier in here also fixes the issue.
Reasoning follows below:

> -        ind_new = ind_old | to_be_set;

with an analysis from Andreas (cc)

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




> -    } while (atomic_cmpxchg(ind_addr, ind_old, ind_new) != ind_old);
> -    trace_virtio_ccw_set_ind(ind_loc, ind_old, ind_new);
> -    cpu_physical_memory_unmap(ind_addr, len, 1, len);
> +        expected = actual;
> +        actual = atomic_cmpxchg(ind_addr, expected, expected | to_be_set);
> +    } while (actual != expected);
> +    trace_virtio_ccw_set_ind(ind_loc, actual, actual | to_be_set);
> +    cpu_physical_memory_unmap((void *)ind_addr, len, 1, len);
>  
> -    return ind_old;
> +    return actual;
>  }
>  
>  static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
> 

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

[Cornelia Huck]

On Tue, 16 Jun 2020 07:58:53 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 16.06.20 06:50, Halil Pasic wrote:
> > The atomic_cmpxchg() loop is broken because we occasionally end up with
> > old and _old having different values (a legit compiler can generate code
> > that accessed *ind_addr again to pick up a value for _old instead of
> > using the value of old that was already fetched according to the
> > rules of the abstract machine). This means the underlying CS instruction
> > may use a different old (_old) than the one we intended to use if
> > atomic_cmpxchg() performed the xchg part.
> > 
> > Let us use volatile to force the rules of the abstract machine for
> > accesses to *ind_addr. Let us also rewrite the loop so, we that the
> > new old is used to compute the new desired value if the xchg part
> > is not performed.
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > Reported-by: Andre Wild <Andre.Wild1@ibm.com>
> > Fixes: 7e7494627f ("s390x/virtio-ccw: Adapter interrupt support.")
> > ---
> >  hw/s390x/virtio-ccw.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index c1f4bb1d33..3c988a000b 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -786,9 +786,10 @@ static inline VirtioCcwDevice *to_virtio_ccw_dev_fast(DeviceState *d)
> >  static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
> >                                       uint8_t to_be_set)
> >  {
> > -    uint8_t ind_old, ind_new;
> > +    uint8_t expected, actual;
> >      hwaddr len = 1;
> > -    uint8_t *ind_addr;
> > +    /* avoid  multiple fetches */
> > +    uint8_t volatile *ind_addr;
> >  
> >      ind_addr = cpu_physical_memory_map(ind_loc, &len, true);
> >      if (!ind_addr) {
> > @@ -796,14 +797,15 @@ static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
> >                       __func__, sch->cssid, sch->ssid, sch->schid);
> >          return -1;
> >      }
> > +    actual = *ind_addr;
> >      do {
> > -        ind_old = *ind_addr;  
> 
> to make things easier to understand. Adding a barrier in here also fixes the issue.
> Reasoning follows below:
> 
> > -        ind_new = ind_old | to_be_set;  
> 
> with an analysis from Andreas (cc)
> 
>  #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.

> 
> 
> 
> 
> > -    } while (atomic_cmpxchg(ind_addr, ind_old, ind_new) != ind_old);
> > -    trace_virtio_ccw_set_ind(ind_loc, ind_old, ind_new);
> > -    cpu_physical_memory_unmap(ind_addr, len, 1, len);
> > +        expected = actual;
> > +        actual = atomic_cmpxchg(ind_addr, expected, expected | to_be_set);
> > +    } while (actual != expected);
> > +    trace_virtio_ccw_set_ind(ind_loc, actual, actual | to_be_set);
> > +    cpu_physical_memory_unmap((void *)ind_addr, len, 1, len);
> >  
> > -    return ind_old;
> > +    return actual;
> >  }
> >  
> >  static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
> >   
> 
> 

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

[Christian Borntraeger]

On 16.06.20 08:33, Cornelia Huck wrote:
> On Tue, 16 Jun 2020 07:58:53 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 16.06.20 06:50, Halil Pasic wrote:
>>> The atomic_cmpxchg() loop is broken because we occasionally end up with
>>> old and _old having different values (a legit compiler can generate code
>>> that accessed *ind_addr again to pick up a value for _old instead of
>>> using the value of old that was already fetched according to the
>>> rules of the abstract machine). This means the underlying CS instruction
>>> may use a different old (_old) than the one we intended to use if
>>> atomic_cmpxchg() performed the xchg part.
>>>
>>> Let us use volatile to force the rules of the abstract machine for
>>> accesses to *ind_addr. Let us also rewrite the loop so, we that the
>>> new old is used to compute the new desired value if the xchg part
>>> is not performed.
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>>> Reported-by: Andre Wild <Andre.Wild1@ibm.com>
>>> Fixes: 7e7494627f ("s390x/virtio-ccw: Adapter interrupt support.")
>>> ---
>>>  hw/s390x/virtio-ccw.c | 18 ++++++++++--------
>>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>>> index c1f4bb1d33..3c988a000b 100644
>>> --- a/hw/s390x/virtio-ccw.c
>>> +++ b/hw/s390x/virtio-ccw.c
>>> @@ -786,9 +786,10 @@ static inline VirtioCcwDevice *to_virtio_ccw_dev_fast(DeviceState *d)
>>>  static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
>>>                                       uint8_t to_be_set)
>>>  {
>>> -    uint8_t ind_old, ind_new;
>>> +    uint8_t expected, actual;
>>>      hwaddr len = 1;
>>> -    uint8_t *ind_addr;
>>> +    /* avoid  multiple fetches */
>>> +    uint8_t volatile *ind_addr;
>>>  
>>>      ind_addr = cpu_physical_memory_map(ind_loc, &len, true);
>>>      if (!ind_addr) {
>>> @@ -796,14 +797,15 @@ static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
>>>                       __func__, sch->cssid, sch->ssid, sch->schid);
>>>          return -1;
>>>      }
>>> +    actual = *ind_addr;
>>>      do {
>>> -        ind_old = *ind_addr;  
>>
>> to make things easier to understand. Adding a barrier in here also fixes the issue.
>> Reasoning follows below:
>>
>>> -        ind_new = ind_old | to_be_set;  
>>
>> with an analysis from Andreas (cc)
>>
>>  #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. 

Having said this, I think that the refactoring from Halil (to re-use actual) 
also makes sense independent of the fix. 
> 
> 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.
> 
>>
>>
>>
>>
>>> -    } while (atomic_cmpxchg(ind_addr, ind_old, ind_new) != ind_old);
>>> -    trace_virtio_ccw_set_ind(ind_loc, ind_old, ind_new);
>>> -    cpu_physical_memory_unmap(ind_addr, len, 1, len);
>>> +        expected = actual;
>>> +        actual = atomic_cmpxchg(ind_addr, expected, expected | to_be_set);
>>> +    } while (actual != expected);
>>> +    trace_virtio_ccw_set_ind(ind_loc, actual, actual | to_be_set);
>>> +    cpu_physical_memory_unmap((void *)ind_addr, len, 1, len);
>>>  
>>> -    return ind_old;
>>> +    return actual;
>>>  }
>>>  
>>>  static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
>>>   
>>
>>
> 

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

[Halil Pasic]

On Tue, 16 Jun 2020 07:58:53 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> 
> 
> On 16.06.20 06:50, Halil Pasic wrote:
> > The atomic_cmpxchg() loop is broken because we occasionally end up with
> > old and _old having different values (a legit compiler can generate code
> > that accessed *ind_addr again to pick up a value for _old instead of
> > using the value of old that was already fetched according to the
> > rules of the abstract machine). This means the underlying CS instruction
> > may use a different old (_old) than the one we intended to use if
> > atomic_cmpxchg() performed the xchg part.
> > 
> > Let us use volatile to force the rules of the abstract machine for
> > accesses to *ind_addr. Let us also rewrite the loop so, we that the
> > new old is used to compute the new desired value if the xchg part
> > is not performed.
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > Reported-by: Andre Wild <Andre.Wild1@ibm.com>
> > Fixes: 7e7494627f ("s390x/virtio-ccw: Adapter interrupt support.")
> > ---
> >  hw/s390x/virtio-ccw.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index c1f4bb1d33..3c988a000b 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -786,9 +786,10 @@ static inline VirtioCcwDevice *to_virtio_ccw_dev_fast(DeviceState *d)
> >  static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
> >                                       uint8_t to_be_set)
> >  {
> > -    uint8_t ind_old, ind_new;
> > +    uint8_t expected, actual;
> >      hwaddr len = 1;
> > -    uint8_t *ind_addr;
> > +    /* avoid  multiple fetches */
> > +    uint8_t volatile *ind_addr;
> >  
> >      ind_addr = cpu_physical_memory_map(ind_loc, &len, true);
> >      if (!ind_addr) {
> > @@ -796,14 +797,15 @@ static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
> >                       __func__, sch->cssid, sch->ssid, sch->schid);
> >          return -1;
> >      }
> > +    actual = *ind_addr;
> >      do {
> > -        ind_old = *ind_addr;
> 
> to make things easier to understand. Adding a barrier in here also fixes the issue.
> Reasoning follows below:
> 
> > -        ind_new = ind_old | to_be_set;
> 
> with an analysis from Andreas (cc)
> 
>  #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;                                                               \   
>  
>  })
>  

There is also the 

#define atomic_cmpxchg(ptr, old, new) __sync_val_compare_and_swap(ptr, old, new)

variant, I guess, when the C11 stuff is not available. I don't know if
that variant is guaranteed to not have problems with multiple loads.



> 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 think accessing the initial value via a volatile pointer initially and
using the value loaded by cmpxchg for subsequent iterations is cleaner.

Regards,
Halil

> 
> > -    } while (atomic_cmpxchg(ind_addr, ind_old, ind_new) != ind_old);
> > -    trace_virtio_ccw_set_ind(ind_loc, ind_old, ind_new);
> > -    cpu_physical_memory_unmap(ind_addr, len, 1, len);
> > +        expected = actual;
> > +        actual = atomic_cmpxchg(ind_addr, expected, expected | to_be_set);
> > +    } while (actual != expected);
> > +    trace_virtio_ccw_set_ind(ind_loc, actual, actual | to_be_set);
> > +    cpu_physical_memory_unmap((void *)ind_addr, len, 1, len);
> >  
> > -    return ind_old;
> > +    return actual;
> >  }
> >  
> >  static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
> > 
> 
> 
> 

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

[Halil Pasic]

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.

Another bunch uses constants as old, which is also fine. And there is
a third bunch where I don't know whats up, partly because I did not
dig deep enough.

Regards,
Halil

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

[Cornelia Huck]

On Tue, 16 Jun 2020 08:45:14 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 16.06.20 08:33, Cornelia Huck wrote:
> > On Tue, 16 Jun 2020 07:58:53 +0200
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 16.06.20 06:50, Halil Pasic wrote:  
> >>> The atomic_cmpxchg() loop is broken because we occasionally end up with
> >>> old and _old having different values (a legit compiler can generate code
> >>> that accessed *ind_addr again to pick up a value for _old instead of
> >>> using the value of old that was already fetched according to the
> >>> rules of the abstract machine). This means the underlying CS instruction
> >>> may use a different old (_old) than the one we intended to use if
> >>> atomic_cmpxchg() performed the xchg part.
> >>>
> >>> Let us use volatile to force the rules of the abstract machine for
> >>> accesses to *ind_addr. Let us also rewrite the loop so, we that the
> >>> new old is used to compute the new desired value if the xchg part
> >>> is not performed.
> >>>
> >>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> >>> Reported-by: Andre Wild <Andre.Wild1@ibm.com>
> >>> Fixes: 7e7494627f ("s390x/virtio-ccw: Adapter interrupt support.")
> >>> ---
> >>>  hw/s390x/virtio-ccw.c | 18 ++++++++++--------
> >>>  1 file changed, 10 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> >>> index c1f4bb1d33..3c988a000b 100644
> >>> --- a/hw/s390x/virtio-ccw.c
> >>> +++ b/hw/s390x/virtio-ccw.c
> >>> @@ -786,9 +786,10 @@ static inline VirtioCcwDevice *to_virtio_ccw_dev_fast(DeviceState *d)
> >>>  static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
> >>>                                       uint8_t to_be_set)
> >>>  {
> >>> -    uint8_t ind_old, ind_new;
> >>> +    uint8_t expected, actual;
> >>>      hwaddr len = 1;
> >>> -    uint8_t *ind_addr;
> >>> +    /* avoid  multiple fetches */
> >>> +    uint8_t volatile *ind_addr;
> >>>  
> >>>      ind_addr = cpu_physical_memory_map(ind_loc, &len, true);
> >>>      if (!ind_addr) {
> >>> @@ -796,14 +797,15 @@ static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
> >>>                       __func__, sch->cssid, sch->ssid, sch->schid);
> >>>          return -1;
> >>>      }
> >>> +    actual = *ind_addr;
> >>>      do {
> >>> -        ind_old = *ind_addr;    
> >>
> >> to make things easier to understand. Adding a barrier in here also fixes the issue.
> >> Reasoning follows below:
> >>  
> >>> -        ind_new = ind_old | to_be_set;    
> >>
> >> with an analysis from Andreas (cc)
> >>
> >>  #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.   
> 
> Having said this, I think that the refactoring from Halil (to re-use actual) 
> also makes sense independent of the fix. 

What about adding a barrier instead, as you suggested?

(Still wondering about other users of atomic_cmpxchg(), though.)

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

[David Hildenbrand]

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

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

[Halil Pasic]

On Fri, 19 Jun 2020 09:33:44 +0200
David Hildenbrand <david@redhat.com> wrote:

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

In kernel I would use READ_ONCE. 

https://elixir.bootlin.com/linux/v5.8-rc1/source/include/asm-generic/atomic.h#L171

In this case we are not manipulating an atomic variable. For uint8_t
that boils down to an access through a volatile pointer. And that is
what I did :).

> 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?
> 

IMHO what we want here is READ_ONCE, i.e. volatile access, and not
necessarily atomic access. But I suppose atomic access implies volatile
access (the C11 standard refers to atomic_load as 
C atomic_load(volatile A *object)). Because QEMU seems to use atomic_read()
in such situations, and does not have READ_ONCE, for me atomic_read would
also do.

Regards,
Halil

Re: [PATCH 0/2] two atomic_cmpxchg() related fixes

[Cornelia Huck]

On Tue, 16 Jun 2020 06:50:33 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> The story short: compiler can generate code that does two
> distinct fetches of *ind_addr for old and _old. If that happens we can
> not figure out if we had the desired xchg or not.
> 
> Halil Pasic (2):
>   virtio-ccw: fix virtio_set_ind_atomic
>   s390x/pci: fix set_ind_atomic
> 
>  hw/s390x/s390-pci-bus.c | 16 +++++++++-------
>  hw/s390x/virtio-ccw.c   | 18 ++++++++++--------
>  2 files changed, 19 insertions(+), 15 deletions(-)
> 
> 
> base-commit: 7d3660e79830a069f1848bb4fa1cdf8f666424fb

Have we managed to reach any kind of agreement on this? (A v2?)

We can still get in fixes post-softfreeze, of course.

Re: [PATCH 0/2] two atomic_cmpxchg() related fixes

[Christian Borntraeger]

On 01.07.20 14:01, Cornelia Huck wrote:
> On Tue, 16 Jun 2020 06:50:33 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> The story short: compiler can generate code that does two
>> distinct fetches of *ind_addr for old and _old. If that happens we can
>> not figure out if we had the desired xchg or not.
>>
>> Halil Pasic (2):
>>   virtio-ccw: fix virtio_set_ind_atomic
>>   s390x/pci: fix set_ind_atomic
>>
>>  hw/s390x/s390-pci-bus.c | 16 +++++++++-------
>>  hw/s390x/virtio-ccw.c   | 18 ++++++++++--------
>>  2 files changed, 19 insertions(+), 15 deletions(-)
>>
>>
>> base-commit: 7d3660e79830a069f1848bb4fa1cdf8f666424fb
> 
> Have we managed to reach any kind of agreement on this? (A v2?)
> 
> We can still get in fixes post-softfreeze, of course.

Unless Halil has a v2 ready, 
I think the current patch is fine as is as a fix. I would suggest
to go with that and we can then do more beautification later when
necessary.

Re: [PATCH 0/2] two atomic_cmpxchg() related fixes

[Cornelia Huck]

On Wed, 1 Jul 2020 14:06:11 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 01.07.20 14:01, Cornelia Huck wrote:
> > On Tue, 16 Jun 2020 06:50:33 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> >> The story short: compiler can generate code that does two
> >> distinct fetches of *ind_addr for old and _old. If that happens we can
> >> not figure out if we had the desired xchg or not.
> >>
> >> Halil Pasic (2):
> >>   virtio-ccw: fix virtio_set_ind_atomic
> >>   s390x/pci: fix set_ind_atomic
> >>
> >>  hw/s390x/s390-pci-bus.c | 16 +++++++++-------
> >>  hw/s390x/virtio-ccw.c   | 18 ++++++++++--------
> >>  2 files changed, 19 insertions(+), 15 deletions(-)
> >>
> >>
> >> base-commit: 7d3660e79830a069f1848bb4fa1cdf8f666424fb  
> > 
> > Have we managed to reach any kind of agreement on this? (A v2?)
> > 
> > We can still get in fixes post-softfreeze, of course.  
> 
> Unless Halil has a v2 ready, 
> I think the current patch is fine as is as a fix. I would suggest
> to go with that and we can then do more beautification later when
> necessary.

Sure, no objection to the patches as they are now.

I would like to see some R-bs/A-bs, though :)

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

[Christian Borntraeger]

On 16.06.20 06:50, Halil Pasic wrote:
> The atomic_cmpxchg() loop is broken because we occasionally end up with
> old and _old having different values (a legit compiler can generate code
> that accessed *ind_addr again to pick up a value for _old instead of
> using the value of old that was already fetched according to the
> rules of the abstract machine). This means the underlying CS instruction
> may use a different old (_old) than the one we intended to use if
> atomic_cmpxchg() performed the xchg part.
> 
> Let us use volatile to force the rules of the abstract machine for
> accesses to *ind_addr. Let us also rewrite the loop so, we that the
> new old is used to compute the new desired value if the xchg part
> is not performed.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Reported-by: Andre Wild <Andre.Wild1@ibm.com>
> Fixes: 7e7494627f ("s390x/virtio-ccw: Adapter interrupt support.")

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/virtio-ccw.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index c1f4bb1d33..3c988a000b 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -786,9 +786,10 @@ static inline VirtioCcwDevice *to_virtio_ccw_dev_fast(DeviceState *d)
>  static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
>                                       uint8_t to_be_set)
>  {
> -    uint8_t ind_old, ind_new;
> +    uint8_t expected, actual;
>      hwaddr len = 1;
> -    uint8_t *ind_addr;
> +    /* avoid  multiple fetches */
> +    uint8_t volatile *ind_addr;
>  
>      ind_addr = cpu_physical_memory_map(ind_loc, &len, true);
>      if (!ind_addr) {
> @@ -796,14 +797,15 @@ static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
>                       __func__, sch->cssid, sch->ssid, sch->schid);
>          return -1;
>      }
> +    actual = *ind_addr;
>      do {
> -        ind_old = *ind_addr;
> -        ind_new = ind_old | to_be_set;
> -    } while (atomic_cmpxchg(ind_addr, ind_old, ind_new) != ind_old);
> -    trace_virtio_ccw_set_ind(ind_loc, ind_old, ind_new);
> -    cpu_physical_memory_unmap(ind_addr, len, 1, len);
> +        expected = actual;
> +        actual = atomic_cmpxchg(ind_addr, expected, expected | to_be_set);
> +    } while (actual != expected);
> +    trace_virtio_ccw_set_ind(ind_loc, actual, actual | to_be_set);
> +    cpu_physical_memory_unmap((void *)ind_addr, len, 1, len);
>  
> -    return ind_old;
> +    return actual;
>  }
>  
>  static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
> 

Re: [PATCH 2/2] s390x/pci: fix set_ind_atomic

[Christian Borntraeger]

On 16.06.20 06:50, Halil Pasic wrote:
> The atomic_cmpxchg() loop is broken because we occasionally end up with
> old and _old having different values (a legit compiler can generate code
> that accessed *ind_addr again to pick up a value for _old instead of
> using the value of old that was already fetched according to the
> rules of the abstract machine). This means the underlying CS instruction
> may use a different old (_old) than the one we intended to use if
> atomic_cmpxchg() performed the xchg part.
> 
> Let us use volatile to force the rules of the abstract machine for
> accesses to *ind_addr. Let us also rewrite the loop so, we that the
> new old is used to compute the new desired value if the xchg part
> is not performed.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Fixes: 8cba80c3a0 ("s390: Add PCI bus support")
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>  hw/s390x/s390-pci-bus.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index c4a4259f0c..80eb957a91 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -637,22 +637,24 @@ static AddressSpace *s390_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>  
>  static uint8_t set_ind_atomic(uint64_t ind_loc, uint8_t to_be_set)
>  {
> -    uint8_t ind_old, ind_new;
> +    uint8_t expected, actual;
>      hwaddr len = 1;
> -    uint8_t *ind_addr;
> +    /* avoid  multiple fetches */
> +    uint8_t volatile *ind_addr;
>  
>      ind_addr = cpu_physical_memory_map(ind_loc, &len, true);
>      if (!ind_addr) {
>          s390_pci_generate_error_event(ERR_EVENT_AIRERR, 0, 0, 0, 0);
>          return -1;
>      }
> +    actual = *ind_addr;
>      do {
> -        ind_old = *ind_addr;
> -        ind_new = ind_old | to_be_set;
> -    } while (atomic_cmpxchg(ind_addr, ind_old, ind_new) != ind_old);
> -    cpu_physical_memory_unmap(ind_addr, len, 1, len);
> +        expected = actual;
> +        actual = atomic_cmpxchg(ind_addr, expected, expected | to_be_set);
> +    } while (actual != expected);
> +    cpu_physical_memory_unmap((void *)ind_addr, len, 1, len);
>  
> -    return ind_old;
> +    return actual;
>  }
>  
>  static void s390_msi_ctrl_write(void *opaque, hwaddr addr, uint64_t data,
> 

Re: [PATCH 0/2] two atomic_cmpxchg() related fixes

[Cornelia Huck]

On Tue, 16 Jun 2020 06:50:33 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> The story short: compiler can generate code that does two
> distinct fetches of *ind_addr for old and _old. If that happens we can
> not figure out if we had the desired xchg or not.
> 
> Halil Pasic (2):
>   virtio-ccw: fix virtio_set_ind_atomic
>   s390x/pci: fix set_ind_atomic
> 
>  hw/s390x/s390-pci-bus.c | 16 +++++++++-------
>  hw/s390x/virtio-ccw.c   | 18 ++++++++++--------
>  2 files changed, 19 insertions(+), 15 deletions(-)
> 
> 
> base-commit: 7d3660e79830a069f1848bb4fa1cdf8f666424fb

Thanks, applied.

Re: [PATCH 0/2] two atomic_cmpxchg() related fixes

[Halil Pasic]

On Wed, 1 Jul 2020 14:06:11 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> 
> 
> On 01.07.20 14:01, Cornelia Huck wrote:
> > On Tue, 16 Jun 2020 06:50:33 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> > 
> >> The story short: compiler can generate code that does two
> >> distinct fetches of *ind_addr for old and _old. If that happens we can
> >> not figure out if we had the desired xchg or not.
> >>
> >> Halil Pasic (2):
> >>   virtio-ccw: fix virtio_set_ind_atomic
> >>   s390x/pci: fix set_ind_atomic
> >>
> >>  hw/s390x/s390-pci-bus.c | 16 +++++++++-------
> >>  hw/s390x/virtio-ccw.c   | 18 ++++++++++--------
> >>  2 files changed, 19 insertions(+), 15 deletions(-)
> >>
> >>
> >> base-commit: 7d3660e79830a069f1848bb4fa1cdf8f666424fb
> > 
> > Have we managed to reach any kind of agreement on this? (A v2?)
> > 
> > We can still get in fixes post-softfreeze, of course.
> 
> Unless Halil has a v2 ready, 
> I think the current patch is fine as is as a fix. I would suggest
> to go with that and we can then do more beautification later when
> necessary.
> 
> 

I think we were waiting for Paolo to chime in regarding the barrier().
The thing I could beautify is using atomic_read(), but frankly I'm not
sure about it, especially considering multi-proccess. My understanding of
volatile is better than my understanding of atomic_read(). On the other
hand, same multi-process question can be asked about atomic_cmpxchg()
and __atomic_compare_exchange_n()...

Regards,
Halil

Re: [PATCH 0/2] two atomic_cmpxchg() related fixes

[Cornelia Huck]

On Fri, 3 Jul 2020 15:37:11 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 1 Jul 2020 14:06:11 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> > 
> > 
> > On 01.07.20 14:01, Cornelia Huck wrote:  
> > > On Tue, 16 Jun 2020 06:50:33 +0200
> > > Halil Pasic <pasic@linux.ibm.com> wrote:
> > >   
> > >> The story short: compiler can generate code that does two
> > >> distinct fetches of *ind_addr for old and _old. If that happens we can
> > >> not figure out if we had the desired xchg or not.
> > >>
> > >> Halil Pasic (2):
> > >>   virtio-ccw: fix virtio_set_ind_atomic
> > >>   s390x/pci: fix set_ind_atomic
> > >>
> > >>  hw/s390x/s390-pci-bus.c | 16 +++++++++-------
> > >>  hw/s390x/virtio-ccw.c   | 18 ++++++++++--------
> > >>  2 files changed, 19 insertions(+), 15 deletions(-)
> > >>
> > >>
> > >> base-commit: 7d3660e79830a069f1848bb4fa1cdf8f666424fb  
> > > 
> > > Have we managed to reach any kind of agreement on this? (A v2?)
> > > 
> > > We can still get in fixes post-softfreeze, of course.  
> > 
> > Unless Halil has a v2 ready, 
> > I think the current patch is fine as is as a fix. I would suggest
> > to go with that and we can then do more beautification later when
> > necessary.
> > 
> >   
> 
> I think we were waiting for Paolo to chime in regarding the barrier().
> The thing I could beautify is using atomic_read(), but frankly I'm not
> sure about it, especially considering multi-proccess. My understanding of
> volatile is better than my understanding of atomic_read(). On the other
> hand, same multi-process question can be asked about atomic_cmpxchg()
> and __atomic_compare_exchange_n()...

We can just improve the code on top later. Having something that
works now beats something not yet developed that might be nicer :)

(But yes, it also might be good to look at non-s390 examples of this
pattern and check if something can be done in a more central place.)

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

[Michael S. Tsirkin]

On Tue, Jun 16, 2020 at 06:50:34AM +0200, Halil Pasic wrote:
> The atomic_cmpxchg() loop is broken because we occasionally end up with
> old and _old having different values (a legit compiler can generate code
> that accessed *ind_addr again to pick up a value for _old instead of
> using the value of old that was already fetched according to the
> rules of the abstract machine). This means the underlying CS instruction
> may use a different old (_old) than the one we intended to use if
> atomic_cmpxchg() performed the xchg part.

And was this ever observed in the field? Or is this a theoretical issue?
commit log should probably say ...

> 
> Let us use volatile to force the rules of the abstract machine for
> accesses to *ind_addr. Let us also rewrite the loop so, we that the

we that -> we know that?

> new old is used to compute the new desired value if the xchg part
> is not performed.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Reported-by: Andre Wild <Andre.Wild1@ibm.com>
> Fixes: 7e7494627f ("s390x/virtio-ccw: Adapter interrupt support.")
> ---
>  hw/s390x/virtio-ccw.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index c1f4bb1d33..3c988a000b 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -786,9 +786,10 @@ static inline VirtioCcwDevice *to_virtio_ccw_dev_fast(DeviceState *d)
>  static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
>                                       uint8_t to_be_set)
>  {
> -    uint8_t ind_old, ind_new;
> +    uint8_t expected, actual;
>      hwaddr len = 1;
> -    uint8_t *ind_addr;
> +    /* avoid  multiple fetches */
> +    uint8_t volatile *ind_addr;
>  
>      ind_addr = cpu_physical_memory_map(ind_loc, &len, true);
>      if (!ind_addr) {
> @@ -796,14 +797,15 @@ static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
>                       __func__, sch->cssid, sch->ssid, sch->schid);
>          return -1;
>      }
> +    actual = *ind_addr;
>      do {
> -        ind_old = *ind_addr;
> -        ind_new = ind_old | to_be_set;
> -    } while (atomic_cmpxchg(ind_addr, ind_old, ind_new) != ind_old);
> -    trace_virtio_ccw_set_ind(ind_loc, ind_old, ind_new);
> -    cpu_physical_memory_unmap(ind_addr, len, 1, len);
> +        expected = actual;
> +        actual = atomic_cmpxchg(ind_addr, expected, expected | to_be_set);
> +    } while (actual != expected);
> +    trace_virtio_ccw_set_ind(ind_loc, actual, actual | to_be_set);
> +    cpu_physical_memory_unmap((void *)ind_addr, len, 1, len);
>  
> -    return ind_old;
> +    return actual;
>  }

I wonder whether cpuXX APIs should accept volatile pointers, too:
casting away volatile is always suspicious.
But that is a separate issue ...

>  static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
> -- 
> 2.17.1

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

[Christian Borntraeger]

On 04.07.20 20:34, Michael S. Tsirkin wrote:
> On Tue, Jun 16, 2020 at 06:50:34AM +0200, Halil Pasic wrote:
>> The atomic_cmpxchg() loop is broken because we occasionally end up with
>> old and _old having different values (a legit compiler can generate code
>> that accessed *ind_addr again to pick up a value for _old instead of
>> using the value of old that was already fetched according to the
>> rules of the abstract machine). This means the underlying CS instruction
>> may use a different old (_old) than the one we intended to use if
>> atomic_cmpxchg() performed the xchg part.
> 
> And was this ever observed in the field? Or is this a theoretical issue?
> commit log should probably say ...

It was observed in the field when the xml specified qemu instead of vhost.

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

[Halil Pasic]

On Sat, 4 Jul 2020 14:34:04 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jun 16, 2020 at 06:50:34AM +0200, Halil Pasic wrote:
> > The atomic_cmpxchg() loop is broken because we occasionally end up with
> > old and _old having different values (a legit compiler can generate code
> > that accessed *ind_addr again to pick up a value for _old instead of
> > using the value of old that was already fetched according to the
> > rules of the abstract machine). This means the underlying CS instruction
> > may use a different old (_old) than the one we intended to use if
> > atomic_cmpxchg() performed the xchg part.
> 
> And was this ever observed in the field? Or is this a theoretical issue?
> commit log should probably say ...
> 

It was observed in the field (Christian already answered). I think the
message already implies this, because the only conjunctive is about the
compiler behavior.

> > 
> > Let us use volatile to force the rules of the abstract machine for
> > accesses to *ind_addr. Let us also rewrite the loop so, we that the
> 
> we that -> we know that?

s/we//

It would be nice to fix this before the patch gets merged.

> 
> > new old is used to compute the new desired value if the xchg part
> > is not performed.
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > Reported-by: Andre Wild <Andre.Wild1@ibm.com>
> > Fixes: 7e7494627f ("s390x/virtio-ccw: Adapter interrupt support.")
> > ---
> >  hw/s390x/virtio-ccw.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index c1f4bb1d33..3c988a000b 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -786,9 +786,10 @@ static inline VirtioCcwDevice *to_virtio_ccw_dev_fast(DeviceState *d)
> >  static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
> >                                       uint8_t to_be_set)
> >  {
> > -    uint8_t ind_old, ind_new;
> > +    uint8_t expected, actual;
> >      hwaddr len = 1;
> > -    uint8_t *ind_addr;
> > +    /* avoid  multiple fetches */
> > +    uint8_t volatile *ind_addr;
> >  
> >      ind_addr = cpu_physical_memory_map(ind_loc, &len, true);
> >      if (!ind_addr) {
> > @@ -796,14 +797,15 @@ static uint8_t virtio_set_ind_atomic(SubchDev *sch, uint64_t ind_loc,
> >                       __func__, sch->cssid, sch->ssid, sch->schid);
> >          return -1;
> >      }
> > +    actual = *ind_addr;
> >      do {
> > -        ind_old = *ind_addr;
> > -        ind_new = ind_old | to_be_set;
> > -    } while (atomic_cmpxchg(ind_addr, ind_old, ind_new) != ind_old);
> > -    trace_virtio_ccw_set_ind(ind_loc, ind_old, ind_new);
> > -    cpu_physical_memory_unmap(ind_addr, len, 1, len);
> > +        expected = actual;
> > +        actual = atomic_cmpxchg(ind_addr, expected, expected | to_be_set);
> > +    } while (actual != expected);
> > +    trace_virtio_ccw_set_ind(ind_loc, actual, actual | to_be_set);
> > +    cpu_physical_memory_unmap((void *)ind_addr, len, 1, len);
> >  
> > -    return ind_old;
> > +    return actual;
> >  }
> 
> I wonder whether cpuXX APIs should accept volatile pointers, too:
> casting away volatile is always suspicious.
> But that is a separate issue ...
> 

Nod.

Thanks for having a look!

> >  static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
> > -- 
> > 2.17.1
>