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