| Message ID | 1364913860-25159-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2 April 2013 15:44, Paolo Bonzini <pbonzini@redhat.com> wrote: > This is no different from, for example, TARGET_LONG_BITS. It does > not belong in configure. > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h > index 42c36e2..19d4e4c 100644 > --- a/target-ppc/cpu.h > +++ b/target-ppc/cpu.h > @@ -27,6 +27,7 @@ > #if defined (TARGET_PPC64) > /* PowerPC 64 definitions */ > #define TARGET_LONG_BITS 64 > +#define TARGET_LONG_ALIGNMENT 8 > #define TARGET_PAGE_BITS 12 Doesn't this incorrectly set the long alignment to 8 for ppc64abi32? (Probably similar problem for sparc32plus and mipsn32. The underlying point here is that alignment is an ABI decision and you can have more than one ABI for a particular TARGET_FOO.) -- PMM
Il 02/04/2013 18:43, Peter Maydell ha scritto: > On 2 April 2013 15:44, Paolo Bonzini <pbonzini@redhat.com> wrote: >> This is no different from, for example, TARGET_LONG_BITS. It does >> not belong in configure. > >> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h >> index 42c36e2..19d4e4c 100644 >> --- a/target-ppc/cpu.h >> +++ b/target-ppc/cpu.h >> @@ -27,6 +27,7 @@ >> #if defined (TARGET_PPC64) >> /* PowerPC 64 definitions */ >> #define TARGET_LONG_BITS 64 >> +#define TARGET_LONG_ALIGNMENT 8 >> #define TARGET_PAGE_BITS 12 > > Doesn't this incorrectly set the long alignment to 8 > for ppc64abi32? (Probably similar problem for > sparc32plus and mipsn32. The underlying point here is that > alignment is an ABI decision and you can have more than one > ABI for a particular TARGET_FOO.) Hmm, seems like you're right _but_ I am not sure if the *current* code is correct. On real hardware, the CPUs are certainly not able to do unaligned 32-bit accesses, and target_long/target_ulong pointers look like they're never used for data that comes from target memory. Rather, they're used for by-reference passing into functions, and stuff like that. What these targets want to have 32-bit alignment is really abi_long/abi_ulong, and that's already okay. Alex, Blue, Aurelien, can you test the above three targets? Paolo
On 2 April 2013 17:56, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 02/04/2013 18:43, Peter Maydell ha scritto: >> Doesn't this incorrectly set the long alignment to 8 >> for ppc64abi32? (Probably similar problem for >> sparc32plus and mipsn32. The underlying point here is that >> alignment is an ABI decision and you can have more than one >> ABI for a particular TARGET_FOO.) > > Hmm, seems like you're right _but_ I am not sure if the *current* code > is correct. On real hardware, the CPUs are certainly not able to do > unaligned 32-bit accesses, and target_long/target_ulong pointers look > like they're never used for data that comes from target memory. Did you check linux-user too? I'm pretty sure we have structs and so on that mirror target memory and use target_ulong. > What these targets want to have 32-bit alignment is really > abi_long/abi_ulong, and that's already okay. Alex, Blue, Aurelien, > can you test the above three targets? Mmm, rather than speculating we should just confirm what gcc thinks the alignment of void* should be on these targets (since "a thing the size of a pointer" is what target_long/ulong represent, I think.) That said, we should keep bugfixes and cleanup patches separated, so on approach for proceeding with these cleanup patches is just to define TARGET_LONG_ALIGNMENT based on TARGET_ABI32 or whatever is appropriate for each target CPU. Then we retain the same behaviour. -- PMM
Il 02/04/2013 19:17, Peter Maydell ha scritto: > On 2 April 2013 17:56, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 02/04/2013 18:43, Peter Maydell ha scritto: >>> Doesn't this incorrectly set the long alignment to 8 >>> for ppc64abi32? (Probably similar problem for >>> sparc32plus and mipsn32. The underlying point here is that >>> alignment is an ABI decision and you can have more than one >>> ABI for a particular TARGET_FOO.) >> >> Hmm, seems like you're right _but_ I am not sure if the *current* code >> is correct. On real hardware, the CPUs are certainly not able to do >> unaligned 32-bit accesses, and target_long/target_ulong pointers look >> like they're never used for data that comes from target memory. > > Did you check linux-user too? I'm pretty sure we have structs > and so on that mirror target memory and use target_ulong. > >> What these targets want to have 32-bit alignment is really >> abi_long/abi_ulong, and that's already okay. Alex, Blue, Aurelien, >> can you test the above three targets? > > Mmm, rather than speculating we should just confirm what gcc > thinks the alignment of void* should be on these targets > (since "a thing the size of a pointer" is what target_long/ulong > represent, I think.) I think "a thing the size of a pointer" should be abi_long/ulong. The pointer is not a CPU concept. > That said, we should keep bugfixes and cleanup patches separated, Indeed. The change was unintended, and your comment is worth a respin. > so on approach for proceeding with these cleanup patches is just > to define TARGET_LONG_ALIGNMENT based on TARGET_ABI32 or whatever is > appropriate for each target CPU. Then we retain the same behaviour. Yes, TARGET_ABI32. I'll have to squash patches 1 and 2, and the testing RFH still holds because this alignment things sounds fishy... Paolo
On 2 April 2013 18:26, Paolo Bonzini <pbonzini@redhat.com> wrote: > I think "a thing the size of a pointer" should be abi_long/ulong. The > pointer is not a CPU concept. Yeah. OTOH type alignment isn't a CPU concept either, so I'm a little suspicious of these defines in general. -- PMM
On Tue, Apr 02, 2013 at 05:43:36PM +0100, Peter Maydell wrote: > On 2 April 2013 15:44, Paolo Bonzini <pbonzini@redhat.com> wrote: > > This is no different from, for example, TARGET_LONG_BITS. It does > > not belong in configure. > > > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h > > index 42c36e2..19d4e4c 100644 > > --- a/target-ppc/cpu.h > > +++ b/target-ppc/cpu.h > > @@ -27,6 +27,7 @@ > > #if defined (TARGET_PPC64) > > /* PowerPC 64 definitions */ > > #define TARGET_LONG_BITS 64 > > +#define TARGET_LONG_ALIGNMENT 8 > > #define TARGET_PAGE_BITS 12 > > Doesn't this incorrectly set the long alignment to 8 > for ppc64abi32? (Probably similar problem for > sparc32plus and mipsn32. The underlying point here is that > alignment is an ABI decision and you can have more than one > ABI for a particular TARGET_FOO.) > I'll do a test for mipsn32, but first I'll try to look how/where it is used. Note that ppc64abi32 and sparc32plus are different than mipsn32. The first two are basically executing a 32-bit binary (with the 32-bit ABI) on a 64-bit emulated CPU, with for sparc access to more instructions. mipsn32 is a different ABI using 64-bit registers, but 32-bit pointers, simlar to x32.
Il 02/04/2013 19:47, Peter Maydell ha scritto: > On 2 April 2013 18:26, Paolo Bonzini <pbonzini@redhat.com> wrote: >> I think "a thing the size of a pointer" should be abi_long/ulong. The >> pointer is not a CPU concept. > > Yeah. OTOH type alignment isn't a CPU concept either, so I'm > a little suspicious of these defines in general. Ok, the main case where the target alignment matters is in 'struct target_elf_prstatus' (linux-user/elfload.c). Linux, in its n32 implementation, explicitly uses a different struct that changes some longs to ints (pr_sigpend, pr_sighold, pr_flag) and keeps longs for others (pr_reg). --- typedef unsigned long elf_greg_t; typedef elf_greg_t elf_gregset_t[ELF_NGREG]; #define elf_prstatus elf_prstatus32 struct elf_prstatus32 { struct elf_siginfo pr_info; short pr_cursig; /* Current signal */ unsigned int pr_sigpend; /* Set of pending signals */ unsigned int pr_sighold; /* Set of held signals */ pid_t pr_pid; pid_t pr_ppid; pid_t pr_pgrp; pid_t pr_sid; struct compat_timeval pr_utime; /* User time */ struct compat_timeval pr_stime; /* System time */ struct compat_timeval pr_cutime;/* Cumulative user time */ struct compat_timeval pr_cstime;/* Cumulative system time */ elf_gregset_t pr_reg; /* GP registers */ int pr_fpvalid; }; --- Instead, we use target_ulong for both (possibly via the target_elf_greg_t typedef). sparc32plus and ppc64abi32 instead use 32-bit for pr_reg too (see include/linux/elfcore-compat.h and fs/compat_binfmt_elf.c). This is also wrong. In any case, what we are doing is doubly wrong. Things that have 4-byte alignment should also have 4-byte size. Things that have 8-byte alignment should also have 8-byte size. Paolo
diff --git a/configure b/configure index 04c2618..5d9a87e 100755 --- a/configure +++ b/configure @@ -4013,10 +4013,6 @@ bflt="no" target_nptl="no" interp_prefix1=`echo "$interp_prefix" | sed "s/%M/$target_arch2/g"` gdb_xml_files="" -target_short_alignment=2 -target_int_alignment=4 -target_long_alignment=4 -target_llong_alignment=8 target_libs_softmmu= TARGET_ARCH="$target_arch2" @@ -4028,10 +4024,8 @@ case "$target_arch2" in ;; x86_64) TARGET_BASE_ARCH=i386 - target_long_alignment=8 ;; alpha) - target_long_alignment=8 target_nptl="yes" ;; arm|armeb) @@ -4039,7 +4033,6 @@ case "$target_arch2" in bflt="yes" target_nptl="yes" gdb_xml_files="arm-core.xml arm-vfp.xml arm-vfp3.xml arm-neon.xml" - target_llong_alignment=4 ;; cris) target_nptl="yes" @@ -4050,9 +4043,6 @@ case "$target_arch2" in m68k) bflt="yes" gdb_xml_files="cf-core.xml cf-fp.xml" - target_int_alignment=2 - target_long_alignment=2 - target_llong_alignment=2 ;; microblaze|microblazeel) TARGET_ARCH=microblaze @@ -4074,7 +4064,6 @@ case "$target_arch2" in TARGET_ARCH=mips64 TARGET_BASE_ARCH=mips echo "TARGET_ABI_MIPSN64=y" >> $config_target_mak - target_long_alignment=8 ;; moxie) ;; @@ -4096,7 +4085,6 @@ case "$target_arch2" in TARGET_BASE_ARCH=ppc TARGET_ABI_DIR=ppc gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml power-spe.xml" - target_long_alignment=8 ;; ppc64abi32) TARGET_ARCH=ppc64 @@ -4114,7 +4102,6 @@ case "$target_arch2" in ;; sparc64) TARGET_BASE_ARCH=sparc - target_long_alignment=8 ;; sparc32plus) TARGET_ARCH=sparc64 @@ -4124,7 +4111,6 @@ case "$target_arch2" in ;; s390x) target_nptl="yes" - target_long_alignment=8 ;; unicore32) ;; @@ -4156,10 +4142,6 @@ case "$cpu" in ;; esac -echo "TARGET_SHORT_ALIGNMENT=$target_short_alignment" >> $config_target_mak -echo "TARGET_INT_ALIGNMENT=$target_int_alignment" >> $config_target_mak -echo "TARGET_LONG_ALIGNMENT=$target_long_alignment" >> $config_target_mak -echo "TARGET_LLONG_ALIGNMENT=$target_llong_alignment" >> $config_target_mak echo "TARGET_ARCH=$TARGET_ARCH" >> $config_target_mak target_arch_name="`upper $TARGET_ARCH`" echo "TARGET_$target_arch_name=y" >> $config_target_mak diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h index 0ae967a..32d11e8 100644 --- a/include/exec/cpu-defs.h +++ b/include/exec/cpu-defs.h @@ -36,6 +36,19 @@ #define TARGET_LONG_SIZE (TARGET_LONG_BITS / 8) +#ifndef TARGET_SHORT_ALIGNMENT +#define TARGET_SHORT_ALIGNMENT 2 +#endif +#ifndef TARGET_INT_ALIGNMENT +#define TARGET_INT_ALIGNMENT 4 +#endif +#ifndef TARGET_LONG_ALIGNMENT +#define TARGET_LONG_ALIGNMENT 4 +#endif +#ifndef TARGET_LLONG_ALIGNMENT +#define TARGET_LLONG_ALIGNMENT 8 +#endif + typedef int16_t target_short __attribute__ ((aligned(TARGET_SHORT_ALIGNMENT))); typedef uint16_t target_ushort __attribute__((aligned(TARGET_SHORT_ALIGNMENT))); typedef int32_t target_int __attribute__((aligned(TARGET_INT_ALIGNMENT))); diff --git a/target-alpha/cpu.h b/target-alpha/cpu.h index 2156a1e..b0338fb 100644 --- a/target-alpha/cpu.h +++ b/target-alpha/cpu.h @@ -24,6 +24,7 @@ #include "qemu-common.h" #define TARGET_LONG_BITS 64 +#define TARGET_LONG_ALIGNMENT 8 #define CPUArchState struct CPUAlphaState diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 2b97221..9a8bb03 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -20,6 +20,7 @@ #define CPU_ARM_H #define TARGET_LONG_BITS 32 +#define TARGET_LLONG_ALIGNMENT 4 #define ELF_MACHINE EM_ARM diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 48f41ca..f1ef87c 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -24,6 +24,7 @@ #ifdef TARGET_X86_64 #define TARGET_LONG_BITS 64 +#define TARGET_LONG_ALIGNMENT 8 #else #define TARGET_LONG_BITS 32 #endif diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h index c90c40c..74612a9 100644 --- a/target-m68k/cpu.h +++ b/target-m68k/cpu.h @@ -21,6 +21,9 @@ #define CPU_M68K_H #define TARGET_LONG_BITS 32 +#define TARGET_INT_ALIGNMENT 2 +#define TARGET_LONG_ALIGNMENT 2 +#define TARGET_LLONG_ALIGNMENT 2 #define CPUArchState struct CPUM68KState diff --git a/target-mips/mips-defs.h b/target-mips/mips-defs.h index bf094a3..624a015 100644 --- a/target-mips/mips-defs.h +++ b/target-mips/mips-defs.h @@ -10,6 +10,7 @@ #if defined(TARGET_MIPS64) #define TARGET_LONG_BITS 64 +#define TARGET_LONG_ALIGNMENT 8 #define TARGET_PHYS_ADDR_SPACE_BITS 36 #define TARGET_VIRT_ADDR_SPACE_BITS 42 #else diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h index 42c36e2..19d4e4c 100644 --- a/target-ppc/cpu.h +++ b/target-ppc/cpu.h @@ -27,6 +27,7 @@ #if defined (TARGET_PPC64) /* PowerPC 64 definitions */ #define TARGET_LONG_BITS 64 +#define TARGET_LONG_ALIGNMENT 8 #define TARGET_PAGE_BITS 12 /* Note that the official physical address space bits is 62-M where M diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h index e351005..8a241d3 100644 --- a/target-s390x/cpu.h +++ b/target-s390x/cpu.h @@ -26,6 +26,7 @@ #include "qemu-common.h" #define TARGET_LONG_BITS 64 +#define TARGET_LONG_ALIGNMENT 8 #define ELF_MACHINE EM_S390 diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h index 6fa7778..9e5941a 100644 --- a/target-sparc/cpu.h +++ b/target-sparc/cpu.h @@ -13,6 +13,7 @@ #define TARGET_VIRT_ADDR_SPACE_BITS 32 #else #define TARGET_LONG_BITS 64 +#define TARGET_LONG_ALIGNMENT 8 #define TARGET_DPREGS 32 #define TARGET_PAGE_BITS 13 /* 8k */ #define TARGET_PHYS_ADDR_SPACE_BITS 41
This is no different from, for example, TARGET_LONG_BITS. It does not belong in configure. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- configure | 18 ------------------ include/exec/cpu-defs.h | 13 +++++++++++++ target-alpha/cpu.h | 1 + target-arm/cpu.h | 1 + target-i386/cpu.h | 1 + target-m68k/cpu.h | 3 +++ target-mips/mips-defs.h | 1 + target-ppc/cpu.h | 1 + target-s390x/cpu.h | 1 + target-sparc/cpu.h | 1 + 10 files changed, 23 insertions(+), 18 deletions(-)