From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43900) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akYwR-0001Jb-UE for qemu-devel@nongnu.org; Mon, 28 Mar 2016 11:18:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1akYwO-000563-NR for qemu-devel@nongnu.org; Mon, 28 Mar 2016 11:18:55 -0400 Received: from mail-lf0-x229.google.com ([2a00:1450:4010:c07::229]:35574) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akYwO-00055n-Ar for qemu-devel@nongnu.org; Mon, 28 Mar 2016 11:18:52 -0400 Received: by mail-lf0-x229.google.com with SMTP id k79so12998704lfb.2 for ; Mon, 28 Mar 2016 08:18:51 -0700 (PDT) From: Sergey Fedorov References: <1458222382-6498-1-git-send-email-sergey.fedorov@linaro.org> <1458222382-6498-5-git-send-email-sergey.fedorov@linaro.org> <56EAC8A2.7060700@redhat.com> <56EAC9E3.60000@gmail.com> Message-ID: <56F94B59.80905@gmail.com> Date: Mon, 28 Mar 2016 18:18:49 +0300 MIME-Version: 1.0 In-Reply-To: <56EAC9E3.60000@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/5] tcg: reorder removal from lists in tb_phys_invalidate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , sergey.fedorov@linaro.org, qemu-devel@nongnu.org Cc: Richard Henderson , Peter Crosthwaite On 17/03/16 18:14, Sergey Fedorov wrote: > On 17/03/16 18:09, Paolo Bonzini wrote: >> >> On 17/03/2016 14:46, sergey.fedorov@linaro.org wrote: >>> void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t >>> page_addr) >>> { >>> - CPUState *cpu; >>> PageDesc *p; >>> unsigned int h, n1; >>> + tb_page_addr_t pc; >>> tb_page_addr_t phys_pc; >>> TranslationBlock *tb1, *tb2; >>> - /* remove the TB from the hash list */ >>> - phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK); >>> - h = tb_phys_hash_func(phys_pc); >>> - tb_hash_remove(&tcg_ctx.tb_ctx.tb_phys_hash[h], tb); >>> - >>> - /* remove the TB from the page list */ >>> - if (tb->page_addr[0] != page_addr) { >>> - p = page_find(tb->page_addr[0] >> TARGET_PAGE_BITS); >>> - tb_page_remove(&p->first_tb, tb); >>> - invalidate_page_bitmap(p); >>> - } >>> - if (tb->page_addr[1] != -1 && tb->page_addr[1] != page_addr) { >>> - p = page_find(tb->page_addr[1] >> TARGET_PAGE_BITS); >>> - tb_page_remove(&p->first_tb, tb); >>> - invalidate_page_bitmap(p); >>> - } >>> - >>> - tcg_ctx.tb_ctx.tb_invalidated_flag = 1; >>> - >> Did you investigate the removal of this setting of tb_invalidated_flag? >> >> My recollection is that it is okay to remove it because at worse it >> would cause a tb_add_jump from an invalidated source to a valid >> destination. This should be harmless as long as the source has been >> tb_phys_invalidated and not tb_flushed. But this needs to be checked. > > Thanks for pointing that. I should investigate it to make sure, > although arm32/arm64/x86_64 Linux boots fine as well as the latest > Alex's kmv-unit-tests pass. The use pattern of 'tb_invalidated_flag' is a bit intricate; correct me, if I'm wrong about the following. Basically, 'tb_invalidated_flag' was meant to catch two events: * some TB has been invalidated by tb_phys_invalidate(); * the whole translation buffer has been flushed by tb_flush(). Then it is checked to ensure: * the last executed TB can be safely patched to directly call the next one in cpu_exec(); * the original TB should be provided for further possible invalidation along with the temporarily generated TB when in cpu_exec_nocache(). What, I think, we couldn't be sure in is the cpu_exec_nocache() case. It could look like a kind of corner case, but it could result in stalls, if the original TB isn't invalidated properly, see b4ac20b4df0d ("cpu-exec: fix cpu_exec_nocache"). So I would suggest the following solution:s (1) Use 'tb->pc' as an indicator of whether TB is valid; check for it in cpu_exec() when deciding on whether to patch the last executed TB or not (2) Use 'tcg_ctx.tb_ctx.tb_flush_count' to check for translation buffer flushes; capture it before calling tb_gen_code() and compare to it afterwards to check if tb_flush() has been called in between What do you think? Kind regards, Sergey