| Message ID | 1286888457-5033-9-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 12 Oct 2010, Paolo Bonzini wrote: > Do not hardcode the list of 64-bit CPUs. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > configure | 14 ++++++++------ > 1 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/configure b/configure > index cb76177..873e4a6 100755 > --- a/configure > +++ b/configure > @@ -1088,12 +1088,14 @@ esac > fi > > # host long bits test > -hostlongbits="32" > -case "$cpu" in > - x86_64|alpha|ia64|sparc64|ppc64|s390x) > - hostlongbits=64 > - ;; > -esac > +cat > $TMPC << EOF > +int sizeof_long_is_8[sizeof(long) == 8 ? 1 : -1]; > +EOF > +if compile_object; then > +hostlongbits=64 > +else > +hostlongbits=32 > +fi This is wrong.
On 10/12/2010 03:47 PM, malc wrote: >> # host long bits test >> -hostlongbits="32" >> -case "$cpu" in >> - x86_64|alpha|ia64|sparc64|ppc64|s390x) >> - hostlongbits=64 >> - ;; >> -esac >> +cat> $TMPC<< EOF >> +int sizeof_long_is_8[sizeof(long) == 8 ? 1 : -1]; >> +EOF >> +if compile_object; then >> +hostlongbits=64 >> +else >> +hostlongbits=32 >> +fi > > This is wrong. Care to expand? $ grep LONG +build*/config-host.h +build-32/config-host.h:#define HOST_LONG_BITS 32 +build-64/config-host.h:#define HOST_LONG_BITS 64 Paolo
On Tue, 12 Oct 2010, Paolo Bonzini wrote: > On 10/12/2010 03:47 PM, malc wrote: > > > # host long bits test > > > -hostlongbits="32" > > > -case "$cpu" in > > > - x86_64|alpha|ia64|sparc64|ppc64|s390x) > > > - hostlongbits=64 > > > - ;; > > > -esac > > > +cat> $TMPC<< EOF > > > +int sizeof_long_is_8[sizeof(long) == 8 ? 1 : -1]; > > > +EOF > > > +if compile_object; then > > > +hostlongbits=64 > > > +else > > > +hostlongbits=32 > > > +fi > > > > This is wrong. > > Care to expand? Gives wrong results on Win64. > > $ grep LONG +build*/config-host.h > +build-32/config-host.h:#define HOST_LONG_BITS 32 > +build-64/config-host.h:#define HOST_LONG_BITS 64 > > Paolo >
On 10/12/2010 04:38 PM, malc wrote: > On Tue, 12 Oct 2010, Paolo Bonzini wrote: > >> On 10/12/2010 03:47 PM, malc wrote: >>>> # host long bits test >>>> -hostlongbits="32" >>>> -case "$cpu" in >>>> - x86_64|alpha|ia64|sparc64|ppc64|s390x) >>>> - hostlongbits=64 >>>> - ;; >>>> -esac >>>> +cat> $TMPC<< EOF >>>> +int sizeof_long_is_8[sizeof(long) == 8 ? 1 : -1]; >>>> +EOF >>>> +if compile_object; then >>>> +hostlongbits=64 >>>> +else >>>> +hostlongbits=32 >>>> +fi >>> >>> This is wrong. >> >> Care to expand? > > Gives wrong results on Win64. Then it's not HOST_LONG_BITS, it's HOST_POINTER_BITS. Paolo
On Tue, 12 Oct 2010, Paolo Bonzini wrote: > On 10/12/2010 04:38 PM, malc wrote: > > On Tue, 12 Oct 2010, Paolo Bonzini wrote: > > > > > On 10/12/2010 03:47 PM, malc wrote: > > > > > # host long bits test > > > > > -hostlongbits="32" > > > > > -case "$cpu" in > > > > > - x86_64|alpha|ia64|sparc64|ppc64|s390x) > > > > > - hostlongbits=64 > > > > > - ;; > > > > > -esac > > > > > +cat> $TMPC<< EOF > > > > > +int sizeof_long_is_8[sizeof(long) == 8 ? 1 : -1]; > > > > > +EOF > > > > > +if compile_object; then > > > > > +hostlongbits=64 > > > > > +else > > > > > +hostlongbits=32 > > > > > +fi > > > > > > > > This is wrong. > > > > > > Care to expand? > > > > Gives wrong results on Win64. > > Then it's not HOST_LONG_BITS, it's HOST_POINTER_BITS. Not quite, [s]size_t/ptrdiff_t are 64 bits wide udner Win64, little to do with pointers. > > Paolo >
On 10/12/2010 04:41 PM, malc wrote: > > > > Gives wrong results on Win64. > > > > Then it's not HOST_LONG_BITS, it's HOST_POINTER_BITS. > > Not quite, [s]size_t/ptrdiff_t are 64 bits wide udner Win64, little > to do with pointers. Before (on Win64): sizeof(long) == 4, HOST_LONG_BITS == 64 After: sizeof(long) == 4, HOST_LONG_BITS == 32 If you say HOST_LONG_BITS should be 64, then I say it is poorly named: it represents sizeof(void*) * CHAR_BIT and should be named HOST_POINTER_BITS. BTW, HOST_LONG_BITS is used mostly for user-mode emulation, which does not matter for Win64. In exec-all.h it is used to second-guess sizeof(tcg_target_long), which would be the size of a pointer. However, it is safe to give a conservative value based on sizeof(long). Finally, in other places it is definitely used to mean sizeof(long). Even though the softmmu is probably not IL32P64-clean, there you have this: #if HOST_LONG_BITS == 32 && TARGET_LONG_BITS == 32 #define CPU_TLB_ENTRY_BITS 4 #else #define CPU_TLB_ENTRY_BITS 5 #endif typedef struct CPUTLBEntry { /* bit TARGET_LONG_BITS to TARGET_PAGE_BITS : virtual address bit TARGET_PAGE_BITS-1..4 : Nonzero for accesses that should not go directly to ram. bit 3 : indicates that the entry is invalid bit 2..0 : zero */ target_ulong addr_read; target_ulong addr_write; target_ulong addr_code; /* Addend to virtual address to get host address. IO accesses use the corresponding iotlb value. */ unsigned long addend; /* padding to get a power of two size */ uint8_t dummy[(1 << CPU_TLB_ENTRY_BITS) - (sizeof(target_ulong) * 3 + ((-sizeof(target_ulong) * 3) & (sizeof(unsigned long) - 1)) + sizeof(unsigned long))]; } CPUTLBEntry; _As things are now_ it is more correct to use sizeof(long), or at least more conservative. I can certainly change it to sizeof(void*) in v2, even though I don't think it's a good idea, but it's your call as a maintainer. Paolo
On Tue, 12 Oct 2010, Paolo Bonzini wrote: > On 10/12/2010 04:41 PM, malc wrote: > > > > > Gives wrong results on Win64. > > > > > > Then it's not HOST_LONG_BITS, it's HOST_POINTER_BITS. > > > > Not quite, [s]size_t/ptrdiff_t are 64 bits wide udner Win64, little > > to do with pointers. > > Before (on Win64): sizeof(long) == 4, HOST_LONG_BITS == 64 > > After: sizeof(long) == 4, HOST_LONG_BITS == 32 > > If you say HOST_LONG_BITS should be 64, then I say it is poorly named: it > represents sizeof(void*) * CHAR_BIT and should be named HOST_POINTER_BITS. > > BTW, HOST_LONG_BITS is used mostly for user-mode emulation, which does not > matter for Win64. In exec-all.h it is used to second-guess > sizeof(tcg_target_long), which would be the size of a pointer. However, > it is safe to give a conservative value based on sizeof(long). > > Finally, in other places it is definitely used to mean sizeof(long). Even > though the softmmu is probably not IL32P64-clean, there you have this: > > #if HOST_LONG_BITS == 32 && TARGET_LONG_BITS == 32 > #define CPU_TLB_ENTRY_BITS 4 > #else > #define CPU_TLB_ENTRY_BITS 5 > #endif > > typedef struct CPUTLBEntry { > /* bit TARGET_LONG_BITS to TARGET_PAGE_BITS : virtual address > bit TARGET_PAGE_BITS-1..4 : Nonzero for accesses that should not > go directly to ram. > bit 3 : indicates that the entry is invalid > bit 2..0 : zero > */ > target_ulong addr_read; > target_ulong addr_write; > target_ulong addr_code; > /* Addend to virtual address to get host address. IO accesses > use the corresponding iotlb value. */ > unsigned long addend; > /* padding to get a power of two size */ > uint8_t dummy[(1 << CPU_TLB_ENTRY_BITS) - > (sizeof(target_ulong) * 3 + > ((-sizeof(target_ulong) * 3) & (sizeof(unsigned long) - 1)) + > sizeof(unsigned long))]; > } CPUTLBEntry; > > _As things are now_ it is more correct to use sizeof(long), or at > least more conservative. > > I can certainly change it to sizeof(void*) in v2, even though I don't think > it's a good idea, but it's your call as a maintainer. > Bottom line: QEMU assumes that long is large enough to hold a pointer, this assumption is wrong, and your patch is wrong (not because it miscalculates size of ABI long) but because of the assumptions current code has. Whether two wrongs makes right is an open question, currently Win64 isnot supported in any shape or form, even though Filip Navarra once did a port[1] which never went into mainline. [1] http://marc.info/?l=qemu-devel&m=124912692531724&w=2
On 10/12/2010 05:12 PM, malc wrote: > Bottom line: QEMU assumes that long is large enough to hold a pointer, > this assumption is wrong, and your patch is wrong (not because it > miscalculates size of ABI long) but because of the assumptions current > code has. Whether two wrongs makes right is an open question, currently > Win64 isnot supported in any shape or form, even though Filip Navarra > once did a port[1] which never went into mainline. > > [1] http://marc.info/?l=qemu-devel&m=124912692531724&w=2 His port apparently is fine with HOST_LONG_BITS == sizeof(void *), I'll adjust for v2. Paolo
diff --git a/configure b/configure index cb76177..873e4a6 100755 --- a/configure +++ b/configure @@ -1088,12 +1088,14 @@ esac fi # host long bits test -hostlongbits="32" -case "$cpu" in - x86_64|alpha|ia64|sparc64|ppc64|s390x) - hostlongbits=64 - ;; -esac +cat > $TMPC << EOF +int sizeof_long_is_8[sizeof(long) == 8 ? 1 : -1]; +EOF +if compile_object; then +hostlongbits=64 +else +hostlongbits=32 +fi ##########################################
Do not hardcode the list of 64-bit CPUs. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- configure | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-)