From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42499) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjXiI-0006XZ-7F for qemu-devel@nongnu.org; Mon, 12 Sep 2016 16:20:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjXiC-00026K-Fn for qemu-devel@nongnu.org; Mon, 12 Sep 2016 16:20:21 -0400 Received: from mail-yw0-f196.google.com ([209.85.161.196]:35825) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjXiC-000264-BL for qemu-devel@nongnu.org; Mon, 12 Sep 2016 16:20:16 -0400 Received: by mail-yw0-f196.google.com with SMTP id u82so3726024ywc.2 for ; Mon, 12 Sep 2016 13:20:16 -0700 (PDT) Sender: Richard Henderson References: <1472935202-3342-1-git-send-email-rth@twiddle.net> <1472935202-3342-7-git-send-email-rth@twiddle.net> <87twdlw6cv.fsf@linaro.org> From: Richard Henderson Message-ID: <0bcd5093-52f2-d642-4a1c-e3557084053a@twiddle.net> Date: Mon, 12 Sep 2016 13:19:12 -0700 MIME-Version: 1.0 In-Reply-To: <87twdlw6cv.fsf@linaro.org> Content-Type: text/plain; charset=utf-8; format=flowed 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: =?UTF-8?Q?Alex_Benn=c3=a9e?= Cc: qemu-devel@nongnu.org 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... >> +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? >> +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~