[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

> 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

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)