From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49568) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4Rzm-0004o1-DX for qemu-devel@nongnu.org; Mon, 15 Jun 2015 06:52:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4Rzh-0004e6-64 for qemu-devel@nongnu.org; Mon, 15 Jun 2015 06:52:02 -0400 Received: from mail-yk0-f180.google.com ([209.85.160.180]:35616) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4Rzh-0004e0-2T for qemu-devel@nongnu.org; Mon, 15 Jun 2015 06:51:57 -0400 Received: by ykar6 with SMTP id r6so26467594yka.2 for ; Mon, 15 Jun 2015 03:51:56 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20150615005200.GP17878@toto> References: <1433500421-22879-1-git-send-email-edgar.iglesias@gmail.com> <1433500421-22879-2-git-send-email-edgar.iglesias@gmail.com> <20150615005200.GP17878@toto> From: Peter Maydell Date: Mon, 15 Jun 2015 11:51:36 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v4 1/6] target-arm: Add CNTVOFF_EL2 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Edgar E. Iglesias" Cc: "Edgar E. Iglesias" , Sergey Fedorov , =?UTF-8?B?QWxleCBCZW5uw6ll?= , QEMU Developers , Alexander Graf On 15 June 2015 at 01:52, Edgar E. Iglesias wrote: > On Fri, Jun 12, 2015 at 05:44:24PM +0100, Peter Maydell wrote: >> On 5 June 2015 at 11:33, Edgar E. Iglesias wrote: >> > + int istatus = (int64_t) (count - offset - gt->cval) >= 0; >> >> This is wrong. Consider the case where: >> count is 0x1000,0000,0000,0002 (it's a really large unsigned number) >> offset is zero >> cval is 1 >> >> The ARM ARM required calculation gives you >> 0x1000,0000,0000,0002 - 1 >= 0 >> ie 0x1000,0000,0000,0001 >= 0 >> which is true. (Note that ARM ARM pseudocode works with infinite >> precision integers, not 2s-complement.) >> >> With your code: >> (count - offset - gt->cval) is 0x1000,0000,0000,0001 >> Cast to an int64_t this is negative (top bit is set) >> Comparison against 0 is done as a signed value, and returns false. >> >> This is exactly the tricky case which is why we must do this as unsigned >> arithmetic. >> >> What you want is >> int istatus = count - offset >= gt->cval; >> >> which comes out to >> 0x1000,0000,0000,0002 >= 1 >> which is true. >> >> (That's the code we had before, but just "use 'count - offset' rather than >> 'count'".) > > Thanks, I've changed it to what you suggest allthough I'm probably missing > something cause I'm still finding the spec confusing :S If we had 128 bit integers we could do it your way, only casting to int128_t rather than int64_t (which would be approximating the pseudocode's infinite-precision signed integers with 128-bit ints, which works because we know we don't have anything out of that range). The tricky stuff with uint64_t is just because we don't want to have to go to 128-bit arithmetic if we can avoid it. As I say the thing it's easy to forget when reading ARM ARM pseudocode is that the integer and real data types are true mathematical integers and reals, not the limited-width uint32_t, uint64_t, float and double types the CPU actually deals with. There's always a conversion to/from bitstring somewhere along the line. (Appendix J9 has a description of the pseudocode data types and syntax.) -- PMM