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