From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39261) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cWOSH-0000WQ-Ba for qemu-devel@nongnu.org; Wed, 25 Jan 2017 09:21:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cWOSE-0007kN-6Y for qemu-devel@nongnu.org; Wed, 25 Jan 2017 09:21:45 -0500 Received: from mail-wm0-x229.google.com ([2a00:1450:400c:c09::229]:36141) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1cWOSD-0007kD-Uv for qemu-devel@nongnu.org; Wed, 25 Jan 2017 09:21:42 -0500 Received: by mail-wm0-x229.google.com with SMTP id c85so28515229wmi.1 for ; Wed, 25 Jan 2017 06:21:41 -0800 (PST) References: <20170119170507.16185-1-alex.bennee@linaro.org> <20170119170507.16185-19-alex.bennee@linaro.org> <3604b49c-6d3b-5525-9a26-8226f4d2f8c9@twiddle.net> <87k29k9ozb.fsf@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: Date: Wed, 25 Jan 2017 14:21:38 +0000 Message-ID: <87h94n9q4t.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v7 18/27] cputlb: introduce tlb_flush_*_all_cpus List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: mttcg@greensocs.com, qemu-devel@nongnu.org, fred.konrad@greensocs.com, a.rigo@virtualopensystems.com, cota@braap.org, bobby.prani@gmail.com, nikunj@linux.vnet.ibm.com, peter.maydell@linaro.org, claudio.fontana@huawei.com, Peter Crosthwaite , jan.kiszka@siemens.com, mark.burton@greensocs.com, serge.fdrv@gmail.com, pbonzini@redhat.com, bamvor.zhangjian@linaro.org Richard Henderson writes: > On 01/24/2017 12:34 PM, Alex Bennée wrote: >> >> Richard Henderson writes: >> >>> On 01/19/2017 09:04 AM, Alex Bennée wrote: >>>> +/* flush_all_helper: run fn across all cpus >>>> + * >>>> + * If the wait flag is set then the src cpu's helper will be queued as >>>> + * "safe" work and the loop exited creating a synchronisation point >>>> + * where all queued work will be finished before execution starts >>>> + * again. >>>> + */ >>>> +static void flush_all_helper(CPUState *src, bool wait, >>>> + run_on_cpu_func fn, run_on_cpu_data d) >>>> +{ >>>> + CPUState *cpu; >>>> + >>>> + if (!wait) { >>>> + CPU_FOREACH(cpu) { >>>> + if (cpu != src) { >>>> + async_run_on_cpu(cpu, fn, d); >>>> + } else { >>>> + g_assert(qemu_cpu_is_self(src)); >>>> + fn(src, d); >>>> + } >>>> + } >>>> + } else { >>>> + CPU_FOREACH(cpu) { >>>> + if (cpu != src) { >>>> + async_run_on_cpu(cpu, fn, d); >>>> + } else { >>>> + async_safe_run_on_cpu(cpu, fn, d); >>>> + } >>>> + >>>> + } >>>> + cpu_loop_exit(src); >>>> + } >>>> +} >>> >>> What's the rationale for not having the target do the exit itself? Surely it >>> can tell, and simple end the TB after the insn. >> >> It's more for the global sync functionality. I wanted to keep all the >> guts of re-starting the loop with the correct async_safe_work all in one >> place with a defined API for the guests rather than have them all do it >> themselves. >> >> For the common case of not needing to sync across the cores I agree the >> guest is perfectly able to end the TB so its safe work completes next. >> In fact the ARM helper calls do exactly that. > > Hmm. Would it make more sense to have two functions then, for wait and !wait? > That would allow the wait function be QEMU_NORETURN, which might make it a bit > more obvious about the interface contract. Seems fair. I was worried about multiplying out too many variants in the API but this seems a good reason to. -- Alex Bennée