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);
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
