Re: [Qemu-devel] [s390] possible deadlock in handle_sigp?
On 09/19/2016 01:25 PM, David Hildenbrand wrote:
[...]
>>
>> We only do the slow path things in QEMU. Maybe we could just have one lock
>> that
>> we trylock and return a condition code of 2 (busy) if we fail. That seems
>> the
>> most simple solution while still being architecturally correct. Something
>> like
>
> According to the architecture, CC=busy is returned in case the access path to
> the CPU is busy. So this might not be optimal but should work for now.
>
>>
>>
>> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
>> index f348745..5706218 100644
>> --- a/target-s390x/kvm.c
>> +++ b/target-s390x/kvm.c
>> @@ -133,6 +133,8 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[]
>> = {
>> KVM_CAP_LAST_INFO
>> };
>>
>> +static QemuMutex qemu_sigp_mutex;
>> +
>> static int cap_sync_regs;
>> static int cap_async_pf;
>> static int cap_mem_op;
>> @@ -358,6 +360,8 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>> rc = compat_disable_facilities(s, fac_mask, ARRAY_SIZE(fac_mask));
>> }
>>
>> +qemu_mutex_init(&qemu_sigp_mutex);
>> +
>> return rc;
>> }
>>
>> @@ -1845,6 +1849,11 @@ static int handle_sigp(S390CPU *cpu, struct kvm_run
>> *run, uint8_t ipa1)
>> status_reg = &env->regs[r1];
>> param = (r1 % 2) ? env->regs[r1] : env->regs[r1 + 1];
>>
>> +if (qemu_mutex_trylock(&qemu_sigp_mutex)) {
>> +setcc(cpu, SIGP_CC_BUSY );
>> +return 0;
>> +}
>> +
>> switch (order) {
>> case SIGP_SET_ARCH:
>> ret = sigp_set_architecture(cpu, param, status_reg);
>> @@ -1854,6 +1863,7 @@ static int handle_sigp(S390CPU *cpu, struct kvm_run
>> *run, uint8_t ipa1)
>> dst_cpu = s390_cpu_addr2state(env->regs[r3]);
>> ret = handle_sigp_single_dst(dst_cpu, order, param, status_reg);
>> }
>> +qemu_mutex_unlock(&qemu_sigp_mutex);
>>
>> trace_kvm_sigp_finished(order, CPU(cpu)->cpu_index,
>> dst_cpu ? CPU(dst_cpu)->cpu_index : -1, ret);
>>
>>
>>
>
> This makes SET ARCHITECTURE handling much more easier.
Yes.
I think doing so in an on top patch is probably safer to keep the fix minimal
(e.g. for backports)
Re: [Qemu-devel] [s390] possible deadlock in handle_sigp?
> On 09/15/2016 09:21 PM, David Hildenbrand wrote:
> >> On 09/12/2016 08:03 PM, Paolo Bonzini wrote:
> >>>
> >>>
> >>> On 12/09/2016 19:37, Christian Borntraeger wrote:
> On 09/12/2016 06:44 PM, Paolo Bonzini wrote:
> > I think that two CPUs doing reciprocal SIGPs could in principle end up
> > waiting on each other to complete their run_on_cpu. If the SIGP has to
> > be synchronous the fix is not trivial (you'd have to put the CPU in a
> > state similar to cpu->halted = 1), otherwise it's enough to replace
> > run_on_cpu with async_run_on_cpu.
>
> IIRC the sigps are supossed to be serialized by the big QEMU lock. WIll
> have a look.
> >>>
> >>> Yes, but run_on_cpu drops it when it waits on the qemu_work_cond
> >>> condition variable. (Related: I stumbled upon it because I wanted to
> >>> remove the BQL from run_on_cpu work items).
> >>
> >> Yes, seems you are right. If both CPUs have just exited from KVM doing a
> >> crossover sigp, they will do the arch_exit handling before the run_on_cpu
> >> stuff which might result in this hang. (luckily it seems quite unlikely
> >> but still we need to fix it).
> >> We cannot simply use async as the callbacks also provide the condition
> >> code for the initiater, so this requires some rework.
> >>
> >>
> >
> > Smells like having to provide a lock per CPU. Trylock that lock, if that's
> > not
> > possible, cc=busy. SIGP SET ARCHITECTURE has to lock all CPUs.
> >
> > That was the initital design, until I realized that this was all protected
> > by
> > the BQL.
> >
> > David
>
> We only do the slow path things in QEMU. Maybe we could just have one lock
> that
> we trylock and return a condition code of 2 (busy) if we fail. That seems the
> most simple solution while still being architecturally correct. Something like
According to the architecture, CC=busy is returned in case the access path to
the CPU is busy. So this might not be optimal but should work for now.
>
>
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index f348745..5706218 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -133,6 +133,8 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[]
> = {
> KVM_CAP_LAST_INFO
> };
>
> +static QemuMutex qemu_sigp_mutex;
> +
> static int cap_sync_regs;
> static int cap_async_pf;
> static int cap_mem_op;
> @@ -358,6 +360,8 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> rc = compat_disable_facilities(s, fac_mask, ARRAY_SIZE(fac_mask));
> }
>
> +qemu_mutex_init(&qemu_sigp_mutex);
> +
> return rc;
> }
>
> @@ -1845,6 +1849,11 @@ static int handle_sigp(S390CPU *cpu, struct kvm_run
> *run, uint8_t ipa1)
> status_reg = &env->regs[r1];
> param = (r1 % 2) ? env->regs[r1] : env->regs[r1 + 1];
>
> +if (qemu_mutex_trylock(&qemu_sigp_mutex)) {
> +setcc(cpu, SIGP_CC_BUSY );
> +return 0;
> +}
> +
> switch (order) {
> case SIGP_SET_ARCH:
> ret = sigp_set_architecture(cpu, param, status_reg);
> @@ -1854,6 +1863,7 @@ static int handle_sigp(S390CPU *cpu, struct kvm_run
> *run, uint8_t ipa1)
> dst_cpu = s390_cpu_addr2state(env->regs[r3]);
> ret = handle_sigp_single_dst(dst_cpu, order, param, status_reg);
> }
> +qemu_mutex_unlock(&qemu_sigp_mutex);
>
> trace_kvm_sigp_finished(order, CPU(cpu)->cpu_index,
> dst_cpu ? CPU(dst_cpu)->cpu_index : -1, ret);
>
>
>
This makes SET ARCHITECTURE handling much more easier.
David
Re: [Qemu-devel] [s390] possible deadlock in handle_sigp?
On 09/15/2016 09:21 PM, David Hildenbrand wrote:
>> On 09/12/2016 08:03 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 12/09/2016 19:37, Christian Borntraeger wrote:
On 09/12/2016 06:44 PM, Paolo Bonzini wrote:
> I think that two CPUs doing reciprocal SIGPs could in principle end up
> waiting on each other to complete their run_on_cpu. If the SIGP has to
> be synchronous the fix is not trivial (you'd have to put the CPU in a
> state similar to cpu->halted = 1), otherwise it's enough to replace
> run_on_cpu with async_run_on_cpu.
IIRC the sigps are supossed to be serialized by the big QEMU lock. WIll
have a look.
>>>
>>> Yes, but run_on_cpu drops it when it waits on the qemu_work_cond
>>> condition variable. (Related: I stumbled upon it because I wanted to
>>> remove the BQL from run_on_cpu work items).
>>
>> Yes, seems you are right. If both CPUs have just exited from KVM doing a
>> crossover sigp, they will do the arch_exit handling before the run_on_cpu
>> stuff which might result in this hang. (luckily it seems quite unlikely
>> but still we need to fix it).
>> We cannot simply use async as the callbacks also provide the condition
>> code for the initiater, so this requires some rework.
>>
>>
>
> Smells like having to provide a lock per CPU. Trylock that lock, if that's not
> possible, cc=busy. SIGP SET ARCHITECTURE has to lock all CPUs.
>
> That was the initital design, until I realized that this was all protected by
> the BQL.
>
> David
We only do the slow path things in QEMU. Maybe we could just have one lock that
we trylock and return a condition code of 2 (busy) if we fail. That seems the
most simple solution while still being architecturally correct. Something like
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index f348745..5706218 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -133,6 +133,8 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
KVM_CAP_LAST_INFO
};
+static QemuMutex qemu_sigp_mutex;
+
static int cap_sync_regs;
static int cap_async_pf;
static int cap_mem_op;
@@ -358,6 +360,8 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
rc = compat_disable_facilities(s, fac_mask, ARRAY_SIZE(fac_mask));
}
+qemu_mutex_init(&qemu_sigp_mutex);
+
return rc;
}
@@ -1845,6 +1849,11 @@ static int handle_sigp(S390CPU *cpu, struct kvm_run
*run, uint8_t ipa1)
status_reg = &env->regs[r1];
param = (r1 % 2) ? env->regs[r1] : env->regs[r1 + 1];
+if (qemu_mutex_trylock(&qemu_sigp_mutex)) {
+setcc(cpu, SIGP_CC_BUSY );
+return 0;
+}
+
switch (order) {
case SIGP_SET_ARCH:
ret = sigp_set_architecture(cpu, param, status_reg);
@@ -1854,6 +1863,7 @@ static int handle_sigp(S390CPU *cpu, struct kvm_run *run,
uint8_t ipa1)
dst_cpu = s390_cpu_addr2state(env->regs[r3]);
ret = handle_sigp_single_dst(dst_cpu, order, param, status_reg);
}
+qemu_mutex_unlock(&qemu_sigp_mutex);
trace_kvm_sigp_finished(order, CPU(cpu)->cpu_index,
dst_cpu ? CPU(dst_cpu)->cpu_index : -1, ret);
Re: [Qemu-devel] [s390] possible deadlock in handle_sigp?
On 15/09/2016 21:21, David Hildenbrand wrote:
> Smells like having to provide a lock per CPU. Trylock that lock, if that's not
> possible, cc=busy. SIGP SET ARCHITECTURE has to lock all CPUs.
Makes sense. On the other hand:
- you have to trylock both the source and the destination, I think.
- since no one ever does a lock, only a trylock, the trylock can be
replaced by a simple test-and-set of a flag in S390CPU (e.g.
cpu->doing_sigp). Because the access to the bitmap _is_ protected by
the BQL, it needn't even use atomics and it can be as simple as
static bool start_sigp(S390CPU *src, S390CPU *dst)
{
if (!src->in_sigp && !dst->in_sigp) {
src->in_sigp = dst->in_sigp = true;
return true;
}
return false;
}
and end_sigp is similarly obvious.
Thanks,
Paolo
> That was the initital design, until I realized that this was all protected by
> the BQL.
Re: [Qemu-devel] [s390] possible deadlock in handle_sigp?
> On 09/12/2016 08:03 PM, Paolo Bonzini wrote: > > > > > > On 12/09/2016 19:37, Christian Borntraeger wrote: > >> On 09/12/2016 06:44 PM, Paolo Bonzini wrote: > >>> I think that two CPUs doing reciprocal SIGPs could in principle end up > >>> waiting on each other to complete their run_on_cpu. If the SIGP has to > >>> be synchronous the fix is not trivial (you'd have to put the CPU in a > >>> state similar to cpu->halted = 1), otherwise it's enough to replace > >>> run_on_cpu with async_run_on_cpu. > >> > >> IIRC the sigps are supossed to be serialized by the big QEMU lock. WIll > >> have a look. > > > > Yes, but run_on_cpu drops it when it waits on the qemu_work_cond > > condition variable. (Related: I stumbled upon it because I wanted to > > remove the BQL from run_on_cpu work items). > > Yes, seems you are right. If both CPUs have just exited from KVM doing a > crossover sigp, they will do the arch_exit handling before the run_on_cpu > stuff which might result in this hang. (luckily it seems quite unlikely > but still we need to fix it). > We cannot simply use async as the callbacks also provide the condition > code for the initiater, so this requires some rework. > > Smells like having to provide a lock per CPU. Trylock that lock, if that's not possible, cc=busy. SIGP SET ARCHITECTURE has to lock all CPUs. That was the initital design, until I realized that this was all protected by the BQL. David
Re: [Qemu-devel] [s390] possible deadlock in handle_sigp?
On 09/12/2016 08:03 PM, Paolo Bonzini wrote: > > > On 12/09/2016 19:37, Christian Borntraeger wrote: >> On 09/12/2016 06:44 PM, Paolo Bonzini wrote: >>> I think that two CPUs doing reciprocal SIGPs could in principle end up >>> waiting on each other to complete their run_on_cpu. If the SIGP has to >>> be synchronous the fix is not trivial (you'd have to put the CPU in a >>> state similar to cpu->halted = 1), otherwise it's enough to replace >>> run_on_cpu with async_run_on_cpu. >> >> IIRC the sigps are supossed to be serialized by the big QEMU lock. WIll >> have a look. > > Yes, but run_on_cpu drops it when it waits on the qemu_work_cond > condition variable. (Related: I stumbled upon it because I wanted to > remove the BQL from run_on_cpu work items). Yes, seems you are right. If both CPUs have just exited from KVM doing a crossover sigp, they will do the arch_exit handling before the run_on_cpu stuff which might result in this hang. (luckily it seems quite unlikely but still we need to fix it). We cannot simply use async as the callbacks also provide the condition code for the initiater, so this requires some rework.
Re: [Qemu-devel] [s390] possible deadlock in handle_sigp?
On 12/09/2016 19:37, Christian Borntraeger wrote: > On 09/12/2016 06:44 PM, Paolo Bonzini wrote: > > I think that two CPUs doing reciprocal SIGPs could in principle end up > > waiting on each other to complete their run_on_cpu. If the SIGP has to > > be synchronous the fix is not trivial (you'd have to put the CPU in a > > state similar to cpu->halted = 1), otherwise it's enough to replace > > run_on_cpu with async_run_on_cpu. > > IIRC the sigps are supossed to be serialized by the big QEMU lock. WIll > have a look. Yes, but run_on_cpu drops it when it waits on the qemu_work_cond condition variable. (Related: I stumbled upon it because I wanted to remove the BQL from run_on_cpu work items). Paolo
Re: [Qemu-devel] [s390] possible deadlock in handle_sigp?
On 09/12/2016 06:44 PM, Paolo Bonzini wrote: > I think that two CPUs doing reciprocal SIGPs could in principle end up > waiting on each other to complete their run_on_cpu. If the SIGP has to > be synchronous the fix is not trivial (you'd have to put the CPU in a > state similar to cpu->halted = 1), otherwise it's enough to replace > run_on_cpu with async_run_on_cpu. IIRC the sigps are supossed to be serialized by the big QEMU lock. WIll have a look.
