[Qemu-devel] [PATCH v2 0/2] Fix for compile on FreeBSD/i386 (and others?)

[Qemu-devel] [PATCH v2 0/2] Fix for compile on FreeBSD/i386 (and others?)

[Alex Bennée]

Only the first patch has actually changed. Instead of moving the read
inside the write seqlock it is now done using the seqlock_read_*
primitives.

Build tested on a FreeBSB/i386 VM with these applied:

  https://github.com/berrange/qemu/tree/freebsd-fixes

There are still a ton of unrelated warnings being kicked out of the compiler
though. On the VM "make check" fails at ... but as the build was
broken beforehand I can't tell if this is a regression. It passes fine
on my Linux box.

Alex Bennée (2):
  cpus: don't use atomic_read for vm_clock_warp_start
  include/qemu/atomic: add compile time asserts

 cpus.c                | 10 ++++++++-
 include/qemu/atomic.h | 58 ++++++++++++++++++++++++++++++---------------------
 2 files changed, 43 insertions(+), 25 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/2] cpus: don't use atomic_read for vm_clock_warp_start

[Alex Bennée]

As vm_clock_warp_start is a 64 bit value this causes problems for the
compiler trying to come up with a suitable atomic operation on 32 bit
hosts. Because the variable is protected by vm_clock_seqlock, we check its
value inside a seqlock critical section.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use seqlock read instead wrapping inside the write sequence lock
---
 cpus.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index 8ae4777..cbeb1f6 100644
--- a/cpus.c
+++ b/cpus.c
@@ -338,10 +338,18 @@ static int64_t qemu_icount_round(int64_t count)
 
 static void icount_warp_rt(void)
 {
+    unsigned seq;
+    int64_t warp_start;
+
     /* The icount_warp_timer is rescheduled soon after vm_clock_warp_start
      * changes from -1 to another value, so the race here is okay.
      */
-    if (atomic_read(&vm_clock_warp_start) == -1) {
+    do {
+        seq = seqlock_read_begin(&timers_state.vm_clock_seqlock);
+        warp_start = vm_clock_warp_start;
+    } while (seqlock_read_retry(&timers_state.vm_clock_seqlock, seq));
+
+    if (warp_start == -1) {
         return;
     }
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/2] include/qemu/atomic: add compile time asserts

[Alex Bennée]

To be safely portable no atomic access should be trying to do more than
the natural word width of the host. The most common abuse is trying to
atomically access 64 bit values on a 32 bit host.

This patch adds some QEMU_BUILD_BUG_ON to the __atomic instrinsic paths
to create a build failure if (sizeof(*ptr) > sizeof(void *)).

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/atomic.h | 58 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 8f1d8d9..5bc4d6c 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -42,30 +42,34 @@
  * loads/stores past the atomic operation load/store. However there is
  * no explicit memory barrier for the processor.
  */
-#define atomic_read(ptr)                          \
-    ({                                            \
-    typeof(*ptr) _val;                            \
-     __atomic_load(ptr, &_val, __ATOMIC_RELAXED); \
-    _val;                                         \
+#define atomic_read(ptr)                              \
+    ({                                                \
+    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
+    typeof(*ptr) _val;                                \
+     __atomic_load(ptr, &_val, __ATOMIC_RELAXED);     \
+    _val;                                             \
     })
 
-#define atomic_set(ptr, i)  do {                  \
-    typeof(*ptr) _val = (i);                      \
-    __atomic_store(ptr, &_val, __ATOMIC_RELAXED); \
+#define atomic_set(ptr, i)  do {                      \
+    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
+    typeof(*ptr) _val = (i);                          \
+    __atomic_store(ptr, &_val, __ATOMIC_RELAXED);     \
 } while(0)
 
 /* Atomic RCU operations imply weak memory barriers */
 
-#define atomic_rcu_read(ptr)                      \
-    ({                                            \
-    typeof(*ptr) _val;                            \
-     __atomic_load(ptr, &_val, __ATOMIC_CONSUME); \
-    _val;                                         \
+#define atomic_rcu_read(ptr)                          \
+    ({                                                \
+    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
+    typeof(*ptr) _val;                                \
+    __atomic_load(ptr, &_val, __ATOMIC_CONSUME);      \
+    _val;                                             \
     })
 
-#define atomic_rcu_set(ptr, i)  do {                    \
-    typeof(*ptr) _val = (i);                            \
-    __atomic_store(ptr, &_val, __ATOMIC_RELEASE);       \
+#define atomic_rcu_set(ptr, i) do {                   \
+    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \
+    typeof(*ptr) _val = (i);                          \
+    __atomic_store(ptr, &_val, __ATOMIC_RELEASE);     \
 } while(0)
 
 /* atomic_mb_read/set semantics map Java volatile variables. They are
@@ -79,6 +83,7 @@
 #if defined(_ARCH_PPC)
 #define atomic_mb_read(ptr)                             \
     ({                                                  \
+    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));   \
     typeof(*ptr) _val;                                  \
      __atomic_load(ptr, &_val, __ATOMIC_RELAXED);       \
      smp_rmb();                                         \
@@ -86,22 +91,25 @@
     })
 
 #define atomic_mb_set(ptr, i)  do {                     \
+    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));   \
     typeof(*ptr) _val = (i);                            \
     smp_wmb();                                          \
     __atomic_store(ptr, &_val, __ATOMIC_RELAXED);       \
     smp_mb();                                           \
 } while(0)
 #else
-#define atomic_mb_read(ptr)                       \
-    ({                                            \
-    typeof(*ptr) _val;                            \
-     __atomic_load(ptr, &_val, __ATOMIC_SEQ_CST); \
-    _val;                                         \
+#define atomic_mb_read(ptr)                             \
+    ({                                                  \
+    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));   \
+    typeof(*ptr) _val;                                  \
+    __atomic_load(ptr, &_val, __ATOMIC_SEQ_CST);        \
+    _val;                                               \
     })
 
-#define atomic_mb_set(ptr, i)  do {               \
-    typeof(*ptr) _val = (i);                      \
-    __atomic_store(ptr, &_val, __ATOMIC_SEQ_CST); \
+#define atomic_mb_set(ptr, i)  do {                     \
+    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));   \
+    typeof(*ptr) _val = (i);                            \
+    __atomic_store(ptr, &_val, __ATOMIC_SEQ_CST);       \
 } while(0)
 #endif
 
@@ -109,6 +117,7 @@
 /* All the remaining operations are fully sequentially consistent */
 
 #define atomic_xchg(ptr, i)    ({                           \
+    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));       \
     typeof(*ptr) _new = (i), _old;                          \
     __atomic_exchange(ptr, &_new, &_old, __ATOMIC_SEQ_CST); \
     _old;                                                   \
@@ -117,6 +126,7 @@
 /* Returns the eventual value, failed or not */
 #define atomic_cmpxchg(ptr, old, new)                                   \
     ({                                                                  \
+    QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *));                   \
     typeof(*ptr) _old = (old), _new = (new);                            \
     __atomic_compare_exchange(ptr, &_old, &_new, false,                 \
                               __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);      \
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 0/2] Fix for compile on FreeBSD/i386 (and others?)

[Paolo Bonzini]

On 04/04/2016 16:35, Alex Bennée wrote:
> Only the first patch has actually changed. Instead of moving the read
> inside the write seqlock it is now done using the seqlock_read_*
> primitives.
> 
> Build tested on a FreeBSB/i386 VM with these applied:
> 
>   https://github.com/berrange/qemu/tree/freebsd-fixes
> 
> There are still a ton of unrelated warnings being kicked out of the compiler
> though. On the VM "make check" fails at ... but as the build was
> broken beforehand I can't tell if this is a regression. It passes fine
> on my Linux box.
> 
> Alex Bennée (2):
>   cpus: don't use atomic_read for vm_clock_warp_start
>   include/qemu/atomic: add compile time asserts
> 
>  cpus.c                | 10 ++++++++-
>  include/qemu/atomic.h | 58 ++++++++++++++++++++++++++++++---------------------
>  2 files changed, 43 insertions(+), 25 deletions(-)
> 

Queued for 2.6, thanks.  I will send a pull request later this week.

Paolo

Re: [Qemu-devel] [PATCH v2 0/2] Fix for compile on FreeBSD/i386 (and others?)

[Peter Maydell]

On 4 April 2016 at 15:44, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 04/04/2016 16:35, Alex Bennée wrote:
>> Only the first patch has actually changed. Instead of moving the read
>> inside the write seqlock it is now done using the seqlock_read_*
>> primitives.
>>
>> Build tested on a FreeBSB/i386 VM with these applied:
>>
>>   https://github.com/berrange/qemu/tree/freebsd-fixes
>>
>> There are still a ton of unrelated warnings being kicked out of the compiler
>> though. On the VM "make check" fails at ... but as the build was
>> broken beforehand I can't tell if this is a regression. It passes fine
>> on my Linux box.
>>
>> Alex Bennée (2):
>>   cpus: don't use atomic_read for vm_clock_warp_start
>>   include/qemu/atomic: add compile time asserts
>>
>>  cpus.c                | 10 ++++++++-
>>  include/qemu/atomic.h | 58 ++++++++++++++++++++++++++++++---------------------
>>  2 files changed, 43 insertions(+), 25 deletions(-)
>>
>
> Queued for 2.6, thanks.  I will send a pull request later this week.

It would be nice to fix this for rc1 (scheduled for tomorrow)...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/2] Fix for compile on FreeBSD/i386 (and others?)

[Paolo Bonzini]

On 04/04/2016 16:56, Peter Maydell wrote:
> On 4 April 2016 at 15:44, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 04/04/2016 16:35, Alex Bennée wrote:
>>> Only the first patch has actually changed. Instead of moving the read
>>> inside the write seqlock it is now done using the seqlock_read_*
>>> primitives.
>>>
>>> Build tested on a FreeBSB/i386 VM with these applied:
>>>
>>>   https://github.com/berrange/qemu/tree/freebsd-fixes
>>>
>>> There are still a ton of unrelated warnings being kicked out of the compiler
>>> though. On the VM "make check" fails at ... but as the build was
>>> broken beforehand I can't tell if this is a regression. It passes fine
>>> on my Linux box.
>>>
>>> Alex Bennée (2):
>>>   cpus: don't use atomic_read for vm_clock_warp_start
>>>   include/qemu/atomic: add compile time asserts
>>>
>>>  cpus.c                | 10 ++++++++-
>>>  include/qemu/atomic.h | 58 ++++++++++++++++++++++++++++++---------------------
>>>  2 files changed, 43 insertions(+), 25 deletions(-)
>>>
>>
>> Queued for 2.6, thanks.  I will send a pull request later this week.
> 
> It would be nice to fix this for rc1 (scheduled for tomorrow)...

Ok, I'll send it tomorrow then.

Paolo

Re: [Qemu-devel] [PATCH v2 0/2] Fix for compile on FreeBSD/i386 (and others?)

[Ed Maste]

On 4 April 2016 at 10:35, Alex Bennée <alex.bennee@linaro.org> wrote:
> Only the first patch has actually changed. Instead of moving the read
> inside the write seqlock it is now done using the seqlock_read_*
> primitives.
>
> Build tested on a FreeBSB/i386 VM with these applied:
>
>   https://github.com/berrange/qemu/tree/freebsd-fixes
>
> There are still a ton of unrelated warnings being kicked out of the compiler
> though. On the VM "make check" fails at ... but as the build was

I'm not sure what the "..." is supposed to be, but FWIW I tried the
test suite just now and have 3 tests failing on FreeBSD 10.3-STABLE on
amd64. I've commented them out locally as a temporary workaround.

The tests (and failures) are:

* tests/test-io-channel-socket

GTESTER tests/test-io-channel-socket
/tank/emaste/src/qemu/tests/Makefile:650: recipe for target
'check-tests/test-io-channel-socket' failed

* tests/test-crypto-pbkdf

Unexpected error in qcrypto_pbkdf2() at crypto/pbkdf-stub.c:41:
No crypto library supporting PBKDF in this build: Function not implemented
(and many simliar)

* tests/ipmi-bt-test

qemu-system-i386: Unable to connect character device ipmi0: address
resolution failed for localhost:40135: Invalid value for ai_flags
**
ERROR:tests/ipmi-bt-test.c:320:test_connect: assertion failed: (rv == 1)
(and three similar errors)

Re: [Qemu-devel] [PATCH v2 0/2] Fix for compile on FreeBSD/i386 (and others?)

[Alex Bennée]

Ed Maste <emaste@freebsd.org> writes:

> On 4 April 2016 at 10:35, Alex Bennée <alex.bennee@linaro.org> wrote:
>> Only the first patch has actually changed. Instead of moving the read
>> inside the write seqlock it is now done using the seqlock_read_*
>> primitives.
>>
>> Build tested on a FreeBSB/i386 VM with these applied:
>>
>>   https://github.com/berrange/qemu/tree/freebsd-fixes
>>
>> There are still a ton of unrelated warnings being kicked out of the compiler
>> though. On the VM "make check" fails at ... but as the build was
>
> I'm not sure what the "..." is supposed to be, but FWIW I tried the
> test suite just now and have 3 tests failing on FreeBSD 10.3-STABLE on
> amd64. I've commented them out locally as a temporary workaround.

Oops, I thought I'd filled that in.

>
> The tests (and failures) are:
>
> * tests/test-io-channel-socket
>
> GTESTER tests/test-io-channel-socket
> /tank/emaste/src/qemu/tests/Makefile:650: recipe for target
> 'check-tests/test-io-channel-socket' failed

This is what I saw. Quoth dpb on #qemu:

<danpb> i can reproduce on my bsd box, so lemme have a poke at it
<danpb> stsquad: oh, i see what it is - getaddrinfo is failing for the
    same stupid V4MAPPED issue i alrady sent a patch for

> * tests/test-crypto-pbkdf
>
> Unexpected error in qcrypto_pbkdf2() at crypto/pbkdf-stub.c:41:
> No crypto library supporting PBKDF in this build: Function not implemented
> (and many simliar)
>
> * tests/ipmi-bt-test
>
> qemu-system-i386: Unable to connect character device ipmi0: address
> resolution failed for localhost:40135: Invalid value for ai_flags
> **
> ERROR:tests/ipmi-bt-test.c:320:test_connect: assertion failed: (rv == 1)
> (and three similar errors)

I didn't get this far but I'll re-test once everything is merged. Thanks
for testing on your setup.

--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 0/2] Fix for compile on FreeBSD/i386 (and others?)

[Peter Maydell]

On 4 April 2016 at 20:26, Alex Bennée <alex.bennee@linaro.org> wrote:
> Ed Maste <emaste@freebsd.org> writes:
>
>> On 4 April 2016 at 10:35, Alex Bennée <alex.bennee@linaro.org> wrote:
>>> Only the first patch has actually changed. Instead of moving the read
>>> inside the write seqlock it is now done using the seqlock_read_*
>>> primitives.
>>>
>>> Build tested on a FreeBSB/i386 VM with these applied:
>>>
>>>   https://github.com/berrange/qemu/tree/freebsd-fixes
>>>
>>> There are still a ton of unrelated warnings being kicked out of the compiler
>>> though. On the VM "make check" fails at ... but as the build was
>>
>> I'm not sure what the "..." is supposed to be, but FWIW I tried the
>> test suite just now and have 3 tests failing on FreeBSD 10.3-STABLE on
>> amd64. I've commented them out locally as a temporary workaround.
>
> Oops, I thought I'd filled that in.
>
>>
>> The tests (and failures) are:
>>
>> * tests/test-io-channel-socket
>>
>> GTESTER tests/test-io-channel-socket
>> /tank/emaste/src/qemu/tests/Makefile:650: recipe for target
>> 'check-tests/test-io-channel-socket' failed
>
> This is what I saw. Quoth dpb on #qemu:
>
> <danpb> i can reproduce on my bsd box, so lemme have a poke at it
> <danpb> stsquad: oh, i see what it is - getaddrinfo is failing for the
>     same stupid V4MAPPED issue i alrady sent a patch for

Dan's v2 patch: http://patchwork.ozlabs.org/patch/605911/

>> * tests/test-crypto-pbkdf
>>
>> Unexpected error in qcrypto_pbkdf2() at crypto/pbkdf-stub.c:41:
>> No crypto library supporting PBKDF in this build: Function not implemented
>> (and many simliar)

Try http://patchwork.ozlabs.org/patch/605897/ ? That should skip
this test if configure didn't find the pbkdf support.

>> * tests/ipmi-bt-test
>>
>> qemu-system-i386: Unable to connect character device ipmi0: address
>> resolution failed for localhost:40135: Invalid value for ai_flags

This is almost certainly the same V4MAPPED issue as above.

thanks
-- PMM