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)