From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55721) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqixe-0000b5-9y for qemu-devel@nongnu.org; Thu, 14 Apr 2016 11:13:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aqixb-0006IH-KM for qemu-devel@nongnu.org; Thu, 14 Apr 2016 11:13:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37049) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqixb-0006Hs-D0 for qemu-devel@nongnu.org; Thu, 14 Apr 2016 11:13:35 -0400 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> <56F94B59.80905@gmail.com> <56F9A051.9090907@redhat.com> <56FA52E3.3000900@gmail.com> <56FA5ADB.7030103@redhat.com> <570FACF1.6020009@gmail.com> From: Paolo Bonzini Message-ID: <570FB396.6040703@redhat.com> Date: Thu, 14 Apr 2016 17:13:26 +0200 MIME-Version: 1.0 In-Reply-To: <570FACF1.6020009@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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: Sergey Fedorov , sergey.fedorov@linaro.org, qemu-devel@nongnu.org Cc: Richard Henderson , Peter Crosthwaite , =?UTF-8?Q?Alex_Benn=c3=a9e?= On 14/04/2016 16:45, Sergey Fedorov wrote: > So what would you suggest to use for x86? I can't think of something > that looks like a really compelling combination when I look at > cpu_get_tb_cpu_state() in target-i386/cpu.h. On x86 I think we should define HF_INVALID_TB to an invalid flag combination. I can think of several solutions: - #defining HF_INVALID_TB to an invalid combination (e.g. HF_CS64_MASK, because it always appears together with HF_LMA_MASK; see code that updates those hflags in cpu_x86_load_seg_cache, cpu_x86_update_cr0, kvm_get_sregs). Advantage: doesn't waste a bit, reasonably self documenting. Disadvantage: a bit tricky, but still my favorite. - rename HF_SOFTMMU_MASK to HF_INVALID_MASK (it's always the same as CONFIG_SOFTMMU so we can remove it), then #define HF_INVALID_TB HF_INVALID_MASK. Advantage: obviously correct. Disadvantage: wastes a bit. My second favorite. - #defining HF_INVALID_TB to -1. Advantage: ?!? Disadvantage: everything. Looks tame, actually a huge hack - #defining HF_INVALID_TB to the "wrong" direction HF_SOFTMMU_MASK (i.e. to 0 if CONFIG_SOFTMMU, . Advantage: obviously correct. Disadvantage: huge hack, HF_SOFTMMU_MASK is unused anyway. Choose your own favorite. :) (Setting cs_base to -1 actually would work on 64-bit x86, but not on qemu-system-i386). > Personally, I'm not so > happy trying to use pc/cs_base/flags to mark an invalid TB. Are my > worries unreasonable? :) Can you explain your worries? The advantages are that it's O(1) and it obviously doesn't affect other TBs than the invalidated one. > Anyway, I am wondering if there is still a way to clear tb_phys_hash an= d > tb_jmp_cache safely. > > Maybe something like this: > * Remove the TB from physical hash list So at this point tb_find_slow cannot find it. > * Memory barrier > * Remove the TB from each vCPU's virtual address hash cache tb_find_fast then cannot find it either. > Would that work? This is very similar to the current code. From 10,000 feet, because tb_find_fast calls tb_find_slow, this could indeed work, but I'm a bit concerned about how to order the removal of the jump lists. The usage of "tcg_ctx.tb_ctx.tb_invalidated_flag =3D 1" in the existing code was what worries me. Indeed the motivation of this patch was removing that single line of code to prepare for the move of tb_invalidated_flag to CPUState. Also, this loop will not be thread-safe anymore as soon as Fred's "tb_jmp_cache lookup outside tb_lock" goes in: CPU_FOREACH(cpu) { if (cpu->tb_jmp_cache[h] =3D=3D tb) { cpu->tb_jmp_cache[h] =3D NULL; } } It should use atomic_cmpxchg (slow!) or to unconditionally NULL out cpu->tb_jmp_cache (a bit hacky). Preparing for that change is an added bonus of the tb-hacking approach. Paolo