* [Qemu-devel] [PATCH v1 0/2] Fix for FreeBSD compile on i386 @ 2016-03-21 16:23 Alex Bennée 2016-03-21 16:23 ` [Qemu-devel] [PATCH v1 1/2] cpus: don't use atomic_read for vm_clock_warp_start Alex Bennée 2016-03-21 16:23 ` [Qemu-devel] [PATCH v1 2/2] include/qemu/atomic: add compile time asserts Alex Bennée 0 siblings, 2 replies; 7+ messages in thread From: Alex Bennée @ 2016-03-21 16:23 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, sbruno, Alex Bennée, peter.maydell The compiler of FreeBSD/i386 complains when trying to make a 64 bit atomic load on a 32 bit host. This fixes the compile failure and adds some compile time guards to the atomic functions. If wanted I can expand the compile time guards to include the __sync_and_fetch* intrinsics version as well. Alex Bennée (2): cpus: don't use atomic_read for vm_clock_warp_start include/qemu/atomic: add compile time asserts cpus.c | 12 ++++------- include/qemu/atomic.h | 58 ++++++++++++++++++++++++++++++--------------------- 2 files changed, 38 insertions(+), 32 deletions(-) -- 2.7.3 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v1 1/2] cpus: don't use atomic_read for vm_clock_warp_start 2016-03-21 16:23 [Qemu-devel] [PATCH v1 0/2] Fix for FreeBSD compile on i386 Alex Bennée @ 2016-03-21 16:23 ` Alex Bennée 2016-03-21 16:26 ` Paolo Bonzini 2016-03-21 16:23 ` [Qemu-devel] [PATCH v1 2/2] include/qemu/atomic: add compile time asserts Alex Bennée 1 sibling, 1 reply; 7+ messages in thread From: Alex Bennée @ 2016-03-21 16:23 UTC (permalink / raw) To: qemu-devel Cc: peter.maydell, Peter Crosthwaite, sbruno, pbonzini, Alex Bennée, Richard Henderson 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. The variable documentation says this is meant to be protected by vm_clock_seqlock so lets just move the code into that section. All other references to vm_clock_warp_start are already protected by the seqlock. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- cpus.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/cpus.c b/cpus.c index 23cf7aa..2fd5381 100644 --- a/cpus.c +++ b/cpus.c @@ -338,15 +338,10 @@ static int64_t qemu_icount_round(int64_t count) static void icount_warp_rt(void) { - /* 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) { - return; - } + bool check_clock = false; seqlock_write_lock(&timers_state.vm_clock_seqlock); - if (runstate_is_running()) { + if (vm_clock_warp_start != -1 && runstate_is_running()) { int64_t clock = REPLAY_CLOCK(REPLAY_CLOCK_VIRTUAL_RT, cpu_get_clock_locked()); int64_t warp_delta; @@ -362,11 +357,12 @@ static void icount_warp_rt(void) warp_delta = MIN(warp_delta, delta); } timers_state.qemu_icount_bias += warp_delta; + check_clock = true; } vm_clock_warp_start = -1; seqlock_write_unlock(&timers_state.vm_clock_seqlock); - if (qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) { + if (check_clock && qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) { qemu_clock_notify(QEMU_CLOCK_VIRTUAL); } } -- 2.7.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/2] cpus: don't use atomic_read for vm_clock_warp_start 2016-03-21 16:23 ` [Qemu-devel] [PATCH v1 1/2] cpus: don't use atomic_read for vm_clock_warp_start Alex Bennée @ 2016-03-21 16:26 ` Paolo Bonzini 2016-03-21 17:45 ` Alex Bennée 0 siblings, 1 reply; 7+ messages in thread From: Paolo Bonzini @ 2016-03-21 16:26 UTC (permalink / raw) To: Alex Bennée, qemu-devel Cc: peter.maydell, sbruno, Richard Henderson, Peter Crosthwaite On 21/03/2016 17:23, Alex Bennée wrote: > 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. The variable documentation says this is meant to be protected by > vm_clock_seqlock so lets just move the code into that section. This neglects the fact that there is a comment explaining the code, so Because the variable is protected by vm_clock_seqlock, we check its value inside a seqlock critical section. is better. In addition, I would prefer if you used seqlock_read_*. Paolo > All other references to vm_clock_warp_start are already protected by the > seqlock. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > cpus.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/cpus.c b/cpus.c > index 23cf7aa..2fd5381 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -338,15 +338,10 @@ static int64_t qemu_icount_round(int64_t count) > > static void icount_warp_rt(void) > { > - /* 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) { > - return; > - } > + bool check_clock = false; > > seqlock_write_lock(&timers_state.vm_clock_seqlock); > - if (runstate_is_running()) { > + if (vm_clock_warp_start != -1 && runstate_is_running()) { > int64_t clock = REPLAY_CLOCK(REPLAY_CLOCK_VIRTUAL_RT, > cpu_get_clock_locked()); > int64_t warp_delta; > @@ -362,11 +357,12 @@ static void icount_warp_rt(void) > warp_delta = MIN(warp_delta, delta); > } > timers_state.qemu_icount_bias += warp_delta; > + check_clock = true; > } > vm_clock_warp_start = -1; > seqlock_write_unlock(&timers_state.vm_clock_seqlock); > > - if (qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) { > + if (check_clock && qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) { > qemu_clock_notify(QEMU_CLOCK_VIRTUAL); > } > } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/2] cpus: don't use atomic_read for vm_clock_warp_start 2016-03-21 16:26 ` Paolo Bonzini @ 2016-03-21 17:45 ` Alex Bennée 0 siblings, 0 replies; 7+ messages in thread From: Alex Bennée @ 2016-03-21 17:45 UTC (permalink / raw) To: Paolo Bonzini Cc: peter.maydell, sbruno, Richard Henderson, qemu-devel, Peter Crosthwaite Paolo Bonzini <pbonzini@redhat.com> writes: > On 21/03/2016 17:23, Alex Bennée wrote: >> 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. The variable documentation says this is meant to be protected by >> vm_clock_seqlock so lets just move the code into that section. > > This neglects the fact that there is a comment explaining the code, so > > Because the variable is protected by vm_clock_seqlock, we check its > value inside a seqlock critical section. > > is better. In addition, I would prefer if you used seqlock_read_*. I see you comment on the other thread passed the posting. I'll respin. > > Paolo > >> All other references to vm_clock_warp_start are already protected by the >> seqlock. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> cpus.c | 12 ++++-------- >> 1 file changed, 4 insertions(+), 8 deletions(-) >> >> diff --git a/cpus.c b/cpus.c >> index 23cf7aa..2fd5381 100644 >> --- a/cpus.c >> +++ b/cpus.c >> @@ -338,15 +338,10 @@ static int64_t qemu_icount_round(int64_t count) >> >> static void icount_warp_rt(void) >> { >> - /* 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) { >> - return; >> - } >> + bool check_clock = false; >> >> seqlock_write_lock(&timers_state.vm_clock_seqlock); >> - if (runstate_is_running()) { >> + if (vm_clock_warp_start != -1 && runstate_is_running()) { >> int64_t clock = REPLAY_CLOCK(REPLAY_CLOCK_VIRTUAL_RT, >> cpu_get_clock_locked()); >> int64_t warp_delta; >> @@ -362,11 +357,12 @@ static void icount_warp_rt(void) >> warp_delta = MIN(warp_delta, delta); >> } >> timers_state.qemu_icount_bias += warp_delta; >> + check_clock = true; >> } >> vm_clock_warp_start = -1; >> seqlock_write_unlock(&timers_state.vm_clock_seqlock); >> >> - if (qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) { >> + if (check_clock && qemu_clock_expired(QEMU_CLOCK_VIRTUAL)) { >> qemu_clock_notify(QEMU_CLOCK_VIRTUAL); >> } >> } -- Alex Bennée ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v1 2/2] include/qemu/atomic: add compile time asserts 2016-03-21 16:23 [Qemu-devel] [PATCH v1 0/2] Fix for FreeBSD compile on i386 Alex Bennée 2016-03-21 16:23 ` [Qemu-devel] [PATCH v1 1/2] cpus: don't use atomic_read for vm_clock_warp_start Alex Bennée @ 2016-03-21 16:23 ` Alex Bennée 2016-04-01 15:03 ` Alex Bennée 1 sibling, 1 reply; 7+ messages in thread From: Alex Bennée @ 2016-03-21 16:23 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, sbruno, Alex Bennée, peter.maydell 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.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v1 2/2] include/qemu/atomic: add compile time asserts 2016-03-21 16:23 ` [Qemu-devel] [PATCH v1 2/2] include/qemu/atomic: add compile time asserts Alex Bennée @ 2016-04-01 15:03 ` Alex Bennée 2016-04-04 8:33 ` Paolo Bonzini 0 siblings, 1 reply; 7+ messages in thread From: Alex Bennée @ 2016-04-01 15:03 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, sbruno, peter.maydell Alex Bennée <alex.bennee@linaro.org> writes: > 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> Ping Paolo. Is this worth including in a re-spin? > --- > 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); \ -- Alex Bennée ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v1 2/2] include/qemu/atomic: add compile time asserts 2016-04-01 15:03 ` Alex Bennée @ 2016-04-04 8:33 ` Paolo Bonzini 0 siblings, 0 replies; 7+ messages in thread From: Paolo Bonzini @ 2016-04-04 8:33 UTC (permalink / raw) To: Alex Bennée, qemu-devel; +Cc: peter.maydell, sbruno On 01/04/2016 17:03, Alex Bennée wrote: > >> > 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> > > Ping Paolo. Is this worth including in a re-spin? Yes, please. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-04-04 8:34 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-21 16:23 [Qemu-devel] [PATCH v1 0/2] Fix for FreeBSD compile on i386 Alex Bennée 2016-03-21 16:23 ` [Qemu-devel] [PATCH v1 1/2] cpus: don't use atomic_read for vm_clock_warp_start Alex Bennée 2016-03-21 16:26 ` Paolo Bonzini 2016-03-21 17:45 ` Alex Bennée 2016-03-21 16:23 ` [Qemu-devel] [PATCH v1 2/2] include/qemu/atomic: add compile time asserts Alex Bennée 2016-04-01 15:03 ` Alex Bennée 2016-04-04 8:33 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).