From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52276) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bonXm-0007Gs-PW for qemu-devel@nongnu.org; Tue, 27 Sep 2016 04:15:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bonXi-0000dk-Jr for qemu-devel@nongnu.org; Tue, 27 Sep 2016 04:15:13 -0400 Received: from mx5-phx2.redhat.com ([209.132.183.37]:57194) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bonXi-0000bO-9V for qemu-devel@nongnu.org; Tue, 27 Sep 2016 04:15:10 -0400 Date: Tue, 27 Sep 2016 04:15:07 -0400 (EDT) From: Paolo Bonzini Message-ID: <2098984102.3019519.1474964107560.JavaMail.zimbra@redhat.com> In-Reply-To: References: <1473847013-20191-1-git-send-email-pbonzini@redhat.com> <1473847013-20191-3-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] target-arm: add env->tbflags List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers > Doing this for all MSR writes is a bit sad, because a lot of them > don't actually change the TB flags, and quite a few of them which > previously we were able to code to not have to do a helper call > at all (direct writes to fields) now get a pointless helper call. True. On the other hand, MSR writes terminate the TB so you are losing all the TB state anyway. Before these patches you weren't recomputing the TB flags in the common case of adjacent MSR writes on the same page (so QEMU could use linked TBs), now you are. However, given the speedup from the patch, I felt it was premature optimization. If there is a case where you get the helper in the profile, it is possible to add a new ARM_CP_KEEP_TBFLAGS flag to ARMCPRegInfo. > You're also recalculating more often than stated here, in that > you also recalc on any gen_lookup_tb() call in the 32-bit > decoder. (This is just as well because for instance vec_len > and vec_stride aren't set via the cp15 system register write > path.) Right. This was of course on purpose, but the commit message was imprecise. > You're treating the PSTATE_SS flag as static, but you don't > have anything which causes a recalculation of it on the code > path which changes it (gen_ss_advance()). > > The 32-bit SETEND instruction changes CPSR_E, which has > an effect on the BE_DATA_MASK flag, but I don't think > that code path will cause us to recalculate flags. This actually points to a bigger deficiency, in that---even outside the PSTATE_SS and SETEND code paths---both pstate_write and cpsr_write need to recompute the flags. But I think that's the only other case left. Do you prefer to have the setend and clear_pstate_ss helpers call cpsr_write/pstate_write, or do you prefer to inline the modification to the tbflags? > I found this patch kind of difficult to review because > it isn't obvious why we recalculate the static flags at > the points where we do (ie whether those points are > necessary and sufficient for correct behaviour). Most > of the comments above are the result of my looking at > whether some particular flags that I suspected of being > tricky were handled correctly :-) You definitely have a point here. Adding an assertion requires looking at CPUARMState in gen_intermediate_code. You're not really supposed to do that, but I guess it's okay as long as it's for debugging purposes. Paolo