Re: [Qemu-devel] [PATCH v2] Make target_phys_addr_t 64 bits unconditionally

[Avi Kivity]

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 <address@hidden>
> 
> 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