* [Qemu-devel] [PATCH v1] s390x/tcg: fix locking problem with tcg_s390_tod_updated
@ 2018-06-29 17:05 David Hildenbrand
2018-06-29 17:24 ` Richard Henderson
2018-07-02 8:30 ` Cornelia Huck
0 siblings, 2 replies; 3+ messages in thread
From: David Hildenbrand @ 2018-06-29 17:05 UTC (permalink / raw)
To: qemu-s390x
Cc: qemu-devel, Richard Henderson, Alexander Graf, Cornelia Huck,
Christian Borntraeger, Thomas Huth, David Hildenbrand
tcg_s390_tod_updated() is always called with the iothread being locked
(e.g. from S390TODClass->set() e.g. via HELPER(sck) or on incomming
migration). The helper we call takes the lock itself - bad.
Let's change that by factoring out updating the ckc timer. This now looks
much nicer than having to call a helper from another function.
While touch it we also make sure that env->ckc is updated even if the new
value is -1ULL, for now it would not have been modified in that case.
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
This survives my tests and the iotests Christian mentioned.
target/s390x/misc_helper.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 7656a9b90a..3f91579570 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -149,26 +149,23 @@ uint64_t HELPER(stck)(CPUS390XState *env)
return tod.low;
}
-/* Set Clock Comparator */
-void HELPER(sckc)(CPUS390XState *env, uint64_t time)
+static void update_ckc_timer(CPUS390XState *env)
{
S390TODState *td = s390_get_todstate();
+ uint64_t time;
/* stop the timer and remove pending CKC IRQs */
timer_del(env->tod_timer);
- qemu_mutex_lock_iothread();
+ g_assert(qemu_mutex_iothread_locked());
env->pending_int &= ~INTERRUPT_EXT_CLOCK_COMPARATOR;
- qemu_mutex_unlock_iothread();
/* the tod has to exceed the ckc, this can never happen if ckc is all 1's */
- if (time == -1ULL) {
+ if (env->ckc == -1ULL) {
return;
}
- env->ckc = time;
-
/* difference between origins */
- time -= td->base.low;
+ time = env->ckc - td->base.low;
/* nanoseconds */
time = tod2time(time);
@@ -176,12 +173,21 @@ void HELPER(sckc)(CPUS390XState *env, uint64_t time)
timer_mod(env->tod_timer, time);
}
+/* Set Clock Comparator */
+void HELPER(sckc)(CPUS390XState *env, uint64_t ckc)
+{
+ env->ckc = ckc;
+
+ qemu_mutex_lock_iothread();
+ update_ckc_timer(env);
+ qemu_mutex_unlock_iothread();
+}
+
void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque)
{
S390CPU *cpu = S390_CPU(cs);
- CPUS390XState *env = &cpu->env;
- helper_sckc(env, env->ckc);
+ update_ckc_timer(&cpu->env);
}
/* Set Clock */
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v1] s390x/tcg: fix locking problem with tcg_s390_tod_updated
2018-06-29 17:05 [Qemu-devel] [PATCH v1] s390x/tcg: fix locking problem with tcg_s390_tod_updated David Hildenbrand
@ 2018-06-29 17:24 ` Richard Henderson
2018-07-02 8:30 ` Cornelia Huck
1 sibling, 0 replies; 3+ messages in thread
From: Richard Henderson @ 2018-06-29 17:24 UTC (permalink / raw)
To: David Hildenbrand, qemu-s390x
Cc: qemu-devel, Alexander Graf, Cornelia Huck, Christian Borntraeger,
Thomas Huth
On 06/29/2018 10:05 AM, David Hildenbrand wrote:
> tcg_s390_tod_updated() is always called with the iothread being locked
> (e.g. from S390TODClass->set() e.g. via HELPER(sck) or on incomming
> migration). The helper we call takes the lock itself - bad.
>
> Let's change that by factoring out updating the ckc timer. This now looks
> much nicer than having to call a helper from another function.
>
> While touch it we also make sure that env->ckc is updated even if the new
> value is -1ULL, for now it would not have been modified in that case.
>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>
> This survives my tests and the iotests Christian mentioned.
>
> target/s390x/misc_helper.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v1] s390x/tcg: fix locking problem with tcg_s390_tod_updated
2018-06-29 17:05 [Qemu-devel] [PATCH v1] s390x/tcg: fix locking problem with tcg_s390_tod_updated David Hildenbrand
2018-06-29 17:24 ` Richard Henderson
@ 2018-07-02 8:30 ` Cornelia Huck
1 sibling, 0 replies; 3+ messages in thread
From: Cornelia Huck @ 2018-07-02 8:30 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-s390x, qemu-devel, Richard Henderson, Alexander Graf,
Christian Borntraeger, Thomas Huth
On Fri, 29 Jun 2018 19:05:20 +0200
David Hildenbrand <david@redhat.com> wrote:
> tcg_s390_tod_updated() is always called with the iothread being locked
> (e.g. from S390TODClass->set() e.g. via HELPER(sck) or on incomming
s/incomming/incoming/
> migration). The helper we call takes the lock itself - bad.
>
> Let's change that by factoring out updating the ckc timer. This now looks
> much nicer than having to call a helper from another function.
>
> While touch it we also make sure that env->ckc is updated even if the new
s/touch/touching/
> value is -1ULL, for now it would not have been modified in that case.
>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>
> This survives my tests and the iotests Christian mentioned.
OK, this will the last patch (I guess) before I send my final 3.0 pull
request.
>
> target/s390x/misc_helper.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
Thanks, applied.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-07-02 8:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-29 17:05 [Qemu-devel] [PATCH v1] s390x/tcg: fix locking problem with tcg_s390_tod_updated David Hildenbrand
2018-06-29 17:24 ` Richard Henderson
2018-07-02 8:30 ` Cornelia Huck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.