[Qemu-devel] [s390] possible deadlock in handle_sigp?

[Qemu-devel] [s390] possible deadlock in handle_sigp?

[Paolo Bonzini]

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.

Thanks,

Paolo

Re: [Qemu-devel] [s390] possible deadlock in handle_sigp?

[Christian Borntraeger]

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.

Re: [Qemu-devel] [s390] possible deadlock in handle_sigp?

[Paolo Bonzini]

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?

[Christian Borntraeger]

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?

[David Hildenbrand]

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

[Paolo Bonzini]

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?

[Christian Borntraeger]

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?

[David Hildenbrand]

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

[Christian Borntraeger]

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)

[Qemu-devel] [PATCH] s390x/kvm: Fix potential deadlock in sigp handling

[Christian Borntraeger]

FYI, unless I find a better alternative,  I plan to go with this patch.
------snip------
[PATCH] s390x/kvm: Fix potential deadlock in sigp handling

If two VCPUs exit at the same time and target each other
with a sigp, both could run into a deadlock as run_on_cpu
on CPU0 will free the BQL when starting the CPU1 target routine.
CPU1 will run its sigp initiater for CPU0 before handling
the run_on_cpu requests, thus resulting in a dead lock.

As all qemu SIGPs are slow path anway we can use a big sigp
lock and allow only one SIGP for the guest at a time. We will
return condition code 2 (BUSY) on contention to the guest.

Reported-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 target-s390x/kvm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index f348745..a9fa831 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)) {
+        ret = SIGP_CC_BUSY;
+        goto out;
+    }
+
     switch (order) {
     case SIGP_SET_ARCH:
         ret = sigp_set_architecture(cpu, param, status_reg);
@@ -1854,7 +1863,9 @@ 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);
 
+out:
     trace_kvm_sigp_finished(order, CPU(cpu)->cpu_index,
                             dst_cpu ? CPU(dst_cpu)->cpu_index : -1, ret);
 
-- 
2.5.5