From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39052) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byxGK-0007TC-OJ for qemu-devel@nongnu.org; Tue, 25 Oct 2016 04:39:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1byxGH-0002tw-L4 for qemu-devel@nongnu.org; Tue, 25 Oct 2016 04:39:12 -0400 Received: from mail-wm0-x236.google.com ([2a00:1450:400c:c09::236]:38507) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1byxGH-0002tM-Dh for qemu-devel@nongnu.org; Tue, 25 Oct 2016 04:39:09 -0400 Received: by mail-wm0-x236.google.com with SMTP id d128so13433478wmf.1 for ; Tue, 25 Oct 2016 01:39:09 -0700 (PDT) References: <87a8dtkdwx.fsf@linaro.org> <87zilt6ql1.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <87zilt6ql1.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> Date: Tue, 25 Oct 2016 09:39:07 +0100 Message-ID: <8760ogkel0.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] Holding the BQL for emulate_ppc_hypercall List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikunj A Dadhania Cc: Bharata B Rao , David Gibson , qemu-ppc@nongnu.org, Qemu Developers Nikunj A Dadhania writes: > Alex Bennée writes: > >> Hi, >> >> In the MTTCG patch set one of the big patches is to remove the >> requirement to hold the BQL while running code: >> >> tcg: drop global lock during TCG code execution >> >> And this broke the PPC code because emulate_ppc_hypercall can cause >> changes to the global state. This function just calls spapr_hypercall() >> and puts the results into the TCG register file. Normally >> spapr_hypercall() is called under the BQL in KVM as >> kvm_arch_handle_exit() does things with the BQL held. >> >> I blithely wrapped the called in a lock/unlock pair only to find the >> ppc64 check builds failed as the hypercall was made during the >> cc->do_interrupt() code which also holds the BQL. >> >> I'm a little confused by the nature of PPC hypercalls in TCG? Are they >> not all detectable at code generation time? What is the case that causes >> an exception to occur rather than the helper function doing the >> hypercall? >> >> I guess it comes down to can I avoid doing: >> >> /* If we come via cc->do_interrupt BQL may already be held */ >> if (!qemu_mutex_iothread_locked()) { >> g_mutex_lock_iothread(); >> env->gpr[3] = spapr_hypercall(cpu, env->gpr[3], &env->gpr[4]); >> g_muetx_unlock_iothread(); >> } else { >> env->gpr[3] = spapr_hypercall(cpu, env->gpr[3], &env->gpr[4]); >> } >> >> Any thoughts? > > Similar discussions happened on this patch: > https://lists.gnu.org/archive/html/qemu-ppc/2016-09/msg00015.html > > This was just working for TCG case, need to fix for KVM. I would need to > handle KVM case to avoid a deadlock. Thanks for the pointer I missed that before. But I think the fix here is too far down the call stack as the spapr_hypercall is called by both TCG and KVM paths. But as discussed on my reply to Dave I think the correct fix is to ensure cpu-exec.c:cpu_handle_exception also takes the BQL when delivering exceptions: if (replay_exception()) { CPUClass *cc = CPU_GET_CLASS(cpu); qemu_mutex_lock_iothread(); cc->do_interrupt(cpu); qemu_mutex_unlock_iothread(); cpu->exception_index = -1; } else if (!replay_has_interrupt()) { I got confused by the if(replay_exception()) which is a bit non-obvious. > > Regards > Nikunj -- Alex Bennée