From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55040) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TKo3r-0003VB-0v for qemu-devel@nongnu.org; Sun, 07 Oct 2012 06:26:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TKo3p-0002OH-LA for qemu-devel@nongnu.org; Sun, 07 Oct 2012 06:26:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44071) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TKo3p-0002O7-Av for qemu-devel@nongnu.org; Sun, 07 Oct 2012 06:26:13 -0400 Message-ID: <507158AE.1050805@redhat.com> Date: Sun, 07 Oct 2012 12:25:50 +0200 From: Avi Kivity MIME-Version: 1.0 References: <1349346964-4151-1-git-send-email-avi@redhat.com> <506DD230.70509@weilnetz.de> In-Reply-To: <506DD230.70509@weilnetz.de> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] Make target_phys_addr_t 64 bits unconditionally List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Weil Cc: Jia Liu , qemu-devel@nongnu.org, Blue Swirl , Max Filippov , Michael Walle , Paul Brook , Anthony Liguori , "Edgar E. Iglesias" , Aurelien Jarno On 10/04/2012 08:15 PM, Stefan Weil wrote: > Am 04.10.2012 12:36, schrieb Avi Kivity: >> The hassle and compile time overhead of maintaining both 32-bit and >> 64-bit >> capable source isn't worth the tiny performance advantage which is >> seen on >> a minority of configurations. Switch to compiling libhw only once, with >> target_phys_addr_t unconditionally typedefed to uint64_t. >> >> Signed-off-by: Avi Kivity > > Hi, > > I noticed that you replaced target_phys_addr_t by uint64_t in two lines. In those two lines, int64_t is more correct than t_p_a_t. Explanation below. > Are there plans to replace target_phys_addr_t everywhere? Yes, by hw_addr (or hwaddr, or phys, or ...). > Should new code use uint64_t, or should it continue to use > target_phys_addr_t? target_phys_addr_t. > > Using target_phys_addr_t might make support for 128 bit in some years > easier > because it allows identifying critical code, Agree. > although I think it will be difficult to avoid wrong use of either > target_phys_addr_t > or uint64_t as long as both are the same size. Some languages allow enforcing this, but C doesn't. > > >> -#if TARGET_PHYS_ADDR_BITS > 32 >> - return low | ((target_phys_addr_t)high << 32); >> -#else >> - return low; >> -#endif >> + return low | ((uint64_t)high << 32); >> Shifting by 32 is not defined for types that are 32 bits or narrower. On x86, for example, a shift by 32 of a 32-bit quantity is the identity operation (where mathematically you would expect the result to be 0, not the first argument). Since the context does not guarantee that target_phys_addr_t is wider than 32 bits, I cast it to a known-wide type, then (implicitly) cast it back to target_phys_addr_t. > >> - *raddrp |= (target_phys_addr_t)(tlb->RPN & 0xF) << 32; >> + *raddrp |= (uint64_t)(tlb->RPN & 0xF) << 32; > Same applies here, of course. Since target_phys_addr_t is 64 bits, it would have worked "by accident", but if we'd have switched back to 32 bits in the future (unlikely but possible) then I would have introduced a bug here. -- error compiling committee.c: too many arguments to function