From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39325) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjhRq-00039Y-8x for qemu-devel@nongnu.org; Tue, 13 Sep 2016 02:44:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjhRm-00052e-4F for qemu-devel@nongnu.org; Tue, 13 Sep 2016 02:44:01 -0400 Received: from mail-wm0-f51.google.com ([74.125.82.51]:36006) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjhRl-00051p-AR for qemu-devel@nongnu.org; Tue, 13 Sep 2016 02:43:58 -0400 Received: by mail-wm0-f51.google.com with SMTP id b187so169789012wme.1 for ; Mon, 12 Sep 2016 23:43:57 -0700 (PDT) References: <1472935202-3342-1-git-send-email-rth@twiddle.net> <1472935202-3342-7-git-send-email-rth@twiddle.net> <87twdlw6cv.fsf@linaro.org> <0bcd5093-52f2-d642-4a1c-e3557084053a@twiddle.net> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <0bcd5093-52f2-d642-4a1c-e3557084053a@twiddle.net> Date: Tue, 13 Sep 2016 07:42:54 +0100 Message-ID: <87k2egwb9t.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3 06/34] tcg: Add EXCP_ATOMIC List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: qemu-devel@nongnu.org Richard Henderson writes: > On 09/12/2016 07:16 AM, Alex Bennée wrote: >>> +void cpu_exec_step(CPUState *cpu) >>> +{ >>> + CPUArchState *env = (CPUArchState *)cpu->env_ptr; >>> + TranslationBlock *tb; >>> + target_ulong cs_base, pc; >>> + uint32_t flags; >>> + bool old_tb_flushed; >>> + >>> + old_tb_flushed = cpu->tb_flushed; >>> + cpu->tb_flushed = false; >> >> Advanced warning, these disappear soon when the async safe work (plus >> safe tb flush) patches get merged. > > Fair enough. > > Having thought about this more, I think it may be better to handle this without > flushing the tb. To have parallel_cpus included in the TB flags or somesuch > and keep that TB around. > > My thinking is that there are certain things, like alignment, that could result > in repeated single-stepping. So better to keep the TB around than keep having > to regenerate it. > >>> + /* ??? When we begin running cpus in parallel, we should >>> + stop all cpus, clear parallel_cpus, and interpret a >>> + single insn with cpu_exec_step. In the meantime, >>> + we should never get here. */ >>> + abort(); >> >> Possibly more correct would be: >> >> g_assert(parallel_cpus == false); >> abort(); > > No, since it is here that we would *set* parallel_cpus to false. Or did you > mean assert parallel_cpus true? Not that that helps for the moment... For SoftMMU this particular loop should never hit because paralell_cpus should be false, hence we never generate any code that might EXCP_ATOMIC. It only fails at the moment because of the "hack" for testing which makes parallel_cpus true. The MTTCG adds a new thread function for MTTCG mode which will have to handle EXCP_ATOMIC as discussed. > >>> +static void step_atomic(CPUState *cpu) >>> +{ >>> + start_exclusive(); >>> + >>> + /* Since we got here, we know that parallel_cpus must be true. */ >>> + parallel_cpus = false; >>> + cpu_exec_step(cpu); >>> + parallel_cpus = true; >>> + >>> + end_exclusive(); >>> +} >>> + >> >> Paolo's safe work patches bring the start/end_exclusive functions into >> cpu-exec-common so I think after that has been merged this function >> can also be moved and called directly from the MTTCG loop on an >> EXCP_ATOMIC exit. > > Excellent. Perhaps I should rebase this upon that right away. Have you got a > pointer to a tree handy? Paolo posted a new version recently but I haven't built a tree with it yet. I was hoping some of the hot-path and maybe barrier stuff would get merged while I finish reviewing this ;-) See: Subject: [PATCH v7 00/16] cpu-exec: Safe work in quiescent state Date: Mon, 12 Sep 2016 13:12:25 +0200 Message-Id: <1473678761-8885-1-git-send-email-pbonzini@redhat.com> > >>> +bool parallel_cpus; >> >> So lets add some words to this to distinguish between this and the mttcg >> enabled flags and its relation to -smp. Something like: >> >> parallel_cpus indicates to the front-ends if code needs to be >> generated taking into account multiple threads of execution. It will >> be true for linux-user after the first thread clone and if system mode >> if MTTCG is enabled. On the transition from false->true any code >> generated while false needs to be invalidated. It may be temporally >> set to false when generating non-cached code in an exclusive context. > > Sure. > > > r~ -- Alex Bennée